netdev - Re: [RFC PATCH 1/2] net: phy: dp83867: add w/a for packet errors seen with short cables

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]

Date:   Thu, 27 Apr 2023 09:32:57 +0530
From:   Siddharth Vadapalli <s-vadapalli@...com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     <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>,
        <linux-arm-kernel@...ts.infradead.org>, <srk@...com>,
        <s-vadapalli@...com>
Subject: Re: [RFC PATCH 1/2] net: phy: dp83867: add w/a for packet errors seen
 with short cables



On 26/04/23 18:06, Andrew Lunn wrote:
>>>> @@ -934,8 +935,20 @@ static int dp83867_phy_reset(struct phy_device *phydev)
>>>>  
>>>>  	usleep_range(10, 20);
>>>>  
>>>> -	return phy_modify(phydev, MII_DP83867_PHYCTRL,
>>>> +	err = phy_modify(phydev, MII_DP83867_PHYCTRL,
>>>>  			 DP83867_PHYCR_FORCE_LINK_GOOD, 0);
>>>> +	if (err < 0)
>>>> +		return err;
>>>> +
>>>> +	phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_DSP_FFE_CFG, 0X0E81);
>>>
>>> Maybe check the return code for errors?
>>
>> The return value of phy_write_mmd() doesn't have to be checked since it will be
>> zero for the following reasons:
>> The dp83867 driver does not have a custom .write_mmd method. Also, the dp83867
>> phy does not support clause 45. Due to this, within __phy_write_mmd(), the ELSE
>> statement will be executed, which results in the return value being zero.
> 
> Interesting.
> 
> I would actually say __phy_write_mmd() is broken, and should be
> returning what __mdiobus_write() returns.
> 
> You should assume it will get fixed, and check the return value. And
> it does no harm to check the return value.

Thank you for clarifying. The reasoning behind the initial patch not checking
the return value was:
At all invocations of phy_write_mmd() in the dp83867 driver, at no place is the
return value checked, which led me to analyze why that was the case. I noticed
that it was due to the return value being guaranteed to be zero for this
particular driver.

Since the existing __phy_write_mmd() implementation is broken as pointed out by
you, I will update this patch to check the return value. Also, I will probably
add a cleanup patch as well, to fix this at all other invocations of
phy_write_mmd() in the driver.

-- 
Regards,
Siddharth.