Received-SPF: pass (google.com: domain of linux-kernel+bounces-93999-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <8a8b4320-924a-4dd3-973b-ca941489f19b@kernel.org>
Date: Wed, 6 Mar 2024 15:48:20 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net RESEND] net: ethernet: ti: am65-cpsw: Add
 IFF_UNICAST_FLT flag to port device
Content-Language: en-US
To: =?UTF-8?B?U2FuanXDoW4gR2FyY8OtYSwgSm9yZ2U=?=
 <Jorge.SanjuanGarcia@duagon.com>, "olteanv@gmail.com" <olteanv@gmail.com>,
 "r-gunasekaran@ti.com" <r-gunasekaran@ti.com>
Cc: "s-vadapalli@ti.com" <s-vadapalli@ti.com>,
 "davem@davemloft.net" <davem@davemloft.net>,
 "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
 "andrew@lunn.ch" <andrew@lunn.ch>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
 "f.fainelli@gmail.com" <f.fainelli@gmail.com>,
 "kuba@kernel.org" <kuba@kernel.org>,
 "edumazet@google.com" <edumazet@google.com>,
 "pabeni@redhat.com" <pabeni@redhat.com>, Pekka Varis <p-varis@ti.com>
References: <20240228111300.2516590-1-jorge.sanjuangarcia@duagon.com>
 <20240228200516.1166a097@kernel.org>
 <03bf515c-9f90-487c-ecfa-90d407dc5d86@ti.com>
 <20240301154957.xex75zuijptswcf3@skbuf>
 <7a39e5266fa3ac781f1eda7ee0b2526bd2f164d0.camel@duagon.com>
From: Roger Quadros <rogerq@kernel.org>
In-Reply-To: <7a39e5266fa3ac781f1eda7ee0b2526bd2f164d0.camel@duagon.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit


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