[PATCH net] amd-xgbe: read STAT1 register twice to get correct value

[PATCH net] amd-xgbe: read STAT1 register twice to get correct value

[Raju Rangoju]

Link status is latched low, so read once to clear
and then read again to get current state.

Fixes: 4f3b20bfbb75 ("amd-xgbe: add support for rx-adaptation")
Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com>
Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
index 6a716337f48b..c83085285e8c 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
@@ -2930,6 +2930,7 @@ static int xgbe_phy_link_status(struct xgbe_prv_data *pdata, int *an_restart)
 
 		/* check again for the link and adaptation status */
 		reg = XMDIO_READ(pdata, MDIO_MMD_PCS, MDIO_STAT1);
+		reg = XMDIO_READ(pdata, MDIO_MMD_PCS, MDIO_STAT1);
 		if ((reg & MDIO_STAT1_LSTATUS) && pdata->rx_adapt_done)
 			return 1;
 	} else if (reg & MDIO_STAT1_LSTATUS)
-- 
2.25.1

Re: [PATCH net] amd-xgbe: read STAT1 register twice to get correct value

[Andrew Lunn]

On Thu, Sep 14, 2023 at 09:49:44AM +0530, Raju Rangoju wrote:
> Link status is latched low, so read once to clear
> and then read again to get current state.

I don't know about your PHY implementation, but within phylib and
Linux PHY drivers, this is considered wrong. You loose out on being
notified of the link going down and then back up again. Or up and then
down again.

But since this is not a Linux PHY driver, you are free to do whatever
you want...

Also, i believe it is latched, not latched low. So i think your commit
message is wrong. You should probably check with IEEE 802.3 clause 22.

    Andrew

Re: [PATCH net] amd-xgbe: read STAT1 register twice to get correct value

[Tom Lendacky]

On 9/15/23 07:41, Andrew Lunn wrote:
> On Thu, Sep 14, 2023 at 09:49:44AM +0530, Raju Rangoju wrote:
>> Link status is latched low, so read once to clear
>> and then read again to get current state.
> 
> I don't know about your PHY implementation, but within phylib and
> Linux PHY drivers, this is considered wrong. You loose out on being
> notified of the link going down and then back up again. Or up and then
> down again.
> 
> But since this is not a Linux PHY driver, you are free to do whatever
> you want...
> 
> Also, i believe it is latched, not latched low. So i think your commit
> message is wrong. You should probably check with IEEE 802.3 clause 22.

Granted I have an old version of IEEE 802.3, but "Table 22-8 - Status 
register bit definitions" has:

1.2	Link Status	1 = link is up	0 = link is down	RO/LL

So latched low.

Thanks,
Tom

> 
>      Andrew
> 

Re: [PATCH net] amd-xgbe: read STAT1 register twice to get correct value

[Raju Rangoju]

On 9/16/2023 12:48 AM, Tom Lendacky wrote:
> 
> 
> On 9/15/23 07:41, Andrew Lunn wrote:
>> On Thu, Sep 14, 2023 at 09:49:44AM +0530, Raju Rangoju wrote:
>>> Link status is latched low, so read once to clear
>>> and then read again to get current state.
>>
>> I don't know about your PHY implementation, but within phylib and
>> Linux PHY drivers, this is considered wrong. You loose out on being
>> notified of the link going down and then back up again. Or up and then
>> down again.
>>
>> But since this is not a Linux PHY driver, you are free to do whatever
>> you want...
>>
>> Also, i believe it is latched, not latched low. So i think your commit
>> message is wrong. You should probably check with IEEE 802.3 clause 22.
> 
> Granted I have an old version of IEEE 802.3, but "Table 22-8 - Status 
> register bit definitions" has:
> 
> 1.2    Link Status    1 = link is up    0 = link is down    RO/LL
> 
> So latched low.

Thanks, Tom.

The following thread (found online) has the detailed info on the IEEE 
802.3 Standard that define the Link Status bit:

https://microchip.my.site.com/s/article/How-to-correctly-read-the-Ethernet-PHY-Link-Status-bit

Thanks,
Raju
> 
> Thanks,
> Tom
> 
>>
>>      Andrew
>>

Re: [PATCH net] amd-xgbe: read STAT1 register twice to get correct value

[Andrew Lunn]

> Thanks, Tom.
> 
> The following thread (found online) has the detailed info on the IEEE 802.3
> Standard that define the Link Status bit:
> 
> https://microchip.my.site.com/s/article/How-to-correctly-read-the-Ethernet-PHY-Link-Status-bit

The relevant section for me is:

    Having a latched-low bit helps to ensure a link drop (no matter
    how short the duration before re-establishing link-up again) gets
    recorded and can be read from PHY register by the upper network
    layer (e.g., MAC processor).

By reading it twice, you are loosing the information the link went
down. This could mean you don't restart dhcp to get a new IP address
for a new subnet, kick of IPv6 getting the new network prefix so it
can create its IPv6 address etc.

When Linux is driving the PHYs using phylib, drivers are written such
that they report the latched link down. This gets report to the stack,
and so up to user space etc. phylib will then ask the driver a second
time to get the link status, and it might then return that the link is
now up. That then gets reported to the stack and user space.

As i said, you are not using phylib, this is not a linux PHY driver,
so i don't actually care what you do here. I just want to point out
why what you are doing could be wrong.

    Andrew

Re: [PATCH net] amd-xgbe: read STAT1 register twice to get correct value

[Raju Rangoju]

Hi Andrew,

Thanks for the explanation. The current approach is holding good for 
amd-xgbe.


Hi Jakub,

Can you please apply this patch.

Thanks,
Raju

On 9/17/2023 7:41 PM, Andrew Lunn wrote:
>> Thanks, Tom.
>>
>> The following thread (found online) has the detailed info on the IEEE 802.3
>> Standard that define the Link Status bit:
>>
>> https://microchip.my.site.com/s/article/How-to-correctly-read-the-Ethernet-PHY-Link-Status-bit
> 
> The relevant section for me is:
> 
>      Having a latched-low bit helps to ensure a link drop (no matter
>      how short the duration before re-establishing link-up again) gets
>      recorded and can be read from PHY register by the upper network
>      layer (e.g., MAC processor).
> 
> By reading it twice, you are loosing the information the link went
> down. This could mean you don't restart dhcp to get a new IP address
> for a new subnet, kick of IPv6 getting the new network prefix so it
> can create its IPv6 address etc.
> 
> When Linux is driving the PHYs using phylib, drivers are written such
> that they report the latched link down. This gets report to the stack,
> and so up to user space etc. phylib will then ask the driver a second
> time to get the link status, and it might then return that the link is
> now up. That then gets reported to the stack and user space.
> 
> As i said, you are not using phylib, this is not a linux PHY driver,
> so i don't actually care what you do here. I just want to point out
> why what you are doing could be wrong.
> 
>      Andrew

Re: [PATCH net] amd-xgbe: read STAT1 register twice to get correct value

[Raju Rangoju]

Hi Jakub,

Can you please apply this patch? Let me know if it needs to be resent.

Thanks,
Raju

On 9/14/2023 9:49 AM, Raju Rangoju wrote:
> Link status is latched low, so read once to clear
> and then read again to get current state.
> 
> Fixes: 4f3b20bfbb75 ("amd-xgbe: add support for rx-adaptation")
> Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com>
> Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>   drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
> index 6a716337f48b..c83085285e8c 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
> @@ -2930,6 +2930,7 @@ static int xgbe_phy_link_status(struct xgbe_prv_data *pdata, int *an_restart)
>   
>   		/* check again for the link and adaptation status */
>   		reg = XMDIO_READ(pdata, MDIO_MMD_PCS, MDIO_STAT1);
> +		reg = XMDIO_READ(pdata, MDIO_MMD_PCS, MDIO_STAT1);
>   		if ((reg & MDIO_STAT1_LSTATUS) && pdata->rx_adapt_done)
>   			return 1;
>   	} else if (reg & MDIO_STAT1_LSTATUS)

Re: [PATCH net] amd-xgbe: read STAT1 register twice to get correct value

[Jakub Kicinski]

On Thu, 12 Oct 2023 22:59:57 +0530 Raju Rangoju wrote:
> Hi Jakub,
> 
> Can you please apply this patch? Let me know if it needs to be resent.

Yes, please resend. And please put in the commit message an explanation
more technically detailed than "The current approach is holding good
for amd-xgbe." :(

Reminder: please don't top post on the ML.