[v4,05/11] gpiolib: provide gpio_device_get_desc()
Commit Message
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Getting the GPIO descriptor directly from the gpio_chip struct is
dangerous as we don't take the reference to the underlying GPIO device.
In order to start working towards removing gpiochip_get_desc(), let's
provide a safer variant that works with an existing reference to struct
gpio_device.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/gpio/gpiolib.c | 46 +++++++++++++++++++++++++++----------
include/linux/gpio/driver.h | 2 ++
2 files changed, 36 insertions(+), 12 deletions(-)
Comments
On Wed, Sep 27, 2023 at 04:29:25PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Getting the GPIO descriptor directly from the gpio_chip struct is
> dangerous as we don't take the reference to the underlying GPIO device.
> In order to start working towards removing gpiochip_get_desc(), let's
> provide a safer variant that works with an existing reference to struct
> gpio_device.
...
> +EXPORT_SYMBOL_GPL(gpiochip_get_desc);
> +struct gpio_desc *
> +gpio_device_get_desc(struct gpio_device *gdev, unsigned int hwnum)
I'm wondering if you move this to be upper than gpiochip_get_desc() and
diff will look better...
On Mon, Oct 2, 2023 at 11:46 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Sep 27, 2023 at 04:29:25PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Getting the GPIO descriptor directly from the gpio_chip struct is
> > dangerous as we don't take the reference to the underlying GPIO device.
> > In order to start working towards removing gpiochip_get_desc(), let's
> > provide a safer variant that works with an existing reference to struct
> > gpio_device.
>
> ...
>
> > +EXPORT_SYMBOL_GPL(gpiochip_get_desc);
>
> > +struct gpio_desc *
> > +gpio_device_get_desc(struct gpio_device *gdev, unsigned int hwnum)
>
> I'm wondering if you move this to be upper than gpiochip_get_desc() and
> diff will look better...
>
There's a limit to bikeshedding in my book and "making the diff look
better" is definitely it. :)
Bart
On Mon, Oct 02, 2023 at 11:54:40AM +0200, Bartosz Golaszewski wrote:
> On Mon, Oct 2, 2023 at 11:46 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Sep 27, 2023 at 04:29:25PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > Getting the GPIO descriptor directly from the gpio_chip struct is
> > > dangerous as we don't take the reference to the underlying GPIO device.
> > > In order to start working towards removing gpiochip_get_desc(), let's
> > > provide a safer variant that works with an existing reference to struct
> > > gpio_device.
...
> > > +EXPORT_SYMBOL_GPL(gpiochip_get_desc);
> >
> > > +struct gpio_desc *
> > > +gpio_device_get_desc(struct gpio_device *gdev, unsigned int hwnum)
> >
> > I'm wondering if you move this to be upper than gpiochip_get_desc() and
> > diff will look better...
>
> There's a limit to bikeshedding in my book and "making the diff look
> better" is definitely it. :)
Right, but if you are going to send a new version it might makes sense
to try, no?
@@ -147,27 +147,49 @@ struct gpio_desc *gpio_to_desc(unsigned gpio)
}
EXPORT_SYMBOL_GPL(gpio_to_desc);
-/**
- * gpiochip_get_desc - get the GPIO descriptor corresponding to the given
- * hardware number for this chip
- * @gc: GPIO chip
- * @hwnum: hardware number of the GPIO for this chip
- *
- * Returns:
- * A pointer to the GPIO descriptor or ``ERR_PTR(-EINVAL)`` if no GPIO exists
- * in the given chip for the specified hardware number.
- */
+/* This function is deprecated and will be removed soon, don't use. */
struct gpio_desc *gpiochip_get_desc(struct gpio_chip *gc,
unsigned int hwnum)
{
- struct gpio_device *gdev = gc->gpiodev;
+ return gpio_device_get_desc(gc->gpiodev, hwnum);
+}
+EXPORT_SYMBOL_GPL(gpiochip_get_desc);
+
+/**
+ * gpio_device_get_desc() - get the GPIO descriptor corresponding to the given
+ * hardware number for this GPIO device
+ * @gdev: GPIO device to get the descriptor from
+ * @hwnum: hardware number of the GPIO for this chip
+ *
+ * Returns:
+ * A pointer to the GPIO descriptor or %EINVAL if no GPIO exists in the given
+ * chip for the specified hardware number or %ENODEV if the underlying chip
+ * already vanished.
+ *
+ * The reference count of struct gpio_device is *NOT* increased like when the
+ * GPIO is being requested for exclusive usage. It's up to the caller to make
+ * sure the GPIO device will stay alive together with the descriptor returned
+ * by this function.
+ */
+struct gpio_desc *
+gpio_device_get_desc(struct gpio_device *gdev, unsigned int hwnum)
+{
+ struct gpio_chip *gc;
+
+ /*
+ * FIXME: This will be locked once we protect gdev->chip everywhere
+ * with SRCU.
+ */
+ gc = gdev->chip;
+ if (!gc)
+ return ERR_PTR(-ENODEV);
if (hwnum >= gdev->ngpio)
return ERR_PTR(-EINVAL);
return &gdev->descs[hwnum];
}
-EXPORT_SYMBOL_GPL(gpiochip_get_desc);
+EXPORT_SYMBOL_GPL(gpio_device_get_desc);
/**
* desc_to_gpio - convert a GPIO descriptor to the integer namespace
@@ -770,6 +770,8 @@ struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
void gpiochip_free_own_desc(struct gpio_desc *desc);
struct gpio_desc *gpiochip_get_desc(struct gpio_chip *gc, unsigned int hwnum);
+struct gpio_desc *
+gpio_device_get_desc(struct gpio_device *gdev, unsigned int hwnum);
#ifdef CONFIG_GPIOLIB