Re: [PATCH net RESEND] net: ethernet: ti: am65-cpsw: Add IFF_UNICAST_FLT flag to port device

[Roger Quadros <rogerq@kernel.org>]

On 04/03/2024 12:27, Sanjuán García, Jorge wrote:
> On Fri, 2024-03-01 at 17:49 +0200, Vladimir Oltean wrote:
>> [No suele recibir correo electrónico de olteanv@gmail.com. Descubra
>> por qué esto es importante en
>> https://aka.ms/LearnAboutSenderIdentification ]
>>
>> On Fri, Mar 01, 2024 at 04:39:50PM +0530, Ravi Gunasekaran wrote:
>>> On 2/29/24 9:35 AM, Jakub Kicinski wrote:
>>>> On Wed, 28 Feb 2024 11:13:23 +0000 Sanjuán García, Jorge wrote:
>>>>> Since commit 8940e6b669ca ("net: dsa: avoid call to
>>>>> __dev_set_promiscuity()
>>>>> while rtnl_mutex isn't held") when conecting one of this
>>>>> switch's port
>>>>> to a DSA switch as the conduit interface, the network interface
>>>>> is set to
>>>>> promiscuous mode by default and cannot be set to not
>>>>> promiscuous mode again
>>>>> from userspace. The reason for this is that the cpsw ports net
>>>>> devices
>>>>> do not have the flag IFF_UNICAST_FLT set in their private
>>>>> flags.
>>>>>
>>>>> The cpsw switch should be able to set not promiscuous mode as
>>>>> otherwise
>>>>> a '1' is written to bit ALE_PORT_MACONLY_CAF which makes
>>>>> ethernet frames
>>>>> get an additional VLAN tag when entering the port connected to
>>>>> the DSA
>>>>> switch. Setting the IFF_UNICAST_FLT flag to all ports allows us
>>>>> to have
>>>>> the conduit interface on the DSA subsystem set as not
>>>>> promiscuous.
>>>>
>>>> It doesn't look like am65-cpsw-nuss supports unicast filtering,
>>>> tho, does it? So we're lying about support to work around some
>>>> CPSW weirdness (additional VLAN tag thing)?
>>>
>>> CPSW driver does not support unicast filtering.
>>
>> Then the driver can't declare IFF_UNICAST_FLT.
>>
>> Why does enabling promiscuous mode cause Ethernet frames to get an
>> additional VLAN tag? 802.3 clause 4.2.4.1.1 Address recognition only
>> says "The MAC sublayer may also provide the capability of operating
>> in
>> the promiscuous receive mode. In this mode of operation, the MAC
>> sublayer recognizes and accepts all valid frames, regardless of their
>> Destination Address field values.". Absolutely nothing about VLAN.
> 
> Hi,
> 
> Thank you all very much for the reviews. It is clear now we should not
> add this IFF_UNICAST_FLT flag to this driver.
> 
> I may do some new investigations to find out exactly why this CPSW
> driver is adding VLAN tags when set to promiscuous mode. The CPSW HW is
> definetly adding VLAN tags whenever bit Iy_REG_Py_MACONLY of register
> CPSW_Iy_ALE_PORTCTL0_y gets a "1". Maybe there is some extra

MAC_ONLY and MAC_ONLY_CAF are different bits in the
CPSW_ALE_PORT_CONTROL_REG_y register [1].

Promiscuous mode sets the MAC_ONLY_CAF bit.

From TRM [1] , MAC =
"
Mac Only Copy All Frames.
When set a Mac Only port will transfer all received good frames to
the host.
When clear a Mac Only port will transfer packets to the host based
on ALE destination address lookup operation (which operates more
like an Ethernet Mac).
A Mac Only port is a port with maconly set.
"

Since you are operating the CPSW in MAC mode I believe MAC_ONLY is
set as well for you. That is fine.

Now, from [2], the CPSW hardware seems to poke around VLAN tags
only if VLAN_AWARE bit in CPSW_CONTROL_REG is set which seems
to be the case by default.

> static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
> {
>         struct am65_cpsw_host *host_p = am65_common_get_host(common);
>         int port_idx, i, ret, tx;
>         struct sk_buff *skb;
>         u32 val, port_mask;
> 
>         if (common->usage_count)
>                 return 0;
> 
>         /* Control register */
>         writel(AM65_CPSW_CTL_P0_ENABLE | AM65_CPSW_CTL_P0_TX_CRC_REMOVE |
>                AM65_CPSW_CTL_VLAN_AWARE | AM65_CPSW_CTL_P0_RX_PAD,
>                common->cpsw_base + AM65_CPSW_REG_CTL);

One thing you could try is to not set AM65_CPSW_CTL_VLAN_AWARE here
and see if it resolves your case.

There was a patch sent recently [3] to play around this bit but it was
not clear why it was required. If AM65_CPSW_CTL_VLAN_AWARE is indeed the
cause of trouble here then it should be disabled by default.

[1] - https://www.ti.com/lit/pdf/spruid7
12.2.1.6.10.12 CPSW_ALE_PORT_CONTROL_REG_y Register

[2] - https://www.ti.com/lit/pdf/spruid7
12.2.1.4.6.4.1 Transmit VLAN Processing

[3] - https://lore.kernel.org/all/20240227082815.2073826-1-s-vadapalli@ti.com/

> configuration needed but as far a the current am65-cpsw-nuss.c
> implementation goes, am65_cpsw_slave_set_promisc() only sets that bit.
> 
> Best regards,
> Jorge

-- 
cheers,
-roger