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

[Jie Luo]

On 11/19/2024 5:12 AM, Rosen Penev wrote:
> On Mon, Nov 18, 2024 at 3:27 AM Russell King (Oracle)
> <linux@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > > > ---
> > > >    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@xxxxxxxxxxx>
> >
> > 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!