diff mbox series

[net] net: phy: smsc: Disable Energy Detect Power-Down in interrupt mode

Message ID 439a3f3168c2f9d44b5fd9bb8d2b551711316be6.1655714438.git.lukas@wunner.de (mailing list archive)
State Accepted
Commit 2642cc6c3bbe0900ba15bab078fd15ad8baccbc5
Delegated to: Netdev Maintainers
Headers show
Series [net] net: phy: smsc: Disable Energy Detect Power-Down in interrupt mode | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 9 of 9 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 24 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Lukas Wunner June 20, 2022, 11:04 a.m. UTC
Simon reports that if two LAN9514 USB adapters are directly connected
without an intermediate switch, the link fails to come up and link LEDs
remain dark.  The issue was introduced by commit 1ce8b37241ed ("usbnet:
smsc95xx: Forward PHY interrupts to PHY driver to avoid polling").

The PHY suffers from a known erratum wherein link detection becomes
unreliable if Energy Detect Power-Down is used.  In poll mode, the
driver works around the erratum by briefly disabling EDPD for 640 msec
to detect a neighbor, then re-enabling it to save power.

In interrupt mode, no interrupt is signaled if EDPD is used by both link
partners, so it must not be enabled at all.

We'll recoup the power savings by enabling SUSPEND1 mode on affected
LAN95xx chips in a forthcoming commit.

Fixes: 1ce8b37241ed ("usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid polling")
Reported-by: Simon Han <z.han@kunbus.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/net/phy/smsc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Florian Fainelli June 20, 2022, 5:03 p.m. UTC | #1
On 6/20/22 04:04, Lukas Wunner wrote:
> Simon reports that if two LAN9514 USB adapters are directly connected
> without an intermediate switch, the link fails to come up and link LEDs
> remain dark.  The issue was introduced by commit 1ce8b37241ed ("usbnet:
> smsc95xx: Forward PHY interrupts to PHY driver to avoid polling").
> 
> The PHY suffers from a known erratum wherein link detection becomes
> unreliable if Energy Detect Power-Down is used.  In poll mode, the
> driver works around the erratum by briefly disabling EDPD for 640 msec
> to detect a neighbor, then re-enabling it to save power.
> 
> In interrupt mode, no interrupt is signaled if EDPD is used by both link
> partners, so it must not be enabled at all.
> 
> We'll recoup the power savings by enabling SUSPEND1 mode on affected
> LAN95xx chips in a forthcoming commit.
> 
> Fixes: 1ce8b37241ed ("usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid polling")
> Reported-by: Simon Han <z.han@kunbus.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>   drivers/net/phy/smsc.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> index 1b54684b68a0..96d3c40932d8 100644
> --- a/drivers/net/phy/smsc.c
> +++ b/drivers/net/phy/smsc.c
> @@ -110,7 +110,7 @@ static int smsc_phy_config_init(struct phy_device *phydev)
>   	struct smsc_phy_priv *priv = phydev->priv;
>   	int rc;
>   
> -	if (!priv->energy_enable)
> +	if (!priv->energy_enable || phydev->irq != PHY_POLL)

phy_interrupt_is_valid() may be more appropriate, since you are assuming 
that you either have PHY_POLL or valid "external" PHY interrupt but 
there is also the special case of PHY_MAC_INTERRUPT that is not dealt with.

>   		return 0;
>   
>   	rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
> @@ -210,6 +210,8 @@ static int lan95xx_config_aneg_ext(struct phy_device *phydev)
>    * response on link pulses to detect presence of plugged Ethernet cable.
>    * The Energy Detect Power-Down mode is enabled again in the end of procedure to
>    * save approximately 220 mW of power if cable is unplugged.
> + * The workaround is only applicable to poll mode. Energy Detect Power-Down may
> + * not be used in interrupt mode lest link change detection becomes unreliable.
>    */
>   static int lan87xx_read_status(struct phy_device *phydev)
>   {
> @@ -217,7 +219,7 @@ static int lan87xx_read_status(struct phy_device *phydev)
>   
>   	int err = genphy_read_status(phydev);
>   
> -	if (!phydev->link && priv->energy_enable) {
> +	if (!phydev->link && priv->energy_enable && phydev->irq == PHY_POLL) {

phy_polling_mode()?

>   		/* Disable EDPD to wake up PHY */
>   		int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
>   		if (rc < 0)
Lukas Wunner June 20, 2022, 7:33 p.m. UTC | #2
On Mon, Jun 20, 2022 at 10:03:26AM -0700, Florian Fainelli wrote:
> On 6/20/22 04:04, Lukas Wunner wrote:
> > --- a/drivers/net/phy/smsc.c
> > +++ b/drivers/net/phy/smsc.c
> > @@ -110,7 +110,7 @@ static int smsc_phy_config_init(struct phy_device *phydev)
> >   	struct smsc_phy_priv *priv = phydev->priv;
> >   	int rc;
> > -	if (!priv->energy_enable)
> > +	if (!priv->energy_enable || phydev->irq != PHY_POLL)
> 
> phy_interrupt_is_valid() may be more appropriate, since you are assuming
> that you either have PHY_POLL or valid "external" PHY interrupt but there is
> also the special case of PHY_MAC_INTERRUPT that is not dealt with.

I deliberately disable EDPD for PHY_MAC_INTERRUPT as well.

That's a proper interrupt, i.e. the PHY signals interrupts
to the MAC (e.g. through an interrupt pin on the MAC),
which forwards the interrupts to phylib.  EDPD cannot
be used in that situation either.


> > @@ -217,7 +219,7 @@ static int lan87xx_read_status(struct phy_device *phydev)
> >   	int err = genphy_read_status(phydev);
> > -	if (!phydev->link && priv->energy_enable) {
> > +	if (!phydev->link && priv->energy_enable && phydev->irq == PHY_POLL) {
> 
> phy_polling_mode()?

Personally I think checking for PHY_POLL is succinct,
but if you or anyone else feels strongly about it
I'll be happy to add such a static inline to
include/linux/phy.h.

Thanks,

Lukas
patchwork-bot+netdevbpf@kernel.org June 22, 2022, 5:50 a.m. UTC | #3
Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 20 Jun 2022 13:04:50 +0200 you wrote:
> Simon reports that if two LAN9514 USB adapters are directly connected
> without an intermediate switch, the link fails to come up and link LEDs
> remain dark.  The issue was introduced by commit 1ce8b37241ed ("usbnet:
> smsc95xx: Forward PHY interrupts to PHY driver to avoid polling").
> 
> The PHY suffers from a known erratum wherein link detection becomes
> unreliable if Energy Detect Power-Down is used.  In poll mode, the
> driver works around the erratum by briefly disabling EDPD for 640 msec
> to detect a neighbor, then re-enabling it to save power.
> 
> [...]

Here is the summary with links:
  - [net] net: phy: smsc: Disable Energy Detect Power-Down in interrupt mode
    https://git.kernel.org/netdev/net/c/2642cc6c3bbe

You are awesome, thank you!
diff mbox series

Patch

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index 1b54684b68a0..96d3c40932d8 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -110,7 +110,7 @@  static int smsc_phy_config_init(struct phy_device *phydev)
 	struct smsc_phy_priv *priv = phydev->priv;
 	int rc;
 
-	if (!priv->energy_enable)
+	if (!priv->energy_enable || phydev->irq != PHY_POLL)
 		return 0;
 
 	rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
@@ -210,6 +210,8 @@  static int lan95xx_config_aneg_ext(struct phy_device *phydev)
  * response on link pulses to detect presence of plugged Ethernet cable.
  * The Energy Detect Power-Down mode is enabled again in the end of procedure to
  * save approximately 220 mW of power if cable is unplugged.
+ * The workaround is only applicable to poll mode. Energy Detect Power-Down may
+ * not be used in interrupt mode lest link change detection becomes unreliable.
  */
 static int lan87xx_read_status(struct phy_device *phydev)
 {
@@ -217,7 +219,7 @@  static int lan87xx_read_status(struct phy_device *phydev)
 
 	int err = genphy_read_status(phydev);
 
-	if (!phydev->link && priv->energy_enable) {
+	if (!phydev->link && priv->energy_enable && phydev->irq == PHY_POLL) {
 		/* Disable EDPD to wake up PHY */
 		int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
 		if (rc < 0)