Re: [PATCH net 10/11] net: au1000: keep the MAC stopped until the PHY link is ready

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] au1000_open() now calls phy_start() before au1000_init().
- [Medium] Reopening the interface after a PHY status read that changed
  phydev->link and then failed can leave MAC RX/TX disabled for good…
- [Low] The new au1000_hard_stop() call in the link-down branch of
  au1000_adjust_link() runs inside spin_lock_irqsave(&aup->lock).

--

net: au1000: keep the MAC stopped until the PHY link is ready

The patch moves phy_start() ahead of au1000_init() in au1000_open(). It
also leaves MAC RX/TX disabled in au1000_init() while the PHY reports no
link, and stops the MAC in the link-down branch of au1000_adjust_link().
If au1000_init() fails, the PHY is now stopped.

[Severity: Low]

au1000_adjust_link() holds spin_lock_irqsave(&aup->lock) at this point,
and au1000_hard_stop() ends with:

	wmb(); /* drain writebuffer */
	mdelay(10);

So each link-down now busy-waits 10 ms with interrupts off.

On close, the sequence looks like this:

au1000_close()
  phy_stop()
    -> au1000_adjust_link() -> au1000_hard_stop()   /* 10 ms */
  spin_lock_irqsave(&aup->lock, flags)
  au1000_reset_mac_unlocked()
    au1000_hard_stop()                              /* 10 ms again */
    two mdelay(2) for the reset                     /* 4 ms */

That is roughly 24 ms per close with interrupts off. The duplex branch
already does something similar on link-up.

Is the second au1000_hard_stop() in au1000_reset_mac_unlocked() still
needed now that the link-down callback stops the MAC?

 			aup->old_speed = 0;
 			aup->old_duplex = -1;
 		}

[ ... ]

[Severity: Medium]

Can the MAC stay stopped after a reopen if old_link and old_duplex are
stale?

With this change, RX/TX get turned back on after open only through
au1000_adjust_link(). It calls au1000_enable_rx_tx() in just one place,
the duplex branch:

	if (phydev->link && (aup->old_duplex != phydev->duplex)) {
		...
		au1000_enable_rx_tx(dev);

Neither au1000_close() nor au1000_reset_mac_unlocked() resets old_link,
old_speed or old_duplex.

Here is one sequence that could leave them stale. Assume autoneg is
disabled and the link is up at 100/Full:

genphy_read_status()
  genphy_update_link()        /* BMSR says link down, phydev->link = 0 */
  genphy_read_status_fixed()  /* BMCR read fails with -ETIMEDOUT */
phy_check_link_status()
  returns the error before phy_link_down()

MDIO timeouts reach phylib since "net: au1000: report MDIO transaction
timeouts". The PHY_ERROR handling, and a later phy_stop() into
PHY_HALTED, call phy_link_down() only if phydev->link is still set.
au1000_adjust_link() therefore never sees the link drop, so old_link
stays 1 and old_duplex stays DUPLEX_FULL.

On the next open, au1000_init() leaves RX/TX disabled because
phydev->link is 0. When the link comes back at the same speed and
duplex, au1000_adjust_link() skips all three branches. The MAC stays
stopped while carrier is reported up.

Before this patch, au1000_init() always set MAC_RX_ENABLE |
MAC_TX_ENABLE, so this case recovered on reopen. Would resetting
old_link, old_speed and old_duplex in the open or close path avoid it?

[Severity: Medium]

Is it safe to start the PHY before the MAC is initialized?
Documentation/networking/phy.rst says:

  Lastly, once the controller is ready to handle network traffic, you call
  phy_start(phydev).

The commit message says "Start the PHY before initializing the MAC" but
doesn't explain why the order has to change.

phy_start() resumes the PHY over MDIO:

au1000_mdiobus_read() / au1000_mdiobus_write()
  au1000_enable_mac(dev, 0)

mac_enabled is 0 after probe or close, so this takes the MAC out of reset
before au1000_init() runs. That contradicts the comment above
au1000_init():

 * conserve power. Thus, whenever au1000_init() is called,
 * the device should already be in reset state.

phy_start() then queues the state machine with zero delay, and
au1000_init() does:

	/* bring the device out of reset */
	au1000_enable_mac(dev, 1);

With force_reset set, this holds the MAC block in reset for about 2 ms,
including mii_control and mii_data. It takes only aup->lock, not the
mii_bus lock. au1000_mdio_read() and au1000_mdio_write() program
mii_control, poll with mdelay(1) and read mii_data without any driver
lock.

Could that forced reset land in the middle of an MDIO transaction from
the phylib worker, for example on a preemptible UP kernel? A read could
then return a reset value, a BMCR or advertisement write could be lost,
or -ETIMEDOUT could put the PHY into PHY_ERROR.

If the worker runs first and already sees link up, au1000_adjust_link()
takes the duplex branch (old_duplex == -1) and calls
au1000_enable_rx_tx(). That happens before the MAC address and RX
descriptors are programmed. au1000_init() then force-resets a MAC with
RX/TX enabled.

There is also a problem with the new check in au1000_init(). It reads
phydev->link under aup->lock, while phylib updates it under
phydev->lock:

	if (!dev->phydev || dev->phydev->link)
		control |= MAC_RX_ENABLE | MAC_TX_ENABLE;

genphy_update_link() can set link to 1 for a moment and then clear it:

	phydev->link = status & BMSR_LSTATUS ? 1 : 0;
	...
	if (phydev->autoneg == AUTONEG_ENABLE && !phydev->autoneg_complete)
		phydev->link = 0;

If au1000_init() reads that brief 1, RX/TX get enabled. The link-down
callback never runs afterwards, because phydev->link still equals
old_link.

Wouldn't that leave the MAC running with no link, which is the state
this patch is meant to prevent?

[ ... ]

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