Message-ID: <49e531f7-9465-40ea-b604-22a3a7f13d62@ti.com>
Date: Wed, 28 Feb 2024 15:34:55 +0530
From: Siddharth Vadapalli <s-vadapalli@...com>
To: Jiri Pirko <jiri@...nulli.us>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
        <pabeni@...hat.com>, <rogerq@...nel.org>, <andrew@...n.ch>,
        <vladimir.oltean@....com>, <hkallweit1@...il.com>,
        <dan.carpenter@...aro.org>, <horms@...nel.org>,
        <yuehaibing@...wei.com>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
        <srk@...com>, <s-vadapalli@...com>
Subject: Re: [PATCH net-next] net: ethernet: ti: am65-cpsw: Add priv-flag for
 Switch VLAN Aware mode



On 28/02/24 13:53, Jiri Pirko wrote:
> Wed, Feb 28, 2024 at 08:06:39AM CET, s-vadapalli@...com wrote:
>>
>>
>> On 27/02/24 18:09, Jiri Pirko wrote:
>>> Tue, Feb 27, 2024 at 09:28:15AM CET, s-vadapalli@...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.

> 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.

Powered by blists - more mailing lists