netdev - Re: [PATCH net] net: mdio-ipq4019: fix wrong NULL check

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: <b8beb23a-6e39-4534-b7a2-5b0627373fb6@quicinc.com>
Date: Tue, 19 Nov 2024 18:54:10 +0800
From: Jie Luo <quic_luoj@...cinc.com>
To: Rosen Penev <rosenp@...il.com>,
        "Russell King (Oracle)"
	<linux@...linux.org.uk>
CC: <netdev@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit
	<hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet
	<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni
	<pabeni@...hat.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net] net: mdio-ipq4019: fix wrong NULL check



On 11/19/2024 5:12 AM, Rosen Penev wrote:
> On Mon, Nov 18, 2024 at 3:27 AM Russell King (Oracle)
> <linux@...linux.org.uk> wrote:
>>
>> On Mon, Nov 18, 2024 at 06:26:27PM +0800, Jie Luo wrote:
>>> On 11/18/2024 5:28 AM, Rosen Penev wrote:
>>>> devm_ioremap_resource returns a PTR_ERR when it fails, not NULL. OTOH
>>>> this is conditionally set to either a PTR_ERR or a valid pointer. Use
>>>> !IS_ERR_OR_NULL to check if we can use this.
>>>>
>>>> Fixes: 23a890d493 ("net: mdio: Add the reset function for IPQ MDIO driver")
>>>>
>>>> Signed-off-by: Rosen Penev <rosenp@...il.com>
>>>> ---
>>>>    drivers/net/mdio/mdio-ipq4019.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
>>>> index dd3ed2d6430b..859302b0d38c 100644
>>>> --- a/drivers/net/mdio/mdio-ipq4019.c
>>>> +++ b/drivers/net/mdio/mdio-ipq4019.c
>>>> @@ -256,7 +256,7 @@ static int ipq_mdio_reset(struct mii_bus *bus)
>>>>      /* To indicate CMN_PLL that ethernet_ldo has been ready if platform resource 1
>>>>       * is specified in the device tree.
>>>>       */
>>>> -   if (priv->eth_ldo_rdy) {
>>>> +   if (!IS_ERR_OR_NULL(priv->eth_ldo_rdy)) {
>>>>              val = readl(priv->eth_ldo_rdy);
>>>>              val |= BIT(0);
>>>>              writel(val, priv->eth_ldo_rdy);
>>>
>>> Reviewed-by: Luo Jie <quic_luoj@...cinc.com>
>>
>> Looking at the setup of this:
>>
>>          /* The platform resource is provided on the chipset IPQ5018 */
>>          /* This resource is optional */
>>          res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>          if (res)
>>                  priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res);
>>
>> While this is optional, surely the optional part is whether resource 1
>> is provided or not. If the resource is provided, but we fail to ioremap
>> it, isn't that an error which should be propagated? In that situation,
>> isn't the firmware saying "we have a second resource" but failing to
>> map it should be an error?

Agree. The fail to ioremap resource 1 needs to be captured and
propagated if the resource 1 is provided by DTS.

> 
> Another way to look at it is, if we convert this to
> 
> devm_platform_get_and_ioremap_resource(pdev, 1, &res);
> 
> it seems to only write to res if platform_get_resource succeeds and
> otherwise doesn't care. The only real way to check if found is
> !IS_ERR().
> 
> Actually the more appropriate function here is
> devm_platform_ioremap_resource , which doesn't write to a struct
> resource.
> 

The resource 1 is optional, so devm_platform_ioremap_resource can't
be used here, otherwise the error will be returned if the resource 1
is not provided.

>>
>> --
>> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
>> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!