[PATCH v2 0/3] of_mdio: use IS_ERR_OR_NULL() and PTR_ERR_OR_ZERO()

[PATCH v2 0/3] of_mdio: use IS_ERR_OR_NULL() and PTR_ERR_OR_ZERO()

[Sergei Shtylyov]

Hello.

   Here's the set of 3 patches against DaveM's 'net-next.git' repo. They deal
with some error checks in the device tree MDIO code...

[1/3] of_mdio: mdio_device_create() never returns NULL
[2/3] of_mdio: use IS_ERR_OR_NULL()
[3/3] of_mdio: use PTR_ERR_OR_ZERO()

MBR, Sergei

[PATCH v2 1/3] of_mdio: mdio_device_create() never returns NULL

[Sergei Shtylyov]

mdio_device_create() never returns NULL, thus checking for it in
of_mdiobus_register_device() is pointless...

Suggested-by: Vladimir Zapolskiy <vz@mleia.com>
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
Changes in version 2:
- new patch.

 drivers/of/of_mdio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: net-next/drivers/of/of_mdio.c
===================================================================
--- net-next.orig/drivers/of/of_mdio.c
+++ net-next/drivers/of/of_mdio.c
@@ -98,7 +98,7 @@ static int of_mdiobus_register_device(st
 	int rc;
 
 	mdiodev = mdio_device_create(mdio, addr);
-	if (!mdiodev || IS_ERR(mdiodev))
+	if (IS_ERR(mdiodev))
 		return 1;
 
 	/* Associate the OF node with the device structure so it

[PATCH v2 2/3] of_mdio: use IS_ERR_OR_NULL()

[Sergei Shtylyov]

IS_ERR_OR_NULL() is open coded in of_mdiobus_register_phy(), so just call
it directly...

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

---
Changes in version 2:
- removed the of_mdiobus_register_device() hunk;
- added the "Reviewed-by:" tag.

 drivers/of/of_mdio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: net-next/drivers/of/of_mdio.c
===================================================================
--- net-next.orig/drivers/of/of_mdio.c
+++ net-next/drivers/of/of_mdio.c
@@ -56,7 +56,7 @@ static int of_mdiobus_register_phy(struc
 		phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
 	else
 		phy = get_phy_device(mdio, addr, is_c45);
-	if (!phy || IS_ERR(phy))
+	if (IS_ERR_OR_NULL(phy))
 		return 1;
 
 	rc = irq_of_parse_and_map(child, 0);

Re: [PATCH v2 2/3] of_mdio: use IS_ERR_OR_NULL()

[Arnd Bergmann]

On Sunday 13 March 2016 00:34:02 Sergei Shtylyov wrote:
> IS_ERR_OR_NULL() is open coded in of_mdiobus_register_phy(), so just call
> it directly...
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>
> Reviewed-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> ---
> Changes in version 2:
> - removed the of_mdiobus_register_device() hunk;
> - added the "Reviewed-by:" tag.
> 
>  drivers/of/of_mdio.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: net-next/drivers/of/of_mdio.c
> ===================================================================
> --- net-next.orig/drivers/of/of_mdio.c
> +++ net-next/drivers/of/of_mdio.c
> @@ -56,7 +56,7 @@ static int of_mdiobus_register_phy(struc
>                 phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
>         else
>                 phy = get_phy_device(mdio, addr, is_c45);
> -       if (!phy || IS_ERR(phy))
> +       if (IS_ERR_OR_NULL(phy))
>                 return 1;
>  
>         rc = irq_of_parse_and_map(child, 0);
> 
> 

IS_ERR_OR_NULL() usually indicates that the code is wrong, or that
an API has been misdesigned.

Can you clarify in the changelog which one it is, or (better) change
it so that 'phy' in this function is either NULL or IS_ERR() in case
of an error but not both?

I think moving the 'if (problem) return 1' into the two if/else
is a correct transformation that also makes it very clear what
is going on here.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] of_mdio: use IS_ERR_OR_NULL()

[Sergei Shtylyov]

On 03/14/2016 01:22 AM, Arnd Bergmann wrote:

>> IS_ERR_OR_NULL() is open coded in of_mdiobus_register_phy(), so just call
>> it directly...
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
>>
>> ---
>> Changes in version 2:
>> - removed the of_mdiobus_register_device() hunk;
>> - added the "Reviewed-by:" tag.
>>
>>   drivers/of/of_mdio.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Index: net-next/drivers/of/of_mdio.c
>> ===================================================================
>> --- net-next.orig/drivers/of/of_mdio.c
>> +++ net-next/drivers/of/of_mdio.c
>> @@ -56,7 +56,7 @@ static int of_mdiobus_register_phy(struc
>>                  phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
>>          else
>>                  phy = get_phy_device(mdio, addr, is_c45);
>> -       if (!phy || IS_ERR(phy))
>> +       if (IS_ERR_OR_NULL(phy))
>>                  return 1;
>>
>>          rc = irq_of_parse_and_map(child, 0);
>>
>>
>
> IS_ERR_OR_NULL() usually indicates that the code is wrong, or that
> an API has been misdesigned.
>
> Can you clarify in the changelog which one it is,

    The second. :-)

> or (better) change
> it so that 'phy' in this function is either NULL or IS_ERR() in case
> of an error but not both?

    I guess you meant to change get_phy_device() to not return NULL (and so 
only return the error codes, not both), am I correct?

> I think moving the 'if (problem) return 1' into the two if/else
> is a correct transformation that also makes it very clear what
> is going on here.

    Can be done too... it's not that I have much time for that many respins 
though. It all started with 1 little patch (this one). :-)

> 	Arnd

MBR, Sergei

[PATCH v2 3/3] of_mdio: use PTR_ERR_OR_ZERO()

[Sergei Shtylyov]

PTR_ERR_OR_ZERO() is open coded in of_phy_register_fixed_link(), so just
call it directly...

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>

---
Changes in version 2:
- added the "Reviewed-by:" tags.

 drivers/of/of_mdio.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Index: net-next/drivers/of/of_mdio.c
===================================================================
--- net-next.orig/drivers/of/of_mdio.c
+++ net-next/drivers/of/of_mdio.c
@@ -412,7 +412,7 @@ int of_phy_register_fixed_link(struct de
 		if (strcmp(managed, "in-band-status") == 0) {
 			/* status is zeroed, namely its .link member */
 			phy = fixed_phy_register(PHY_POLL, &status, -1, np);
-			return IS_ERR(phy) ? PTR_ERR(phy) : 0;
+			return PTR_ERR_OR_ZERO(phy);
 		}
 	}
 
@@ -434,7 +434,7 @@ int of_phy_register_fixed_link(struct de
 			return -EPROBE_DEFER;
 
 		phy = fixed_phy_register(PHY_POLL, &status, link_gpio, np);
-		return IS_ERR(phy) ? PTR_ERR(phy) : 0;
+		return PTR_ERR_OR_ZERO(phy);
 	}
 
 	/* Old binding */
@@ -446,7 +446,7 @@ int of_phy_register_fixed_link(struct de
 		status.pause = be32_to_cpu(fixed_link_prop[3]);
 		status.asym_pause = be32_to_cpu(fixed_link_prop[4]);
 		phy = fixed_phy_register(PHY_POLL, &status, -1, np);
-		return IS_ERR(phy) ? PTR_ERR(phy) : 0;
+		return PTR_ERR_OR_ZERO(phy);
 	}
 
 	return -ENODEV;

Re: [PATCH v2 0/3] of_mdio: use IS_ERR_OR_NULL() and PTR_ERR_OR_ZERO()

[David Miller]

From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Sun, 13 Mar 2016 00:32:13 +0300

>    Here's the set of 3 patches against DaveM's 'net-next.git'
> repo. They deal with some error checks in the device tree MDIO
> code...

Series applied, thanks.