| Message ID | 1456491974-26997-1-git-send-email-bamv2005@gmail.com |
|---|---|
| State | New |
| Headers | show |
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".
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
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
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
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);