| Message ID | 805ccdc606bd8898d59931bd4c7c68537ed6e550.1651040826.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 07caad0bb1f8963e6e99b665bc17842d93762469 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: Deduplicate interrupt disablement on PHY attach | expand |
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
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!
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);
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(-)