Re: [PATCH net-next v3 01/26] switchdev: introduce get/set attrs ops

[roopa <roopa@cumulusnetworks.com>]

On 4/2/15, 11:16 AM, Jiri Pirko wrote:
> Thu, Apr 02, 2015 at 07:52:28PM CEST, sfeldma@gmail.com wrote:
>> On Thu, Apr 2, 2015 at 2:09 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> [snip]
>>>
>>>> +       int err = -EOPNOTSUPP;
>>>> +
>>>> +       if (!(dev->features & NETIF_F_HW_SWITCH_OFFLOAD))
>>>> +               return err;
>>> That check is currently going to prevent DSA from working, since we do
>>> not yet advertise NETIF_HW_SWITCH_OFFLOAD (which should be fixed). In
>>> general, though I am not sure this is entirely desirable to check that
>>> here for multiple reasons:
>>>
>>> - stacked devices typically propagate lower devices dev->features, but
>>> if they are purposely not doing it, this might start breaking
>>> - is not that check, used as it is now, that unconditionally, end-up
>>> being redundant with e.g: getting the switch device id to identify
>>> this net_device as a switch port net_device?
>>>
>>> I kind of preferred when we had this moved into the
>>> __swdev_attr_{get,set} caller, such we had finer control over whether
>>> or not checking for these kinds of features makes sense for a
>>> particular operation.
>> I dropped it in my v1 patch set all together, but then it came back in
>> v2.  I understand how it works, giving the user the ability to turn
>> on/off offload support on a port at run-time, but I don't understand
>> the application.  I agree with you that we already have finer-grained
>> ability to know if a sub-feature is supported or not based on what the
>> driver implements (like switch ID or STP state).  I don't know what
>> this master switch is used for.  Why would the user turn off
>> offloading on a port at run-time after the device has already been
>> programmed with some offloading tasks?  What tells the device to stop
>> those offloads now.  And then later, the user flips the switch to turn
>> back on offloads on the port.   How do we restore the device?
>>
>> Roopa, can you help us understand how NETIF_F_HW_SWITCH_OFFLOAD is used?
> Documentation patch required?
I can provide a documentation patch.
The use for this today is:
a) to recognize a switch port, instead of walking lowerdevs all the time 
(sometimes this could be in the packet path).
b) There are drivers today that support bridge l2 ndo ops only via 
'self' (directly going to the nic driver).
the bridge driver vlan offload bridge_setlink/bridge_getlink switchdev 
calls also use the same ndo ops when you add a vlan to a bridge (this is 
from within the bridge driver. Same will be true for fdb). This flag 
also serves the purpose of not breaking those drivers because they dont 
advertise this flag
c) I see it as a nice switch to turn off offload in some environments. 
The driver has a choice to support this or simply reject it.
In the cases where it supports it, maybe it could flush all its tables 
and send pkts to software.

Agree that the use of swdev_get_parent_id in the same place is making it 
redundant.
In the initial days, if i remember correctly, we used an ndo op with a 
similar name that was also shared by other nic
devices... which made it difficult to rely on the op.

since the rest of the kernel has always used features on netdev instead 
of ops, seems like a feature check instead of a 'op' check will keep it 
consistent.

Thanks,
Roopa