pinctrl-sx150x.c broken in 4.11

pinctrl-sx150x.c broken in 4.11

[Nikita Yushchenko]

Hi

Looks like recent pinctrl changes - possibly commit 99e4f67508e1
("pinctrl: core: Use delayed work for hogs") - breaks pinctrl-sx150x
driver in all setups where it has any pinctrl settings in device tree.

AFAIU, pinctrl-sx150x is not a real pinctrl/pinmux driver, but it uses
pinctrl subsystem to provide control over GPIO lines it provides.  But
for user, it is just a i2c-gpio device that is enabled via device tree,
and, like other devices, can have pinctrl-0 that points to "real" pinmux
configuration for involved hardware lines (i.e. line used for interrupt).

Problem is that when pinctrl-sx150x driver registers itself via
pinctrl_register(), pinctrl map that corresponds to pinctrl-0 property
of sx150x device tree node, is misinterpreted as hog.  Corresponding
call chain is

  pinctrl_enable() ->
    pinctrl_claim_hogs() ->
      create_pinctrl()

at this point, registered pinctrl maps are scanned and matched by device
name only, without checking map's control device. Then map is passed to
add_setting() with pctldev set to sx150x which does not provide
pinmux_ops, which errors out:

sx150x-pinctrl 10-0020: does not support mux function
sx150x-pinctrl 10-0020: could not map pin config for "VF610_PAD_PTB1"
sx150x-pinctrl 10-0020: error claiming hogs: -22
sx150x-pinctrl 10-0020: could not claim hogs: -22
sx150x-pinctrl 10-0020: Failed to register pinctrl device
sx150x-pinctrl: probe of 10-0020 failed with error -22

Before commit 99e4f67508e1 ("pinctrl: core: Use delayed work for hogs")
problem was hidden by not passing pinctrl device to add_setting(), but
instead getting that from map.


What is proper fix for this?
Maybe something like

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 32822b0d9cd0..78dde7594825 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1038,6 +1038,9 @@ static struct pinctrl *create_pinctrl(struct
device *dev,
                /* Map must be for this device */
                if (strcmp(map->dev_name, devname))
                        continue;
+               if (pctldev &&
+                   strcmp(dev_name(pctldev->dev), map->ctrl_dev_name))
+                       continue;

                ret = add_setting(p, pctldev, map);
                /*

Re: pinctrl-sx150x.c broken in 4.11

[Linus Walleij]

On Wed, May 10, 2017 at 9:32 PM, Nikita Yushchenko
<nikita.yoush@cogentembedded.com> wrote:

> Looks like recent pinctrl changes - possibly commit 99e4f67508e1
> ("pinctrl: core: Use delayed work for hogs") - breaks pinctrl-sx150x
> driver in all setups where it has any pinctrl settings in device tree.
>
> AFAIU, pinctrl-sx150x is not a real pinctrl/pinmux driver,

It's as real as it gets.

> but it uses
> pinctrl subsystem to provide control over GPIO lines it provides.  But
> for user, it is just a i2c-gpio device that is enabled via device tree,
> and, like other devices, can have pinctrl-0 that points to "real" pinmux
> configuration for involved hardware lines (i.e. line used for interrupt).
>
> Problem is that when pinctrl-sx150x driver registers itself via
> pinctrl_register(), pinctrl map that corresponds to pinctrl-0 property
> of sx150x device tree node, is misinterpreted as hog.  Corresponding
> call chain is
>
>   pinctrl_enable() ->
>     pinctrl_claim_hogs() ->
>       create_pinctrl()
>
> at this point, registered pinctrl maps are scanned and matched by device
> name only, without checking map's control device. Then map is passed to
> add_setting() with pctldev set to sx150x which does not provide
> pinmux_ops, which errors out:
>
> sx150x-pinctrl 10-0020: does not support mux function
> sx150x-pinctrl 10-0020: could not map pin config for "VF610_PAD_PTB1"
> sx150x-pinctrl 10-0020: error claiming hogs: -22
> sx150x-pinctrl 10-0020: could not claim hogs: -22
> sx150x-pinctrl 10-0020: Failed to register pinctrl device
> sx150x-pinctrl: probe of 10-0020 failed with error -22
>
> Before commit 99e4f67508e1 ("pinctrl: core: Use delayed work for hogs")
> problem was hidden by not passing pinctrl device to add_setting(), but
> instead getting that from map.
>
> What is proper fix for this?

I bet Tony has the answer to this.

I think something similar to:
commit 6118714275f0a313ecc296a87ed1af32d9691bed
"pinctrl: core: Fix pinctrl_register_and_init() with pinctrl_enable()"
is needed?

Maybe we need to go over everything in drivers/pinctrl and check that
we don't have more of these broken hogs.

Yours,
Linus Walleij

Re: pinctrl-sx150x.c broken in 4.11

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [170511 00:26]:
> On Wed, May 10, 2017 at 9:32 PM, Nikita Yushchenko
> <nikita.yoush@cogentembedded.com> wrote:
> 
> > Looks like recent pinctrl changes - possibly commit 99e4f67508e1
> > ("pinctrl: core: Use delayed work for hogs") - breaks pinctrl-sx150x
> > driver in all setups where it has any pinctrl settings in device tree.
> >
> > AFAIU, pinctrl-sx150x is not a real pinctrl/pinmux driver,
> 
> It's as real as it gets.
> 
> > but it uses
> > pinctrl subsystem to provide control over GPIO lines it provides.  But
> > for user, it is just a i2c-gpio device that is enabled via device tree,
> > and, like other devices, can have pinctrl-0 that points to "real" pinmux
> > configuration for involved hardware lines (i.e. line used for interrupt).
> >
> > Problem is that when pinctrl-sx150x driver registers itself via
> > pinctrl_register(), pinctrl map that corresponds to pinctrl-0 property
> > of sx150x device tree node, is misinterpreted as hog.  Corresponding
> > call chain is
> >
> >   pinctrl_enable() ->
> >     pinctrl_claim_hogs() ->
> >       create_pinctrl()
> >
> > at this point, registered pinctrl maps are scanned and matched by device
> > name only, without checking map's control device. Then map is passed to
> > add_setting() with pctldev set to sx150x which does not provide
> > pinmux_ops, which errors out:
> >
> > sx150x-pinctrl 10-0020: does not support mux function
> > sx150x-pinctrl 10-0020: could not map pin config for "VF610_PAD_PTB1"
> > sx150x-pinctrl 10-0020: error claiming hogs: -22
> > sx150x-pinctrl 10-0020: could not claim hogs: -22
> > sx150x-pinctrl 10-0020: Failed to register pinctrl device
> > sx150x-pinctrl: probe of 10-0020 failed with error -22
> >
> > Before commit 99e4f67508e1 ("pinctrl: core: Use delayed work for hogs")
> > problem was hidden by not passing pinctrl device to add_setting(), but
> > instead getting that from map.
> >
> > What is proper fix for this?
> 
> I bet Tony has the answer to this.
> 
> I think something similar to:
> commit 6118714275f0a313ecc296a87ed1af32d9691bed
> "pinctrl: core: Fix pinctrl_register_and_init() with pinctrl_enable()"
> is needed?

Hmm maybe yeah. I don't quite follow the above the "pinctrl-0 property
of sx150x device tree node, is misinterpreted as hog" part though.

But at least with updating the probe to use pinctrl_register_and_init()
and pinctrl_enable() the driver can do something before the hogs are
claimed. I just don't know what the driver would here as I don't
understand the "misinterpreted as hog" part :)

Anyways, care to test if the following patch fixes the issue somehow?
And also maybe try to see what is missing in the REVISIT part of
the patch.. Compile tested only.

Hmm maybe the driver can claim pinctrl-0 before the hogs are claimed
then release it after the hogs are claimed? Just guessing though.

> Maybe we need to go over everything in drivers/pinctrl and check that
> we don't have more of these broken hogs.

It would be probably best to update all the drivers to probe with
pinctrl_register_and_init() with pinctrl_enable() as otherwise we'll
never know where the issue is going to pop up again. Right now all
it might take is configuring hogs for a driver in dts.. Anybody
know coccinelle?

But let's first figure out what exactly is the problem here.

Regards,

Tony

8< ----------------------
diff --git a/drivers/pinctrl/pinctrl-sx150x.c b/drivers/pinctrl/pinctrl-sx150x.c
--- a/drivers/pinctrl/pinctrl-sx150x.c
+++ b/drivers/pinctrl/pinctrl-sx150x.c
@@ -1225,13 +1225,16 @@ static int sx150x_probe(struct i2c_client *client,
 	pctl->pinctrl_desc.npins = pctl->data->npins;
 	pctl->pinctrl_desc.owner = THIS_MODULE;
 
-	pctl->pctldev = pinctrl_register(&pctl->pinctrl_desc, dev, pctl);
-	if (IS_ERR(pctl->pctldev)) {
-		dev_err(dev, "Failed to register pinctrl device\n");
-		return PTR_ERR(pctl->pctldev);
+	ret = pinctrl_register_and_init(&pctl->pinctrl_desc, dev,
+					pctl, &pctl->pctldev);
+	if (ret) {
+		dev_err(dev, "Failed to register pinctrl\n");
+		return ret;
 	}
 
-	return 0;
+	/* REVISIT: Do something with pinctrl-0 misinterpreted as hog? */
+
+	return pinctrl_enable(pctl->pctldev);
 }
 
 static struct i2c_driver sx150x_driver = {
-- 
2.12.2

Re: pinctrl-sx150x.c broken in 4.11

[Nikita Yushchenko]

> Hmm maybe yeah. I don't quite follow the above the "pinctrl-0 property
> of sx150x device tree node, is misinterpreted as hog" part though.

sx150x is i2c-gpio device.  It has 16 GPIO lines that are communicated
with via i2c bus, and an interrupt line.

Interrupt line is typically connected to SoC's pin.
This pin has to be configured.
This is done by providing appropriate subnode in SoC's pinmux node, with
information with pin configuration, and pinctrl-0 property in sx150x's
node with phandle to that subnode:

...
&i2c0 {
	sx1503@20 {
		compatible = "semtech,sx1503q";
		pinctrl-names = "default";
		pinctrl-0 = <&pinctrl_sx1503_20>;
		...
	};
};
...
&iomuxc {
	pinctrl_sx1503_20: pinctrl-sx1503-20 {
		fsl,pins = <
			VF610_PAD_PTB1__GPIO_23         0x219d
		>;
	};
};

This pin configuration is handled by driver core, i.e. before probe()
for sx150x is called, core applies pin configuration.

However sx150x driver is currently implemented as a pinctrl driver.

When it initializes, pinctrl searches for "hog", i.e. pin config that
should be applied at driver registration time.

While doing so, core searches for any registered pinctrl_map for device
being register. Search loop is in create_pinctrl().

In this case, this loop finds map that is defined above.

This is *not* hog.  This is pin setting already applied in SoC's pinmux
controller for sx1503 device.

However code in create_pinctrl() tries to apply it, and use sx1503's
methods to do so. Which is plain wrong and errors out.
	

> But at least with updating the probe to use pinctrl_register_and_init()
> and pinctrl_enable() the driver can do something before the hogs are
> claimed. I just don't know what the driver would here as I don't
> understand the "misinterpreted as hog" part :)

Tried to explain above :)

I don't know whar can be done in sx1503 driver to avoid that scenario...
perhaps port it back from pinctrl subsystem to gpio subsystem.  However
I guess it was ported to pinctrl subsystem for a reason (that I don't know).


> Anyways, care to test if the following patch fixes the issue somehow?

No that patch does not help here.

Nikita

Re: pinctrl-sx150x.c broken in 4.11

[Tony Lindgren]

* Nikita Yushchenko <nikita.yoush@cogentembedded.com> [170511 09:27]:
> > Hmm maybe yeah. I don't quite follow the above the "pinctrl-0 property
> > of sx150x device tree node, is misinterpreted as hog" part though.
> 
> sx150x is i2c-gpio device.  It has 16 GPIO lines that are communicated
> with via i2c bus, and an interrupt line.
> 
> Interrupt line is typically connected to SoC's pin.
> This pin has to be configured.
> This is done by providing appropriate subnode in SoC's pinmux node, with
> information with pin configuration, and pinctrl-0 property in sx150x's
> node with phandle to that subnode:
> 
> ...
> &i2c0 {
> 	sx1503@20 {
> 		compatible = "semtech,sx1503q";
> 		pinctrl-names = "default";
> 		pinctrl-0 = <&pinctrl_sx1503_20>;
> 		...
> 	};
> };
> ...
> &iomuxc {
> 	pinctrl_sx1503_20: pinctrl-sx1503-20 {
> 		fsl,pins = <
> 			VF610_PAD_PTB1__GPIO_23         0x219d
> 		>;
> 	};
> };
> 
> This pin configuration is handled by driver core, i.e. before probe()
> for sx150x is called, core applies pin configuration.
> 
> However sx150x driver is currently implemented as a pinctrl driver.
> 
> When it initializes, pinctrl searches for "hog", i.e. pin config that
> should be applied at driver registration time.
> 
> While doing so, core searches for any registered pinctrl_map for device
> being register. Search loop is in create_pinctrl().
> 
> In this case, this loop finds map that is defined above.
> 
> This is *not* hog.  This is pin setting already applied in SoC's pinmux
> controller for sx1503 device.
> 
> However code in create_pinctrl() tries to apply it, and use sx1503's
> methods to do so. Which is plain wrong and errors out.

Maybe create_pinctrl() could check if the pin controller device
for a potential hog points to the device itself and bail out
if that's not the case?

> > But at least with updating the probe to use pinctrl_register_and_init()
> > and pinctrl_enable() the driver can do something before the hogs are
> > claimed. I just don't know what the driver would here as I don't
> > understand the "misinterpreted as hog" part :)
> 
> Tried to explain above :)

Yup OK based on that this seems like a pinctrl core issue.

Regards,

Tony

Re: pinctrl-sx150x.c broken in 4.11

[Nikita Yushchenko]

>>> Hmm maybe yeah. I don't quite follow the above the "pinctrl-0 property
>>> of sx150x device tree node, is misinterpreted as hog" part though.
>>
>> sx150x is i2c-gpio device.  It has 16 GPIO lines that are communicated
>> with via i2c bus, and an interrupt line.
>>
>> Interrupt line is typically connected to SoC's pin.
>> This pin has to be configured.
>> This is done by providing appropriate subnode in SoC's pinmux node, with
>> information with pin configuration, and pinctrl-0 property in sx150x's
>> node with phandle to that subnode:
>>
>> ...
>> &i2c0 {
>> 	sx1503@20 {
>> 		compatible = "semtech,sx1503q";
>> 		pinctrl-names = "default";
>> 		pinctrl-0 = <&pinctrl_sx1503_20>;
>> 		...
>> 	};
>> };
>> ...
>> &iomuxc {
>> 	pinctrl_sx1503_20: pinctrl-sx1503-20 {
>> 		fsl,pins = <
>> 			VF610_PAD_PTB1__GPIO_23         0x219d
>> 		>;
>> 	};
>> };
>>
>> This pin configuration is handled by driver core, i.e. before probe()
>> for sx150x is called, core applies pin configuration.
>>
>> However sx150x driver is currently implemented as a pinctrl driver.
>>
>> When it initializes, pinctrl searches for "hog", i.e. pin config that
>> should be applied at driver registration time.
>>
>> While doing so, core searches for any registered pinctrl_map for device
>> being register. Search loop is in create_pinctrl().
>>
>> In this case, this loop finds map that is defined above.
>>
>> This is *not* hog.  This is pin setting already applied in SoC's pinmux
>> controller for sx1503 device.
>>
>> However code in create_pinctrl() tries to apply it, and use sx1503's
>> methods to do so. Which is plain wrong and errors out.
> 
> Maybe create_pinctrl() could check if the pin controller device
> for a potential hog points to the device itself and bail out
> if that's not the case?

Well that's exactly what patch from my first mail in this thread does.
This indeed fixes my case, but I don't know if it is correct in generic
case.

Should I submit it? Do you ack?

Nikita

Re: pinctrl-sx150x.c broken in 4.11

[Tony Lindgren]

* Nikita Yushchenko <nikita.yoush@cogentembedded.com> [170511 10:01]:
> >>> Hmm maybe yeah. I don't quite follow the above the "pinctrl-0 property
> >>> of sx150x device tree node, is misinterpreted as hog" part though.
> >>
> >> sx150x is i2c-gpio device.  It has 16 GPIO lines that are communicated
> >> with via i2c bus, and an interrupt line.
> >>
> >> Interrupt line is typically connected to SoC's pin.
> >> This pin has to be configured.
> >> This is done by providing appropriate subnode in SoC's pinmux node, with
> >> information with pin configuration, and pinctrl-0 property in sx150x's
> >> node with phandle to that subnode:
> >>
> >> ...
> >> &i2c0 {
> >> 	sx1503@20 {
> >> 		compatible = "semtech,sx1503q";
> >> 		pinctrl-names = "default";
> >> 		pinctrl-0 = <&pinctrl_sx1503_20>;
> >> 		...
> >> 	};
> >> };
> >> ...
> >> &iomuxc {
> >> 	pinctrl_sx1503_20: pinctrl-sx1503-20 {
> >> 		fsl,pins = <
> >> 			VF610_PAD_PTB1__GPIO_23         0x219d
> >> 		>;
> >> 	};
> >> };
> >>
> >> This pin configuration is handled by driver core, i.e. before probe()
> >> for sx150x is called, core applies pin configuration.
> >>
> >> However sx150x driver is currently implemented as a pinctrl driver.
> >>
> >> When it initializes, pinctrl searches for "hog", i.e. pin config that
> >> should be applied at driver registration time.
> >>
> >> While doing so, core searches for any registered pinctrl_map for device
> >> being register. Search loop is in create_pinctrl().
> >>
> >> In this case, this loop finds map that is defined above.
> >>
> >> This is *not* hog.  This is pin setting already applied in SoC's pinmux
> >> controller for sx1503 device.
> >>
> >> However code in create_pinctrl() tries to apply it, and use sx1503's
> >> methods to do so. Which is plain wrong and errors out.
> > 
> > Maybe create_pinctrl() could check if the pin controller device
> > for a potential hog points to the device itself and bail out
> > if that's not the case?
> 
> Well that's exactly what patch from my first mail in this thread does.
> This indeed fixes my case, but I don't know if it is correct in generic
> case.

OK, yeah I just looked it up as I was not in cc.

> Should I submit it? Do you ack?

Yeah please submit a proper patch. I assume you already checked
that this change only affects the pinctrl hogs only, not regular
device pins? I'd assume so as it's in create_pinctrl()..

Regards,

Tony

Re: pinctrl-sx150x.c broken in 4.11

[Nikita Yushchenko]

>>> Maybe create_pinctrl() could check if the pin controller device
>>> for a potential hog points to the device itself and bail out
>>> if that's not the case?
>>
>> Well that's exactly what patch from my first mail in this thread does.
>> This indeed fixes my case, but I don't know if it is correct in generic
>> case.
> 
> OK, yeah I just looked it up as I was not in cc.
> 
>> Should I submit it? Do you ack?
> 
> Yeah please submit a proper patch. I assume you already checked
> that this change only affects the pinctrl hogs only, not regular
> device pins? I'd assume so as it's in create_pinctrl()..

Regular device pins go via pinctrl_get() which calls create_pinctrl()
with second argument set to NULL.

Re: pinctrl-sx150x.c broken in 4.11

[Linus Walleij]

On Thu, May 11, 2017 at 7:51 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Nikita Yushchenko <nikita.yoush@cogentembedded.com> [170511 10:01]:

>> Well that's exactly what patch from my first mail in this thread does.
>> This indeed fixes my case, but I don't know if it is correct in generic
>> case.
>
> OK, yeah I just looked it up as I was not in cc.
>
>> Should I submit it? Do you ack?
>
> Yeah please submit a proper patch. I assume you already checked
> that this change only affects the pinctrl hogs only, not regular
> device pins? I'd assume so as it's in create_pinctrl()..

Thanks for sorting this out, both of you.
Much appreciated!

Yours,
Linus Walleij