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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YmsyKI5yIY055UqP@lunn.ch>
Date:   Fri, 29 Apr 2022 02:32:40 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Lukas Wunner <lukas@...ner.de>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: phy: Deduplicate interrupt disablement on
 PHY attach

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@...n.ch>

However the git history is interesting:

commit 7d3ba9360c6dac7c077fbd6631e08f32ea2bcd53
Author: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.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@...il.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