netdev - Re: [PATCH net-next v3 2/3] net: bcmgenet: Return PTR_ERR() for fixed_phy

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] [day] [month] [year] [list]

Message-ID: <2d12cdb3-e6ef-46d2-3bfb-58b5c54f6ab3@huawei.com>
Date: Mon, 21 Aug 2023 09:15:50 +0800
From: Ruan Jinjie <ruanjinjie@...wei.com>
To: Simon Horman <horms@...nel.org>
CC: <rafal@...ecki.pl>, <bcm-kernel-feedback-list@...adcom.com>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <opendmb@...il.com>, <florian.fainelli@...adcom.com>,
	<bryan.whitehead@...rochip.com>, <UNGLinuxDriver@...rochip.com>,
	<netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v3 2/3] net: bcmgenet: Return PTR_ERR() for
 fixed_phy_register()



On 2023/8/20 1:10, Simon Horman wrote:
> On Sat, Aug 19, 2023 at 07:06:15PM +0200, Simon Horman wrote:
>> On Fri, Aug 18, 2023 at 03:07:06PM +0800, Ruan Jinjie wrote:
>>> fixed_phy_register() returns -EPROBE_DEFER, -EINVAL and -EBUSY,
>>> etc, in addition to -ENODEV. The Best practice is to return these
>>> error codes with PTR_ERR().
>>>
>>> Signed-off-by: Ruan Jinjie <ruanjinjie@...wei.com>
>>> ---
>>> v3:
>>> - Split the return value check into another patch set.
>>> - Update the commit title and message.
>>> ---
>>>  drivers/net/ethernet/broadcom/genet/bcmmii.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>> index 0092e46c46f8..4012a141a229 100644
>>> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>> @@ -619,7 +619,7 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
>>>  		phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
>>>  		if (!phydev || IS_ERR(phydev)) {
>>>  			dev_err(kdev, "failed to register fixed PHY device\n");
>>> -			return -ENODEV;
>>> +			return PTR_ERR(phydev);
>>
>> Hi Ruan,
>>
>> thanks for your patch.
>>
>> Perhaps I am missing something, but this doesn't seem right to me.
>> In the case where phydev is NULL will return 0.
>> But bcmgenet_mii_pd_init() also returns 0 on success.
>>
>> Perhaps this is better?
>>
>> 		if (!phydev || IS_ERR(phydev)) {
>> 			dev_err(kdev, "failed to register fixed PHY device\n");
>> 			return physdev ? PTR_ERR(phydev) : -ENODEV;
>> 		}
>>
>> I have a similar concern for patch 1/3 of this series.
>> Patch 3/3 seems fine in this regard.
> 
> Sorry for the noise.
> 
> I now see that fixed_phy_register() never returns NULL,
> and that condition is being removed by another patchset [1].
> 
> I'm fine with this, other than that I suspect your two series
> conflict with each other.

Thank you! I'll resend this patch to be consistent.

> 
> [1] https://lore.kernel.org/all/20230818051221.3634844-1-ruanjinjie@huawei.com/
>