[PATCH 1/2] gpiolib: irqchip: prevent driver unloading if gpio is used as irq only

[PATCH 1/2] gpiolib: irqchip: prevent driver unloading if gpio is used as irq only

[Grygorii Strashko]

Now nothing prevents GPIO driver from being unloaded if its gpios
were requested as GPIO IRQs only (without calling gpio_request()).

Hence, add calls of try_module_get()/module_put() into
gpiochip_irq_reqres/relres() to track such scenario properly.

Cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpiolib.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index be42ab3..9b1d247 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -522,10 +522,14 @@ static int gpiochip_irq_reqres(struct irq_data *d)
 {
 	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
 
+	if (!try_module_get(chip->owner))
+		return -ENODEV;
+
 	if (gpiochip_lock_as_irq(chip, d->hwirq)) {
 		chip_err(chip,
 			"unable to lock HW IRQ %lu for IRQ\n",
 			d->hwirq);
+		module_put(chip->owner);
 		return -EINVAL;
 	}
 	return 0;
@@ -536,6 +540,7 @@ static void gpiochip_irq_relres(struct irq_data *d)
 	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
 
 	gpiochip_unlock_as_irq(chip, d->hwirq);
+	module_put(chip->owner);
 }
 
 static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
-- 
2.4.4

[PATCH 2/2] gpiolib: assign chip owner to dev->driver->owner if not set

[Grygorii Strashko]

Assign GPIO chip owner field to chip->dev->driver->owner if it was not
configured by GPIO driver.

Cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
Hi,

 There is also one positive additional side effect:
 lines like below can be removed from a lot of GPIO
 drivers
	rdc321x_gpio_dev->chip.owner = THIS_MODULE;

 drivers/gpio/gpiolib.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 9b1d247..d51af5d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -287,6 +287,9 @@ int gpiochip_add(struct gpio_chip *chip)
 	INIT_LIST_HEAD(&chip->pin_ranges);
 #endif
 
+	if (!chip->owner && chip->dev && chip->dev->driver)
+		chip->owner = chip->dev->driver->owner;
+
 	of_gpiochip_add(chip);
 	acpi_gpiochip_add(chip);
 
-- 
2.4.4

Re: [PATCH 2/2] gpiolib: assign chip owner to dev->driver->owner if not set

[Alexandre Courbot]

On Fri, Jun 26, 2015 at 2:30 AM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> Assign GPIO chip owner field to chip->dev->driver->owner if it was not
> configured by GPIO driver.

Sounds good, setting this field manually seems to make less and less
sense anyway as most drivers follow the device model.

>
> Cc: Johan Hovold <johan@kernel.org>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
> Hi,
>
>  There is also one positive additional side effect:
>  lines like below can be removed from a lot of GPIO
>  drivers
>         rdc321x_gpio_dev->chip.owner = THIS_MODULE;

A good idea for a follow-up patch! ;)

Acked-by: Alexandre Courbot <acourbot@nvidia.com>

Re: [PATCH 1/2] gpiolib: irqchip: prevent driver unloading if gpio is used as irq only

[Alexandre Courbot]

On Fri, Jun 26, 2015 at 2:30 AM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> Now nothing prevents GPIO driver from being unloaded if its gpios
> were requested as GPIO IRQs only (without calling gpio_request()).
>
> Hence, add calls of try_module_get()/module_put() into
> gpiochip_irq_reqres/relres() to track such scenario properly.

Bad things could happen indeed.

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

>
> Cc: Johan Hovold <johan@kernel.org>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
>  drivers/gpio/gpiolib.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index be42ab3..9b1d247 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -522,10 +522,14 @@ static int gpiochip_irq_reqres(struct irq_data *d)
>  {
>         struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>
> +       if (!try_module_get(chip->owner))
> +               return -ENODEV;
> +
>         if (gpiochip_lock_as_irq(chip, d->hwirq)) {
>                 chip_err(chip,
>                         "unable to lock HW IRQ %lu for IRQ\n",
>                         d->hwirq);
> +               module_put(chip->owner);
>                 return -EINVAL;
>         }
>         return 0;
> @@ -536,6 +540,7 @@ static void gpiochip_irq_relres(struct irq_data *d)
>         struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>
>         gpiochip_unlock_as_irq(chip, d->hwirq);
> +       module_put(chip->owner);
>  }
>
>  static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
> --
> 2.4.4
>

Re: [PATCH 1/2] gpiolib: irqchip: prevent driver unloading if gpio is used as irq only

[Linus Walleij]

On Thu, Jun 25, 2015 at 7:30 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> Now nothing prevents GPIO driver from being unloaded if its gpios
> were requested as GPIO IRQs only (without calling gpio_request()).
>
> Hence, add calls of try_module_get()/module_put() into
> gpiochip_irq_reqres/relres() to track such scenario properly.
>
> Cc: Johan Hovold <johan@kernel.org>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied with Alexandre's review tag.

Yours,
Linus Walleij

Re: [PATCH 2/2] gpiolib: assign chip owner to dev->driver->owner if not set

[Linus Walleij]

On Thu, Jun 25, 2015 at 7:30 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> Assign GPIO chip owner field to chip->dev->driver->owner if it was not
> configured by GPIO driver.
>
> Cc: Johan Hovold <johan@kernel.org>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied with Alex' review tag.

>  There is also one positive additional side effect:
>  lines like below can be removed from a lot of GPIO
>  drivers
>         rdc321x_gpio_dev->chip.owner = THIS_MODULE;

Yes let's do this :)

Yours,
Linus Walleij

Re: [PATCH 2/2] gpiolib: assign chip owner to dev->driver->owner if not set

[Linus Walleij]

On Thu, Jul 16, 2015 at 1:22 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Jun 25, 2015 at 7:30 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
>
>> Assign GPIO chip owner field to chip->dev->driver->owner if it was not
>> configured by GPIO driver.
>>
>> Cc: Johan Hovold <johan@kernel.org>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>
> Patch applied with Alex' review tag.
>
>>  There is also one positive additional side effect:
>>  lines like below can be removed from a lot of GPIO
>>  drivers
>>         rdc321x_gpio_dev->chip.owner = THIS_MODULE;
>
> Yes let's do this :)

Or actually, I have had some second thought to why gpio_chip
is duplicating struct members from struct device at all.

Why should it even have "owner" and "of_node"?

Should we not just rewrite this code to follow the struct device *dev
pointer in gpio_chip and use "owner" and "of_node" from there?

Yours,
Linus Walleij

Re: [PATCH 2/2] gpiolib: assign chip owner to dev->driver->owner if not set

[Grygorii Strashko]

On 07/16/2015 02:25 PM, Linus Walleij wrote:
> On Thu, Jul 16, 2015 at 1:22 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Thu, Jun 25, 2015 at 7:30 PM, Grygorii Strashko
>> <grygorii.strashko@ti.com> wrote:
>>
>>> Assign GPIO chip owner field to chip->dev->driver->owner if it was not
>>> configured by GPIO driver.
>>>
>>> Cc: Johan Hovold <johan@kernel.org>
>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>>
>> Patch applied with Alex' review tag.
>>
>>>   There is also one positive additional side effect:
>>>   lines like below can be removed from a lot of GPIO
>>>   drivers
>>>          rdc321x_gpio_dev->chip.owner = THIS_MODULE;
>>
>> Yes let's do this :)
>
> Or actually, I have had some second thought to why gpio_chip
> is duplicating struct members from struct device at all.
>
> Why should it even have "owner" and "of_node"?
>
> Should we not just rewrite this code to follow the struct device *dev
> pointer in gpio_chip and use "owner" and "of_node" from there?
>

Seems not all drivers implemented using Dev/Driver approach,
so they don't have dev at all ;)

gpio-samsung.c for example (non-DT driver).


-- 
regards,
-grygorii