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

[Florian Fainelli <f.fainelli@gmail.com>]

On 10/29/26 09:15, netdev-bot+sashiko@kernel.org wrote:
> 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.

Agreed on the completion polling. I’ll use readl_poll_timeout() for the 
new write completion wait, retaining the 20 ms timeout.

The upper-bit concern is pre-existing and speculative without hardware 
evidence, so I’ll leave that unchanged in this series.

pw-bot: cr
-- 
Florian