diff mbox series

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

Message ID 20240228111300.2516590-1-jorge.sanjuangarcia@duagon.com
State New
Headers
Series [net,RESEND] net: ethernet: ti: am65-cpsw: Add IFF_UNICAST_FLT flag to port device |

Commit Message

Sanjuán García, Jorge Feb. 28, 2024, 11:13 a.m. UTC 
  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(+)

Comments

Jakub Kicinski Feb. 29, 2024, 4:05 a.m. UTC | #1 
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)?
Andrew Lunn Feb. 29, 2024, 1:22 p.m. UTC | #2 
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
Ravi Gunasekaran March 1, 2024, 11:09 a.m. UTC | #3 
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.
Vladimir Oltean March 1, 2024, 3:49 p.m. UTC | #4 
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.
Sanjuán García, Jorge March 4, 2024, 10:27 a.m. UTC | #5 
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
diff mbox series

Patch

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;