[PATCH net-next] net: phy: Deduplicate interrupt disablement on PHY attach

[PATCH net-next] net: phy: Deduplicate interrupt disablement on PHY attach

[Lukas Wunner]

phy_attach_direct() first calls phy_init_hw() (which restores interrupt
settings through ->config_intr()), then calls phy_disable_interrupts().

So if phydev->interrupts was previously set to 1, interrupts are briefly
enabled, then disabled, which seems nonsensical.

If it was previously set to 0, interrupts are disabled twice, which is
equally nonsensical.

Deduplicate interrupt disablement.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/net/phy/phy_device.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8406ac739def..f867042b2eb4 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1449,6 +1449,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 	phydev->state = PHY_READY;
 
+	phydev->interrupts = PHY_INTERRUPT_DISABLED;
+
 	/* Port is set to PORT_TP by default and the actual PHY driver will set
 	 * it to different value depending on the PHY configuration. If we have
 	 * the generic PHY driver we can't figure it out, thus set the old
@@ -1471,10 +1473,6 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	if (err)
 		goto error;
 
-	err = phy_disable_interrupts(phydev);
-	if (err)
-		return err;
-
 	phy_resume(phydev);
 	phy_led_triggers_register(phydev);
 
-- 
2.35.2

Re: [PATCH net-next] net: phy: Deduplicate interrupt disablement on PHY attach

[Andrew Lunn]

On Wed, Apr 27, 2022 at 08:30:51AM +0200, Lukas Wunner wrote:
> phy_attach_direct() first calls phy_init_hw() (which restores interrupt
> settings through ->config_intr()), then calls phy_disable_interrupts().
> 
> So if phydev->interrupts was previously set to 1, interrupts are briefly
> enabled, then disabled, which seems nonsensical.
> 
> If it was previously set to 0, interrupts are disabled twice, which is
> equally nonsensical.

I agree this is non nonsensical.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

However the git history is interesting:

commit 7d3ba9360c6dac7c077fbd6631e08f32ea2bcd53
Author: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Date:   Wed Sep 9 14:43:14 2020 +0900

    net: phy: call phy_disable_interrupts() in phy_attach_direct() instead
    
    Since the micrel phy driver calls phy_init_hw() as a workaround,
    the commit 9886a4dbd2aa ("net: phy: call phy_disable_interrupts()
    in phy_init_hw()") disables the interrupt unexpectedly. So,
    call phy_disable_interrupts() in phy_attach_direct() instead.
    Otherwise, the phy cannot link up after the ethernet cable was
    disconnected.
    
    Note that other drivers (like at803x.c) also calls phy_init_hw().
    So, perhaps, the driver caused a similar issue too.
    
This removes the call to phy_disable_interrupts() in phy_init_hw()
because it breaks some micrel PHYs.

And then:

ommit 4c0d2e96ba055bd8911bb8287def4f8ebbad15b6
Author: Heiner Kallweit <hkallweit1@gmail.com>
Date:   Thu Feb 11 22:32:52 2021 +0100

    net: phy: consider that suspend2ram may cut off PHY power
    
    Claudiu reported that on his system S2R cuts off power to the PHY and
    after resuming certain PHY settings are lost. The PM folks confirmed
    that cutting off power to selected components in S2R is a valid case.
    Therefore resuming from S2R, same as from hibernation, has to assume
    that the PHY has power-on defaults. As a consequence use the restore
    callback also as resume callback.
    In addition make sure that the interrupt configuration is restored.
    Let's do this in phy_init_hw() and ensure that after this call
    actual interrupt configuration is in sync with phydev->interrupts.
    Currently, if interrupt was enabled before hibernation, we would
    resume with interrupt disabled because that's the power-on default.
    
    This fix applies cleanly only after the commit marked as fixed.
    
    I don't have an affected system, therefore change is compile-tested
    only.

This puts the interrupt handling back into phy_init_hw()!

So it seems like something might be broken here, but your patch should
not make it worse.

    Andrew

Re: [PATCH net-next] net: phy: Deduplicate interrupt disablement on PHY attach

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 27 Apr 2022 08:30:51 +0200 you wrote:
> phy_attach_direct() first calls phy_init_hw() (which restores interrupt
> settings through ->config_intr()), then calls phy_disable_interrupts().
> 
> So if phydev->interrupts was previously set to 1, interrupts are briefly
> enabled, then disabled, which seems nonsensical.
> 
> If it was previously set to 0, interrupts are disabled twice, which is
> equally nonsensical.
> 
> [...]

Here is the summary with links:
  - [net-next] net: phy: Deduplicate interrupt disablement on PHY attach
    https://git.kernel.org/netdev/net-next/c/07caad0bb1f8

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html