netdev - Re: [PATCH net-next] 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] [day] [month] [year] [list]

Message-ID: <t34jbpf2dwhuszreyxby3qzvhachu3nt2wsbqiq4fkbw3uoxrb@aq6uqo3kg6ct>
Date: Fri, 10 Jan 2025 16:11:08 +0530
From: Siddharth Vadapalli <s-vadapalli@...com>
To: "A. Sverdlin" <alexander.sverdlin@...mens.com>
CC: Siddharth Vadapalli <s-vadapalli@...com>, <netdev@...r.kernel.org>,
        Andrew
 Lunn <andrew+netdev@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub
 Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
        Roger Quadros
	<rogerq@...nel.org>, Chintan Vankar <c-vankar@...com>,
        Julien Panis
	<jpanis@...libre.com>
Subject: Re: [PATCH net-next] net: ethernet: ti: am65-cpsw: VLAN-aware CPSW
 only if !DSA

On Fri, Jan 10, 2025 at 10:26:21AM +0100, A. Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@...mens.com>
> 
> Only configure VLAN-aware CPSW mode if no port is used as DSA CPU port.
> VLAN-aware mode interferes with some DSA tagging schemes and makes stacking
> DSA switches downstream of CPSW impossible. Previous attempts to address
> the issue linked below.
> 
> Link: https://lore.kernel.org/netdev/20240227082815.2073826-1-s-vadapalli@ti.com/
> Link: https://lore.kernel.org/linux-arm-kernel/4699400.vD3TdgH1nR@localhost/
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@...mens.com>
> ---
>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index dcb6662b473d..e445acb29e16 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,23 @@ 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_P0_RX_PAD;
> +	for (port_idx = 0; port_idx < common->port_num; port_idx++) {
> +		struct am65_cpsw_port *port = &common->ports[port_idx];
> +
> +		if (netdev_uses_dsa(port->ndev))
> +			break;
> +	}
> +	/* VLAN aware CPSW mode is incompatible with some DSA tagging schemes */
> +	if (port_idx == common->port_num)
> +		val |= AM65_CPSW_CTL_VLAN_AWARE;
> +	writel(val, common->cpsw_base + AM65_CPSW_REG_CTL);
> +

Though it functionally enables VLAN_AWARE mode only when none of the ports
are a DSA port, the implementation and the comment appear inconsistent.
The comment states that VLAN aware CPSW mode is incompatible with DSA
and the lines following it are enabling VLAN_AWARE mode albeit for the
case where none of the ports are a DSA port. Since the IF condition
doesn't indicate that in an obvious manner, the implementation could be
improved to maintain consistency between what the comment states and what
the code does.

How about the following?
-------------------------------------------------------------------------
	/* 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;
>  
> -	/* 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);

Regards,
Siddharth.