Re: [PATCH net v4] net: phy: marvell: keep WOL_EIE across interrupt reconfiguration

[netdev-bot+sashiko@kernel.org]

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

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

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] The patch applies 88E1318S/88E1510 bit-7 semantics to every
  Marvell PHY that shares…
- [Medium] marvell_config_intr() with phydev->interrupts ==
  PHY_INTERRUPT_DISABLED no longer clears WOL_EIE (reg 0x12 bit 7), so…

--

net: phy: marvell: keep WOL_EIE across interrupt reconfiguration

This commit switches marvell_config_intr() to phy_modify(), so bit 7
(WOL_EIE) of the interrupt enable register survives interrupt
reconfiguration and only m88e1318_set_wol() changes it. It also makes
m88e1318_set_wol() clear the bit when WoL is disabled, and extends the
shared marvell_handle_interrupt() to claim WoL events.

> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index f71cffa884062..5f9a43a4a0634 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -55,6 +55,12 @@
>  #define MII_M1011_IMASK			0x12
>  #define MII_M1011_IMASK_INIT		0x6400
>  #define MII_M1011_IMASK_CLEAR		0x0000
> +#define MII_M1011_IMASK_WOL		BIT(7)
> +/* Bits updated by config_intr. The WoL interrupt enable is owned by
> + * set_wol, so the config_intr call from phy_init_hw() on resume does not
> + * silently disarm Wake-on-LAN.
> + */
> +#define MII_M1011_IMASK_CONFIG_MASK	(U16_MAX & ~MII_M1011_IMASK_WOL)

[ ... ]

> @@ -393,11 +394,13 @@ static int marvell_config_intr(struct phy_device *phydev)
>  		if (err)
>  			return err;
>  
> -		err = phy_write(phydev, MII_M1011_IMASK,
> -				MII_M1011_IMASK_INIT);
> +		err = phy_modify(phydev, MII_M1011_IMASK,
> +				 MII_M1011_IMASK_CONFIG_MASK,
> +				 MII_M1011_IMASK_INIT);
>  	} else {
> -		err = phy_write(phydev, MII_M1011_IMASK,
> -				MII_M1011_IMASK_CLEAR);
> +		err = phy_modify(phydev, MII_M1011_IMASK,
> +				 MII_M1011_IMASK_CONFIG_MASK,
> +				 MII_M1011_IMASK_CLEAR);

[Severity: Medium]
The disable branch now keeps bit 7. Does phy_disable_interrupts() still
quiet the PHY when WoL is armed?

phy_free_interrupt() expects the PHY to be quiet before it removes the
handler:

drivers/net/phy/phy.c:phy_free_interrupt() {
	...
	phy_disable_interrupts(phydev);
	free_irq(phydev->irq, phydev);
	...
}

Consider an 88E1318S or 88E1510 that uses a PHY IRQ:

ethtool -s ethX wol g
  m88e1318_set_wol()
    sets MII_M1011_IMASK_WOL, routes LED[2] as active-low INTn

interface stopped, MAC driver calls phy_disconnect()
  phy_free_interrupt()
    marvell_config_intr()   /* bit 7 stays enabled */
    free_irq()

The PHY stays powered here, because phy_suspend() returns -EBUSY when
WoL is enabled.

A later magic packet or link-up event then latches IEVENT bit 7. The
comment in m88e1318_set_wol() says INTn stays asserted until IEVENT is
read, and after free_irq() nothing reads it.

phy_request_interrupt() requests the IRQ with IRQF_ONESHOT | IRQF_SHARED.
If that level-triggered line is shared with another device, such as a
sibling PHY, could this cause an interrupt storm until the spurious IRQ
detector disables the line for every device on it?

The same applies to phy_probe(), which calls phy_disable_interrupts()
before any handler is registered. That call used to write 0x0000 to
MII_M1011_IMASK, which cleared a WOL_EIE left by a previous boot or
kexec. Now the bit stays set.

After this patch, the new block in m88e1318_set_wol() is the only code
that clears the bit, and it only runs on an explicit wol d. Should the
teardown and probe paths still clear MII_M1011_IMASK_WOL?

>  		if (err)
>  			return err;
>  
> @@ -417,7 +420,7 @@ static irqreturn_t marvell_handle_interrupt(struct phy_device *phydev)
>  		return IRQ_NONE;
>  	}
>  
> -	if (!(irq_status & MII_M1011_IMASK_INIT))
> +	if (!(irq_status & (MII_M1011_IMASK_INIT | MII_M1011_IMASK_WOL)))
>  		return IRQ_NONE;

[Severity: Medium]
The commit message says:

  The interrupt register layout is the same across the PHYs this driver
  handles, so extend the shared handler to also claim a WoL event instead
  of adding a per-PHY one.

Is this true for bit 7?

About 21 entries in marvell_drivers[] use marvell_config_intr() and
marvell_handle_interrupt(). These include 88E1101, 88E1111, 88E1118,
88E1121R, 88E1145, 88E1240, 88E1540, 88E3016 and the 88E6xxx families.
Only the 88E1318S and 88E1510 entries have .set_wol = m88e1318_set_wol.

The in-tree Yukon definitions for 88E1111-class PHYs use bit 7 of this
register for a FIFO error, not a WoL event:

drivers/net/ethernet/marvell/skge.h:
	PHY_M_IS_FIFO_ERROR	= 1<<7, /* FIFO Overflow/Underrun Error */

sky2.h has the same definition.

On the PHYs without set_wol, two things change.

First, MII_M1011_IMASK_CONFIG_MASK leaves bit 7 out of every config_intr
write. A bit 7 enable left by firmware, a bootloader or a previous
kernel is never cleared, even by config_intr() with
PHY_INTERRUPT_DISABLED.

Second, the handler now returns IRQ_HANDLED and calls
phy_trigger_machine() whenever IEVENT bit 7 is set. This happens even on
models where the driver never enabled that source.

Could the bit 7 handling be limited to the PHYs that support WoL, for
example with separate config_intr/handle_interrupt callbacks for the
88E1318S and 88E1510? If the shared change is intended, could the commit
message say that PHYs without WoL change behaviour too?

[ ... ]

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