netdev - Re: [PATCH net-next v3 4/6] net: phy: microchip_t1s: fix reset complete status handling

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: <8a46450d-7c6e-68a4-c09d-3b195a935907@microchip.com>
Date: Fri, 26 May 2023 06:00:08 +0000
From: <Parthiban.Veerasooran@...rochip.com>
To: <ramon.nordin.rodriguez@...roamp.se>
CC: <andrew@...n.ch>, <hkallweit1@...il.com>, <linux@...linux.org.uk>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <Horatiu.Vultur@...rochip.com>,
	<Woojung.Huh@...rochip.com>, <Nicolas.Ferre@...rochip.com>,
	<Thorsten.Kummermehr@...rochip.com>
Subject: Re: [PATCH net-next v3 4/6] net: phy: microchip_t1s: fix reset
 complete status handling

Hi Ramon,

On 25/05/23 11:56 pm, Ramón Nordin Rodriguez wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> +     /* Read STS2 register and check for the Reset Complete status to do the
>> +      * init configuration. If the Reset Complete is not set, wait for 5us
>> +      * and then read STS2 register again and check for Reset Complete status.
>> +      * Still if it is failed then declare PHY reset error or else proceed
>> +      * for the PHY initial register configuration.
>> +      */
>> +     err = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_STS2);
>> +     if (err < 0)
>> +             return err;
>> +
>> +     if (!(err & LAN867x_RESET_COMPLETE_STS)) {
>> +             udelay(5);
>> +             err = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_STS2);
>> +             if (err < 0)
>> +                     return err;
>> +             if (!(err & LAN867x_RESET_COMPLETE_STS)) {
>> +                     phydev_err(phydev, "PHY reset failed\n");
>> +                     return -ENODEV;
>> +             }
>> +     }
> 
> This comment explains exactly what the code does, which is also obvious
> from reading the code. A meaningful comment would be explaining why the
> state can change 5us later.
> 
As per design, LAN867x reset to be completed by 3us. Just for a safer 
side it is recommended to use 5us. With the assumption of more than 3us 
completion, the first read checks for the Reset Complete. If the 
config_init is more faster, then once again checks for it after 5us.

As you mentioned, can we remove the existing block comment as it 
explains the code and add the above comment to explain 5us delay.
What is your opinion on this proposal?

Best Regards,
Parthiban V