Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Stephen Warren]

On 12/12/2012 01:25 PM, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> This makes the device core auto-grab the pinctrl handle and set
> the "default" (PINCTRL_STATE_DEFAULT) state for every device
> that is present in the device model right before probe. This will
> account for the lion's share of embedded silicon devcies.

There are quite a few problems with this patch, and they end up
completely breaking at least Tegra in next-20130110.

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

> +int pinctrl_bind_pins(struct device *dev)
> +{
> +	struct dev_pin_info *dpi;
> +	int ret;
> +
> +	/* Allocate a pin state container on-the-fly */
> +	if (!dev->pins) {
> +		dpi = devm_kzalloc(dev, sizeof(*dpi), GFP_KERNEL);

This is allocated using a devm_ function. If -EPROBE_DEFER is returned
below after the assignment to dev->pins or if the driver's own probe()
returns -EPROBE_DEFER, this allocation will be freed by the driver core.
This can leave dev->pins pointing to something non-NULL, yet invalid.

I haven't fully verified this, but I believe this issue is causing
crashes on Tegra. Certainly if I force this code to follow the path that
always allocates new structs or performs new gets, then the crashes go away.

> +		if (!dpi)
> +			return -ENOMEM;
> +	} else
> +		dpi = dev->pins;
> +
> +	/*
> +	 * Check if we already have a pinctrl handle, as we may arrive here
> +	 * after a deferral in the state selection below
> +	 */
> +	if (!dpi->p) {
> +		dpi->p = devm_pinctrl_get(dev);

That won't succeed for a pinctrl device that has a default state in
order to implement hogs. This will then cause the pin controller device
to always defer probe and never activate. This will leave HW
unconfigured and/or prevent other devices from successfully calling
pinctrl_get().

This issue also happens on Tegra.

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

> @@ -734,9 +734,16 @@ static struct pinctrl *pinctrl_get_locked(struct device *dev)
>  	if (WARN_ON(!dev))
>  		return ERR_PTR(-EINVAL);
>  
> +	/*
> +	 * See if somebody else (such as the device core) has already
> +	 * obtained a handle to the pinctrl for this device. In that case,
> +	 * return another pointer to it.
> +	 */
>  	p = find_pinctrl(dev);
> -	if (p != NULL)
> -		return ERR_PTR(-EBUSY);

I deliberately returned an error here, because there's no reference
counting on the struct pinctrl objects. If a driver calls pinctrl_get(),
with the new code below, it will retrieve the same struct. If it later
calls pinctrl_put(), the put will immediately free the structure. This
will invalidate the pointers that reference it in struct device's pins
field.

This issue will probably trigger on Tegra, since we at least have a
pinctrl-based I2C mux that calls pinctrl_get().

> +	if (p != NULL) {
> +		dev_dbg(dev, "obtain a copy of previously claimed pinctrl\n");
> +		return p;
> +	}
>  
>  	return create_pinctrl(dev);
>  }

Perhaps we could remove this patch from linux-next, and have a V3?

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Stephen Warren]

On 01/10/2013 01:42 PM, Stephen Warren wrote:
> On 12/12/2012 01:25 PM, Linus Walleij wrote:
>> From: Linus Walleij <linus.walleij@linaro.org>
>>
>> This makes the device core auto-grab the pinctrl handle and set
>> the "default" (PINCTRL_STATE_DEFAULT) state for every device
>> that is present in the device model right before probe. This will
>> account for the lion's share of embedded silicon devcies.
> 
> There are quite a few problems with this patch, and they end up
> completely breaking at least Tegra in next-20130110.
> 
>> diff --git a/drivers/base/pinctrl.c b/drivers/base/pinctrl.c
> 
>> +int pinctrl_bind_pins(struct device *dev)
>> +{
>> +	struct dev_pin_info *dpi;
>> +	int ret;
>> +
>> +	/* Allocate a pin state container on-the-fly */
>> +	if (!dev->pins) {
>> +		dpi = devm_kzalloc(dev, sizeof(*dpi), GFP_KERNEL);
>> +		if (!dpi)
>> +			return -ENOMEM;
>> +	} else
>> +		dpi = dev->pins;
>> +
>> +	/*
>> +	 * Check if we already have a pinctrl handle, as we may arrive here
>> +	 * after a deferral in the state selection below
>> +	 */
>> +	if (!dpi->p) {
>> +		dpi->p = devm_pinctrl_get(dev);
> 
> That won't succeed for a pinctrl device that has a default state in
> order to implement hogs. This will then cause the pin controller device
> to always defer probe and never activate. This will leave HW
> unconfigured and/or prevent other devices from successfully calling
> pinctrl_get().

I see that an attempt was made to solve this problem, in the patch
immediately preceding this one (at least, as applied in the pinctrl
tree). However, that patch only addresses the case where the pin
controller is being looked up in the map, and not the case when
converting device tree to the map in the first place. The patch below
solves this:

diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index fe2d1af..fd40a11 100644
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -141,6 +141,11 @@ static int dt_to_map_one_config(struct pinctrl *p,
const char *statename,
                pctldev = find_pinctrl_by_of_node(np_pctldev);
                if (pctldev)
                        break;
+               /* Do not defer probing of hogs (circular loop) */
+               if (np_pctldev == p->dev->of_node) {
+                       of_node_put(np_pctldev);
+                       return -ENODEV;
+               }
        }
        of_node_put(np_pctldev);

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Linus Walleij]

On Thu, Jan 10, 2013 at 11:07 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> I see that an attempt was made to solve this problem, in the patch
> immediately preceding this one (at least, as applied in the pinctrl
> tree). However, that patch only addresses the case where the pin
> controller is being looked up in the map, and not the case when
> converting device tree to the map in the first place. The patch below
> solves this:
>
> diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
> index fe2d1af..fd40a11 100644
> --- a/drivers/pinctrl/devicetree.c
> +++ b/drivers/pinctrl/devicetree.c
> @@ -141,6 +141,11 @@ static int dt_to_map_one_config(struct pinctrl *p,
> const char *statename,
>                 pctldev = find_pinctrl_by_of_node(np_pctldev);
>                 if (pctldev)
>                         break;
> +               /* Do not defer probing of hogs (circular loop) */
> +               if (np_pctldev == p->dev->of_node) {
> +                       of_node_put(np_pctldev);
> +                       return -ENODEV;
> +               }
>         }
>         of_node_put(np_pctldev);

OK I've duplicated this, will send out as patch.

Yours,
Linus Walleij

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Linus Walleij]

On Thu, Jan 10, 2013 at 9:42 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 12/12/2012 01:25 PM, Linus Walleij wrote:
>> From: Linus Walleij <linus.walleij@linaro.org>
>>
>> This makes the device core auto-grab the pinctrl handle and set
>> the "default" (PINCTRL_STATE_DEFAULT) state for every device
>> that is present in the device model right before probe. This will
>> account for the lion's share of embedded silicon devcies.
>
> There are quite a few problems with this patch, and they end up
> completely breaking at least Tegra in next-20130110.

But I have not put this patch into linux-next.

I was still waiting for Greg to ACK it...

Apparently it was included in a pull request to the SH tree or something,
Simon can you remove these patches for now, this patch needs to
be elaborated on in the pinctrl tree first.

Yours,
Linus Walleij

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Laurent Pinchart]

Hi Simon,

On Friday 11 January 2013 21:12:43 Linus Walleij wrote:
> On Thu, Jan 10, 2013 at 9:42 PM, Stephen Warren wrote:
> > On 12/12/2012 01:25 PM, Linus Walleij wrote:
> >> From: Linus Walleij <linus.walleij@linaro.org>
> >> 
> >> This makes the device core auto-grab the pinctrl handle and set
> >> the "default" (PINCTRL_STATE_DEFAULT) state for every device
> >> that is present in the device model right before probe. This will
> >> account for the lion's share of embedded silicon devcies.
> > 
> > There are quite a few problems with this patch, and they end up
> > completely breaking at least Tegra in next-20130110.
> 
> But I have not put this patch into linux-next.
> 
> I was still waiting for Greg to ACK it...
> 
> Apparently it was included in a pull request to the SH tree or something,
> Simon can you remove these patches for now, this patch needs to
> be elaborated on in the pinctrl tree first.

I've sent several patch series for the SH PFC (Pin Function Controller) to the 
linux-sh mailing list. One of the series included pinctrl core patches for 
easier testing, but I made it clear that they should *not* be pushed to 
mainline through the SH tree.

Actually the whole PFC series have been found out today by Guennadi 
Liakhovetski to be buggy. I will fix the problems and send a new version. In 
the meantime the pinmux-pinctrl and pinmux-dt series should not be pushed to 
mainline.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Linus Walleij]

On Fri, Jan 11, 2013 at 9:36 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> I've sent several patch series for the SH PFC (Pin Function Controller) to the
> linux-sh mailing list. One of the series included pinctrl core patches for
> easier testing, but I made it clear that they should *not* be pushed to
> mainline through the SH tree.

No big deal, what fun is linux-next if we don't break it ;-)

Yours,
Linus Walleij

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Stephen Warren]

On 01/11/2013 01:45 PM, Linus Walleij wrote:
> On Fri, Jan 11, 2013 at 9:36 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> 
>> I've sent several patch series for the SH PFC (Pin Function Controller) to the
>> linux-sh mailing list. One of the series included pinctrl core patches for
>> easier testing, but I made it clear that they should *not* be pushed to
>> mainline through the SH tree.
> 
> No big deal, what fun is linux-next if we don't break it ;-)

Hmm. It's causing a lot of engineers here a lot of trouble, since they
all see linux-next won't boot, and haven't been paying enough attention
to know which commit to revert:-(. Lots of lost productivity:-(

Simon, the offending commit:

6d3ef6b drivers/pinctrl: grab default handles from device core

is still in next-20130116. Can you please remove it?

Or does it make sense for Stephen Rothwell to revert it when building
linux-next?

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Linus Walleij]

On Wed, Jan 16, 2013 at 6:49 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> Hmm. It's causing a lot of engineers here a lot of trouble, since they
> all see linux-next won't boot, and haven't been paying enough attention
> to know which commit to revert:-(. Lots of lost productivity:-(
>
> Simon, the offending commit:
>
> 6d3ef6b drivers/pinctrl: grab default handles from device core
>
> is still in next-20130116. Can you please remove it?

Not that this should be in the SH tree, but I have merged your fix
to the pinctrl tree and it should be in -next, does it fix the problem?

Mainly asking because once Greg ACKs that patch I want to
put it into the pinctrl tree...

Yours,
Linus Walleij

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Greg KH]

On Thu, Jan 17, 2013 at 01:59:52AM +0100, Linus Walleij wrote:
> On Wed, Jan 16, 2013 at 6:49 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
> > Hmm. It's causing a lot of engineers here a lot of trouble, since they
> > all see linux-next won't boot, and haven't been paying enough attention
> > to know which commit to revert:-(. Lots of lost productivity:-(
> >
> > Simon, the offending commit:
> >
> > 6d3ef6b drivers/pinctrl: grab default handles from device core
> >
> > is still in next-20130116. Can you please remove it?
> 
> Not that this should be in the SH tree, but I have merged your fix
> to the pinctrl tree and it should be in -next, does it fix the problem?
> 
> Mainly asking because once Greg ACKs that patch I want to
> put it into the pinctrl tree...

I want to see the updated one with all of the fixes before I ack
anything.

thanks,

greg k-h

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Stephen Warren]

On 01/16/2013 05:59 PM, Linus Walleij wrote:
> On Wed, Jan 16, 2013 at 6:49 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>> Hmm. It's causing a lot of engineers here a lot of trouble, since they
>> all see linux-next won't boot, and haven't been paying enough attention
>> to know which commit to revert:-(. Lots of lost productivity:-(
>>
>> Simon, the offending commit:
>>
>> 6d3ef6b drivers/pinctrl: grab default handles from device core
>>
>> is still in next-20130116. Can you please remove it?
> 
> Not that this should be in the SH tree, but I have merged your fix
> to the pinctrl tree and it should be in -next, does it fix the problem?

There were two problems; one was the issues handling hogs during DT->map
conversion, for which you applied a patch. The other was the crashes due
to incorrect devm_* usage and interactions with -EPROBE_DEFER in the
patch which Simon applied, which requires that patch to be revised.

> Mainly asking because once Greg ACKs that patch I want to
> put it into the pinctrl tree...

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Simon Horman]

On Wed, Jan 16, 2013 at 10:49:05AM -0700, Stephen Warren wrote:
> On 01/11/2013 01:45 PM, Linus Walleij wrote:
> > On Fri, Jan 11, 2013 at 9:36 PM, Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > 
> >> I've sent several patch series for the SH PFC (Pin Function Controller) to the
> >> linux-sh mailing list. One of the series included pinctrl core patches for
> >> easier testing, but I made it clear that they should *not* be pushed to
> >> mainline through the SH tree.
> > 
> > No big deal, what fun is linux-next if we don't break it ;-)
> 
> Hmm. It's causing a lot of engineers here a lot of trouble, since they
> all see linux-next won't boot, and haven't been paying enough attention
> to know which commit to revert:-(. Lots of lost productivity:-(
> 
> Simon, the offending commit:
> 
> 6d3ef6b drivers/pinctrl: grab default handles from device core
> 
> is still in next-20130116. Can you please remove it?

I removed the commit yesterday.
Please let me know if it seems to be lingering.

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Stephen Warren]

On 01/16/2013 11:02 PM, Simon Horman wrote:
> On Wed, Jan 16, 2013 at 10:49:05AM -0700, Stephen Warren wrote:
>> On 01/11/2013 01:45 PM, Linus Walleij wrote:
>>> On Fri, Jan 11, 2013 at 9:36 PM, Laurent Pinchart
>>> <laurent.pinchart@ideasonboard.com> wrote:
>>>
>>>> I've sent several patch series for the SH PFC (Pin Function Controller) to the
>>>> linux-sh mailing list. One of the series included pinctrl core patches for
>>>> easier testing, but I made it clear that they should *not* be pushed to
>>>> mainline through the SH tree.
>>>
>>> No big deal, what fun is linux-next if we don't break it ;-)
>>
>> Hmm. It's causing a lot of engineers here a lot of trouble, since they
>> all see linux-next won't boot, and haven't been paying enough attention
>> to know which commit to revert:-(. Lots of lost productivity:-(
>>
>> Simon, the offending commit:
>>
>> 6d3ef6b drivers/pinctrl: grab default handles from device core
>>
>> is still in next-20130116. Can you please remove it?
> 
> I removed the commit yesterday.
> Please let me know if it seems to be lingering.

Thanks. I believe it's gone from next-20130117, judging by a quick
search through "git log" at least.

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Simon Horman]

On Thu, Jan 17, 2013 at 09:31:01AM -0700, Stephen Warren wrote:
> On 01/16/2013 11:02 PM, Simon Horman wrote:
> > On Wed, Jan 16, 2013 at 10:49:05AM -0700, Stephen Warren wrote:
> >> On 01/11/2013 01:45 PM, Linus Walleij wrote:
> >>> On Fri, Jan 11, 2013 at 9:36 PM, Laurent Pinchart
> >>> <laurent.pinchart@ideasonboard.com> wrote:
> >>>
> >>>> I've sent several patch series for the SH PFC (Pin Function Controller) to the
> >>>> linux-sh mailing list. One of the series included pinctrl core patches for
> >>>> easier testing, but I made it clear that they should *not* be pushed to
> >>>> mainline through the SH tree.
> >>>
> >>> No big deal, what fun is linux-next if we don't break it ;-)
> >>
> >> Hmm. It's causing a lot of engineers here a lot of trouble, since they
> >> all see linux-next won't boot, and haven't been paying enough attention
> >> to know which commit to revert:-(. Lots of lost productivity:-(
> >>
> >> Simon, the offending commit:
> >>
> >> 6d3ef6b drivers/pinctrl: grab default handles from device core
> >>
> >> is still in next-20130116. Can you please remove it?
> > 
> > I removed the commit yesterday.
> > Please let me know if it seems to be lingering.
> 
> Thanks. I believe it's gone from next-20130117, judging by a quick
> search through "git log" at least.

Thanks and sorry for the mess.

Re: [PATCH v2] drivers/pinctrl: grab default handles from device core

[Linus Walleij]

On Thu, Jan 10, 2013 at 9:42 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

>> +     /* Allocate a pin state container on-the-fly */
>> +     if (!dev->pins) {
>> +             dpi = devm_kzalloc(dev, sizeof(*dpi), GFP_KERNEL);
>
> This is allocated using a devm_ function. If -EPROBE_DEFER is returned
> below after the assignment to dev->pins or if the driver's own probe()
> returns -EPROBE_DEFER, this allocation will be freed by the driver core.
> This can leave dev->pins pointing to something non-NULL, yet invalid.

OK I (think I) fixed it like this:

For the second deferral, set dev->pins to NULL.

I did some other fixes here to explicitly devm_kfree() the container
if no pinctrl handle was found. No point in keeping it around.

>> +     if (!dpi->p) {
>> +             dpi->p = devm_pinctrl_get(dev);
>
> That won't succeed for a pinctrl device that has a default state in
> order to implement hogs. This will then cause the pin controller device
> to always defer probe and never activate. This will leave HW
> unconfigured and/or prevent other devices from successfully calling
> pinctrl_get().

Fixed by your suggested patch, thanks.

>> +     /*
>> +      * See if somebody else (such as the device core) has already
>> +      * obtained a handle to the pinctrl for this device. In that case,
>> +      * return another pointer to it.
>> +      */
>>       p = find_pinctrl(dev);
>> -     if (p != NULL)
>> -             return ERR_PTR(-EBUSY);
>
> I deliberately returned an error here, because there's no reference
> counting on the struct pinctrl objects. If a driver calls pinctrl_get(),
> with the new code below, it will retrieve the same struct. If it later
> calls pinctrl_put(), the put will immediately free the structure. This
> will invalidate the pointers that reference it in struct device's pins
> field.
>
> This issue will probably trigger on Tegra, since we at least have a
> pinctrl-based I2C mux that calls pinctrl_get().

OK I just introduced a reference counter using <linux/kref.h>.

I've retested on U300 and U8500.

Yours,
Linus Walleij