Received-SPF: pass (google.com: domain of linux-kernel+bounces-205902-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 7 Jun 2024 14:18:30 +0300
From: Vladimir Oltean <olteanv@gmail.com>
To: Martin Schiller <ms@dev.tdt.de>
Cc: martin.blumenstingl@googlemail.com, hauke@hauke-m.de, andrew@lunn.ch,
	f.fainelli@gmail.com, davem@davemloft.net, edumazet@google.com,
	kuba@kernel.org, pabeni@redhat.com, robh@kernel.org,
	krzk+dt@kernel.org, conor+dt@kernel.org, netdev@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next 05/13] net: dsa: lantiq_gswip: do also enable or
 disable cpu port
Message-ID: <20240607111830.jfi3roiry27bmwih@skbuf>
References: <20240606085234.565551-1-ms@dev.tdt.de>
 <20240606085234.565551-6-ms@dev.tdt.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240606085234.565551-6-ms@dev.tdt.de>

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@dev.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.