Re: [PATCH net v5 1/4] net: phy: move irq_suspended and irq_rerun out of the bitfield

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch adds the comment "/* Written without phydev->lock, so
  not bitfields */" above irq_suspended and irq_rerun in struct…

--

net: phy: move irq_suspended and irq_rerun out of the bitfield

This commit turns irq_suspended and irq_rerun in struct phy_device from
1-bit bitfields into bools. phy_interrupt() sets irq_rerun without
phydev->lock, and that write could lose an update to link and
autoneg_complete, which share the same word. After this change the two
flags are separate memory locations.

> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 21afc2082b174..bbe28ff97e51c 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -726,8 +726,9 @@ struct phy_device {
>  
>  	/* Interrupts are enabled */
>  	unsigned interrupts:1;
> -	unsigned irq_suspended:1;
> -	unsigned irq_rerun:1;
> +	/* Written without phydev->lock, so not bitfields */

[Severity: Low]
This isn't a bug, but the new comment states a rule that the
neighbouring fields don't follow. Several of them are written without
phydev->lock. They are still 1-bit bitfields in the same run as link
and autoneg_complete, and the state machine writes those two under the
lock.

interrupts, the field just above, is written in phy_config_interrupt():

drivers/net/phy/phy.c:phy_config_interrupt() {
    ...
	phydev->interrupts = interrupts ? 1 : 0;
    ...
}

This is reached from phy_request_interrupt()/phy_free_interrupt(). Some
PHY drivers also set it in their suspend/resume paths (dp83867, micrel,
marvell-88q2xxx).

phy_suspend() writes wol_enabled and suspended:

drivers/net/phy/phy_device.c:phy_suspend() {
    ...
	phydev->wol_enabled = phy_may_wakeup(phydev) ||
    ...
		phydev->suspended = true;
    ...
}

mdio_bus_phy_suspend() and mdio_bus_phy_resume() write
suspended_by_mdio_bus:

	phydev->suspended_by_mdio_bus = 1;

The commit message itself names these unlocked writers of the same word.

Later in the series, "net: phy: postpone PHY interrupts during sleep
with MAC-managed PM" adds another bitfield to this word, wol_phy:1.
phy_suspend() writes it without phydev->lock, and phy_irq_wake_source()
reads it.

Could the comment say that these two fields are split out because
phy_interrupt() writes them from the IRQ thread? As written, a reader
could assume the remaining bitfields are all lock-protected.

If any of those unlocked writers can run at the same time as each
other, or as the locked link/autoneg_complete writers, the lost-update
problem this commit describes would still apply to them. I haven't
shown that concurrency here.

> +	bool irq_suspended;
> +	bool irq_rerun;
>  
>  	unsigned default_timestamp:1;
>

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