netdev - Re: [PATCH v3 1/3] net: phy: add EXPORT_SYMBOL to phy_disable

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: <1567686748.473254.1671551305632.JavaMail.zimbra@savoirfairelinux.com>
Date:   Tue, 20 Dec 2022 10:48:25 -0500 (EST)
From:   Enguerrand de Ribaucourt 
        <enguerrand.de-ribaucourt@...oirfairelinux.com>
To:     Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>
Cc:     netdev <netdev@...r.kernel.org>, Paolo Abeni <pabeni@...hat.com>,
        woojung huh <woojung.huh@...rochip.com>,
        davem <davem@...emloft.net>,
        UNGLinuxDriver <UNGLinuxDriver@...rochip.com>,
        Russell King - ARM Linux <linux@...linux.org.uk>
Subject: Re: [PATCH v3 1/3] net: phy: add EXPORT_SYMBOL to
 phy_disable_interrupts()

> From: "Heiner Kallweit" <hkallweit1@...il.com>
> To: "Enguerrand de Ribaucourt" <enguerrand.de-ribaucourt@...oirfairelinux.com>
> Cc: "netdev" <netdev@...r.kernel.org>, "Paolo Abeni" <pabeni@...hat.com>, "woojung huh" <woojung.huh@...rochip.com>,
> "davem" <davem@...emloft.net>, "UNGLinuxDriver" <UNGLinuxDriver@...rochip.com>, "Andrew Lunn" <andrew@...n.ch>,
> "Russell King - ARM Linux" <linux@...linux.org.uk>
> Sent: Tuesday, December 20, 2022 4:19:40 PM
> Subject: Re: [PATCH v3 1/3] net: phy: add EXPORT_SYMBOL to phy_disable_interrupts()

> On 20.12.2022 16:02, Enguerrand de Ribaucourt wrote:
> >> From: "Heiner Kallweit" <hkallweit1@...il.com>
> >> To: "Enguerrand de Ribaucourt" <enguerrand.de-ribaucourt@...oirfairelinux.com>,
> >> "netdev" <netdev@...r.kernel.org>
> >> Cc: "Paolo Abeni" <pabeni@...hat.com>, "woojung huh"
> >> <woojung.huh@...rochip.com>, "davem" <davem@...emloft.net>, "UNGLinuxDriver"
> >> <UNGLinuxDriver@...rochip.com>, "Andrew Lunn" <andrew@...n.ch>, "Russell King -
> >> ARM Linux" <linux@...linux.org.uk>
> >> Sent: Tuesday, December 20, 2022 3:40:15 PM
> >> Subject: Re: [PATCH v3 1/3] net: phy: add EXPORT_SYMBOL to
> >> phy_disable_interrupts()

> >> On 20.12.2022 14:19, Enguerrand de Ribaucourt wrote:
> >>> It seems EXPORT_SYMBOL was forgotten when phy_disable_interrupts() was
> >>> made non static. For consistency with the other exported functions in
> >>> this file, EXPORT_SYMBOL should be used.

> >> No, it wasn't forgotten. It's intentional. The function is supposed to
> >> be used within phylib only.

> >> None of the phylib maintainers was on the addressee list of your patch.
> >> Seems you didn't check with get_maintainers.pl.

> >> You should explain your use case to the phylib maintainers. Maybe lan78xx
> >> uses phylib in a wrong way, maybe an extension to phylib is needed.
> >> Best start with explaining why lan78xx_link_status_change() needs to
> >> fiddle with the PHY interrupt. It would help be helpful to understand
> >> what "chip" refers to in the comment. The MAC, or the PHY?
> >> Does the lan78xx code assume that a specific PHY is used, and the
> >> functionality would actually belong to the respective PHY driver?

> > Thank you for your swift reply,

> > The requirement to toggle the PHY interrupt in lan78xx_link_status_change() (the
> > LAN7801 MAC driver) comes from a workaround by the original author which resets
> > the fixed speed in the PHY when the Ethernet cable is swapped. According to his
> > message, the link could not be correctly setup without this workaround.

> > Unfortunately, I don't have the cables to test the code without the workaround
> > and it's description doesn't explain what problem happens more precisely.

> > The PHY the original author used is a LAN8835. The workaround code directly
> > modified the interrupt configuration registers of this LAN8835 PHY within
> > lan78xx_link_status_change(). This caused problems if a different PHY was used
> > because the register at this address did not correspond to the interrupts
> > configuration. As suggested by the lan78xx.c maintainer, a generic function
> > should be used instead to toggle the interrupts of the PHY. However, it seems
> > that maybe the MAC driver shouldn't meddle with the PHY's interrupts according
> > to you. Would you consider this use case a valid one?

> So this workaround works around a silicon bug in LAN8835?
> Then the code supposedly should go to the link_change_notify handler of the
> Microchip PHY driver for LAN8835.
> There's just a generic PHY driver for LAN88xx. Would be helpful to know
> which Microchip PHY's are affected.

link_change_notify() seems very promising indeed!

My proposed approach would be to copy the original workaround actions
within link_change_notify():
 1. disable interrupts
 2. reset speed
 3. enable interrupts

However, I don't have access to the LAN8835 to test if this would work. I also
don't have knowledge about which other Microchip PHYs could be impacted. Maybe
there is an active Microchip developer we could communicate with to find out?

Either way, it now seems clear that the LAN8835 interrupt code should be
removed from lan78xx.c.

> > Enguerrand

> >>> Fixes: 3dd4ef1bdbac ("net: phy: make phy_disable_interrupts() non-static")
> >>> Signed-off-by: Enguerrand de Ribaucourt
> >>> <enguerrand.de-ribaucourt@...oirfairelinux.com>
> >>> ---
> >>> drivers/net/phy/phy.c | 1 +
> >>> 1 file changed, 1 insertion(+)

> >>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> >>> index e5b6cb1a77f9..33250da76466 100644
> >>> --- a/drivers/net/phy/phy.c
> >>> +++ b/drivers/net/phy/phy.c
> >>> @@ -992,6 +992,7 @@ int phy_disable_interrupts(struct phy_device *phydev)
> >>> /* Disable PHY interrupts */
> >>> return phy_config_interrupt(phydev, PHY_INTERRUPT_DISABLED);
> >>> }
> >>> +EXPORT_SYMBOL(phy_disable_interrupts);

> >>> /**
> >>> * phy_interrupt - PHY interrupt handler