[PATCH v3 0/2] net: phy: call phy_disable_interrupts() in phy_init_hw()

[PATCH v3 0/2] net: phy: call phy_disable_interrupts() in phy_init_hw()

[Jisheng Zhang]

We face an issue with rtl8211f, a pin is shared between INTB and PMEB,
and the PHY Register Accessible Interrupt is enabled by default, so
the INTB/PMEB pin is always active in polling mode case.

As Heiner pointed out "I was thinking about calling
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."

patch1 makes phy_disable_interrupts() non-static so that it could be used
in phy_init_hw() to have a defined init state.

patch2 calls phy_disable_interrupts() in phy_init_hw() to have a
defined init state.

Since v2:
  - Don't export phy_disable_interrupts() but just make it non-static

Since v1:
  - EXPORT the correct symbol

Jisheng Zhang (2):
  net: phy: make phy_disable_interrupts() non-static
  net: phy: call phy_disable_interrupts() in phy_init_hw()

 drivers/net/phy/phy.c        | 2 +-
 drivers/net/phy/phy_device.c | 7 +++++--
 include/linux/phy.h          | 1 +
 3 files changed, 7 insertions(+), 3 deletions(-)

-- 
2.27.0

[PATCH v3 1/2] net: phy: make phy_disable_interrupts() non-static

[Jisheng Zhang]

We face an issue with rtl8211f, a pin is shared between INTB and PMEB,
and the PHY Register Accessible Interrupt is enabled by default, so
the INTB/PMEB pin is always active in polling mode case.

As Heiner pointed out "I was thinking about calling
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."

Make phy_disable_interrupts() non-static so that it could be used in
phy_init_hw() to have a defined init state.

Suggested-by: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
---
 drivers/net/phy/phy.c | 2 +-
 include/linux/phy.h   | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 1de3938628f4..56cfae950472 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -840,7 +840,7 @@ static void phy_error(struct phy_device *phydev)
  * phy_disable_interrupts - Disable the PHY interrupts from the PHY side
  * @phydev: target phy_device struct
  */
-static int phy_disable_interrupts(struct phy_device *phydev)
+int phy_disable_interrupts(struct phy_device *phydev)
 {
 	int err;
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 8c05d0fb5c00..b693b609b2f5 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1416,6 +1416,7 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
 int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd);
 int phy_do_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd);
 int phy_do_ioctl_running(struct net_device *dev, struct ifreq *ifr, int cmd);
+int phy_disable_interrupts(struct phy_device *phydev);
 void phy_request_interrupt(struct phy_device *phydev);
 void phy_free_interrupt(struct phy_device *phydev);
 void phy_print_status(struct phy_device *phydev);
-- 
2.27.0

[PATCH v3 2/2] net: phy: call phy_disable_interrupts() in phy_init_hw()

[Jisheng Zhang]

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@gmail.com>
Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.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);
 }
 EXPORT_SYMBOL(phy_init_hw);
 
-- 
2.27.0

Re: [PATCH v3 2/2] net: phy: call phy_disable_interrupts() in phy_init_hw()

[Florian Fainelli]

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@gmail.com>
> Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.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.)
-- 
Florian

Re: [PATCH v3 2/2] net: phy: call phy_disable_interrupts() in phy_init_hw()

[Russell King - ARM Linux admin]

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@gmail.com>
> > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.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!

Re: [PATCH v3 0/2] net: phy: call phy_disable_interrupts() in phy_init_hw()

[David Miller]

From: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
Date: Wed, 24 Jun 2020 11:25:16 +0800

> We face an issue with rtl8211f, a pin is shared between INTB and PMEB,
> and the PHY Register Accessible Interrupt is enabled by default, so
> the INTB/PMEB pin is always active in polling mode case.
> 
> As Heiner pointed out "I was thinking about calling
> 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."
> 
> patch1 makes phy_disable_interrupts() non-static so that it could be used
> in phy_init_hw() to have a defined init state.
> 
> patch2 calls phy_disable_interrupts() in phy_init_hw() to have a
> defined init state.
> 
> Since v2:
>   - Don't export phy_disable_interrupts() but just make it non-static
> 
> Since v1:
>   - EXPORT the correct symbol

Series applied, thank you.

Re: [PATCH v3 0/2] net: phy: call phy_disable_interrupts() in phy_init_hw()

[Florian Fainelli]

On 6/24/20 2:43 PM, David Miller wrote:
> From: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> Date: Wed, 24 Jun 2020 11:25:16 +0800
> 
>> We face an issue with rtl8211f, a pin is shared between INTB and PMEB,
>> and the PHY Register Accessible Interrupt is enabled by default, so
>> the INTB/PMEB pin is always active in polling mode case.
>>
>> As Heiner pointed out "I was thinking about calling
>> 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."
>>
>> patch1 makes phy_disable_interrupts() non-static so that it could be used
>> in phy_init_hw() to have a defined init state.
>>
>> patch2 calls phy_disable_interrupts() in phy_init_hw() to have a
>> defined init state.
>>
>> Since v2:
>>   - Don't export phy_disable_interrupts() but just make it non-static
>>
>> Since v1:
>>   - EXPORT the correct symbol
> 
> Series applied, thank you.

Did you mean that you applied v4? It does not look like you pushed your
local changes to net-next yet, so I cannot tell for sure.
-- 
Florian

Re: [PATCH v3 0/2] net: phy: call phy_disable_interrupts() in phy_init_hw()

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Wed, 24 Jun 2020 15:10:51 -0700

> Did you mean that you applied v4? It does not look like you pushed your
> local changes to net-next yet, so I cannot tell for sure.

I ended up applying v4, yes.

Re: [PATCH v3 0/2] net: phy: call phy_disable_interrupts() in phy_init_hw()

[Florian Fainelli]

On 6/24/2020 4:34 PM, David Miller wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Wed, 24 Jun 2020 15:10:51 -0700
> 
>> Did you mean that you applied v4? It does not look like you pushed your
>> local changes to net-next yet, so I cannot tell for sure.
> 
> I ended up applying v4, yes.
> 

OK, just making sure :) thanks!
-- 
Florian