Re: [PATCH net-next v4 01/15] net: dsa: vsc73xx: use read_poll_timeout instead delay loop

Vladimir Oltean
2024-02-14T23:46:22Z

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
>