LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: David Brownell <david-b@pacbell.net>
To: Guennadi Liakhovetski <g.liakhovetski@pengutronix.de>
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] prevent gpio chip drivers from unloading while used
Date: Fri, 8 Feb 2008 16:01:10 -0800	[thread overview]
Message-ID: <200802081601.10673.david-b@pacbell.net> (raw)
In-Reply-To: <Pine.LNX.4.64.0802081159580.5301@axis700.grange>

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@pengutronix.de>
> 
> ---
> 
> 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);
> 



  reply	other threads:[~2008-02-09  0:01 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-08 11:10 Guennadi Liakhovetski
2008-02-09  0:01 ` David Brownell [this message]
2008-02-09  0:45   ` Guennadi Liakhovetski
2008-02-09  1:14     ` David Brownell
2008-02-09 11:41       ` Guennadi Liakhovetski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=200802081601.10673.david-b@pacbell.net \
    --to=david-b@pacbell.net \
    --cc=g.liakhovetski@pengutronix.de \
    --cc=linux-kernel@vger.kernel.org \
    --subject='Re: [PATCH 1/2] prevent gpio chip drivers from unloading while used' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).