netdev - Re: [PATCH v3 2/2] net: phy: call phy_disable_interrupts() in phy_init

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20200624093923.GX1551@shell.armlinux.org.uk>
Date:   Wed, 24 Jun 2020 10:39:23 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Jisheng Zhang <Jisheng.Zhang@...aptics.com>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] net: phy: call phy_disable_interrupts() in
 phy_init_hw()

On Tue, Jun 23, 2020 at 08:36:46PM -0700, Florian Fainelli wrote:
> Le 2020-06-23 à 20:26, Jisheng Zhang a écrit :
> > Call phy_disable_interrupts() in phy_init_hw() to "have a defined init
> > state as we don't know in which state the PHY is if the PHY driver is
> > loaded. We shouldn't assume that it's the chip power-on defaults, BIOS
> > or boot loader could have changed this. Or in case of dual-boot
> > systems the other OS could leave the PHY in whatever state." as pointed
> > out by Heiner.
> > 
> > Suggested-by: Heiner Kallweit <hkallweit1@...il.com>
> > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@...aptics.com>
> > ---
> >  drivers/net/phy/phy_device.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index 04946de74fa0..f17d397ba689 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -1090,10 +1090,13 @@ int phy_init_hw(struct phy_device *phydev)
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	if (phydev->drv->config_init)
> > +	if (phydev->drv->config_init) {
> >  		ret = phydev->drv->config_init(phydev);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> >  
> > -	return ret;
> > +	return phy_disable_interrupts(phydev);
> 
> Not sure if the order makes sense here, it may seem more natural for a
> driver writer to have interrupts disabled first and then config_init
> called (which could enable interrupts not related to link management
> like thermal events etc.)

If this is a shared interrupt, and the PHY interrupt has been left
enabled, wouldn't we want to ensure that the PHY interrupt is disabled
as early as possible - in other words, when the device is probed,
rather than when the PHY is eventually bound to the network device.

However, I must point out that even disabling the PHY interrupt at
probe time is only reducing the window for problems - if that shared
interrupt has already been claimed by other users, and if the PHY
interrupt is unmasked in the PHY, then the PHY can do whatever it
likes with the shared interrupt before we even get to probe the PHY.
So the only real solution to this is to fix the environment that is
passing control to the kernel.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!