| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1491992.y6nTRlJ009@vostro.rjw.lan>
Date: Tue, 25 Feb 2014 15:10:24 +0100
From: "Rafael J. Wysocki" <rjw@...ysocki.net>
To: Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Alexandre Courbot <gnurou@...il.com>,
Lan Tianyu <tianyu.lan@...el.com>,
Lv Zheng <lv.zheng@...el.com>, Alan Cox <alan.cox@...el.com>,
Mathias Nyman <mathias.nyman@...ux.intel.com>,
linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/6] gpiolib: Allow GPIO chips to request their own GPIOs
On Monday, February 24, 2014 06:00:06 PM Mika Westerberg wrote:
> Sometimes it is useful to allow GPIO chips themselves to request GPIOs they
> own through gpiolib API. One usecase is ACPI ASL code that should be able
> to toggle GPIOs through GPIO operation regions.
>
> We can't really use gpio_request() and its counterparts because it will pin
> the module to the kernel forever (as it calls module_get()). Instead we
> provide a gpiolib internal functions gpiochip_request/free_own_desc() that
> work the same as gpio_request() but don't manipulate module refrence count.
>
> Since it's the GPIO chip driver who requests the GPIOs in the first place
> we can be sure that it cannot be unloaded without the driver knowing about
> that. Furthermore we only limit this functionality to be available only
> inside gpiolib.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@...ux.intel.com>
> ---
> drivers/gpio/gpiolib.c | 57 +++++++++++++++++++++++++++++++++++++++++++-------
> drivers/gpio/gpiolib.h | 3 +++
> 2 files changed, 53 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index f60d74bc2fce..489a63524eb6 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1458,7 +1458,8 @@ EXPORT_SYMBOL_GPL(gpiochip_remove_pin_ranges);
> * on each other, and help provide better diagnostics in debugfs.
> * They're called even less than the "set direction" calls.
> */
> -static int gpiod_request(struct gpio_desc *desc, const char *label)
> +static int __gpiod_request(struct gpio_desc *desc, const char *label,
> + bool module_refcount)
> {
> struct gpio_chip *chip;
> int status = -EPROBE_DEFER;
> @@ -1475,8 +1476,10 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
> if (chip == NULL)
> goto done;
>
> - if (!try_module_get(chip->owner))
> - goto done;
> + if (module_refcount) {
> + if (!try_module_get(chip->owner))
> + goto done;
> + }
I'm wondering why you're not moving the module refcount manipulation entirely
to gpiod_request()?
I guess that's because of the locking, but I suppose that desc->chip will never
be NULL in gpiochip_request_own_desc(), so you don't even need to check it there?
So it looks like gpiochip_request_own_desc() can do something like
lock
__gpiod_request(stuff)
unlock
where __gpiod_request() is just the internal part starting at the "NOTE" comment.
>
> /* NOTE: gpio_request() can be called in early boot,
> * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
> @@ -1487,7 +1490,8 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
> status = 0;
> } else {
> status = -EBUSY;
> - module_put(chip->owner);
> + if (module_refcount)
> + module_put(chip->owner);
> goto done;
> }
>
> @@ -1499,7 +1503,8 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
>
> if (status < 0) {
> desc_set_label(desc, NULL);
> - module_put(chip->owner);
> + if (module_refcount)
> + module_put(chip->owner);
> clear_bit(FLAG_REQUESTED, &desc->flags);
> goto done;
> }
> @@ -1517,13 +1522,18 @@ done:
> return status;
> }
>
> +static int gpiod_request(struct gpio_desc *desc, const char *label)
> +{
> + return __gpiod_request(desc, label, true);
> +}
> +
> int gpio_request(unsigned gpio, const char *label)
> {
> return gpiod_request(gpio_to_desc(gpio), label);
> }
> EXPORT_SYMBOL_GPL(gpio_request);
>
> -static void gpiod_free(struct gpio_desc *desc)
> +static void __gpiod_free(struct gpio_desc *desc, bool module_refcount)
> {
> unsigned long flags;
> struct gpio_chip *chip;
> @@ -1548,7 +1558,8 @@ static void gpiod_free(struct gpio_desc *desc)
> spin_lock_irqsave(&gpio_lock, flags);
> }
> desc_set_label(desc, NULL);
> - module_put(desc->chip->owner);
> + if (module_refcount)
> + module_put(desc->chip->owner);
> clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
> clear_bit(FLAG_REQUESTED, &desc->flags);
> clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
> @@ -1559,6 +1570,11 @@ static void gpiod_free(struct gpio_desc *desc)
> spin_unlock_irqrestore(&gpio_lock, flags);
> }
>
> +static void gpiod_free(struct gpio_desc *desc)
> +{
> + __gpiod_free(desc, true);
> +}
> +
> void gpio_free(unsigned gpio)
> {
> gpiod_free(gpio_to_desc(gpio));
> @@ -1678,6 +1694,33 @@ const char *gpiochip_is_requested(struct gpio_chip *chip, unsigned offset)
> }
> EXPORT_SYMBOL_GPL(gpiochip_is_requested);
>
> +/**
> + * gpiochip_request_own_desc - Allow GPIO chip to request its own descriptor
> + * @desc: GPIO descriptor to request
> + * @label: label for the GPIO
> + *
> + * Function allows GPIO chip drivers to request and use their own GPIO
> + * descriptors via gpiolib API. Difference to gpiod_request() is that this
> + * function will not increase reference count of the GPIO chip module. This
> + * allows the GPIO chip module to be unloaded as needed (we assume that the
> + * GPIO chip driver handles freeing the GPIOs it has requested).
> + */
> +int gpiochip_request_own_desc(struct gpio_desc *desc, const char *label)
> +{
> + return __gpiod_request(desc, label, false);
> +}
> +
> +/**
> + * gpiochip_free_own_desc - Free GPIO requested by the chip driver
> + * @desc: GPIO descriptor to free
> + *
> + * Function frees the given GPIO requested previously with
> + * gpiochip_request_own_desc().
> + */
> +void gpiochip_free_own_desc(struct gpio_desc *desc)
> +{
> + __gpiod_free(desc, false);
> +}
>
> /* Drivers MUST set GPIO direction before making get/set calls. In
> * some cases this is done in early boot, before IRQs are enabled.
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index 82be586c1f90..cf092941a9fd 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -43,4 +43,7 @@ acpi_get_gpiod_by_index(struct device *dev, int index,
> }
> #endif
>
> +int gpiochip_request_own_desc(struct gpio_desc *desc, const char *label);
> +void gpiochip_free_own_desc(struct gpio_desc *desc);
> +
> #endif /* GPIOLIB_H */
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists