Re: [PATCH 1/2] prevent gpio chip drivers from unloading while used

[David Brownell]

On Friday 08 February 2008, Guennadi Liakhovetski wrote:
> As long as one or more GPIOs on a gpio chip are used its driver should not 
> be unloaded.

The mechanism currently in place is to have gpiochip_remove() fail
if the platform's teardown() logic doesn't reject it.  (It may be
practical to have the teardown code get rid of the users...)  That
would be reflected as an "rmmod" failure.

Are you saying that doesn't work?


I'm not a huge fan of explicit manipulation of module refcounts,
though there can be a place for such things.

Supporting removal of a gpio_chip is my least favorite feature of
this framework.  Especially for controller drivers which could
realistically manage per-GPIO IRQs ... since genirq doesn't seem to
like the notion of irq_chip removal.  (The MCP23s08 and its I2C
sibling the MCP23008 have real IRQ control logic, but the pca953x
and pcf857x chips have a much less functional IRQ mechanism.)


> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxxxxxxxxxx>
> 
> ---
> 
> Note, that existing drivers do not have to be modified,

If they call gpiochip_remove(), they should be modified to ensure
they initialize this new "owner" field.  That would be all of the
drivers currently in drivers/gpio (not just pca953x).


> for example those,  
> that are always statically linked in the kernel, as long as the respective 
> struct gpio_chip is nullified before calling gpiochip_add().

By "nullified", I presume you mean to suggest that the "owner" field
be initialized to NULL ... which will normally be the case, when the
gpio_chip is a static data structure or comes from kzalloc().

 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index d8db2f8..dd535e1 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -177,6 +177,9 @@ int gpio_request(unsigned gpio, const char *label)
>  	if (desc->chip == NULL)
>  		goto done;
>  
> +	if (!try_module_get(desc->chip->owner))
> +		goto done;
> +
>  	/* NOTE:  gpio_request() can be called in early boot,
>  	 * before IRQs are enabled.
>  	 */
> @@ -184,8 +187,10 @@ int gpio_request(unsigned gpio, const char *label)
>  	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
>  		desc_set_label(desc, label ? : "?");
>  		status = 0;
> -	} else
> +	} else {
>  		status = -EBUSY;
> +		module_put(desc->chip->owner);
> +	}
>  
>  done:
>  	if (status)
> @@ -209,9 +214,10 @@ void gpio_free(unsigned gpio)
>  	spin_lock_irqsave(&gpio_lock, flags);
>  
>  	desc = &gpio_desc[gpio];
> -	if (desc->chip && test_and_clear_bit(FLAG_REQUESTED, &desc->flags))
> +	if (desc->chip && test_and_clear_bit(FLAG_REQUESTED, &desc->flags)) {
>  		desc_set_label(desc, NULL);
> -	else
> +		module_put(desc->chip->owner);
> +	} else
>  		WARN_ON(extra_checks);
>  
>  	spin_unlock_irqrestore(&gpio_lock, flags);
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index 806b86c..f6d389a 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -3,6 +3,8 @@
>  
>  #ifdef CONFIG_HAVE_GPIO_LIB
>  
> +#include <linux/module.h>
> +
>  /* Platforms may implement their GPIO interface with library code,
>   * at a small performance cost for non-inlined operations and some
>   * extra memory (for code and for per-GPIO table entries).
> @@ -52,6 +54,7 @@ struct seq_file;
>   */
>  struct gpio_chip {
>  	char			*label;
> +	struct module		*owner;
>  
>  	int			(*direction_input)(struct gpio_chip *chip,
>  						unsigned offset);
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/