Re: [PATCH RFC v3 3/3] pinctrl: add pinctrl gpio binding support

Re: [PATCH RFC v3 3/3] pinctrl: add pinctrl gpio binding support

[Stephen Warren]

On 05/23/2012 07:22 AM, Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> This patch implements a standard common binding for pinctrl gpio ranges.
> Each SoC can add gpio ranges through device tree by adding a gpio-maps property
> under their pinctrl devices node with the format:
> <&gpio $gpio_offset $pin_offset $npin>.
> 
> Then the pinctrl driver can call pinctrl_dt_add_gpio_ranges(pctldev, node)
> to parse and register the gpio ranges from device tree.
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>

This is mostly good. Just a few comments:

> +gpio-maps: 4 integers array, each entry in the array represents a gpio
> +range with the format: <&gpio $gpio_offset $pin_offset $count>
> +- gpio: phandle pointing at gpio device node
> +- gpio_offset: integer, the local offset of $gpio
> +- pin_offset: integer, the pin offset or pin id
> +- npins: integer, the gpio ranges starting from pin_offset

This uses a single cell to represent a GPIO ID within a GPIO controller.
The standard GPIO bindings use #gpio-cells, where that's a property in
the GPIO controller's node. I wonder if we shouldn't do the same here,
and call into the GPIO driver to parse #gpio-cells and give back the
Linux GPIO ID, just like of_get_named_gpio_flags() does. This would also
make this code able to cope with the GPIO of_xlate function returning a
different GPIO chip, which Grant put in place for banked GPIO controllers.

> diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c

> +int pinctrl_dt_add_gpio_ranges(struct pinctrl_dev *pctldev,

The locking I was talking about before is between the following line:

> +		ranges[i].gc = of_node_to_gpiochip(np_gpio);

and this code:

> +		ranges[i].name = dev_name(pctldev->dev);
> +		ranges[i].base = ranges[i].gc->base + gpio_offset;
> +		ranges[i].pin_base = pin_offset;
> +		ranges[i].npins = npins;

If of_node_to_gpiochip() doesn't mark the GPIO chip as "in use", then
the module that provides that device could be unloaded between the two
blocks of code above.

Re: your locking comments in your other email: ranges[i].gc doesn't
appear to be used anywhere else in pinctrl, so I think it's OK not to
lock the GPIO chip for any more time than between the above two blocks
of code.

Finally, just a minor nit:

> +		ranges[i].gc = of_node_to_gpiochip(np_gpio);
> +		if (!ranges[i].gc) {
> +			dev_err(pctldev->dev,
> +				"can not find gpio chip of node(%s)\n",
> +				np_gpio->name);
> +			of_node_put(np_gpio);
> +			return -EPROBE_DEFER;
> +		}
> +
> +		of_node_put(np_gpio);

could be slightly simpler:

+		ranges[i].gc = of_node_to_gpiochip(np_gpio);
+		of_node_put(np_gpio); <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
+		if (!ranges[i].gc) {
+			dev_err(pctldev->dev,
+				"can not find gpio chip of node(%s)\n",
+				np_gpio->name);
+			return -EPROBE_DEFER;
+		}

Re: [PATCH RFC v3 3/3] pinctrl: add pinctrl gpio binding support

[Dong Aisheng]

On Thu, May 24, 2012 at 4:44 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/23/2012 07:22 AM, Dong Aisheng wrote:
>> From: Dong Aisheng <dong.aisheng@linaro.org>
>>
>> This patch implements a standard common binding for pinctrl gpio ranges.
>> Each SoC can add gpio ranges through device tree by adding a gpio-maps property
>> under their pinctrl devices node with the format:
>> <&gpio $gpio_offset $pin_offset $npin>.
>>
>> Then the pinctrl driver can call pinctrl_dt_add_gpio_ranges(pctldev, node)
>> to parse and register the gpio ranges from device tree.
>>
>> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
>
> This is mostly good. Just a few comments:
>
>> +gpio-maps: 4 integers array, each entry in the array represents a gpio
>> +range with the format: <&gpio $gpio_offset $pin_offset $count>
>> +- gpio: phandle pointing at gpio device node
>> +- gpio_offset: integer, the local offset of $gpio
>> +- pin_offset: integer, the pin offset or pin id
>> +- npins: integer, the gpio ranges starting from pin_offset
>
> This uses a single cell to represent a GPIO ID within a GPIO controller.
> The standard GPIO bindings use #gpio-cells, where that's a property in
> the GPIO controller's node. I wonder if we shouldn't do the same here,
> and call into the GPIO driver to parse #gpio-cells and give back the
> Linux GPIO ID, just like of_get_named_gpio_flags() does. This would also
> make this code able to cope with the GPIO of_xlate function returning a
> different GPIO chip, which Grant put in place for banked GPIO controllers.
>
I checked the code, the second cell only represents gpio flag in
of_gpio_simple_xlate which seems meaningless to pinctrl, so it looks
increase overhead to pinctrl gpio ranges map.
However, it seems i may have to agree that we need keep align with the
exist of gpio design to use the standard way to get gpio number via
of_xlate function rather than do it privately in pinctrl driver.

One disadvantage is that i can not reuse of_get_named_gpio_flags due
to different format
for gpio-maps, i may have to write a slightly different one as
of_get_named_gpio_flags
for gpio-maps.

>> diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
>
>> +int pinctrl_dt_add_gpio_ranges(struct pinctrl_dev *pctldev,
>
> The locking I was talking about before is between the following line:
>
>> +             ranges[i].gc = of_node_to_gpiochip(np_gpio);
>
> and this code:
>
>> +             ranges[i].name = dev_name(pctldev->dev);
>> +             ranges[i].base = ranges[i].gc->base + gpio_offset;
>> +             ranges[i].pin_base = pin_offset;
>> +             ranges[i].npins = npins;
>
> If of_node_to_gpiochip() doesn't mark the GPIO chip as "in use", then
> the module that provides that device could be unloaded between the two
> blocks of code above.
>
Correct.

> Re: your locking comments in your other email: ranges[i].gc doesn't
> appear to be used anywhere else in pinctrl, so I think it's OK not to
> lock the GPIO chip for any more time than between the above two blocks
> of code.
>
So i will add lock between them like:
ranges[i].gc = of_node_to_gpiochip(np_gpio);
if (!try_module_get(ranges[i].gc->owner))
    err...
ranges[i].name = dev_name(pctldev->dev);
ranges[i].base = ranges[i].gc->base + gpio_offset;
ranges[i].pin_base = pin_offset;
ranges[i].npins = npins;
module_put(ranges[i].gc->owner)
If anything wrong please let me know.

> Finally, just a minor nit:
>
>> +             ranges[i].gc = of_node_to_gpiochip(np_gpio);
>> +             if (!ranges[i].gc) {
>> +                     dev_err(pctldev->dev,
>> +                             "can not find gpio chip of node(%s)\n",
>> +                             np_gpio->name);
>> +                     of_node_put(np_gpio);
>> +                     return -EPROBE_DEFER;
>> +             }
>> +
>> +             of_node_put(np_gpio);
>
> could be slightly simpler:
>
> +               ranges[i].gc = of_node_to_gpiochip(np_gpio);
> +               of_node_put(np_gpio); <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
> +               if (!ranges[i].gc) {
> +                       dev_err(pctldev->dev,
> +                               "can not find gpio chip of node(%s)\n",
> +                               np_gpio->name);
Because here still uese np_gpio,  Can i still use it after of_node_put?

> +                       return -EPROBE_DEFER;
> +               }

Regards
Dong Aisheng

Re: [PATCH RFC v3 3/3] pinctrl: add pinctrl gpio binding support

[Stephen Warren]

On 05/23/2012 07:42 PM, Dong Aisheng wrote:
> On Thu, May 24, 2012 at 4:44 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 05/23/2012 07:22 AM, Dong Aisheng wrote:
>>> From: Dong Aisheng <dong.aisheng@linaro.org>
>>>
>>> This patch implements a standard common binding for pinctrl gpio ranges.
>>> Each SoC can add gpio ranges through device tree by adding a gpio-maps property
>>> under their pinctrl devices node with the format:
>>> <&gpio $gpio_offset $pin_offset $npin>.
>>>
>>> Then the pinctrl driver can call pinctrl_dt_add_gpio_ranges(pctldev, node)
>>> to parse and register the gpio ranges from device tree.
>>>
>>> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
>>
>> This is mostly good. Just a few comments:
>>
>>> +gpio-maps: 4 integers array, each entry in the array represents a gpio
>>> +range with the format: <&gpio $gpio_offset $pin_offset $count>
>>> +- gpio: phandle pointing at gpio device node
>>> +- gpio_offset: integer, the local offset of $gpio
>>> +- pin_offset: integer, the pin offset or pin id
>>> +- npins: integer, the gpio ranges starting from pin_offset
>>
>> This uses a single cell to represent a GPIO ID within a GPIO controller.
>> The standard GPIO bindings use #gpio-cells, where that's a property in
>> the GPIO controller's node. I wonder if we shouldn't do the same here,
>> and call into the GPIO driver to parse #gpio-cells and give back the
>> Linux GPIO ID, just like of_get_named_gpio_flags() does. This would also
>> make this code able to cope with the GPIO of_xlate function returning a
>> different GPIO chip, which Grant put in place for banked GPIO controllers.
>>
> I checked the code, the second cell only represents gpio flag in
> of_gpio_simple_xlate which seems meaningless to pinctrl, so it looks
> increase overhead to pinctrl gpio ranges map.

With the simple translation function, yes it's just flags. However, not
all GPIO controllers use the simple translation function; I think I
recall the Exynos binding having 4 or 5 cells. In other words, the
format is defined by each individual GPIO controller, even if many/most
do happen to follow the same format.

> However, it seems i may have to agree that we need keep align with the
> exist of gpio design to use the standard way to get gpio number via
> of_xlate function rather than do it privately in pinctrl driver.
> 
> One disadvantage is that i can not reuse of_get_named_gpio_flags due
> to different format
> for gpio-maps, i may have to write a slightly different one as
> of_get_named_gpio_flags
> for gpio-maps.
> 
>>> diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
>>
>>> +int pinctrl_dt_add_gpio_ranges(struct pinctrl_dev *pctldev,
>>
>> The locking I was talking about before is between the following line:
>>
>>> +             ranges[i].gc = of_node_to_gpiochip(np_gpio);
>>
>> and this code:
>>
>>> +             ranges[i].name = dev_name(pctldev->dev);
>>> +             ranges[i].base = ranges[i].gc->base + gpio_offset;
>>> +             ranges[i].pin_base = pin_offset;
>>> +             ranges[i].npins = npins;
>>
>> If of_node_to_gpiochip() doesn't mark the GPIO chip as "in use", then
>> the module that provides that device could be unloaded between the two
>> blocks of code above.
>>
> Correct.
> 
>> Re: your locking comments in your other email: ranges[i].gc doesn't
>> appear to be used anywhere else in pinctrl, so I think it's OK not to
>> lock the GPIO chip for any more time than between the above two blocks
>> of code.
>>
> So i will add lock between them like:
> ranges[i].gc = of_node_to_gpiochip(np_gpio);
> if (!try_module_get(ranges[i].gc->owner))
>     err...

I think that module_get() needs to happen inside of_node_to_gpiochip(),
so that it executes inside any lock that function takes.

> ranges[i].name = dev_name(pctldev->dev);
> ranges[i].base = ranges[i].gc->base + gpio_offset;
> ranges[i].pin_base = pin_offset;
> ranges[i].npins = npins;
> module_put(ranges[i].gc->owner)
> If anything wrong please let me know.
> 
>> Finally, just a minor nit:
>>
>>> +             ranges[i].gc = of_node_to_gpiochip(np_gpio);
>>> +             if (!ranges[i].gc) {
>>> +                     dev_err(pctldev->dev,
>>> +                             "can not find gpio chip of node(%s)\n",
>>> +                             np_gpio->name);
>>> +                     of_node_put(np_gpio);
>>> +                     return -EPROBE_DEFER;
>>> +             }
>>> +
>>> +             of_node_put(np_gpio);
>>
>> could be slightly simpler:
>>
>> +               ranges[i].gc = of_node_to_gpiochip(np_gpio);
>> +               of_node_put(np_gpio); <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>> +               if (!ranges[i].gc) {
>> +                       dev_err(pctldev->dev,
>> +                               "can not find gpio chip of node(%s)\n",
>> +                               np_gpio->name);
> Because here still uese np_gpio,  Can i still use it after of_node_put?

Oh right, that makes sense, yes.

Re: [PATCH RFC v3 3/3] pinctrl: add pinctrl gpio binding support

[Dong Aisheng]

On Thu, May 24, 2012 at 12:42:19PM +0800, Stephen Warren wrote:
> On 05/23/2012 07:42 PM, Dong Aisheng wrote:
> > On Thu, May 24, 2012 at 4:44 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> >> On 05/23/2012 07:22 AM, Dong Aisheng wrote:
> >>> From: Dong Aisheng <dong.aisheng@linaro.org>
> >>>
> >>> This patch implements a standard common binding for pinctrl gpio ranges.
> >>> Each SoC can add gpio ranges through device tree by adding a gpio-maps property
> >>> under their pinctrl devices node with the format:
> >>> <&gpio $gpio_offset $pin_offset $npin>.
> >>>
> >>> Then the pinctrl driver can call pinctrl_dt_add_gpio_ranges(pctldev, node)
> >>> to parse and register the gpio ranges from device tree.
> >>>
> >>> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> >>
> >> This is mostly good. Just a few comments:
> >>
> >>> +gpio-maps: 4 integers array, each entry in the array represents a gpio
> >>> +range with the format: <&gpio $gpio_offset $pin_offset $count>
> >>> +- gpio: phandle pointing at gpio device node
> >>> +- gpio_offset: integer, the local offset of $gpio
> >>> +- pin_offset: integer, the pin offset or pin id
> >>> +- npins: integer, the gpio ranges starting from pin_offset
> >>
> >> This uses a single cell to represent a GPIO ID within a GPIO controller.
> >> The standard GPIO bindings use #gpio-cells, where that's a property in
> >> the GPIO controller's node. I wonder if we shouldn't do the same here,
> >> and call into the GPIO driver to parse #gpio-cells and give back the
> >> Linux GPIO ID, just like of_get_named_gpio_flags() does. This would also
> >> make this code able to cope with the GPIO of_xlate function returning a
> >> different GPIO chip, which Grant put in place for banked GPIO controllers.
> >>
> > I checked the code, the second cell only represents gpio flag in
> > of_gpio_simple_xlate which seems meaningless to pinctrl, so it looks
> > increase overhead to pinctrl gpio ranges map.
> 
> With the simple translation function, yes it's just flags. However, not
> all GPIO controllers use the simple translation function; I think I
> recall the Exynos binding having 4 or 5 cells. In other words, the
> format is defined by each individual GPIO controller, even if many/most
> do happen to follow the same format.
> 
Correct, it should be SoC dependent.

> > However, it seems i may have to agree that we need keep align with the
> > exist of gpio design to use the standard way to get gpio number via
> > of_xlate function rather than do it privately in pinctrl driver.
> > 
> > One disadvantage is that i can not reuse of_get_named_gpio_flags due
> > to different format
> > for gpio-maps, i may have to write a slightly different one as
> > of_get_named_gpio_flags
> > for gpio-maps.
> > 
> >>> diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
> >>
> >>> +int pinctrl_dt_add_gpio_ranges(struct pinctrl_dev *pctldev,
> >>
> >> The locking I was talking about before is between the following line:
> >>
> >>> +             ranges[i].gc = of_node_to_gpiochip(np_gpio);
> >>
> >> and this code:
> >>
> >>> +             ranges[i].name = dev_name(pctldev->dev);
> >>> +             ranges[i].base = ranges[i].gc->base + gpio_offset;
> >>> +             ranges[i].pin_base = pin_offset;
> >>> +             ranges[i].npins = npins;
> >>
> >> If of_node_to_gpiochip() doesn't mark the GPIO chip as "in use", then
> >> the module that provides that device could be unloaded between the two
> >> blocks of code above.
> >>
> > Correct.
> > 
> >> Re: your locking comments in your other email: ranges[i].gc doesn't
> >> appear to be used anywhere else in pinctrl, so I think it's OK not to
> >> lock the GPIO chip for any more time than between the above two blocks
> >> of code.
> >>
> > So i will add lock between them like:
> > ranges[i].gc = of_node_to_gpiochip(np_gpio);
> > if (!try_module_get(ranges[i].gc->owner))
> >     err...
> 
> I think that module_get() needs to happen inside of_node_to_gpiochip(),
> so that it executes inside any lock that function takes.
Can you please help explain a bit more?
I did not quite understand.
It looks to me of_node_to_gpiochip is only convert the gpio node to gpio chip.
Why need get the module inside this function?
For gpio_request function, it also calls try_module_get(gc) after find the gpio
chip.

> 
> > ranges[i].name = dev_name(pctldev->dev);
> > ranges[i].base = ranges[i].gc->base + gpio_offset;
> > ranges[i].pin_base = pin_offset;
> > ranges[i].npins = npins;
> > module_put(ranges[i].gc->owner)
> > If anything wrong please let me know.
> > 
> >> Finally, just a minor nit:
> >>
> >>> +             ranges[i].gc = of_node_to_gpiochip(np_gpio);
> >>> +             if (!ranges[i].gc) {
> >>> +                     dev_err(pctldev->dev,
> >>> +                             "can not find gpio chip of node(%s)\n",
> >>> +                             np_gpio->name);
> >>> +                     of_node_put(np_gpio);
> >>> +                     return -EPROBE_DEFER;
> >>> +             }
> >>> +
> >>> +             of_node_put(np_gpio);
> >>
> >> could be slightly simpler:
> >>
> >> +               ranges[i].gc = of_node_to_gpiochip(np_gpio);
> >> +               of_node_put(np_gpio); <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
> >> +               if (!ranges[i].gc) {
> >> +                       dev_err(pctldev->dev,
> >> +                               "can not find gpio chip of node(%s)\n",
> >> +                               np_gpio->name);
> > Because here still uese np_gpio,  Can i still use it after of_node_put?
> 
> Oh right, that makes sense, yes.
> 
I guess you mean no(can not use the node after of_node_put), right?

Regards
Dong Aisheng

Re: [PATCH RFC v3 3/3] pinctrl: add pinctrl gpio binding support

[Stephen Warren]

On 05/23/2012 11:19 PM, Dong Aisheng wrote:
> On Thu, May 24, 2012 at 12:42:19PM +0800, Stephen Warren wrote:
>> On 05/23/2012 07:42 PM, Dong Aisheng wrote:
>>> On Thu, May 24, 2012 at 4:44 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>>>> On 05/23/2012 07:22 AM, Dong Aisheng wrote:
>>>>> From: Dong Aisheng <dong.aisheng-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>>>
>>>>> This patch implements a standard common binding for pinctrl gpio ranges.
>>>>> Each SoC can add gpio ranges through device tree by adding a gpio-maps property
>>>>> under their pinctrl devices node with the format:
>>>>> <&gpio $gpio_offset $pin_offset $npin>.
>>>>>
>>>>> Then the pinctrl driver can call pinctrl_dt_add_gpio_ranges(pctldev, node)
>>>>> to parse and register the gpio ranges from device tree.
...
>>>> Re: your locking comments in your other email: ranges[i].gc doesn't
>>>> appear to be used anywhere else in pinctrl, so I think it's OK not to
>>>> lock the GPIO chip for any more time than between the above two blocks
>>>> of code.
>>>
>>> So i will add lock between them like:
>>> ranges[i].gc = of_node_to_gpiochip(np_gpio);
>>> if (!try_module_get(ranges[i].gc->owner))
>>>     err...
>>
>> I think that module_get() needs to happen inside of_node_to_gpiochip(),
>> so that it executes inside any lock that function takes.
>
> Can you please help explain a bit more?
> I did not quite understand.
> It looks to me of_node_to_gpiochip is only convert the gpio node to gpio chip.
> Why need get the module inside this function?
> For gpio_request function, it also calls try_module_get(gc) after find the gpio
> chip.

The problem is this:

Thread 1: Call of_node_to_gpiochip(), returns a gpio_chip.
Thread 2: Unregisters the same gpio_chip that was returned above.
Thread 1: Accesses the now unregistered (and possibly free'd) gpio_chip
-> at best, bad data, at worst, OOPS.

In order to prevent this, of_node_to_gpiochip() should take measures to
prevent another thread from unregistering the gpio_chip until thread 1
has completed its step above.

The existing of_get_named_gpio_flags() is safe from this, since
gpiochip_find() acquires the GPIO lock, and all accesses to the fouond
gpio chip occur with that lock held, inside the match function. Perhaps
a similar approach could be used here.

>>>> Finally, just a minor nit:
...
>>>> could be slightly simpler:
...
>>> Because here still uese np_gpio,  Can i still use it after of_node_put?
>>
>> Oh right, that makes sense, yes.
>>
> I guess you mean no(can not use the node after of_node_put), right?

I mean the original code in your patch is fine.

Re: [PATCH RFC v3 3/3] pinctrl: add pinctrl gpio binding support

[Dong Aisheng]

On Thu, May 24, 2012 at 11:22:13PM +0800, Stephen Warren wrote:
> On 05/23/2012 11:19 PM, Dong Aisheng wrote:
> > On Thu, May 24, 2012 at 12:42:19PM +0800, Stephen Warren wrote:
> >> On 05/23/2012 07:42 PM, Dong Aisheng wrote:
> >>> On Thu, May 24, 2012 at 4:44 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> >>>> On 05/23/2012 07:22 AM, Dong Aisheng wrote:
> >>>>> From: Dong Aisheng <dong.aisheng-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>>>>
> >>>>> This patch implements a standard common binding for pinctrl gpio ranges.
> >>>>> Each SoC can add gpio ranges through device tree by adding a gpio-maps property
> >>>>> under their pinctrl devices node with the format:
> >>>>> <&gpio $gpio_offset $pin_offset $npin>.
> >>>>>
> >>>>> Then the pinctrl driver can call pinctrl_dt_add_gpio_ranges(pctldev, node)
> >>>>> to parse and register the gpio ranges from device tree.
> ...
> >>>> Re: your locking comments in your other email: ranges[i].gc doesn't
> >>>> appear to be used anywhere else in pinctrl, so I think it's OK not to
> >>>> lock the GPIO chip for any more time than between the above two blocks
> >>>> of code.
> >>>
> >>> So i will add lock between them like:
> >>> ranges[i].gc = of_node_to_gpiochip(np_gpio);
> >>> if (!try_module_get(ranges[i].gc->owner))
> >>>     err...
> >>
> >> I think that module_get() needs to happen inside of_node_to_gpiochip(),
> >> so that it executes inside any lock that function takes.
> >
> > Can you please help explain a bit more?
> > I did not quite understand.
> > It looks to me of_node_to_gpiochip is only convert the gpio node to gpio chip.
> > Why need get the module inside this function?
> > For gpio_request function, it also calls try_module_get(gc) after find the gpio
> > chip.
> 
> The problem is this:
> 
> Thread 1: Call of_node_to_gpiochip(), returns a gpio_chip.
> Thread 2: Unregisters the same gpio_chip that was returned above.
> Thread 1: Accesses the now unregistered (and possibly free'd) gpio_chip
> -> at best, bad data, at worst, OOPS.
> 
Correct. We did have this issue.
Thanks for clarify.

> In order to prevent this, of_node_to_gpiochip() should take measures to
> prevent another thread from unregistering the gpio_chip until thread 1
> has completed its step above.
> 
> The existing of_get_named_gpio_flags() is safe from this, since
> gpiochip_find() acquires the GPIO lock, and all accesses to the fouond
> gpio chip occur with that lock held, inside the match function. Perhaps
> a similar approach could be used here.
Why it looks to me of_get_named_gpio_flags has the same issue and also not safe?
For of_node_to_gpiochip itself called in of_get_named_gpio_flags, it's safe.
But after that, i'm suspecting it has the same issue as you described above, right?

For example:
int of_get_named_gpio_flags(struct device_node *np, const char *propname,
                           int index, enum of_gpio_flags *flags)
{
...
	gc = of_node_to_gpiochip(gpiospec.np);
	if (!gc) {
		pr_debug("%s: gpio controller %s isn't registered\n",
			 np->full_name, gpiospec.np->full_name);
		ret = -ENODEV;
		goto err1;
	}

	===> the gc may be unregistered here by another thread and
	     even already have been freed, right?

	ret = gc->of_xlate(gc, &gpiospec, flags);
...
}

Maybe we need get the lock in of_node_to_gpiochip and release it by calling
of_gpio_put(..) after using?

Regards
Dong Aisheng

Re: [PATCH RFC v3 3/3] pinctrl: add pinctrl gpio binding support

[Stephen Warren]

On 05/24/2012 09:22 PM, Dong Aisheng wrote:
> On Thu, May 24, 2012 at 11:22:13PM +0800, Stephen Warren wrote:
...
>> The problem is this:
>>
>> Thread 1: Call of_node_to_gpiochip(), returns a gpio_chip.
>> Thread 2: Unregisters the same gpio_chip that was returned above.
>> Thread 1: Accesses the now unregistered (and possibly free'd) gpio_chip
>> -> at best, bad data, at worst, OOPS.
>>
> Correct. We did have this issue.
> Thanks for clarify.
> 
>> In order to prevent this, of_node_to_gpiochip() should take measures to
>> prevent another thread from unregistering the gpio_chip until thread 1
>> has completed its step above.
>>
>> The existing of_get_named_gpio_flags() is safe from this, since
>> gpiochip_find() acquires the GPIO lock, and all accesses to the fouond
>> gpio chip occur with that lock held, inside the match function. Perhaps
>> a similar approach could be used here.
>
> Why it looks to me of_get_named_gpio_flags has the same issue and also not safe?
> For of_node_to_gpiochip itself called in of_get_named_gpio_flags, it's safe.

Uggh. Yes, I meant that of_node_to_gpiochip() itself doesn't have this
issue, but you're right, it looks like of_get_named_gpio_flags() does.

> But after that, i'm suspecting it has the same issue as you described above, right?
> 
> For example:
> int of_get_named_gpio_flags(struct device_node *np, const char *propname,
>                            int index, enum of_gpio_flags *flags)
> {
> ...
> 	gc = of_node_to_gpiochip(gpiospec.np);
> 	if (!gc) {
> 		pr_debug("%s: gpio controller %s isn't registered\n",
> 			 np->full_name, gpiospec.np->full_name);
> 		ret = -ENODEV;
> 		goto err1;
> 	}
> 
> 	===> the gc may be unregistered here by another thread and
> 	     even already have been freed, right?
> 
> 	ret = gc->of_xlate(gc, &gpiospec, flags);
> ...
> }
> 
> Maybe we need get the lock in of_node_to_gpiochip and release it by calling
> of_gpio_put(..) after using?

Yes, something like that; it should take the module lock, not the gpio lock.

Re: [PATCH RFC v3 3/3] pinctrl: add pinctrl gpio binding support

[Dong Aisheng]

On Fri, May 25, 2012 at 12:59:47PM +0800, Stephen Warren wrote:
> On 05/24/2012 09:22 PM, Dong Aisheng wrote:
> > On Thu, May 24, 2012 at 11:22:13PM +0800, Stephen Warren wrote:
> ...
> >> The problem is this:
> >>
> >> Thread 1: Call of_node_to_gpiochip(), returns a gpio_chip.
> >> Thread 2: Unregisters the same gpio_chip that was returned above.
> >> Thread 1: Accesses the now unregistered (and possibly free'd) gpio_chip
> >> -> at best, bad data, at worst, OOPS.
> >>
> > Correct. We did have this issue.
> > Thanks for clarify.
> > 
> >> In order to prevent this, of_node_to_gpiochip() should take measures to
> >> prevent another thread from unregistering the gpio_chip until thread 1
> >> has completed its step above.
> >>
> >> The existing of_get_named_gpio_flags() is safe from this, since
> >> gpiochip_find() acquires the GPIO lock, and all accesses to the fouond
> >> gpio chip occur with that lock held, inside the match function. Perhaps
> >> a similar approach could be used here.
> >
> > Why it looks to me of_get_named_gpio_flags has the same issue and also not safe?
> > For of_node_to_gpiochip itself called in of_get_named_gpio_flags, it's safe.
> 
> Uggh. Yes, I meant that of_node_to_gpiochip() itself doesn't have this
> issue, but you're right, it looks like of_get_named_gpio_flags() does.
> 
> > But after that, i'm suspecting it has the same issue as you described above, right?
> > 
> > For example:
> > int of_get_named_gpio_flags(struct device_node *np, const char *propname,
> >                            int index, enum of_gpio_flags *flags)
> > {
> > ...
> > 	gc = of_node_to_gpiochip(gpiospec.np);
> > 	if (!gc) {
> > 		pr_debug("%s: gpio controller %s isn't registered\n",
> > 			 np->full_name, gpiospec.np->full_name);
> > 		ret = -ENODEV;
> > 		goto err1;
> > 	}
> > 
> > 	===> the gc may be unregistered here by another thread and
> > 	     even already have been freed, right?
> > 
> > 	ret = gc->of_xlate(gc, &gpiospec, flags);
> > ...
> > }
> > 
> > Maybe we need get the lock in of_node_to_gpiochip and release it by calling
> > of_gpio_put(..) after using?
> 
> Yes, something like that; it should take the module lock, not the gpio lock.
> 
Okay, i will try to add it.

Regards
Dong Aisheng