Received-SPF: pass (google.com: domain of linux-kernel+bounces-84665-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 28 Feb 2024 09:23:04 +0100
From: Jiri Pirko <jiri@resnulli.us>
To: Siddharth Vadapalli <s-vadapalli@ti.com>
Cc: davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
	pabeni@redhat.com, rogerq@kernel.org, andrew@lunn.ch,
	vladimir.oltean@nxp.com, hkallweit1@gmail.com,
	dan.carpenter@linaro.org, horms@kernel.org, yuehaibing@huawei.com,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, srk@ti.com
Subject: Re: [PATCH net-next] net: ethernet: ti: am65-cpsw: Add priv-flag for
 Switch VLAN Aware mode
Message-ID: <Zd7taFB2nEvtZh8E@nanopsycho>
References: <20240227082815.2073826-1-s-vadapalli@ti.com>
 <Zd3YHQRMnv-ZvSWs@nanopsycho>
 <7d1496da-100a-4336-b744-33e843eba930@ti.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <7d1496da-100a-4336-b744-33e843eba930@ti.com>

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
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
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
is most probably not the correct place to put it. If it is, make sure
you advocate for it properly in the patch description.

pw-bot: cr


>
>-- 
>Regards,
>Siddharth.