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

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: <c40cc5fb-a84d-23f2-a400-c01b5b419bc9@gmail.com>
Date:   Mon, 20 Jun 2022 10:03:26 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Lukas Wunner <lukas@...ner.de>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>
Cc:     netdev@...r.kernel.org, linux-usb@...r.kernel.org,
        Steve Glendinning <steve.glendinning@...well.net>,
        UNGLinuxDriver@...rochip.com, Oliver Neukum <oneukum@...e.com>,
        Andre Edich <andre.edich@...rochip.com>,
        Oleksij Rempel <linux@...pel-privat.de>,
        Martyn Welch <martyn.welch@...labora.com>,
        Gabriel Hojda <ghojda@...urs.ro>,
        Christoph Fritz <chf.fritz@...glemail.com>,
        Lino Sanfilippo <LinoSanfilippo@....de>,
        Philipp Rosenberger <p.rosenberger@...bus.com>,
        Simon Han <z.han@...bus.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Russell King <linux@...linux.org.uk>,
        Ferry Toth <fntoth@...il.com>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        linux-samsung-soc@...r.kernel.org
Subject: Re: [PATCH net] net: phy: smsc: Disable Energy Detect Power-Down in
 interrupt mode

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@...bus.com>
> Signed-off-by: Lukas Wunner <lukas@...ner.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)


-- 
Florian