netdev - Re: [PATCH net-next 05/13] net: dsa: lantiq_gswip: do also enable or disable cpu port

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 for Android: free password hash cracker in your pocket

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

Date: Fri, 7 Jun 2024 14:18:30 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Martin Schiller <ms@....tdt.de>
Cc: martin.blumenstingl@...glemail.com, hauke@...ke-m.de, andrew@...n.ch,
	f.fainelli@...il.com, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, robh@...nel.org,
	krzk+dt@...nel.org, conor+dt@...nel.org, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 05/13] net: dsa: lantiq_gswip: do also enable or
 disable cpu port

On Thu, Jun 06, 2024 at 10:52:26AM +0200, Martin Schiller wrote:
> Before commit 74be4babe72f ("net: dsa: do not enable or disable non user
> ports"), gswip_port_enable/disable() were also executed for the cpu port
> in gswip_setup() which disabled the cpu port during initialization.

Ah, you also noticed this.

> 
> Let's restore this by removing the dsa_is_user_port checks. Also, let's
> clean up the gswip_port_enable() function so that we only have to check
> for the cpu port once.
> 
> Fixes: 74be4babe72f ("net: dsa: do not enable or disable non user ports")

Fixes tags shouldn't be taken lightly. If you think there's a functional
user-visible problem caused by that change, you need to explain what
that problem is and what it affects. Additionally, bug fix patches are
sent out to the 'net' tree, not bundled up with 'net-next' material
(unless they fix a change that's also exclusive to net-next).
Otherwise, just drop the 'Fixes' tag.

> Signed-off-by: Martin Schiller <ms@....tdt.de>
> ---
>  drivers/net/dsa/lantiq_gswip.c | 24 ++++++++----------------
>  1 file changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
> index 3fd5599fca52..38b5f743e5ee 100644
> --- a/drivers/net/dsa/lantiq_gswip.c
> +++ b/drivers/net/dsa/lantiq_gswip.c
> @@ -695,13 +695,18 @@ static int gswip_port_enable(struct dsa_switch *ds, int port,
>  	struct gswip_priv *priv = ds->priv;
>  	int err;
>  
> -	if (!dsa_is_user_port(ds, port))
> -		return 0;
> -
>  	if (!dsa_is_cpu_port(ds, port)) {
> +		u32 mdio_phy = 0;
> +
>  		err = gswip_add_single_port_br(priv, port, true);
>  		if (err)
>  			return err;
> +
> +		if (phydev)
> +			mdio_phy = phydev->mdio.addr & GSWIP_MDIO_PHY_ADDR_MASK;
> +
> +		gswip_mdio_mask(priv, GSWIP_MDIO_PHY_ADDR_MASK, mdio_phy,
> +				GSWIP_MDIO_PHYp(port));
>  	}
>  
>  	/* RMON Counter Enable for port */
> @@ -714,16 +719,6 @@ static int gswip_port_enable(struct dsa_switch *ds, int port,
>  	gswip_switch_mask(priv, 0, GSWIP_SDMA_PCTRL_EN,
>  			  GSWIP_SDMA_PCTRLp(port));
>  
> -	if (!dsa_is_cpu_port(ds, port)) {
> -		u32 mdio_phy = 0;
> -
> -		if (phydev)
> -			mdio_phy = phydev->mdio.addr & GSWIP_MDIO_PHY_ADDR_MASK;
> -
> -		gswip_mdio_mask(priv, GSWIP_MDIO_PHY_ADDR_MASK, mdio_phy,
> -				GSWIP_MDIO_PHYp(port));
> -	}
> -
>  	return 0;
>  }

It would be good to state in the commit message that the operation
reordering is safe. The commit seems to be concerned mainly with code
cleanliness, which does not always take side effects into account.