| Message ID | 20241117212827.13763-1-rosenp@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: mdio-ipq4019: fix wrong NULL check | expand |
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@gmail.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@quicinc.com> Thanks, Jie
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@gmail.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@quicinc.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?
On Mon, Nov 18, 2024 at 3:27 AM Russell King (Oracle) <linux@armlinux.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@gmail.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@quicinc.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!
On 11/19/2024 5:12 AM, Rosen Penev wrote: > On Mon, Nov 18, 2024 at 3:27 AM Russell King (Oracle) > <linux@armlinux.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@gmail.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@quicinc.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!
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);
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@gmail.com> --- drivers/net/mdio/mdio-ipq4019.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)