Re: [PATCH net-next] net: dsa: mv88e6xxx: Allow dynamic reconfiguration of tag protocol

[Tobias Waldekranz <tobias@waldekranz.com>]

On Tue, Mar 23, 2021 at 09:53, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 3/23/2021 7:49 AM, Tobias Waldekranz wrote:
>> On Tue, Mar 23, 2021 at 13:41, Andrew Lunn <andrew@lunn.ch> wrote:
>>> On Tue, Mar 23, 2021 at 11:23:26AM +0100, Tobias Waldekranz wrote:
>>>> All devices are capable of using regular DSA tags. Support for
>>>> Ethertyped DSA tags sort into three categories:
>>>>
>>>> 1. No support. Older chips fall into this category.
>>>>
>>>> 2. Full support. Datasheet explicitly supports configuring the CPU
>>>>    port to receive FORWARDs with a DSA tag.
>>>>
>>>> 3. Undocumented support. Datasheet lists the configuration from
>>>>    category 2 as "reserved for future use", but does empirically
>>>>    behave like a category 2 device.
>>>>
>>>> Because there are ethernet controllers that do not handle regular DSA
>>>> tags in all cases, it is sometimes preferable to rely on the
>>>> undocumented behavior, as the alternative is a very crippled
>>>> system. But, in those cases, make sure to log the fact that an
>>>> undocumented feature has been enabled.
>>>
>>> Hi Tobias
>>>
>>> I wonder if dynamic reconfiguration is the correct solution here. By
>>> default it will be wrong for this board, and you need user space to
>>> flip it.
>>>
>>> Maybe a DT property would be better. Extend dsa_switch_parse_of() to
>>> look for the optional property dsa,tag-protocol, a string containing
>>> the name of the tag ops to be used.
>> 
>> This was my initial approach. It gets quite messy though. Since taggers
>> can be modules, there is no way of knowing if a supplied protocol name
>> is garbage ("asdf"), or just part of a module in an initrd that is not
>> loaded yet when you are probing the tree. Even when the tagger is
>> available, there is no way to verify if the driver is compatible with
>> it. So I think we would have to:
>> 
>> - Keep the list of protcol names compiled in with the DSA module, such
>>   that "edsa" can be resolved to DSA_TAG_PROTO_EDSA without having the
>>   tagger module loaded.
>> 
>> - Add (yet) another op so that we can ask the driver if the given
>>   protocol is acceptable. Calling .change_tag_protocol will not work as
>>   drivers will assume that the driver's .setup has already executed
>>   before it is called.
>> 
>> - Have each driver check (during .setup?) if it should configure the
>>   device to use its preferred protocol or if the user has specified
>>   something else.
>> 
>> That felt like a lot to take on board just to solve a corner case like
>> this. I am happy to be told that there is a much easier way to do it, or
>> that the above would be acceptable if there isn't one.
>> 
>
> The other problem with specifying the tag within the Device Tree is that
> you are half way between providing a policy (which tag to use) and
> describing how the hardware works (which tag is actually supported).

Yeah it is a grey area for sure. Still, I think of it more as a hint
from the OEM. You could argue that the "label" property is policy, you
could also see it as a recommendation that will make the product easier
to use.

We can of course keep modifying drivers as incompatible ones are
discovered, hoping that we will never hit one that does not allow you to
disable the block responsible for dropping the frames.

I guess one argument for applying this change anyway is that it gives
you an easy way to test if your controller behaves better if all frames
are guaranteed to honor Ethernet II. Especially for users that might not
be comfortable with building their own kernels.

> FWIW, the b53/bcm_sf2 binding allows one to specify whether an internal
> port should have Broadcom tags enabled because the accelerator behind
> that port would require it (brcm,use-bcm-hdr).