Since commit aab5c6f20023 ("gpio: set device type for GPIO chips"),
`gdev->dev.release` is unset. As a result, the reference count to
`gdev->dev` isn't dropped on the error handling paths.
Drop the reference on errors.
Also reorder the instructions to make the intent more clear. Now
gpiochip_add_data_with_key() roughly looks like:
>>> Some memory allocation. Go to ERR ZONE 1 on errors.
>>> device_initialize().
(gpiodev_release() takes over the responsibility for freeing the
resources of `gdev->dev`. The following error handling paths
shouldn't go through ERR ZONE 1 again which leads to double free.)
>>> Some initialization mainly on `gdev`.
>>> The rest of initialization. Go to ERR ZONE 2 on errors.
>>> Chip registration success and exit.
>>> ERR ZONE 2. gpio_device_put() and exit.
>>> ERR ZONE 1.
Cc: [email protected]
Fixes: aab5c6f20023 ("gpio: set device type for GPIO chips")
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
drivers/gpio/gpiolib.c | 81 ++++++++++++++++++++----------------------
1 file changed, 38 insertions(+), 43 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index ba9323432e3a..6fac59716405 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -893,13 +893,15 @@ static const struct device_type gpio_dev_type = {
#define gcdev_unregister(gdev) device_del(&(gdev)->dev)
#endif
+/*
+ * An initial reference count has been held in gpiochip_add_data_with_key().
+ * The caller should drop the reference via gpio_device_put() on errors.
+ */
static int gpiochip_setup_dev(struct gpio_device *gdev)
{
struct fwnode_handle *fwnode = dev_fwnode(&gdev->dev);
int ret;
- device_initialize(&gdev->dev);
-
/*
* If fwnode doesn't belong to another device, it's safe to clear its
* initialized flag.
@@ -965,9 +967,11 @@ static void gpiochip_setup_devs(void)
list_for_each_entry_srcu(gdev, &gpio_devices, list,
srcu_read_lock_held(&gpio_devices_srcu)) {
ret = gpiochip_setup_dev(gdev);
- if (ret)
+ if (ret) {
+ gpio_device_put(gdev);
dev_err(&gdev->dev,
"Failed to initialize gpio device (%d)\n", ret);
+ }
}
}
@@ -1048,24 +1052,12 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc,
void *data,
int base = 0;
int ret;
- /*
- * First: allocate and populate the internal stat container, and
- * set up the struct device.
- */
gdev = kzalloc(sizeof(*gdev), GFP_KERNEL);
if (!gdev)
return -ENOMEM;
-
- gdev->dev.type = &gpio_dev_type;
- gdev->dev.bus = &gpio_bus_type;
- gdev->dev.parent = gc->parent;
- rcu_assign_pointer(gdev->chip, gc);
-
gc->gpiodev = gdev;
gpiochip_set_data(gc, data);
- device_set_node(&gdev->dev, gpiochip_choose_fwnode(gc));
-
ret = ida_alloc(&gpio_ida, GFP_KERNEL);
if (ret < 0)
goto err_free_gdev;
@@ -1075,17 +1067,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc,
void *data,
if (ret)
goto err_free_ida;
- if (gc->parent && gc->parent->driver)
- gdev->owner = gc->parent->driver->owner;
- else if (gc->owner)
- /* TODO: remove chip->owner */
- gdev->owner = gc->owner;
- else
- gdev->owner = THIS_MODULE;
-
ret = gpiochip_get_ngpios(gc, &gdev->dev);
if (ret)
goto err_free_dev_name;
+ gdev->ngpio = gc->ngpio;
gdev->descs = kcalloc(gc->ngpio, sizeof(*gdev->descs), GFP_KERNEL);
if (!gdev->descs) {
@@ -1099,21 +1084,37 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc,
void *data,
goto err_free_descs;
}
- gdev->ngpio = gc->ngpio;
- gdev->can_sleep = gc->can_sleep;
-
- rwlock_init(&gdev->line_state_lock);
- RAW_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
- BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier);
-
ret = init_srcu_struct(&gdev->srcu);
if (ret)
goto err_free_label;
+ rcu_assign_pointer(gdev->chip, gc);
ret = init_srcu_struct(&gdev->desc_srcu);
if (ret)
goto err_cleanup_gdev_srcu;
+ device_initialize(&gdev->dev);
+ /* From this point, the .release() function cleans up gdev->dev */
+ gdev->dev.type = &gpio_dev_type;
+ gdev->dev.bus = &gpio_bus_type;
+ gdev->dev.parent = gc->parent;
+ device_set_node(&gdev->dev, gpiochip_choose_fwnode(gc));
+
+ gdev->can_sleep = gc->can_sleep;
+ rwlock_init(&gdev->line_state_lock);
+ RAW_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
+ BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier);
+#ifdef CONFIG_PINCTRL
+ INIT_LIST_HEAD(&gdev->pin_ranges);
+#endif
+ if (gc->parent && gc->parent->driver)
+ gdev->owner = gc->parent->driver->owner;
+ else if (gc->owner)
+ /* TODO: remove chip->owner */
+ gdev->owner = gc->owner;
+ else
+ gdev->owner = THIS_MODULE;
+
scoped_guard(mutex, &gpio_devices_lock) {
/*
* TODO: this allocates a Linux GPIO number base in the global
@@ -1128,7 +1129,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void
*data,
if (base < 0) {
ret = base;
base = 0;
- goto err_cleanup_desc_srcu;
+ goto err_put_device;
}
/*
@@ -1148,14 +1149,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc,
void *data,
ret = gpiodev_add_to_list_unlocked(gdev);
if (ret) {
gpiochip_err(gc, "GPIO integer space overlap, cannot
add chip\n");
- goto err_cleanup_desc_srcu;
+ goto err_put_device;
}
}
-#ifdef CONFIG_PINCTRL
- INIT_LIST_HEAD(&gdev->pin_ranges);
-#endif
-
if (gc->names)
gpiochip_set_desc_names(gc);
@@ -1249,13 +1246,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc,
void *data,
scoped_guard(mutex, &gpio_devices_lock)
list_del_rcu(&gdev->list);
synchronize_srcu(&gpio_devices_srcu);
- if (gdev->dev.release) {
- /* release() has been registered by gpiochip_setup_dev() */
- gpio_device_put(gdev);
- goto err_print_message;
- }
-err_cleanup_desc_srcu:
- cleanup_srcu_struct(&gdev->desc_srcu);
+err_put_device:
+ gpio_device_put(gdev);
+ goto err_print_message;
+
err_cleanup_gdev_srcu:
cleanup_srcu_struct(&gdev->srcu);
err_free_label:
@@ -1268,6 +1262,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void
*data,
ida_free(&gpio_ida, gdev->id);
err_free_gdev:
kfree(gdev);
+
err_print_message:
/* failures here can mean systems won't boot... */
if (ret != -EPROBE_DEFER) {
--
2.52.0.457.g6b5491de43-goog
