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: <1201721565.475609.1671553321565.JavaMail.zimbra@savoirfairelinux.com>
Date:   Tue, 20 Dec 2022 11:22:01 -0500 (EST)
From:   Enguerrand de Ribaucourt 
        <enguerrand.de-ribaucourt@...oirfairelinux.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        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>
Subject: Re: [PATCH v3 1/3] net: phy: add EXPORT_SYMBOL to
 phy_disable_interrupts()

----- Original Message -----
> From: "Russell King (Oracle)" <linux@...linux.org.uk>
> To: "Enguerrand de Ribaucourt" <enguerrand.de-ribaucourt@...oirfairelinux.com>
> Cc: "Heiner Kallweit" <hkallweit1@...il.com>, "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>
> Sent: Tuesday, December 20, 2022 5:13:31 PM
> Subject: Re: [PATCH v3 1/3] net: phy: add EXPORT_SYMBOL to phy_disable_interrupts()

> Hi,

> On Tue, Dec 20, 2022 at 10:02:56AM -0500, 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?

> This sounds to me like you're just trying to get a workaround merged
> upstream using a different approach, but you can't actually test to
> see whether it does work or not. Would that be a fair assessment?

> Do you know anyone who would be able to test? If not, I would suggest
> not trying to upstream code for a workaround that can't be tested as
> working.

Hi,

I definitely agree with you that the workaround code can't be moved from
lan78xx.c to microchip.c without testing. Unfortunately, I don't know
who to contact to test on the hardware.

In the meantime, I suggest removing the workaround from lan78xx.c since
it is not compatible with most of the PHYs users of lan78xx.c could use.
Some previous messages on the mailing list attests to it.
My Patch v1 could also do it, since it keeps the workaround but limits
it with the phy_id.

Regards,
Enguerrand

> Thanks.

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