Skip to contentSkip to conversation

pinmux: fix pinctrl_register error handling

RFC PATCH In review Axel Lin 2 participants 4 messages Sep 30, 2011 – Oct 5, 2011 Pin control

lore
Share & tools
lore

Axel Lin

author
#0 lore raw

RFC PATCH 1/1 pinmux: fix pinctrl_register error handling

If device_register fails, current code kfree pctldev multiple times. current code calls "put_device(&pctldev->dev);kfree(pctldev);" before and after goto out_err.

The correct fix should be:
If device_register fails, we just need to call put_device(&pctldev->dev); Since we have "release" callback in pinctrl_type, we don't need to call kfree(pctldev) after put_device().

If pinctrl_register_pins fails, we need to delete the successfully registerd device from system by calling device_del().

In pinctrl_unregister(), calling device_unregister(&pctldev->dev) will free pctldev ( by the release callback ).
We should not call kfree(pctldev) after device_unregister().

---
Hi Linus,
The fix is base on my understanding of the driver-model code, and it is untested ( only compile test), thus I add [RFC] on subject line. Also CC Greg KH who is the maintainer of drivers/base/*.

Regards,
Axel

 drivers/pinctrl/core.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)
drivers/pinctrl/core.c +6 -8
index f9263b2..28bdb74 100644
@@ -559,9 +559,7 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
 	ret = device_register(&pctldev->dev);
 	if (ret != 0) {
 		pr_err("error in device registration\n");
-		put_device(&pctldev->dev);
-		kfree(pctldev);
-		goto out_err;
+		goto out_reg_dev_err;
 	}
 	dev_set_drvdata(&pctldev->dev, pctldev);
 
@@ -573,7 +571,7 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
 		pr_err("error during pin registration\n");
 		pinctrl_free_pindescs(pctldev, pctldesc->pins,
 				      pctldesc->npins);
-		goto out_err;
+		goto out_reg_pins_err;
 	}
 
 	pinctrl_init_device_debugfs(pctldev);
@@ -583,9 +581,10 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
 	pinmux_hog_maps(pctldev);
 	return pctldev;
 
-out_err:
+out_reg_pins_err:
+	device_del(&pctldev->dev);
+out_reg_dev_err:
 	put_device(&pctldev->dev);
-	kfree(pctldev);
 	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(pinctrl_register);
@@ -606,11 +605,10 @@ void pinctrl_unregister(struct pinctrl_dev *pctldev)
 	mutex_lock(&pinctrldev_list_mutex);
 	list_del(&pctldev->node);
 	mutex_unlock(&pinctrldev_list_mutex);
-	device_unregister(&pctldev->dev);
 	/* Destroy descriptor tree */
 	pinctrl_free_pindescs(pctldev, pctldev->desc->pins,
 			      pctldev->desc->npins);
-	kfree(pctldev);
+	device_unregister(&pctldev->dev);
 }
 EXPORT_SYMBOL_GPL(pinctrl_unregister);
 
Signature · 1 line
1.7.4.1
#1 lore raw
Quoted 4 lines · Axel Lin

Hi Linus,
The fix is base on my understanding of the driver-model code, and it is untested ( only compile test), thus I add [RFC] on subject line. Also CC Greg KH who is the maintainer of drivers/base/*.

You understand it better than me for sure, I have rebased the patch on the v9 patchset and applied it to my tree.

Thanks!
Linus Walleij

author
#2 lore raw

2011/10/3 Linus Walleij <[email protected]>:

Quoted 10 lines · Axel Lin
Quoted 5 lines

Hi Linus,
The fix is base on my understanding of the driver-model code, and it is untested ( only compile test), thus I add [RFC] on subject line. Also CC Greg KH who is the maintainer of drivers/base/*.

You understand it better than me for sure, I have rebased the patch on the v9 patchset and applied it to my tree.

Hi Linus,
It seems it is not fixed in your v9.
I read the v9 patch from here:
https://lkml.org/lkml/2011/10/3/62

Regards,
Axel

#3 lore raw
Quoted 8 lines · Axel Lin

2011/10/3 Linus Walleij <[email protected]>:

Quoted 3 lines

You understand it better than me for sure, I have rebased the patch on the v9 patchset and applied it to my tree.

Hi Linus,
It seems it is not fixed in your v9.
I read the v9 patch from here:
https://lkml.org/lkml/2011/10/3/62

I have not squashed it into my patch, instead I applied it on top and pushed it to my for-next branch.

Hopefully if you check in linux-next it'll be there...

Yours,
Linus Walleij