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

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

[Rosen Penev]

marvell_config_intr() rewrote the whole interrupt enable register, so
the config_intr call from phy_init_hw() on resume silently cleared
WOL_EIE on the 88E1318S and 88E1510: m88e1318_get_wol() still reported
WAKE_MAGIC, but a matched magic packet was no longer routed to INTn and
the board did not wake.

Make config_intr update every bit except WOL_EIE with phy_modify(), so
the WoL interrupt enable is owned only by set_wol and always matches
what it programmed. The read-modify-write runs under the MDIO bus lock,
as do the accesses in m88e1318_set_wol(), so the two can no longer
overwrite each other's update. Have m88e1318_set_wol() clear WOL_EIE
when WoL is fully disabled, since config_intr no longer does.

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.

Fixes: 3871c3876f80 ("mv643xx_eth with 88E1318S: support Wake on LAN")
Assisted-by: LLM
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v4: remove MII_88E1318S_PHY_CSIER_ defines.
 v3: use phy_modify()
 v2: resolved review warnings, including wol d.
 drivers/net/phy/marvell.c | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index f71cffa88406..5f9a43a4a063 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)
 
 #define MII_M1011_PHY_SCR			0x10
 #define MII_M1011_PHY_SCR_DOWNSHIFT_EN		BIT(11)
@@ -139,11 +145,6 @@
 #define MII_88E1318S_PHY_MSCR1_REG	16
 #define MII_88E1318S_PHY_MSCR1_PAD_ODD	BIT(6)
 
-/* Copper Specific Interrupt Enable Register */
-#define MII_88E1318S_PHY_CSIER				0x12
-/* WOL Event Interrupt Enable */
-#define MII_88E1318S_PHY_CSIER_WOL_EIE			BIT(7)
-
 #define MII_88E1318S_PHY_LED_FUNC		0x10
 #define MII_88E1318S_PHY_LED_FUNC_OFF		(0x8)
 #define MII_88E1318S_PHY_LED_FUNC_ON		(0x9)
@@ -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);
 		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;
 
 	phy_trigger_machine(phydev);
@@ -1991,8 +1994,8 @@ static int m88e1318_set_wol(struct phy_device *phydev,
 			__phy_read(phydev, MII_M1011_IEVENT);
 
 		/* Enable the WOL interrupt */
-		err = __phy_set_bits(phydev, MII_88E1318S_PHY_CSIER,
-				     MII_88E1318S_PHY_CSIER_WOL_EIE);
+		err = __phy_set_bits(phydev, MII_M1011_IMASK,
+				     MII_M1011_IMASK_WOL);
 		if (err < 0)
 			goto error;
 
@@ -2074,6 +2077,18 @@ static int m88e1318_set_wol(struct phy_device *phydev,
 			goto error;
 	}
 
+	if (!(wol->wolopts & (WAKE_MAGIC | WAKE_PHY))) {
+		err = marvell_write_page(phydev, MII_MARVELL_COPPER_PAGE);
+		if (err < 0)
+			goto error;
+
+		/* Disable the WOL interrupt, config_intr leaves it alone */
+		err = __phy_clear_bits(phydev, MII_M1011_IMASK,
+				       MII_M1011_IMASK_WOL);
+		if (err < 0)
+			goto error;
+	}
+
 error:
 	return phy_restore_page(phydev, oldpage, err);
 }
-- 
2.56.0

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Rosen Penev]

On Wed, Oct 7, 2026 at 11:24 AM <netdev-bot+sinfo@kernel.org> wrote:
>
> Hi!
>
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
>
>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.
Tested on Helios 4.
>
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
>
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

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

[Igor Velkov]

Tested-by: Igor Velkov <iav@iav.lv>

Helios4 (Armada 388, 88E1512), 7.2.9. With this patch WOL_EIE stays set
over 3 link down/up cycles, "wol d" clears it, and a magic packet on a
running board is a handled interrupt. Without it, WOL_EIE is lost on the
first down/up, survives "wol d", and the magic packet is an unhandled
interrupt.

--
Igor Velkov

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

[Jakub Kicinski]

On Wed, 7 Oct 2026 12:20:37 -0700 Rosen Penev wrote:
> >  - How the issue was discovered, e.g. hit in production, hit during
> >    development, syzbot report, manual code inspection, LLM or static
> >    analysis tool scan.  
> Tested on Helios 4.

The question is how you run into this, now what platform you tested 
it on.

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

[netdev-bot+sashiko]

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