[RFD] gpiolib: gpiochip is always dangling after remove

Message ID 1456491974-26997-1-git-send-email-bamv2005@gmail.com
State New
Headers show

Commit Message

Bamvor Zhang Feb. 26, 2016, 1:06 p.m.
From: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>


In my gpiochip mockup driver, after call gpiochip_remove, the
gpipchip is not actually removed even if there is no clients in
userspace. It could be removed if remove the corresonding reference
count in this patch.

But such gpiochip will be removed even if user space open the
corresponding chardev. I am not sure If I do not right thing.
Please correct me if I am wrong.

Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>

---
 drivers/gpio/gpiolib.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

-- 
2.6.2

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

Comments

Bamvor Zhang Jian Feb. 27, 2016, 3:08 a.m. | #1
Hi, Mark

On 26 February 2016 at 22:05, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Feb 26, 2016 at 09:06:14PM +0800, Bamvor Jian Zhang wrote:

>

>> --- a/drivers/gpio/gpiolib.c

>> +++ b/drivers/gpio/gpiolib.c

>> @@ -452,7 +452,6 @@ static void gpiodevice_release(struct device *dev)

>>  {

>>       struct gpio_device *gdev = dev_get_drvdata(dev);

>>

>> -     cdev_del(&gdev->chrdev);

>

> This seems weird - we're moving the deletion of the chardev (which is

> the route userspace has to opening the device) later which seems like it

> isn't relevant to the issue and is likely to create problems since it

> means userspace can start to try to use the device while we're in the

> process of trying to tear it down.  If this is needed it should probably

> be explicitly discussed in the changelog, it may be worth splitting into

> a separate patch.

>

>> @@ -633,7 +632,6 @@ int gpiochip_add_data(struct gpio_chip *chip, void *data)

>>

>>       /* From this point, the .release() function cleans up gpio_device */

>>       gdev->dev.release = gpiodevice_release;

>> -     get_device(&gdev->dev);

>>       pr_debug("%s: registered GPIOs %d to %d on device: %s (%s)\n",

>>                __func__, gdev->base, gdev->base + gdev->ngpio - 1,

>>                dev_name(&gdev->dev), chip->label ? : "generic");

>

>> +     device_del(&gdev->dev);

>

> We're removing a get but adding a delete?  Again this is surprising,

> explicitly saying what took the reference we're going to delete would

> probably make this a lot clearer.

>

> In general I'd say your changelog for a change like this should be in

> the form of "When $THING happens $PROBLEM occurs because $REASON,

> instead do $FIX which avoids that because $REASON".

As you said, the commit message is not clear enough to know what is
going on here. Try this one:

When gpiochip_remove is called the gpiochips is not removed because
the refcount is not going down to zero.

The issue I found is that after gpiochip_remove, the gpipchio is not
remove(in dangling state), the reference count in
gdev(gdev->dev->kobj->kref->refcount.count) is 4. So, my first thought
is that where the reference count came from.
On gpiochip_add_data:
refcount    after this function
1           device_initialize(&gdev->dev);
2           status = cdev_add(&gdev->chrdev, gdev->dev.devt, 1);
4           status = device_add(&gdev->dev);
5           get_device(&gdev->dev);
Notes: "gdev->chrdev.kobj.parent" is "&gdev->dev.kobj;"

On gpiochip_remove:
refcount    after this function
4           put_device(&gdev->dev);

And I also check the other code in which chardev is the children of
the device. I found that the flows of add and remove are(e.g.
evdev.c):
Add(e.g. evdev_connect):
device_initialize();
cdev_add();
device_add();

Remove(e.g. evdev_disconnect):
device_del();
cdev_del();
put_device();

If I change the flows in gpiochip_add_data and gpiochip_remove like
above. The gpiochip could be removed in the put_device. But it seems
that it conflict with the comment before put_device:
/*
 * The gpiochip side puts its use of the device to rest here:
 * if there are no userspace clients, the chardev and device will
 * be removed, else it will be dangling until the last user is
 * gone.
 */

So, I feel that maybe I do not fix root issue. Hope I could learn/
help.

Regards

Bamvor
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij March 8, 2016, 8:53 a.m. | #2
On Fri, Feb 26, 2016 at 8:06 PM, Bamvor Jian Zhang <bamv2005@gmail.com> wrote:

> From: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>

>

> In my gpiochip mockup driver, after call gpiochip_remove, the

> gpipchip is not actually removed even if there is no clients in

> userspace. It could be removed if remove the corresonding reference

> count in this patch.

>

> But such gpiochip will be removed even if user space open the

> corresponding chardev. I am not sure If I do not right thing.

> Please correct me if I am wrong.

>

> Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>


I think you're onto the right thing, thank you for looking into this.

My idea was that the gdev->dev and gdev->cdev should stay around
until the last userspace user is gone, even if the gpiochip_remove()
was called.

Maybe I just got the refcounts wrong :(

It'd be great if you can come up with a patch that has the following
properties:

gpiochip_add_data(), no userspace users, gpiochip_remove()
-> refcount goes to zero and gpiodevice_release() gets called

gpiochip_add_data(), add some userspace users, gpiochip_remove()
-> refcount does not go to zero and gpiodevice_release() is not called
Then remove the userspace users:
-> refcount goes to zero and gpiodevice_release() gets called

It might be that using device_add(), device_del() directly like I'm
doing cannot really solve this :( then we need to rethink the mechanism
in terms of device_register()/device_unregister(), but I couldn't
get that to work with the way chardevs are added.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bamvor Zhang Jian March 9, 2016, 5:32 a.m. | #3
Hi, Linus

On 8 March 2016 at 15:53, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Feb 26, 2016 at 8:06 PM, Bamvor Jian Zhang <bamv2005@gmail.com> wrote:

>

>> From: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>

>>

>> In my gpiochip mockup driver, after call gpiochip_remove, the

>> gpipchip is not actually removed even if there is no clients in

>> userspace. It could be removed if remove the corresonding reference

>> count in this patch.

>>

>> But such gpiochip will be removed even if user space open the

>> corresponding chardev. I am not sure If I do not right thing.

>> Please correct me if I am wrong.

>>

>> Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>

>

> I think you're onto the right thing, thank you for looking into this.

>

> My idea was that the gdev->dev and gdev->cdev should stay around

> until the last userspace user is gone, even if the gpiochip_remove()

> was called.

>

> Maybe I just got the refcounts wrong :(

>

> It'd be great if you can come up with a patch that has the following

> properties:

>

> gpiochip_add_data(), no userspace users, gpiochip_remove()

> -> refcount goes to zero and gpiodevice_release() gets called

>

> gpiochip_add_data(), add some userspace users, gpiochip_remove()

> -> refcount does not go to zero and gpiodevice_release() is not called

> Then remove the userspace users:

> -> refcount goes to zero and gpiodevice_release() gets called

>

> It might be that using device_add(), device_del() directly like I'm

> doing cannot really solve this :( then we need to rethink the mechanism

> in terms of device_register()/device_unregister(), but I couldn't

> get that to work with the way chardevs are added.

Ok, I will do it.

Regards

Bamvor
> Yours,

> Linus Walleij

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

Patch hide | download patch | download mbox

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index bc788b9..862b574 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -452,7 +452,6 @@  static void gpiodevice_release(struct device *dev)
 {
 	struct gpio_device *gdev = dev_get_drvdata(dev);
 
-	cdev_del(&gdev->chrdev);
 	list_del(&gdev->list);
 	ida_simple_remove(&gpio_ida, gdev->id);
 	kfree(gdev);
@@ -633,7 +632,6 @@  int gpiochip_add_data(struct gpio_chip *chip, void *data)
 
 	/* From this point, the .release() function cleans up gpio_device */
 	gdev->dev.release = gpiodevice_release;
-	get_device(&gdev->dev);
 	pr_debug("%s: registered GPIOs %d to %d on device: %s (%s)\n",
 		 __func__, gdev->base, gdev->base + gdev->ngpio - 1,
 		 dev_name(&gdev->dev), chip->label ? : "generic");
@@ -713,12 +711,8 @@  void gpiochip_remove(struct gpio_chip *chip)
 		dev_crit(&gdev->dev,
 			 "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED\n");
 
-	/*
-	 * The gpiochip side puts its use of the device to rest here:
-	 * if there are no userspace clients, the chardev and device will
-	 * be removed, else it will be dangling until the last user is
-	 * gone.
-	 */
+	device_del(&gdev->dev);
+	cdev_del(&gdev->chrdev);
 	put_device(&gdev->dev);
 }
 EXPORT_SYMBOL_GPL(gpiochip_remove);