Re: [PATCH net v5 2/4] net: phy: keep the line of a postponed PHY interrupt balanced

[netdev-bot+sashiko@kernel.org]

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] phy_free_interrupt() (drivers/net/phy/phy.c) can re-enable a
  shared line that the PHY may still be asserting.
- [Medium] phy_free_interrupt() gives back the depth that the postponed
  disable_irq_nosync() took.
- [Low] In phy_free_interrupt() (drivers/net/phy/phy.c), the check
  irq_has_action(phydev->irq) and the following enable_irq(phydev->irq)…

--

net: phy: keep the line of a postponed PHY interrupt balanced

This patch makes only the phy_interrupt() run that sets irq_rerun disable
the line. phy_free_interrupt() now consumes irq_rerun after free_irq() and
calls enable_irq() when other actions remain on the line. It also adds
irq_pm_lock to serialize phy_free_interrupt() against the rerun in
mdio_bus_phy_resume_irq().

> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 9af303d60bd6b..ea22ebb14a832 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c

[ ... ]

> @@ -1507,7 +1510,18 @@ EXPORT_SYMBOL(phy_request_interrupt);
>  void phy_free_interrupt(struct phy_device *phydev)
>  {
>  	phy_disable_interrupts(phydev);
> +
> +	mutex_lock(&phydev->irq_pm_lock);
>  	free_irq(phydev->irq, phydev);
> +	/* Give the depth a postponed interrupt took back to the other users of
> +	 * a shared line; requesting a line we were alone on resets its depth.
> +	 */
> +	if (phydev->irq_rerun) {
> +		phydev->irq_rerun = false;
> +		if (irq_has_action(phydev->irq))
> +			enable_irq(phydev->irq);

[Severity: Medium]
Can this unmask a shared line while the PHY is still asserting it?

phy_free_interrupt() ignores the return value of phy_disable_interrupts().
Also, a postponed interrupt never acknowledged the PHY status: the
postponed path in phy_interrupt() returns IRQ_HANDLED without calling the
driver's handle_interrupt().

If the MDIO write in phy_disable_interrupts() fails, the PHY interrupt
mask and latched status stay set. On a level-triggered shared line,
enable_irq() then unmasks the line with no PHY handler installed.

If none of the remaining handlers claims the interrupt, would
note_interrupt() eventually disable the line with IRQS_SPURIOUS_DISABLED
for every remaining user?

That failure looks reachable with cpsw_new once the later patch "net: phy:
postpone PHY interrupts during sleep with MAC-managed PM" is applied. The
davinci_mdio controller is a child created by devm_of_platform_populate(),
so it suspends first, and davinci_mdio_suspend() calls
pm_runtime_force_suspend(). Then:

cpsw_suspend()
  cpsw_ndo_stop()
    phy_disconnect()
      phy_free_interrupt()
        phy_disable_interrupts()
          davinci_mdio_write()
            pm_runtime_resume_and_get()  <- fails, runtime PM disabled

Before this patch, the line stayed disabled until mdio_bus_phy_resume(),
which runs after the MAC reopens and installs the PHY handler again. With
this patch, the line is unmasked from the close until
suspend_device_irqs(). It is unmasked again from resume_device_irqs()
until the MAC reopens. Nothing clears the source in either window.

The commit message says:

    Re-enabling after free_irq(), not
    before, leaves no PHY handler to run again on a source that
    phy_disable_interrupts() did not clear.

That covers the PHY handler, but what about the other sharers on the
line? Should the enable_irq() be skipped when phy_disable_interrupts()
fails?

[Severity: Medium]
Is this enable_irq() still balanced if phy_free_interrupt() runs inside
the noirq window on a wake-armed line?

The commit message names stmmac_reset_subtask(), whose workqueue does not
freeze, as a path into phy_free_interrupt() during the sleep transition.
stmmac_suspend() does not set STMMAC_DOWN, so the subtask can run.

bcm_phy_set_wol() calls enable_irq_wake(phydev->irq). Several irqchips set
IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND: gpio-mxc, gpio-vf610, pinctrl-msm,
qcom-pdc, pinctrl-amd and pinctrl-starfive-jhb100.

Consider this sequence, with another action still on the line:

phy_interrupt()
  disable_irq_nosync()           depth 0 -> 1

suspend_device_irq()
  __enable_irq()                 depth 1 -> 0
  irqd_set(irqd, IRQD_IRQ_ENABLED_ON_SUSPEND)

phy_free_interrupt()
  irq_has_action() is true
  enable_irq()
    __enable_irq() case 0        WARN "Unbalanced enable for IRQ"

resume_irq()
  __disable_irq()                depth 0 -> 1

The line is not IRQS_SUSPENDED, so nothing enables it again on resume.
irq_rerun has already been consumed, so mdio_bus_phy_resume_irq() does
nothing either. A shared re-request from dev_open() does not touch the
depth.

Would the line then stay disabled after resume for every user, including
the reopened PHY? Before this patch, irq_rerun stayed set until
mdio_bus_phy_resume(), which ran after resume_irq() had restored depth 1.

[Severity: Low]
Can the irq_has_action() check race with another driver requesting the
same line?

By this point free_irq() has already dropped desc->request_mutex.
irq_has_action() only returns a snapshot. irq_pm_lock is per-phydev, so
other drivers don't take it.

Suppose the PHY was the last user and a postponed interrupt left depth
at 1:

free_irq()
  irq_shutdown()                 depth 1 -> 2

                                 other driver: request_irq()
                                   __setup_irq()
                                     irq_startup()   depth = 0

irq_has_action() returns true
enable_irq()
  __enable_irq() case 0          WARN "Unbalanced enable for IRQ"

If the new requester used IRQ_NOAUTOEN, irq_startup() resets the depth to
1 instead. This enable_irq() would then start a line that driver asked to
keep disabled.

> +	}
> +	mutex_unlock(&phydev->irq_pm_lock);
>  }
>  EXPORT_SYMBOL(phy_free_interrupt);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-b4-phylib-mac-pm-irq-v3-v5-0-bbfdb5afb7d3%40iav.lv