Re: [PATCH] net: phy: fix autoneg invalid error state of GBSR register.

Re: [PATCH] net: phy: fix autoneg invalid error state of GBSR register.

[Andrew Lunn]

On Wed, Sep 01, 2021 at 06:56:08PM +0800, Qiang Ma wrote:
> When the PHY is not link and working in polling mode, PHY state
> machine will periodically read GBSR register. Normally, the GBSR
> register value is 0. But as the log show, in very small cases the
> value is 0x8640. The value 0x8640 means Master/Slave resolution
> failed. The PHY state machine will enter PHY_HALTED state and the
> PHY will never be able to link.
> 
> [49176.903012] [debug]: lpagb:0x0 adv:0x300
> [49177.927025] [debug]: lpagb:0x8640 adv:0x300
> [49177.927034] Generic PHY stmmac-18:00: Master/Slave resolution failed
> [49177.927241] [debug]: lpagb:0x0 adv:0x300
> 
> According to the RTL8211E PHY chip datasheet, the contents of GBSR register
> are valid only when auto negotiation is completed(BMSR[5]: Auto-
> Negotiation Complete = 1). This patch adds the condition before
> reading GBSR register to fix this error state.
> 
> Signed-off-by: Ming Wang <wangming01@loongson.cn>
> Signed-off-by: Qiang Ma <maqianga@uniontech.com>
> ---
>  drivers/net/phy/phy_device.c | 10 ++++------
>  include/linux/phy.h          |  1 +
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index b884b681d5c5..b77ed5ec31c6 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1532,10 +1532,8 @@ int genphy_update_link(struct phy_device *phydev)
>  	if (status < 0)
>  		return status;
>  
> -	if ((status & BMSR_LSTATUS) == 0)
> -		phydev->link = 0;
> -	else
> -		phydev->link = 1;
> +	phydev->link = status & BMSR_LSTATUS ? 1 : 0;
> +	phydev->autoneg_complete = status & BMSR_ANEGCOMPLETE ? 1 : 0;
>  
>  	return 0;
>  }

What tree is this against? Both net-next/master and net/master have:

        if (status < 0)
                return status;
done:
        phydev->link = status & BMSR_LSTATUS ? 1 : 0;
        phydev->autoneg_complete = status & BMSR_ANEGCOMPLETE ? 1 : 0;

        /* Consider the case that autoneg was started and "aneg complete"
         * bit has been reset, but "link up" bit not yet.
         */
        if (phydev->autoneg == AUTONEG_ENABLE && !phydev->autoneg_complete)
                phydev->link = 0;

        return 0;
}

It looks like you are using an old tree.

	Andrew

Re: [PATCH] net: phy: fix autoneg invalid error state of GBSR register.

[Andrew Lunn]

> > It looks like you are using an old tree.
> Yes, it is linux-4.19.y, since 4.19.y does not have autoneg_complete flag,，
> This patch adds the condition before
> reading GBSR register to fix this error state.

So you first need to fix it in net/master, and then backport it to
older kernels.

   Andrew

Re: [PATCH] net: phy: fix autoneg invalid error state of GBSR register.

[Jakub Kicinski]

On Wed, 1 Sep 2021 15:01:54 +0200 Andrew Lunn wrote:
> > > It looks like you are using an old tree.  
> > Yes, it is linux-4.19.y, since 4.19.y does not have autoneg_complete flag,，
> > This patch adds the condition before
> > reading GBSR register to fix this error state.  
> 
> So you first need to fix it in net/master, and then backport it to
> older kernels.

Hm, the list is not seeing the emails you're responding to.

Gotta be something with uniontech's email server.

Adding John in case he can grok something from vger logs.

Re: [PATCH] net: phy: fix autoneg invalid error state of GBSR register.

[Florian Fainelli]

On 9/1/2021 6:40 AM, 马强 wrote:
>  > > > It looks like you are using an old tree.
>  > > Yes, it is linux-4.19.y, since 4.19.y does not have 
> autoneg_complete flag,，
>  > > This patch adds the condition before
>  > > reading GBSR register to fix this error state.
>  >
>  > So you first need to fix it in net/master, and then backport it to
>  > older kernels.
> 
> This patch is modified according to the contents of the latest kernels,
> the following is a reference:
> [ Upstream commit b6163f194c699ff75fa6aa4565b1eb8946c2c652 ]
> [ Upstream commit 4950c2ba49cc6f2b38dbedcfa0ff67acf761419a ]

Then you need to be extremely clear in the commit message which specific 
branch you are targeting, which commits you used as a reference, and how 
you are fixing the problem. Also, the register is commonly named BMSR 
(Basic Mode Status Register), no idea what GBSR means.
-- 
Florian