netdev - Re: [PATCH net-next v2] net: ethernet: ti: am65-cpsw: VLAN-aware CPSW only if !DSA

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <92f332a16eb5bb011e47290ba60ebc4a8dbb3dc3.camel@siemens.com>
Date: Fri, 10 Jan 2025 12:05:57 +0000
From: "Sverdlin, Alexander" <alexander.sverdlin@...mens.com>
To: "s-vadapalli@...com" <s-vadapalli@...com>
CC: "andrew+netdev@...n.ch" <andrew+netdev@...n.ch>, "c-vankar@...com"
	<c-vankar@...com>, "davem@...emloft.net" <davem@...emloft.net>,
	"jpanis@...libre.com" <jpanis@...libre.com>, "pabeni@...hat.com"
	<pabeni@...hat.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"rogerq@...nel.org" <rogerq@...nel.org>, "kuba@...nel.org" <kuba@...nel.org>
Subject: Re: [PATCH net-next v2] net: ethernet: ti: am65-cpsw: VLAN-aware CPSW
 only if !DSA

On Fri, 2025-01-10 at 17:32 +0530, s-vadapalli@...com wrote:
> > > > diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > > > index 5465bf872734..58c840fb7e7e 100644
> > > > --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > > > +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > > > @@ -32,6 +32,7 @@
> > > >   #include <linux/dma/ti-cppi5.h>
> > > >   #include <linux/dma/k3-udma-glue.h>
> > > >   #include <net/page_pool/helpers.h>
> > > > +#include <net/dsa.h>
> > > >   #include <net/switchdev.h>
> > > >   
> > > >   #include "cpsw_ale.h"
> > > > @@ -724,13 +725,22 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
> > > >   	u32 val, port_mask;
> > > >   	struct page *page;
> > > >   
> > > > +	/* Control register */
> > > > +	val = AM65_CPSW_CTL_P0_ENABLE | AM65_CPSW_CTL_P0_TX_CRC_REMOVE |
> > > > +	      AM65_CPSW_CTL_VLAN_AWARE | AM65_CPSW_CTL_P0_RX_PAD;
> > > > +	/* VLAN aware CPSW mode is incompatible with some DSA tagging schemes.
> > > > +	 * Therefore disable VLAN_AWARE mode if any of the ports is a DSA Port.
> > > > +	 */
> > > > +	for (port_idx = 0; port_idx < common->port_num; port_idx++)
> > > > +		if (netdev_uses_dsa(common->ports[port_idx].ndev)) {
> > > > +			val &= ~AM65_CPSW_CTL_VLAN_AWARE;
> > > > +			break;
> > > > +		}
> > > > +	writel(val, common->cpsw_base + AM65_CPSW_REG_CTL);
> > > > +
> > > >   	if (common->usage_count)
> > > >   		return 0;
> > > 
> > > The changes above should be present HERE, i.e. below the
> > > "common->usage_count" check, as was the case earlier.
> > 
> > It has been moved deliberately, consider first port is being brought up
> > and only then the second port is being brought up (as part of
> > dsa_conduit_setup(), which sets dev->dsa_ptr right before opening the
> > port). As this isn't RMW operation and actually happens under RTNL lock,
> > moving in front of "if" looks safe to me... What do you think?
> 
> I understand the issue now. Does the following work?
> 
> 1. am65_cpsw_nuss_common_open() can be left as-is i.e. no changes to be
> made.
> 2. Interfaces being brought up will invoke am65_cpsw_nuss_ndo_slave_open()
>    which then invokes am65_cpsw_nuss_common_open().
> 3. Within am65_cpsw_nuss_ndo_slave_open(), immediately after the call to
>    am65_cpsw_nuss_common_open() returns, clear AM65_CPSW_CTL_VLAN_AWARE
>    bit within AM65_CPSW_REG_CTL register if the interface is DSA.

This would fail if the port involved into DSA story would be opened first
and the one not involved into DSA afterwards, wouldn't it?

> The patch then effectively is the DSA.h include plus the following diff:
> ------------------------------------------------------------------------------------------------
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index 5465bf872734..7819a5674f9c 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -1014,6 +1014,15 @@ static int am65_cpsw_nuss_ndo_slave_open(struct net_device *ndev)
> 
>         common->usage_count++;
> 
> +       /* VLAN aware CPSW mode is incompatible with some DSA tagging schemes.
> +        * Therefore disable VLAN_AWARE mode if any of the ports is a DSA Port.
> +        */
> +       if (netdev_uses_dsa(port->ndev) {
> +               reg = readl(common->cpsw_base + AM65_CPSW_REG_CTL);
> +               reg &= ~AM65_CPSW_CTL_VLAN_AWARE;
> +               writel(reg, common->cpsw_base + AM65_CPSW_REG_CTL);
> +       }
> +
>         am65_cpsw_port_set_sl_mac(port, ndev->dev_addr);
>         am65_cpsw_port_enable_dscp_map(port);
> ------------------------------------------------------------------------------------------------
> 
> > 
> > > > -	/* 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);
> > > >   	/* Max length register */
> > > >   	writel(AM65_CPSW_MAX_PACKET_SIZE,
> > > >   	       host_p->port_base + AM65_CPSW_PORT_REG_RX_MAXLEN);

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com