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

[PATCH v3] 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>
---
 v3: use phy_modify()
 v2: resolved review warnings, including wol d.
 drivers/net/phy/marvell.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index f71cffa88406..56650182bf3e 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -55,6 +55,11 @@
 #define MII_M1011_IMASK			0x12
 #define MII_M1011_IMASK_INIT		0x6400
 #define MII_M1011_IMASK_CLEAR		0x0000
+/* 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_88E1318S_PHY_CSIER_WOL_EIE)
 
 #define MII_M1011_PHY_SCR			0x10
 #define MII_M1011_PHY_SCR_DOWNSHIFT_EN		BIT(11)
@@ -393,11 +398,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 +424,8 @@ 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_88E1318S_PHY_CSIER_WOL_EIE)))
 		return IRQ_NONE;
 
 	phy_trigger_machine(phydev);
@@ -2074,6 +2082,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_88E1318S_PHY_CSIER_WOL_EIE);
+		if (err < 0)
+			goto error;
+	}
+
 error:
 	return phy_restore_page(phydev, oldpage, err);
 }
-- 
2.56.0

Re: [PATCH v3] 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 v3] net: phy: marvell: keep WOL_EIE across interrupt reconfiguration

[Rosen Penev]

On Mon, Oct 5, 2026 at 3:55 PM <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.
This was tested on a Marvell Helios 4 device.
>
> 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 v3] net: phy: marvell: keep WOL_EIE across interrupt reconfiguration

[Andrew Lunn]

>  #define MII_M1011_IMASK			0x12
>  #define MII_M1011_IMASK_INIT		0x6400
>  #define MII_M1011_IMASK_CLEAR		0x0000
> +/* 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_88E1318S_PHY_CSIER_WOL_EIE)

I still don't like this. The name MII_88E1318S_PHY_CSIER_ suggests
this belongs to the MII_88E1318S_PHY_CSIER register.

> +		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);

But here you apply it to the MII_M1011_IMASK register.

> +	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_88E1318S_PHY_CSIER_WOL_EIE);

And here it much more obviously looks wrong. These prefixes are there
to catch dumb typos, and somebody is going to look at this, and think
it is a dumb typo and report it.

Please fix the naming.


    Andrew

---
pw-bot: cr

Re: [PATCH v3] 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 commit message says the interrupt register layout is the
  same across all PHYs this driver handles.
- [Medium] In the PHY_INTERRUPT_DISABLED branch, marvell_config_intr()
  now calls phy_modify(phydev, MII_M1011_IMASK,…

--

net: phy: marvell: keep WOL_EIE across interrupt reconfiguration

This patch makes marvell_config_intr() use phy_modify() with a mask that
leaves WOL_EIE alone. With that, the config_intr call from phy_init_hw() on
resume no longer disarms Wake-on-LAN on the 88E1318S and 88E1510. When WoL
is disabled, m88e1318_set_wol() now clears WOL_EIE itself, and the shared
marvell_handle_interrupt() also claims WoL events.

> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index f71cffa884062..56650182bf3e4 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -55,6 +55,11 @@
>  #define MII_M1011_IMASK			0x12
>  #define MII_M1011_IMASK_INIT		0x6400
>  #define MII_M1011_IMASK_CLEAR		0x0000
> +/* 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_88E1318S_PHY_CSIER_WOL_EIE)
>  
>  #define MII_M1011_PHY_SCR			0x10
>  #define MII_M1011_PHY_SCR_DOWNSHIFT_EN		BIT(11)
> @@ -393,11 +398,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]
When interrupts are disabled, this branch no longer clears WOL_EIE. Does it
still meet the config_intr contract in include/linux/phy.h?

  @config_intr: Enables or disables interrupts.
  It should also clear any pending interrupts prior to enabling the
  IRQs and after disabling them.

The commit message only covers the resume path through phy_init_hw().
Teardown also reaches this branch:

phy_disconnect()
  phy_free_interrupt()
    phy_disable_interrupts()
      marvell_config_intr()   /* WOL_EIE is kept */
    free_irq()

Take an 88E1318S or 88E1510 in interrupt mode with WoL armed by
m88e1318_set_wol(). WOL_EIE stays set, and LED[2] is still routed as INTn
through MII_88E1318S_PHY_LED_TCR_INTn_ENABLE. Nothing undoes either one on
teardown.

Suppose a magic packet (WAKE_MAGIC) or a link-up event (WAKE_PHY) arrives
after ifdown. Can the PHY then assert the active-low INTn after its handler
has been freed? Nothing would be left to read MII_M1011_IEVENT and release
the pin.

phy_request_interrupt() requests the line with IRQF_ONESHOT | IRQF_SHARED.
If another device shares the line, could the IRQ core end up disabling the
line for that device as unhandled?

Before this patch, the disabled branch wrote 0x0000, which cleared WOL_EIE.
Would it work to keep WOL_EIE only across the resume reconfiguration, or to
track the WoL state and re-apply it after config_intr, and still clear it
in the disable path?

>  		if (err)
>  			return err;
>  
> @@ -417,7 +424,8 @@ 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_88E1318S_PHY_CSIER_WOL_EIE)))
>  		return IRQ_NONE;
>  
>  	phy_trigger_machine(phydev);

[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.

Does that hold for bit 7? drivers/net/ethernet/marvell/skge.h describes the
same registers (PHY_MARV_INT_MASK = 0x12, PHY_MARV_INT_STAT = 0x13), but
there bit 7 is a FIFO error:

  PHY_M_IS_FIFO_ERROR	= 1<<7, /* FIFO Overflow/Underrun Error */

skge's default interrupt mask also enables this bit.

About 20 entries in marvell_drivers[] share marvell_config_intr() and
marvell_handle_interrupt(): 88E1101, 88E1112, 88E1111, 88E1111 (Finisar),
88E1118, 88E1121R, 88E1145, 88E1149R, 88E1240, 88E1116R, 88E1540/1545,
88E3016, 88E1340S, 88E1548P and the 88E6xxx switch PHYs. Of these, only
88E1318S and 88E1510 have .set_wol.

On the other PHYs, MII_M1011_IMASK_CONFIG_MASK means config_intr never
clears bit 7 of register 0x12. If a bootloader or a marvell,reg-init entry
left it set, does it stay armed even after phylib asks config_intr to
disable interrupts (polling mode, phy_probe(), phy_free_interrupt())?
Nothing else in the driver manages bit 7 on these PHYs.

marvell_handle_interrupt() now also returns IRQ_HANDLED and triggers the
state machine for IEVENT bit 7 on all of these PHYs. On the older parts,
that bit is a FIFO over/underrun event the driver never enabled. Since
phylib requests the IRQ with IRQF_SHARED, could this also hide spurious
interrupt detection?

Would it be better to limit the bit 7 handling to the 88E1318S and 88E1510?
That could be their own config_intr/handle_interrupt callbacks, or a
per-driver mask.

[ ... ]

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