Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5a5b4348-0744-ff6b-6354-c4d0243c4fc6@ti.com>
Date:   Thu, 27 Apr 2023 09:32:57 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
CC:     <hkallweit1@gmail.com>, <linux@armlinux.org.uk>,
        <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
        <pabeni@redhat.com>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>, <srk@ti.com>,
        <s-vadapalli@ti.com>
Subject: Re: [RFC PATCH 1/2] net: phy: dp83867: add w/a for packet errors seen
 with short cables
To:     Andrew Lunn <andrew@lunn.ch>
References: <20230425054429.3956535-1-s-vadapalli@ti.com>
 <20230425054429.3956535-2-s-vadapalli@ti.com>
 <f29411d2-c596-4a07-8b6a-7d6e203c25e0@lunn.ch>
 <540149d0-a353-7225-7c58-a4e9738b7c7c@ti.com>
 <38d9b4f9-06b5-4920-8b09-daa115bd52f4@lunn.ch>
Content-Language: en-US
From:   Siddharth Vadapalli <s-vadapalli@ti.com>
In-Reply-To: <38d9b4f9-06b5-4920-8b09-daa115bd52f4@lunn.ch>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk


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.