[v3,1/3] net: phy: add EXPORT_SYMBOL to phy_disable_interrupts()

Commit Message

Enguerrand de Ribaucourt Dec. 20, 2022, 1:19 p.m. UTC

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.

Fixes: 3dd4ef1bdbac ("net: phy: make phy_disable_interrupts() non-static")
Signed-off-by: Enguerrand de Ribaucourt <[e-mail]>
---
 drivers/net/phy/phy.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Heiner Kallweit Dec. 20, 2022, 2:40 p.m. UTC | #1

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?

> Fixes: 3dd4ef1bdbac ("net: phy: make phy_disable_interrupts() non-static")
> Signed-off-by: Enguerrand de Ribaucourt <[e-mail]>
> ---
>  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

Enguerrand de Ribaucourt Dec. 20, 2022, 3:02 p.m. UTC | #2

> From: "Heiner Kallweit" <[e-mail]>
> To: "Enguerrand de Ribaucourt" <[e-mail]>,
> "netdev" <[e-mail]>
> Cc: "Paolo Abeni" <[e-mail]>, "woojung huh"
> <[e-mail]>, "davem" <[e-mail]>, "UNGLinuxDriver"
> <[e-mail]>, "Andrew Lunn" <[e-mail]>, "Russell King -
> ARM Linux" <[e-mail]>
> 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
> > <[e-mail]>
> > ---
> > 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

Andrew Lunn Dec. 20, 2022, 3:17 p.m. UTC | #3

On Tue, Dec 20, 2022 at 10:02:56AM -0500, Enguerrand de Ribaucourt wrote:
> > From: "Heiner Kallweit" <[e-mail]>
> > To: "Enguerrand de Ribaucourt" <[e-mail]>,
> > "netdev" <[e-mail]>
> > Cc: "Paolo Abeni" <[e-mail]>, "woojung huh"
> > <[e-mail]>, "davem" <[e-mail]>, "UNGLinuxDriver"
> > <[e-mail]>, "Andrew Lunn" <[e-mail]>, "Russell King -
> > ARM Linux" <[e-mail]>
> > 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.

This seems like a PHY bug, so the workaround should be in the PHY
driver, not the MAC driver. It will then apply to all MAC:PHY
combinations, not just the lan78xx paired with this PHY.

The PHY driver has the callback link_change_notify. You might be able
to use that.

   Andrew

Heiner Kallweit Dec. 20, 2022, 3:19 p.m. UTC | #4

On 20.12.2022 16:02, Enguerrand de Ribaucourt wrote:
>> From: "Heiner Kallweit" <[e-mail]>
>> To: "Enguerrand de Ribaucourt" <[e-mail]>,
>> "netdev" <[e-mail]>
>> Cc: "Paolo Abeni" <[e-mail]>, "woojung huh"
>> <[e-mail]>, "davem" <[e-mail]>, "UNGLinuxDriver"
>> <[e-mail]>, "Andrew Lunn" <[e-mail]>, "Russell King -
>> ARM Linux" <[e-mail]>
>> 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.

> Enguerrand
> 
>>> Fixes: 3dd4ef1bdbac ("net: phy: make phy_disable_interrupts() non-static")
>>> Signed-off-by: Enguerrand de Ribaucourt
>>> <[e-mail]>
>>> ---
>>> 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

Enguerrand de Ribaucourt Dec. 20, 2022, 3:48 p.m. UTC | #5

> From: "Heiner Kallweit" <[e-mail]>
> To: "Enguerrand de Ribaucourt" <[e-mail]>
> Cc: "netdev" <[e-mail]>, "Paolo Abeni" <[e-mail]>, "woojung huh" <[e-mail]>,
> "davem" <[e-mail]>, "UNGLinuxDriver" <[e-mail]>, "Andrew Lunn" <[e-mail]>,
> "Russell King - ARM Linux" <[e-mail]>
> 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" <[e-mail]>
> >> To: "Enguerrand de Ribaucourt" <[e-mail]>,
> >> "netdev" <[e-mail]>
> >> Cc: "Paolo Abeni" <[e-mail]>, "woojung huh"
> >> <[e-mail]>, "davem" <[e-mail]>, "UNGLinuxDriver"
> >> <[e-mail]>, "Andrew Lunn" <[e-mail]>, "Russell King -
> >> ARM Linux" <[e-mail]>
> >> 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
> >>> <[e-mail]>
> >>> ---
> >>> 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

Russell King (Oracle) Dec. 20, 2022, 4:13 p.m. UTC | #6

Hi,

On Tue, Dec 20, 2022 at 10:02:56AM -0500, Enguerrand de Ribaucourt wrote:
> > From: "Heiner Kallweit" <[e-mail]>
> > To: "Enguerrand de Ribaucourt" <[e-mail]>,
> > "netdev" <[e-mail]>
> > Cc: "Paolo Abeni" <[e-mail]>, "woojung huh"
> > <[e-mail]>, "davem" <[e-mail]>, "UNGLinuxDriver"
> > <[e-mail]>, "Andrew Lunn" <[e-mail]>, "Russell King -
> > ARM Linux" <[e-mail]>
> > 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.

Thanks.

Enguerrand de Ribaucourt Dec. 20, 2022, 4:22 p.m. UTC | #7

----- Original Message -----
> From: "Russell King (Oracle)" <[e-mail]>
> To: "Enguerrand de Ribaucourt" <[e-mail]>
> Cc: "Heiner Kallweit" <[e-mail]>, "netdev" <[e-mail]>, "Paolo Abeni" <[e-mail]>,
> "woojung huh" <[e-mail]>, "davem" <[e-mail]>, "UNGLinuxDriver"
> <[e-mail]>, "Andrew Lunn" <[e-mail]>
> 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" <[e-mail]>
> > > To: "Enguerrand de Ribaucourt" <[e-mail]>,
> > > "netdev" <[e-mail]>
> > > Cc: "Paolo Abeni" <[e-mail]>, "woojung huh"
> > > <[e-mail]>, "davem" <[e-mail]>, "UNGLinuxDriver"
> > > <[e-mail]>, "Andrew Lunn" <[e-mail]>, "Russell King -
> > > ARM Linux" <[e-mail]>
> > > 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!

Sven Schuchmann Dec. 20, 2022, 4:47 p.m. UTC | #8

> -----Ursprüngliche Nachricht-----
> Von: Enguerrand de Ribaucourt <[e-mail]>
> Gesendet: Dienstag, 20. Dezember 2022 17:22
> An: Russell King (Oracle) <[e-mail]>
> Cc: Heiner Kallweit <[e-mail]>; netdev <[e-mail]>; Paolo Abeni
> <[e-mail]>; woojung huh <[e-mail]>; davem <[e-mail]>;
> UNGLinuxDriver <[e-mail]>; Andrew Lunn <[e-mail]>
> Betreff: Re: [PATCH v3 1/3] net: phy: add EXPORT_SYMBOL to phy_disable_interrupts()
> 
> 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.

Same over here, we are using the lan7801 together with a dp83tc811 from TI
and also had to remove this code. On the raspberrypi kernel there is also
this patch https://github.com/raspberrypi/linux/commit/2cc41cfc79323d90b7fd8f28b22e77db2a0c3360
which is also modifying LAN88XX registers from the lan78xx driver.
But I also have no idea how this could be fixed. 

Overall I would say to remove any PHY specific code from the LAN drivers
maybe by "fixup" function or something else. But at this time everyone
has to modify the lan78xx code, as the above patch indicates even 
you *are* using a LAN88XX Phy.

Maybe someone else as some ideas?

Sven

Andrew Lunn Dec. 20, 2022, 4:55 p.m. UTC | #9

On Tue, Dec 20, 2022 at 10:48:25AM -0500, Enguerrand de Ribaucourt wrote:
> > From: "Heiner Kallweit" <[e-mail]>
> > To: "Enguerrand de Ribaucourt" <[e-mail]>
> > Cc: "netdev" <[e-mail]>, "Paolo Abeni" <[e-mail]>, "woojung huh" <[e-mail]>,
> > "davem" <[e-mail]>, "UNGLinuxDriver" <[e-mail]>, "Andrew Lunn" <[e-mail]>,
> > "Russell King - ARM Linux" <[e-mail]>
> > Sent: Tuesday, December 20, 2022 4:19:40 PM
> > Subject: Re: [PATCH v3 1/3] net: phy: add EXPORT_SYMBOL to phy_disable_interrupts()
> 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?

Woojung Huh added this code, and he sometimes contributes here.

Woojung, do you still have access to the hardware?

	 Andrew

Woojung Huh Dec. 20, 2022, 7:43 p.m. UTC | #10

Hi Andrew,

Let me check with our team.

Best regards,
Woojung

> -----Original Message-----
> From: Andrew Lunn <[e-mail]>
> Sent: Tuesday, December 20, 2022 11:55 AM
> To: Enguerrand de Ribaucourt <enguerrand.de-
> [e-mail]>
> Cc: Heiner Kallweit <[e-mail]>; netdev
> <[e-mail]>; Paolo Abeni <[e-mail]>; Woojung
> Huh - C21699 <[e-mail]>; davem
> <[e-mail]>; UNGLinuxDriver
> <[e-mail]>; Russell King - ARM Linux
> <[e-mail]>
> Subject: Re: [PATCH v3 1/3] net: phy: add EXPORT_SYMBOL to
> phy_disable_interrupts()
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Tue, Dec 20, 2022 at 10:48:25AM -0500, Enguerrand de Ribaucourt wrote:
> > > From: "Heiner Kallweit" <[e-mail]>
> > > To: "Enguerrand de Ribaucourt" <enguerrand.de-
> [e-mail]>
> > > Cc: "netdev" <[e-mail]>, "Paolo Abeni"
> <[e-mail]>, "woojung huh" <[e-mail]>,
> > > "davem" <[e-mail]>, "UNGLinuxDriver"
> <[e-mail]>, "Andrew Lunn" <[e-mail]>,
> > > "Russell King - ARM Linux" <[e-mail]>
> > > Sent: Tuesday, December 20, 2022 4:19:40 PM
> > > Subject: Re: [PATCH v3 1/3] net: phy: add EXPORT_SYMBOL to
> phy_disable_interrupts()
> > 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?
> 
> Woojung Huh added this code, and he sometimes contributes here.
> 
> Woojung, do you still have access to the hardware?
> 
>          Andrew

Yuiko Oshino Dec. 21, 2022, 6:03 p.m. UTC | #11

Hi Andrew and all,

Microchip team can review the drivers and test affected devices next month.
We will submit additional patches if necessary.

Thank you and best regards,
Yuiko

>-----Original Message-----
>From: Woojung Huh - C21699 <[e-mail]>
>Sent: Tuesday, December 20, 2022 2:43 PM
>To: Andrew Lunn <[e-mail]>; Enguerrand de Ribaucourt <enguerrand.de-
>[e-mail]>
>Cc: Heiner Kallweit <[e-mail]>; netdev <[e-mail]>;
>Paolo Abeni <[e-mail]>; davem <[e-mail]>;
>UNGLinuxDriver <[e-mail]>; Russell King - ARM Linux
><[e-mail]>
>Subject: RE: [PATCH v3 1/3] net: phy: add EXPORT_SYMBOL to
>phy_disable_interrupts()
>
>Hi Andrew,
>
>Let me check with our team.
>
>Best regards,
>Woojung
>
>> -----Original Message-----
>> From: Andrew Lunn <[e-mail]>
>> Sent: Tuesday, December 20, 2022 11:55 AM
>> To: Enguerrand de Ribaucourt <enguerrand.de-
>> [e-mail]>
>> Cc: Heiner Kallweit <[e-mail]>; netdev
>> <[e-mail]>; Paolo Abeni <[e-mail]>; Woojung Huh
>> - C21699 <[e-mail]>; davem <[e-mail]>;
>> UNGLinuxDriver <[e-mail]>; Russell King - ARM
>> Linux <[e-mail]>
>> Subject: Re: [PATCH v3 1/3] net: phy: add EXPORT_SYMBOL to
>> phy_disable_interrupts()
>>
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>> the content is safe
>>
>> On Tue, Dec 20, 2022 at 10:48:25AM -0500, Enguerrand de Ribaucourt wrote:
>> > > From: "Heiner Kallweit" <[e-mail]>
>> > > To: "Enguerrand de Ribaucourt" <enguerrand.de-
>> [e-mail]>
>> > > Cc: "netdev" <[e-mail]>, "Paolo Abeni"
>> <[e-mail]>, "woojung huh" <[e-mail]>,
>> > > "davem" <[e-mail]>, "UNGLinuxDriver"
>> <[e-mail]>, "Andrew Lunn" <[e-mail]>,
>> > > "Russell King - ARM Linux" <[e-mail]>
>> > > Sent: Tuesday, December 20, 2022 4:19:40 PM
>> > > Subject: Re: [PATCH v3 1/3] net: phy: add EXPORT_SYMBOL to
>> phy_disable_interrupts()
>> > 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?
>>
>> Woojung Huh added this code, and he sometimes contributes here.
>>
>> Woojung, do you still have access to the hardware?
>>
>>          Andrew

Patch

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