Re: [PATCH net 04/11] net: au1000: report MDIO transaction timeouts

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new completion poll in au1000_mdio_write() reads MAC_MII_BUSY
  right after `writel(mii_control, mii_control_reg)`.

Pre-existing issues:
- [Low] On success, au1000_mdio_read() returns the raw 32-bit
  MAC_MII_DATA value (`return readl(mii_data_reg);`, au1000_eth.c:298)…

--

net: au1000: report MDIO transaction timeouts

This commit changes au1000_mdio_write() to return an int. Timeouts from
the busy wait before the transaction, and from a new completion poll after
the write, now go back to phylib through au1000_mdiobus_write(). The read
timeout paths in au1000_mdio_read() now return -ETIMEDOUT instead of -1.

> diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
> index 70319b432182f..a507a524370fd 100644
> --- a/drivers/net/ethernet/amd/au1000_eth.c
> +++ b/drivers/net/ethernet/amd/au1000_eth.c

[ ... ]

> @@ -292,14 +292,14 @@ static int au1000_mdio_read(struct net_device *dev, int phy_addr, int reg)
>  		mdelay(1);
>  		if (--timedout == 0) {
>  			netdev_err(dev, "mdio_read busy timeout!!\n");
> -			return -1;
> +			return -ETIMEDOUT;
>  		}
>  	}
>  	return readl(mii_data_reg);

[Severity: Low]
This isn't a bug introduced by this patch, but should au1000_mdio_read()
mask the data register value to 16 bits before returning it?

On success the raw 32-bit MAC_MII_DATA value goes back through
au1000_mdiobus_read() to phylib. __mdiobus_read() expects either a
register value or a negative errno, and it passes the value through
without masking:

drivers/net/phy/mdio_bus.c:__mdiobus_read() {
    ...
	if (bus->read)
		retval = bus->read(bus, addr, regnum);
    ...
}

If bits 31:16 of mii_data ever read back non-zero, phylib would get a
value wider than 16 bits. If bit 31 were set, it would read as an error.

Nothing in the tree shows whether those upper bits are ever non-zero on
this hardware. Would something like readl(mii_data_reg) & 0xffff be
safer, now that negative values on this path are proper errnos?

>  }

[ ... ]

> @@ -320,6 +320,17 @@ static void au1000_mdio_write(struct net_device *dev, int phy_addr,
>  
>  	writel(value, mii_data_reg);
>  	writel(mii_control, mii_control_reg);
> +
> +	timedout = 20;
> +	while (readl(mii_control_reg) & MAC_MII_BUSY) {
> +		mdelay(1);

[Severity: Low]
With this new completion poll, will every MDIO write now busy-wait for at
least 1 ms?

A clause 22 management frame is about 64 MDC cycles, which is roughly
26 us at 2.5 MHz. So the first readl() right after
writel(mii_control, mii_control_reg) will almost always see MAC_MII_BUSY
set, and mdelay(1) will run at least once.

The mii_bus->write callback runs in process context under
bus->mdio_lock, which is a mutex. Could this use readl_poll_timeout()
with a sleep of a few tens of microseconds and the same ~20 ms timeout?

To be fair, before this patch the access that followed a write already
paid this mdelay(1) in its own BUSY wait before starting. So for
back-to-back sequences the cost mostly moves into the write. The new cost
falls on the last write of a burst and on isolated writes. The existing
completion poll in au1000_mdio_read() has the same 1 ms granularity.

> +		if (--timedout == 0) {
> +			netdev_err(dev, "mdio_write busy timeout!!\n");
> +			return -ETIMEDOUT;
> +		}
> +	}
> +
> +	return 0;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027223140.3774570-1-f.fainelli%40gmail.com