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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKxU2N_zcNB-0gBKPnPo3kjZ7mZpzbiB63JFGVZ5jd1U-LjxSA@mail.gmail.com>
Date: Mon, 18 Nov 2024 13:12:41 -0800
From: Rosen Penev <rosenp@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Jie Luo <quic_luoj@...cinc.com>, 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 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?

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.

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