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".
Attachment:
signature.asc
Description: PGP signature
