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

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

[Rosen Penev]

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);
-- 
2.47.0

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

[Jie Luo]

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

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

[Russell King (Oracle)]

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?

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

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

[Rosen Penev]

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!

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@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!