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

[Florian Fainelli <f.fainelli@gmail.com>]

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