On Wed, 30 May 2012 12:10:58 +0800, Dong Aisheng [off-list ref] wrote:

Hi Grant,

On Fri, May 25, 2012 at 06:25:00PM -0600, Grant Likely wrote:

quoted

On Fri, 25 May 2012 21:36:18 +0800, Dong Aisheng [off-list ref] wrote:

quoted

From: Dong Aisheng <redacted>

The module lock will be automatically claimed for gpiochip_find function
in case the gpio module is removed during the using of gpiochip instance.
Users are responsible to call gpiochip_put to release the lock after
the using.

Signed-off-by: Dong Aisheng <redacted>

...

quoted

Also, it doesn't do anything to protect against the gpio_chip being
removed after the gpio number is resolved, which means the gpio number
may no longer be valid, or may no longer point to the same gpio chip.
It looks like the locking protection needs to be wider to be useful.

I understand the issue now.
It's correct that we did not lock gpio_chip before calling gpio_request
after the gpio number is resolved.

I thought about adding a new API called of_gpio_request to hide the lock
to users like:
int of_gpio_request(..)
{
	spin_lock_irqsave(&gpio_lock, flags);
	ret = of_get_named_gpio(..);
	if (ret < 0)
		do_err..
	ret = gpio_request(..)

	spin_unlock_irqrestore(&gpio_lock, flags);
	return ret;
}
But it seems it does not work since the gpio_request may sleep and we may
need a new sleepable lock rather using the exist gpio_lock.

In the same time, i'm also thinking about a question that do we really
need to do this to protect gpio_chip being removed afer gpio number is
resolved?

Probably not (for this series) because it shouldn't cause an oops if
it happens.  Ultimately however it would be good to have proper
reference counting on gpio chips to prevent any problems.

g.