Skip to contentSkip to conversation

can: c_can: checking IS_ERR() instead of NULL

PATCH In review Dan Carpenter 3 participants 5 messages Aug 1, 2014

Subsystems (2)
Kernel janitors CAN network drivers
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 can: c_can: checking IS_ERR() instead of NULL

devm_ioremap() returns NULL on error, not an ERR_PTR().

Fixes 33cf75656923 ('can: c_can_platform: Fix raminit, use devm_ioremap() instead of devm_ioremap_resource()')
Signed-off-by Dan Carpenter <[email protected]>

---
This will need to go to -stable because the patch which introduces the bug is going to be pushed to -stable.

drivers/net/can/c_can/c_can_platform.c +1 -1
index 5dede6e..109cb44 100644
@@ -280,7 +280,7 @@ static int c_can_plat_probe(struct platform_device *pdev)
 
 		priv->raminit_ctrlreg = devm_ioremap(&pdev->dev, res->start,
 						     resource_size(res));
-		if (IS_ERR(priv->raminit_ctrlreg) || priv->instance < 0)
+		if (!priv->raminit_ctrlreg || priv->instance < 0)
 			dev_info(&pdev->dev, "control memory is not used for raminit\n");
 		else
 			priv->raminit = c_can_hw_raminit_ti;
maintainer
#1 lore raw
Quoted 3 lines · Dan Carpenter

devm_ioremap() returns NULL on error, not an ERR_PTR().

Fixes 33cf75656923 ('can: c_can_platform: Fix raminit, use devm_ioremap() instead of devm_ioremap_resource()')

Doh!

Quoted 10 lines
Signed-off-by Dan Carpenter <[email protected]>

---
This will need to go to -stable because the patch which introduces the bug is going to be pushed to -stable.

drivers/net/can/c_can/c_can_platform.c +0 -0
index 5dede6e..109cb44 100644

drivers/net/can/c_can/c_can_platform.c static int c_can_plat_probe(struct platform_device *pdev) · line 283

Quoted 5 lines
drivers/net/can/c_can/c_can_platform.c +1 -1
@@ -280,7 +280,7 @@ static int c_can_plat_probe(struct platform_device *pdev)
 
 		priv->raminit_ctrlreg = devm_ioremap(&pdev->dev, res->start,
 						     resource_size(res));
-		if (IS_ERR(priv->raminit_ctrlreg) || priv->instance < 0)
+		if (!priv->raminit_ctrlreg || priv->instance < 0)

What about using PTR_ERR_OR_ZERO()?

drivers/net/can/c_can/c_can_platform.c static int c_can_plat_probe(struct platform_device *pdev) · line 286

Quoted 4 lines
drivers/net/can/c_can/c_can_platform.c +0 -0
 			dev_info(&pdev->dev, "control memory is not used for raminit\n");
 		else
 			priv->raminit = c_can_hw_raminit_ti;

Marc

Signature · 4 lines
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |
author
#2 lore raw
Quoted 6 lines · Marc Kleine-Budde
Quoted 6 lines
drivers/net/can/c_can/c_can_platform.c +0 -0
index 5dede6e..109cb44 100644
@@ -280,7 +280,7 @@ static int c_can_plat_probe(struct platform_device *pdev)
Quoted 6 lines

drivers/net/can/c_can/c_can_platform.c static int c_can_plat_probe(struct platform_device *pdev) · line 283

Quoted 4 lines
drivers/net/can/c_can/c_can_platform.c +1 -1
 		priv->raminit_ctrlreg = devm_ioremap(&pdev->dev, res->start,
 						     resource_size(res));
-		if (IS_ERR(priv->raminit_ctrlreg) || priv->instance < 0)
+		if (!priv->raminit_ctrlreg || priv->instance < 0)

What about using PTR_ERR_OR_ZERO()?

No. It doesn't ever return a PTR_ERR(). Checking for that would just introduce a new static checker warning (signs of confusion may indicate buggy code).

regards,
dan carpenter

maintainer
#3 lore raw
Quoted 15 lines · Dan Carpenter
Quoted 6 lines · Marc Kleine-Budde
Quoted 6 lines
drivers/net/can/c_can/c_can_platform.c +0 -0
index 5dede6e..109cb44 100644
@@ -280,7 +280,7 @@ static int c_can_plat_probe(struct platform_device *pdev)
Quoted 6 lines

drivers/net/can/c_can/c_can_platform.c static int c_can_plat_probe(struct platform_device *pdev) · line 283

Quoted 4 lines
drivers/net/can/c_can/c_can_platform.c +1 -1
 		priv->raminit_ctrlreg = devm_ioremap(&pdev->dev, res->start,
 						     resource_size(res));
-		if (IS_ERR(priv->raminit_ctrlreg) || priv->instance < 0)
+		if (!priv->raminit_ctrlreg || priv->instance < 0)

What about using PTR_ERR_OR_ZERO()?

Quoted 3 lines

No. It doesn't ever return a PTR_ERR(). Checking for that would just introduce a new static checker warning (signs of confusion may indicate buggy code).

Okay, thanks for the clarification.

Marc

Signature · 4 lines
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |
#4 lore raw
In reply to Dan Carpenter
Quoted 1 line
Fixes 33cf75656923 ('can: c_can_platform: Fix raminit, use devm_ioremap() instead of devm_ioremap_resource()')

Reverting this patch is the correct solution. Its approach is totally bogus anyhow.