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: <1721908413.470634.1671548576554.JavaMail.zimbra@savoirfairelinux.com>
Date:   Tue, 20 Dec 2022 10:02:56 -0500 (EST)
From:   Enguerrand de Ribaucourt 
        <enguerrand.de-ribaucourt@...oirfairelinux.com>
To:     Heiner Kallweit <hkallweit1@...il.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>
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>,
> "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?

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