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

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

[Sanjuán García, Jorge]

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.

Signed-off-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 3c7854537cb5..a3c5360d27c2 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -2207,6 +2207,7 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
 	port->ndev->vlan_features |=  NETIF_F_SG;
 	port->ndev->netdev_ops = &am65_cpsw_nuss_netdev_ops;
 	port->ndev->ethtool_ops = &am65_cpsw_ethtool_ops_slave;
+	port->ndev->priv_flags |= IFF_UNICAST_FLT;
 
 	/* Configuring Phylink */
 	port->slave.phylink_config.dev = &port->ndev->dev;
-- 
2.34.1

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

[Jakub Kicinski]

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)?

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

[Andrew Lunn]

On Wed, Feb 28, 2024 at 08:05:16PM -0800, 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)?

I considered that as well, it is also not clear to me if this actually
works. At minimum the description needs additions.

The interesting thing here is, this is a switch port. If there is an
entry in the FDB, that is as good as a unicast filter.

Now, i've no idea if this actually works. This is more Vladimirs
area. What especially needs testing is a port not a member of a
bridge, when you don't really expect there to be any FDB entries.

	Andrew

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

[Ravi Gunasekaran]

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. 

-- 
Regards,
Ravi

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

[Vladimir Oltean]

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.

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

[Sanjuán García, Jorge]

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

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

[Roger Quadros]

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