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

[Siddharth Vadapalli]

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.