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

Message-ID: <ZzskoS2jwC6eLlmD@shell.armlinux.org.uk>
Date: Mon, 18 Nov 2024 11:27:29 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Jie Luo <quic_luoj@...cinc.com>
Cc: Rosen Penev <rosenp@...il.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 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?

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