[RFC] ethtool: Support for driver private ioctl's

[RFC] ethtool: Support for driver private ioctl's

[Jose Abreu]

Hi All,

I would like to know your opinion regarding adding support for
driver private ioctl's in ethtool.

Background: Synopsys Ethernet IP's have a certain number of
features which can be reconfigured at runtime. Giving you two
examples: One of the most recent one is the safety features,
which can be enabled/disabled and forced at runtime. Another one
is a Flexible RX Parser which can route specific packets to
specific RX DMA channels. Given that these are features specific
to our IP's it would not be useful to add an uniform API for this
because the users would only be one or two drivers ...

This new feature would change the help usage for ethtool so that
each driver private option would be shown, and then each driver
specific file would have a structure with all the available
options. Finally, each driver would have to handle the private
IOCTL's.

We already have this working locally and now I would like to know
your opinion about upstreaming this ... Do you think this can be
useful for anyone else? Or should we change direction to use, for
example, debugfs/configfs?

Thanks and Best Regards,
Jose Miguel Abreu

Re: [RFC] ethtool: Support for driver private ioctl's

[Florian Fainelli]

On 04/05/2018 03:47 AM, Jose Abreu wrote:
> Hi All,
> 
> I would like to know your opinion regarding adding support for
> driver private ioctl's in ethtool.
> 
> Background: Synopsys Ethernet IP's have a certain number of
> features which can be reconfigured at runtime. Giving you two
> examples: One of the most recent one is the safety features,
> which can be enabled/disabled and forced at runtime. Another one
> is a Flexible RX Parser which can route specific packets to
> specific RX DMA channels. Given that these are features specific
> to our IP's it would not be useful to add an uniform API for this
> because the users would only be one or two drivers ...

Parsing of packets and directing the matched packets to specific
queues/channels can be done through ethtool rxnfc API, tc/cls_flower as
well, so you should really check whether those APIs don't already allow
you to do what you want.

ethtool already supports a concept of private  flags, not ioctl() though
which allows you to toggle boolean values for instance (or technically
up to how many bits a "flag" is used to represent) is that enough or do
you need to turn on/off the feature as well as pass configuration
parameters?

> 
> This new feature would change the help usage for ethtool so that
> each driver private option would be shown, and then each driver
> specific file would have a structure with all the available
> options. Finally, each driver would have to handle the private
> IOCTL's.
> 
> We already have this working locally and now I would like to know
> your opinion about upstreaming this ... Do you think this can be
> useful for anyone else? Or should we change direction to use, for
> example, debugfs/configfs?

In general, even if there is only one driver implementing a particular
feature, the approach chosen is to come up with an API that is as
generic as possible. Even if there is a single user of that API in tree,
having something that was thought to be generic is better than allowing
uncontrolled private ioctl() implementations.
-- 
Florian

Re: [RFC] ethtool: Support for driver private ioctl's

[Michal Kubecek]

On Thu, Apr 05, 2018 at 08:50:49AM -0700, Florian Fainelli wrote:
> On 04/05/2018 03:47 AM, Jose Abreu wrote:
> > Background: Synopsys Ethernet IP's have a certain number of
> > features which can be reconfigured at runtime. Giving you two
> > examples: One of the most recent one is the safety features,
> > which can be enabled/disabled and forced at runtime. Another one
> > is a Flexible RX Parser which can route specific packets to
> > specific RX DMA channels. Given that these are features specific
> > to our IP's it would not be useful to add an uniform API for this
> > because the users would only be one or two drivers ...
> 
> Parsing of packets and directing the matched packets to specific
> queues/channels can be done through ethtool rxnfc API, tc/cls_flower as
> well, so you should really check whether those APIs don't already allow
> you to do what you want.
> 
> ethtool already supports a concept of private  flags, not ioctl() though
> which allows you to toggle boolean values for instance (or technically
> up to how many bits a "flag" is used to represent) is that enough or do
> you need to turn on/off the feature as well as pass configuration
> parameters?

Perhaps introducing "driver/device specific tunables" (i.e. something
like tunables or PHY tunables but specific to a particular device) could
be a way. But it could get out of control quickly and users wouldn't be
happy if they had to set the same (or almost the same) parameter under
five different names for five NIC vendors.

Michal Kubecek

Re: [RFC] ethtool: Support for driver private ioctl's

[Jose Abreu]

Hi Michal,

On 06-04-2018 10:07, Michal Kubecek wrote:
> On Thu, Apr 05, 2018 at 08:50:49AM -0700, Florian Fainelli wrote:
>> On 04/05/2018 03:47 AM, Jose Abreu wrote:
>>> Background: Synopsys Ethernet IP's have a certain number of
>>> features which can be reconfigured at runtime. Giving you two
>>> examples: One of the most recent one is the safety features,
>>> which can be enabled/disabled and forced at runtime. Another one
>>> is a Flexible RX Parser which can route specific packets to
>>> specific RX DMA channels. Given that these are features specific
>>> to our IP's it would not be useful to add an uniform API for this
>>> because the users would only be one or two drivers ...
>> Parsing of packets and directing the matched packets to specific
>> queues/channels can be done through ethtool rxnfc API, tc/cls_flower as
>> well, so you should really check whether those APIs don't already allow
>> you to do what you want.
>>
>> ethtool already supports a concept of private  flags, not ioctl() though
>> which allows you to toggle boolean values for instance (or technically
>> up to how many bits a "flag" is used to represent) is that enough or do
>> you need to turn on/off the feature as well as pass configuration
>> parameters?
> Perhaps introducing "driver/device specific tunables" (i.e. something
> like tunables or PHY tunables but specific to a particular device) could
> be a way. But it could get out of control quickly and users wouldn't be
> happy if they had to set the same (or almost the same) parameter under
> five different names for five NIC vendors.

Yeah, that wouldn't be good but I think this should be a
responsibility to developer: To see if there is an existing
API/ethtool entry before implementing the "tunable". I think a
big concern, for me at least, is that ethtool already has a lot
of options and introducing even more would lead the user to
confusion ...

Thanks and Best Regards,
Jose Miguel Abreu

>
> Michal Kubecek

Re: [RFC] ethtool: Support for driver private ioctl's

[Jose Abreu]

Hi Florian,

On 05-04-2018 16:50, Florian Fainelli wrote:
>
> On 04/05/2018 03:47 AM, Jose Abreu wrote:
>> Hi All,
>>
>> I would like to know your opinion regarding adding support for
>> driver private ioctl's in ethtool.
>>
>> Background: Synopsys Ethernet IP's have a certain number of
>> features which can be reconfigured at runtime. Giving you two
>> examples: One of the most recent one is the safety features,
>> which can be enabled/disabled and forced at runtime. Another one
>> is a Flexible RX Parser which can route specific packets to
>> specific RX DMA channels. Given that these are features specific
>> to our IP's it would not be useful to add an uniform API for this
>> because the users would only be one or two drivers ...
> Parsing of packets and directing the matched packets to specific
> queues/channels can be done through ethtool rxnfc API, tc/cls_flower as
> well, so you should really check whether those APIs don't already allow
> you to do what you want.

Hmm, but in our case this is directly done by HW, we just have to
program a kind of a table which will route automatically the
packets. Does this API support this?

>
> ethtool already supports a concept of private  flags, not ioctl() though
> which allows you to toggle boolean values for instance (or technically
> up to how many bits a "flag" is used to represent) is that enough or do
> you need to turn on/off the feature as well as pass configuration
> parameters?

Some of them I can just turn on/off but the remaining need
configuration and sometimes the configuration is extensive (like
in the case of RX Parser when we have to pass the routing table).

>
>> This new feature would change the help usage for ethtool so that
>> each driver private option would be shown, and then each driver
>> specific file would have a structure with all the available
>> options. Finally, each driver would have to handle the private
>> IOCTL's.
>>
>> We already have this working locally and now I would like to know
>> your opinion about upstreaming this ... Do you think this can be
>> useful for anyone else? Or should we change direction to use, for
>> example, debugfs/configfs?
> In general, even if there is only one driver implementing a particular
> feature, the approach chosen is to come up with an API that is as
> generic as possible. Even if there is a single user of that API in tree,
> having something that was thought to be generic is better than allowing
> uncontrolled private ioctl() implementations.

I understand your point of view but this seems like an overkill
to the -net subsystem because its specific to our IP, or are you
just mentioning a new ethtool entry? i.e. adding a new #define to
the list, plus -net handling ...

Thanks and Best Regards,
Jose Miguel Abreu

Re: [RFC] ethtool: Support for driver private ioctl's

[Andrew Lunn]

On Fri, Apr 06, 2018 at 02:51:15PM +0100, Jose Abreu wrote:
> Hi Florian,
> 
> On 05-04-2018 16:50, Florian Fainelli wrote:
> >
> > On 04/05/2018 03:47 AM, Jose Abreu wrote:
> >> Hi All,
> >>
> >> I would like to know your opinion regarding adding support for
> >> driver private ioctl's in ethtool.
> >>
> >> Background: Synopsys Ethernet IP's have a certain number of
> >> features which can be reconfigured at runtime. Giving you two
> >> examples: One of the most recent one is the safety features,
> >> which can be enabled/disabled and forced at runtime.

Hi Jose

Is there a reason somebody would decide to use the Ethernet in
'unsafe' mode? Cannot you just turn it on by default?

	 Andrew

Re: [RFC] ethtool: Support for driver private ioctl's

[Jose Abreu]

Hi Andrew,

On 06-04-2018 15:47, Andrew Lunn wrote:
> On Fri, Apr 06, 2018 at 02:51:15PM +0100, Jose Abreu wrote:
>> Hi Florian,
>>
>> On 05-04-2018 16:50, Florian Fainelli wrote:
>>> On 04/05/2018 03:47 AM, Jose Abreu wrote:
>>>> Hi All,
>>>>
>>>> I would like to know your opinion regarding adding support for
>>>> driver private ioctl's in ethtool.
>>>>
>>>> Background: Synopsys Ethernet IP's have a certain number of
>>>> features which can be reconfigured at runtime. Giving you two
>>>> examples: One of the most recent one is the safety features,
>>>> which can be enabled/disabled and forced at runtime.
> Hi Jose
>
> Is there a reason somebody would decide to use the Ethernet in
> 'unsafe' mode? Cannot you just turn it on by default?

Yes, its already on by default. I was just trying to give an
example of an user-reconfigurable feature, maybe it was not the
best one :)

Thanks and Best Regards,
Jose Miguel Abreu

>
> 	 Andrew

Re: [RFC] ethtool: Support for driver private ioctl's

[Florian Fainelli]

On 04/06/2018 06:51 AM, Jose Abreu wrote:
> Hi Florian,
> 
> On 05-04-2018 16:50, Florian Fainelli wrote:
>>
>> On 04/05/2018 03:47 AM, Jose Abreu wrote:
>>> Hi All,
>>>
>>> I would like to know your opinion regarding adding support for
>>> driver private ioctl's in ethtool.
>>>
>>> Background: Synopsys Ethernet IP's have a certain number of
>>> features which can be reconfigured at runtime. Giving you two
>>> examples: One of the most recent one is the safety features,
>>> which can be enabled/disabled and forced at runtime. Another one
>>> is a Flexible RX Parser which can route specific packets to
>>> specific RX DMA channels. Given that these are features specific
>>> to our IP's it would not be useful to add an uniform API for this
>>> because the users would only be one or two drivers ...
>> Parsing of packets and directing the matched packets to specific
>> queues/channels can be done through ethtool rxnfc API, tc/cls_flower as
>> well, so you should really check whether those APIs don't already allow
>> you to do what you want.
> 
> Hmm, but in our case this is directly done by HW, we just have to
> program a kind of a table which will route automatically the
> packets. Does this API support this?

I was sort of expecting you to look at the ethtool rxnfc API to see if
it is suitable given your hardware, but if this is indeed a table
programming, then yes, this is what it is designed for. You might want
to consider using the newer, albeit more complex tc/cls_flower if that
works for your use case.

> 
>>
>> ethtool already supports a concept of private  flags, not ioctl() though
>> which allows you to toggle boolean values for instance (or technically
>> up to how many bits a "flag" is used to represent) is that enough or do
>> you need to turn on/off the feature as well as pass configuration
>> parameters?
> 
> Some of them I can just turn on/off but the remaining need
> configuration and sometimes the configuration is extensive (like
> in the case of RX Parser when we have to pass the routing table).
> 
>>
>>> This new feature would change the help usage for ethtool so that
>>> each driver private option would be shown, and then each driver
>>> specific file would have a structure with all the available
>>> options. Finally, each driver would have to handle the private
>>> IOCTL's.
>>>
>>> We already have this working locally and now I would like to know
>>> your opinion about upstreaming this ... Do you think this can be
>>> useful for anyone else? Or should we change direction to use, for
>>> example, debugfs/configfs?
>> In general, even if there is only one driver implementing a particular
>> feature, the approach chosen is to come up with an API that is as
>> generic as possible. Even if there is a single user of that API in tree,
>> having something that was thought to be generic is better than allowing
>> uncontrolled private ioctl() implementations.
> 
> I understand your point of view but this seems like an overkill
> to the -net subsystem because its specific to our IP, or are you
> just mentioning a new ethtool entry? i.e. adding a new #define to
> the list, plus -net handling ...

It depends on the feature, it can be a new set of defines just like it
can be a completely new ethtool command number with custom data
structures between user and kernel space.
-- 
Florian

Re: [RFC] ethtool: Support for driver private ioctl's

[Jose Abreu]

Hi Florian,

On 07-04-2018 20:58, Florian Fainelli wrote:
>
> On 04/06/2018 06:51 AM, Jose Abreu wrote:
>> Hi Florian,
>>
>> On 05-04-2018 16:50, Florian Fainelli wrote:
>>> On 04/05/2018 03:47 AM, Jose Abreu wrote:
>>>> Hi All,
>>>>
>>>> I would like to know your opinion regarding adding support for
>>>> driver private ioctl's in ethtool.
>>>>
>>>> Background: Synopsys Ethernet IP's have a certain number of
>>>> features which can be reconfigured at runtime. Giving you two
>>>> examples: One of the most recent one is the safety features,
>>>> which can be enabled/disabled and forced at runtime. Another one
>>>> is a Flexible RX Parser which can route specific packets to
>>>> specific RX DMA channels. Given that these are features specific
>>>> to our IP's it would not be useful to add an uniform API for this
>>>> because the users would only be one or two drivers ...
>>> Parsing of packets and directing the matched packets to specific
>>> queues/channels can be done through ethtool rxnfc API, tc/cls_flower as
>>> well, so you should really check whether those APIs don't already allow
>>> you to do what you want.
>> Hmm, but in our case this is directly done by HW, we just have to
>> program a kind of a table which will route automatically the
>> packets. Does this API support this?
> I was sort of expecting you to look at the ethtool rxnfc API to see if
> it is suitable given your hardware, but if this is indeed a table
> programming, then yes, this is what it is designed for. You might want
> to consider using the newer, albeit more complex tc/cls_flower if that
> works for your use case.
>

I took a quick look at rxrnfc API and it doesn't seem to match
entirely my requirements.

The feature I want to introduce is called Flexible RX Parser and
will let me route specific packets to specific DMA channels
number. This is different from rxrnfc API because, and I far as I
understand, the API was designed to add rules to packet types
whilst in my case I can add a rule to *any* of the packet content
(within the first 256 bytes of packet, at max). So technically I
can route packets based on destination/ source mac address,
packet type, lenght, protocol, source/destination IP , ....

So, I guess cls_flower it will be ... I'm slowing looking at some
code and docs but it would be great if you could pin point me to
some HW that has similar behavior ?

Thanks and Best Regards,
Jose Miguel Abreu