Received-SPF: pass (google.com: domain of linux-kernel+bounces-66130-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 15 Feb 2024 01:46:17 +0200
From: Vladimir Oltean <olteanv@gmail.com>
To: Pawel Dembicki <paweldembicki@gmail.com>
Cc: netdev@vger.kernel.org, linus.walleij@linaro.org,
	Russell King <linux@armlinux.org.uk>, Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Claudiu Manoil <claudiu.manoil@nxp.com>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	UNGLinuxDriver@microchip.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v4 01/15] net: dsa: vsc73xx: use
 read_poll_timeout instead delay loop
Message-ID: <20240214234617.c4vz66ntydr7dw3v@skbuf>
References: <20240213220331.239031-1-paweldembicki@gmail.com>
 <20240213220331.239031-2-paweldembicki@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240213220331.239031-2-paweldembicki@gmail.com>

On Tue, Feb 13, 2024 at 11:03:14PM +0100, Pawel Dembicki wrote:
> This commit switches delay loop to read_poll_timeout macro during
> Arbiter empty check in adjust link function.

Replace "This commit does X" with imperative mood: "Switch the delay
loop during the Arbiter empty check from vsc73xx_adjust_link() to use
read_poll_timeout(). Functionally, one msleep() call is eliminated at
the end of the loop, in the timeout case".

> 
> As Russel King suggested:

s/Russel/Russell/

> 
> "This [change] avoids the issue that on the last iteration, the code reads
> the register, test it, find the condition that's being waiting for is

s/test/tests/
s/find/finds/

> false, _then_ waits and end up printing the error message - that last
> wait is rather useless, and as the arbiter state isn't checked after
> waiting, it could be that we had success during the last wait."
> 
> It also remove one short msleep delay.

Apart from the fact that there's a grammatical mistake in this phrase
("it remove" -> "it removes"), it's also a bit redundant, since
Russell's explanation above implies this is what would happen. Anyway,
I've suggested a replacement for it in the first paragraph, the one
describing the change.

> Suggested-by: Russell King <linux@armlinux.org.uk>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---
> v4:
>   - Resend patch
> v3:
>   - Add "Reviewed-by" to commit message only
> v2:
>   - introduced patch
> 
>  drivers/net/dsa/vitesse-vsc73xx-core.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index ae70eac3be28..8b2219404601 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -779,7 +779,7 @@ static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
>  	 * after a PHY or the CPU port comes up or down.
>  	 */
>  	if (!phydev->link) {
> -		int maxloop = 10;
> +		int ret, err;
>  
>  		dev_dbg(vsc->dev, "port %d: went down\n",
>  			port);
> @@ -794,19 +794,16 @@ static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
>  				    VSC73XX_ARBDISC, BIT(port), BIT(port));
>  
>  		/* Wait until queue is empty */
> -		vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
> -			     VSC73XX_ARBEMPTY, &val);
> -		while (!(val & BIT(port))) {
> -			msleep(1);
> -			vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
> -				     VSC73XX_ARBEMPTY, &val);
> -			if (--maxloop == 0) {
> -				dev_err(vsc->dev,
> -					"timeout waiting for block arbiter\n");
> -				/* Continue anyway */
> -				break;
> -			}
> -		}
> +		ret = read_poll_timeout(vsc73xx_read, err,
> +					err < 0 || (val & BIT(port)),
> +					1000, 10000, false,

Some #defines for 1000 and 10000 please (VSC73XX_ARBITER_SLEEP_US,
VSC73XX_ARBITER_TIMEOUT_US)?

> +					vsc, VSC73XX_BLOCK_ARBITER, 0,
> +					VSC73XX_ARBEMPTY, &val);
> +		if (ret)
> +			dev_err(vsc->dev,
> +				"timeout waiting for block arbiter\n");
> +		else if (err < 0)
> +			dev_err(vsc->dev, "error reading arbiter\n");
>  
>  		/* Put this port into reset */
>  		vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
> -- 
> 2.34.1
>