Re: [PATCH net-next] net: ethernet: ti: am65-cpsw: Add priv-flag for Switch VLAN Aware mode

[Jiri Pirko <jiri@resnulli.us>]

Wed, Feb 28, 2024 at 11:04:55AM CET, s-vadapalli@ti.com wrote:
>
>
>On 28/02/24 13:53, Jiri Pirko wrote:
>> Wed, Feb 28, 2024 at 08:06:39AM CET, s-vadapalli@ti.com wrote:
>>>
>>>
>>> On 27/02/24 18:09, Jiri Pirko wrote:
>>>> Tue, Feb 27, 2024 at 09:28:15AM CET, s-vadapalli@ti.com wrote:
>>>>> The CPSW Ethernet Switch on TI's K3 SoCs can be configured to operate in
>>>>> VLAN Aware or VLAN Unaware modes of operation. This is different from
>>>>> the ALE being VLAN Aware and Unaware. The Ethernet Switch being VLAN Aware
>>>>> results in the addition/removal/replacement of VLAN tag of packets during
>>>>> egress as described in section "12.2.1.4.6.4.1 Transmit VLAN Processing" of
>>>>> the AM65x Technical Reference Manual available at:
>>>>> https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
>>>>> In VLAN Unaware mode, packets remain unmodified on egress.
>>>>>
>>>>> The driver currently configures the Ethernet Switch in VLAN Aware mode by
>>>>> default and there is no support to toggle this capability of the Ethernet
>>>>> Switch at runtime. Thus, add support to toggle the capability by exporting
>>>>> it via the ethtool "priv-flags" interface.
>>>>
>>>> I don't follow. You have all the means to offload all bridge/vlan
>>>> configurations properly and setup your hw according to that. See mlxsw
>>>> for a reference. I don't see the need for any custom driver knobs.
>>>>
>>>
>>> Thank you for reviewing the patch. Please note that the "VLAN Aware mode" being
>>> referred to here is different from ALE being VLAN aware. The hw offload of
>>> bridge/vlan configurations is already supported in the context of the ALE. The
>>> Ethernet Switch being VLAN Aware is a layer on top of that, which enables
>>> further processing on top of the untagged/VLAN packets. This patch aims to
>>> provide a method to enable the following use-cases:
>>> 1. ALE VLAN Aware + CPSW VLAN Aware
>>> 2. ALE VLAN Aware + CPSW VLAN Unaware
>>>
>>> All hw offloads of bridge/vlan configurations are w.r.t. ALE VLAN Aware alone.
>>> Currently, only use-case 1 is enabled by the driver by default and there is no
>>> knob to toggle to use-case 2.
>>>
>>> I am quoting sections of the Technical Reference Manual mentioned in my commit
>>> message, in order to clarify the CPSW VLAN Unaware and CPSW VLAN Aware terminology.
>>>
>>> CPSW VLAN Unaware:
>>> Transmit packets are NOT modified during switch egress.
>>>
>>> CPSW VLAN Aware:
>>> 1. Untagged Packet Operations
>>> Untagged packets are all packets that are not a VLAN packet or a priority tagged
>>> packet. According to the CPWS0_FORCE_UNTAGGED_EGRESS_REG[1-0] MASK bit in the
>>> packet header the packet may exit the switch with a VLAN tag inserted or the
>>> packet may leave the switch unchanged....
>>> 2. Priority Tagged Packet Operations (VLAN VID == 0 && EN_VID0_MODE ==0h)
>>> Priority tagged packets are packets that contain a VLAN header with VID = 0.
>>> According to the CPSW_ALE_FORCE_UNTAGGED_EGRESS_REG[1-0] MASK bit in the packet
>>> header, priority tagged packets may exit the switch with their VLAN ID and
>>> priority replaced or they may have their priority tag completely removed....
>>> 3. VLAN Tagged Packet Operations (VLAN VID != 0 || (EN_VID0_MODE ==1h && VLAN
>>> VID ==0))
>>> VLAN tagged packets are packets that contain a VLAN header specifying the VLAN
>>> the packet belongs to
>>> (VID), the packet priority (PRI), and the drop eligibility indicator (CFI).
>>> According to the CPSW_ALE_FORCE_UNTAGGED_EGRESS_REG[1-0] MASK bit in the packet
>>> header, VLAN tagged packets may exit the switch with their VLAN priority
>>> replaced or they may have their VLAN header completely removed...
>>>
>>> I hope that this clarifies that CPSW VLAN Unaware/Aware is a layer on top of the
>>> hw offload-able bridge/vlan configuration.  Please let me know if there is
>>> anything specific that could enable this without requiring the "priv-flag" based
>>> implementation of this patch.
>> 
>> I have no clue what "ALE" is. But in general. User provided
>
>ALE is Address Lookup Engine.
>
>> configuration, using ip/bridge/etc tools/uapi. According to this
>> configuration, kernel is bahaving. When you do offload, you should just
>> make sure to mimic/mirror the kernel behaviour. With this in mind, why
>
>What if there is no kernel behavior associated with it? How can it be mimicked
>then? This patch isn't offloading any feature that is supported in software. It
>might not be possible to offload features which act on the forwarding path of
>packets entirely in Hardware within the Ethernet Switch.
>
>Please consider the following:
>Untagged packets sent from Software via the corresponding VLAN interfaces will
>be tagged which is the expected behavior. However, if this is offloaded, it will
>imply that even untagged packets that are simply forwarded in the Ethernet
>Switch and never get to software will also have to be tagged by the Ethernet
>Switch. This is not allowing the choice of leaving untagged packets as-is on the
>Ethernet Switch's forwarding path. This patch attempts to allow configuring
>something quite similar to this, where it is possible to *choose* whether or not
>to tag packets being forwarded.

What would kernel datapath do? That is the question you need to ask and
configure the hw accordingly. If 2 interfaces are in the bridge, vlans
involved, etc, the forward behavior is well defined, isn't it. What am I
missing?


>
>> can't you do it without adding additional knob? And if you really need
>> it because the know does some internal hw/fw tuning, priv flag of netdev
>
>The feature can be turned on or off depending on the use-case. Is it acceptable
>to have build configs scattered in the driver code? I don't suppose that is
>acceptable, due to which it will be preferable to have a runtime configuration
>option, which is what this patch provides.
>
>> is most probably not the correct place to put it. If it is, make sure
>
>Please suggest an alternative if this isn't the right place. Otherwise, I can
>only assume that there isn't one.
>
>> you advocate for it properly in the patch description.
>> 
>> pw-bot: cr
>>
>
>-- 
>Regards,
>Siddharth.