netdev - Re: [PATCH net 2/3] net: phy: broadcom: Only set BMCR.PDOWN to suspend

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: <d2dec3e9-146a-1e07-5eb5-690b972c3315@gmail.com>
Date:   Wed, 10 Mar 2021 22:07:03 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>, netdev@...r.kernel.org
Cc:     andrew@...n.ch, kuba@...nel.org, davem@...emloft.net
Subject: Re: [PATCH net 2/3] net: phy: broadcom: Only set BMCR.PDOWN to
 suspend

On 10.03.2021 21:41, Florian Fainelli wrote:
> B50212E PHYs have been observed to get into an incorrect state with the
> visible effect of having both activity and link LEDs flashing
> alternatively instead of being turned off as intended when
> genphy_suspend() was issued. The BCM54810 is a similar design and
> equally suffers from that issue.
> 
> The datasheet is not particularly clear whether a read/modify/write
> sequence is acceptable and only indicates that BMCR.PDOWN=1 should be
> utilized to enter the power down mode. When this was done the PHYs were
> always measured to have power levels that match the expectations and
> LEDs powered off.
> 
> Fixes: fe26821fa614 ("net: phy: broadcom: Wire suspend/resume for BCM54810")
> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
> ---
>  drivers/net/phy/broadcom.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
> index b8eb736fb456..b33ffd44f799 100644
> --- a/drivers/net/phy/broadcom.c
> +++ b/drivers/net/phy/broadcom.c
> @@ -388,6 +388,21 @@ static int bcm54xx_config_init(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int bcm54xx_suspend(struct phy_device *phydev)
> +{
> +	/* We cannot perform a read/modify/write like what genphy_suspend()
> +	 * does because depending on the time we can observe the PHY having
> +	 * both of its LEDs flashing indicating that it is in an incorrect
> +	 * state and not powered down as expected.
> +	 *
> +	 * There is not a clear indication in the datasheet whether a
> +	 * read/modify/write would be acceptable, but a blind write to the
> +	 * register has been proven to be functional unlike the
> +	 * Read/Modify/Write.
> +	 */
> +	return phy_write(phydev, MII_BMCR, BMCR_PDOWN);

This clears all other bits in MII_BMCR, incl. ANENABLE and the ones used in
forced mode. So you have to rely on somebody calling genphy_config_aneg()
to sync the register bits with the values cached in struct phy_device
on resume. Typically the phylib state machine takes care, but do we have
to consider use cases where this is not the case?

Heiner

> +}
> +
>  static int bcm54xx_resume(struct phy_device *phydev)
>  {
>  	int ret;
> @@ -778,7 +793,7 @@ static struct phy_driver broadcom_drivers[] = {
>  	.config_aneg    = bcm5481_config_aneg,
>  	.config_intr    = bcm_phy_config_intr,
>  	.handle_interrupt = bcm_phy_handle_interrupt,
> -	.suspend	= genphy_suspend,
> +	.suspend	= bcm54xx_suspend,
>  	.resume		= bcm54xx_resume,
>  }, {
>  	.phy_id         = PHY_ID_BCM54811,
>