diff mbox series

pinctrl: devicetree: Fix pctldev pointer overwrite

Message ID 1528390293-8022-1-git-send-email-festevam@gmail.com
State New
Headers show
Series pinctrl: devicetree: Fix pctldev pointer overwrite | expand

Commit Message

Fabio Estevam June 7, 2018, 4:51 p.m. UTC 
From: Fabio Estevam <fabio.estevam@nxp.com>

Commit b89405b6102f ("pinctrl: devicetree: Fix dt_to_map_one_config
handling of hogs") causes the pinctrl hog pins to not get initialized
on i.MX platforms leaving them with the IOMUX settings untouched.

This causes several regressions on i.MX such as:

- OV5640 camera driver can not be probed anymore on imx6qdl-sabresd
because the camera clock pin is in a pinctrl_hog group and since
its pinctrl initialization is skipped, the camera clock is kept
in GPIO functionality instead of CLK_CKO function.

- Audio stopped working on imx6qdl-wandboard and imx53-qsb for
the same reason.

Richard Fitzgerald explains the problem:

"I see the bug. If the hog node isn't a 1st level child of the pinctrl
parent node it will go around the for(;;) loop again but on the first
pass I overwrite pctldev with the result of
get_pinctrl_dev_from_of_node() so it doesn't point to the pinctrl driver
any more."

Fix the issue by stashing the original pctldev so it doesn't
get overwritten.

Fixes:  b89405b6102f ("pinctrl: devicetree: Fix dt_to_map_one_config handling of hogs")
Cc: <stable@vger.kernel.org>
Reported-by: Mika Penttilä <mika.penttila@nextfour.com>
Reported-by: Steve Longerbeam <slongerbeam@gmail.com>
Suggested-by: Richard Fitzgerald <rf@opensource.cirrus.com>
Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
 drivers/pinctrl/devicetree.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Dong Aisheng June 8, 2018, 5:38 a.m. UTC | #1 
PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBGYWJpbyBFc3RldmFtIFttYWls
dG86ZmVzdGV2YW1AZ21haWwuY29tXQ0KPiBTZW50OiBGcmlkYXksIEp1bmUgOCwgMjAxOCAxMjo1
MiBBTQ0KPiBUbzogbGludXMud2FsbGVpakBsaW5hcm8ub3JnDQo+IENjOiByZkBvcGVuc291cmNl
LmNpcnJ1cy5jb207IGxpbnV4LWdwaW9Admdlci5rZXJuZWwub3JnOw0KPiBzbG9uZ2VyYmVhbUBn
bWFpbC5jb207IHNoYXduZ3VvQGtlcm5lbC5vcmc7IGtlcm5lbEBwZW5ndXRyb25peC5kZTsNCj4g
ZGwtbGludXgtaW14IDxsaW51eC1pbXhAbnhwLmNvbT47IG1pa2EucGVudHRpbGFAbmV4dGZvdXIu
Y29tOyBGYWJpbw0KPiBFc3RldmFtIDxmYWJpby5lc3RldmFtQG54cC5jb20+OyBzdGFibGVAdmdl
ci5rZXJuZWwub3JnDQo+IFN1YmplY3Q6IFtQQVRDSF0gcGluY3RybDogZGV2aWNldHJlZTogRml4
IHBjdGxkZXYgcG9pbnRlciBvdmVyd3JpdGUNCj4gDQo+IEZyb206IEZhYmlvIEVzdGV2YW0gPGZh
YmlvLmVzdGV2YW1AbnhwLmNvbT4NCj4gDQo+IENvbW1pdCBiODk0MDViNjEwMmYgKCJwaW5jdHJs
OiBkZXZpY2V0cmVlOiBGaXggZHRfdG9fbWFwX29uZV9jb25maWcNCj4gaGFuZGxpbmcgb2YgaG9n
cyIpIGNhdXNlcyB0aGUgcGluY3RybCBob2cgcGlucyB0byBub3QgZ2V0IGluaXRpYWxpemVkIG9u
IGkuTVgNCj4gcGxhdGZvcm1zIGxlYXZpbmcgdGhlbSB3aXRoIHRoZSBJT01VWCBzZXR0aW5ncyB1
bnRvdWNoZWQuDQo+IA0KPiBUaGlzIGNhdXNlcyBzZXZlcmFsIHJlZ3Jlc3Npb25zIG9uIGkuTVgg
c3VjaCBhczoNCj4gDQo+IC0gT1Y1NjQwIGNhbWVyYSBkcml2ZXIgY2FuIG5vdCBiZSBwcm9iZWQg
YW55bW9yZSBvbiBpbXg2cWRsLXNhYnJlc2QNCj4gYmVjYXVzZSB0aGUgY2FtZXJhIGNsb2NrIHBp
biBpcyBpbiBhIHBpbmN0cmxfaG9nIGdyb3VwIGFuZCBzaW5jZSBpdHMgcGluY3RybA0KPiBpbml0
aWFsaXphdGlvbiBpcyBza2lwcGVkLCB0aGUgY2FtZXJhIGNsb2NrIGlzIGtlcHQgaW4gR1BJTyBm
dW5jdGlvbmFsaXR5IGluc3RlYWQNCj4gb2YgQ0xLX0NLTyBmdW5jdGlvbi4NCj4gDQo+IC0gQXVk
aW8gc3RvcHBlZCB3b3JraW5nIG9uIGlteDZxZGwtd2FuZGJvYXJkIGFuZCBpbXg1My1xc2IgZm9y
IHRoZSBzYW1lDQo+IHJlYXNvbi4NCj4gDQo+IFJpY2hhcmQgRml0emdlcmFsZCBleHBsYWlucyB0
aGUgcHJvYmxlbToNCj4gDQo+ICJJIHNlZSB0aGUgYnVnLiBJZiB0aGUgaG9nIG5vZGUgaXNuJ3Qg
YSAxc3QgbGV2ZWwgY2hpbGQgb2YgdGhlIHBpbmN0cmwgcGFyZW50IG5vZGUNCj4gaXQgd2lsbCBn
byBhcm91bmQgdGhlIGZvcig7OykgbG9vcCBhZ2FpbiBidXQgb24gdGhlIGZpcnN0IHBhc3MgSSBv
dmVyd3JpdGUgcGN0bGRldg0KPiB3aXRoIHRoZSByZXN1bHQgb2YNCj4gZ2V0X3BpbmN0cmxfZGV2
X2Zyb21fb2Zfbm9kZSgpIHNvIGl0IGRvZXNuJ3QgcG9pbnQgdG8gdGhlIHBpbmN0cmwgZHJpdmVy
IGFueQ0KPiBtb3JlLiINCj4gDQo+IEZpeCB0aGUgaXNzdWUgYnkgc3Rhc2hpbmcgdGhlIG9yaWdp
bmFsIHBjdGxkZXYgc28gaXQgZG9lc24ndCBnZXQgb3ZlcndyaXR0ZW4uDQo+IA0KPiBGaXhlczog
IGI4OTQwNWI2MTAyZiAoInBpbmN0cmw6IGRldmljZXRyZWU6IEZpeCBkdF90b19tYXBfb25lX2Nv
bmZpZw0KPiBoYW5kbGluZyBvZiBob2dzIikNCj4gQ2M6IDxzdGFibGVAdmdlci5rZXJuZWwub3Jn
Pg0KPiBSZXBvcnRlZC1ieTogTWlrYSBQZW50dGlsw6QgPG1pa2EucGVudHRpbGFAbmV4dGZvdXIu
Y29tPg0KPiBSZXBvcnRlZC1ieTogU3RldmUgTG9uZ2VyYmVhbSA8c2xvbmdlcmJlYW1AZ21haWwu
Y29tPg0KPiBTdWdnZXN0ZWQtYnk6IFJpY2hhcmQgRml0emdlcmFsZCA8cmZAb3BlbnNvdXJjZS5j
aXJydXMuY29tPg0KPiBTaWduZWQtb2ZmLWJ5OiBGYWJpbyBFc3RldmFtIDxmYWJpby5lc3RldmFt
QG54cC5jb20+DQoNCkxvb2tzIGdvb2QgdG8gbWUuDQoNClJldmlld2VkLWJ5OiBEb25nIEFpc2hl
bmcgPGFpc2hlbmcuZG9uZ0BueHAuY29tPg0KDQpSZWdhcmRzDQpEb25nIEFpc2hlbmcNCg0KPiAt
LS0NCj4gIGRyaXZlcnMvcGluY3RybC9kZXZpY2V0cmVlLmMgfCA3ICsrKysrLS0NCj4gIDEgZmls
ZSBjaGFuZ2VkLCA1IGluc2VydGlvbnMoKyksIDIgZGVsZXRpb25zKC0pDQo+IA0KPiBkaWZmIC0t
Z2l0IGEvZHJpdmVycy9waW5jdHJsL2RldmljZXRyZWUuYyBiL2RyaXZlcnMvcGluY3RybC9kZXZp
Y2V0cmVlLmMgaW5kZXgNCj4gYjYwMTAzOS4uYzRhYTQxMSAxMDA2NDQNCj4gLS0tIGEvZHJpdmVy
cy9waW5jdHJsL2RldmljZXRyZWUuYw0KPiArKysgYi9kcml2ZXJzL3BpbmN0cmwvZGV2aWNldHJl
ZS5jDQo+IEBAIC0xMDEsMTAgKzEwMSwxMSBAQCBzdHJ1Y3QgcGluY3RybF9kZXYgKm9mX3BpbmN0
cmxfZ2V0KHN0cnVjdA0KPiBkZXZpY2Vfbm9kZSAqbnApICB9DQo+IA0KPiAgc3RhdGljIGludCBk
dF90b19tYXBfb25lX2NvbmZpZyhzdHJ1Y3QgcGluY3RybCAqcCwNCj4gLQkJCQlzdHJ1Y3QgcGlu
Y3RybF9kZXYgKnBjdGxkZXYsDQo+ICsJCQkJc3RydWN0IHBpbmN0cmxfZGV2ICpob2dfcGN0bGRl
diwNCj4gIAkJCQljb25zdCBjaGFyICpzdGF0ZW5hbWUsDQo+ICAJCQkJc3RydWN0IGRldmljZV9u
b2RlICpucF9jb25maWcpDQo+ICB7DQo+ICsJc3RydWN0IHBpbmN0cmxfZGV2ICpwY3RsZGV2ID0g
TlVMTDsNCj4gIAlzdHJ1Y3QgZGV2aWNlX25vZGUgKm5wX3BjdGxkZXY7DQo+ICAJY29uc3Qgc3Ry
dWN0IHBpbmN0cmxfb3BzICpvcHM7DQo+ICAJaW50IHJldDsNCj4gQEAgLTEyMyw4ICsxMjQsMTAg
QEAgc3RhdGljIGludCBkdF90b19tYXBfb25lX2NvbmZpZyhzdHJ1Y3QgcGluY3RybCAqcCwNCj4g
IAkJCXJldHVybiAtRVBST0JFX0RFRkVSOw0KPiAgCQl9DQo+ICAJCS8qIElmIHdlJ3JlIGNyZWF0
aW5nIGEgaG9nIHdlIGNhbiB1c2UgdGhlIHBhc3NlZCBwY3RsZGV2ICovDQo+IC0JCWlmIChwY3Rs
ZGV2ICYmIChucF9wY3RsZGV2ID09IHAtPmRldi0+b2Zfbm9kZSkpDQo+ICsJCWlmIChob2dfcGN0
bGRldiAmJiAobnBfcGN0bGRldiA9PSBwLT5kZXYtPm9mX25vZGUpKSB7DQo+ICsJCQlwY3RsZGV2
ID0gaG9nX3BjdGxkZXY7DQo+ICAJCQlicmVhazsNCj4gKwkJfQ0KPiAgCQlwY3RsZGV2ID0gZ2V0
X3BpbmN0cmxfZGV2X2Zyb21fb2Zfbm9kZShucF9wY3RsZGV2KTsNCj4gIAkJaWYgKHBjdGxkZXYp
DQo+ICAJCQlicmVhazsNCj4gLS0NCj4gMi43LjQNCg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Fitzgerald June 8, 2018, 9:06 a.m. UTC | #2 
On 07/06/18 17:51, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@nxp.com>
> 
> Commit b89405b6102f ("pinctrl: devicetree: Fix dt_to_map_one_config
> handling of hogs") causes the pinctrl hog pins to not get initialized
> on i.MX platforms leaving them with the IOMUX settings untouched.
> 
> This causes several regressions on i.MX such as:
> 
> - OV5640 camera driver can not be probed anymore on imx6qdl-sabresd
> because the camera clock pin is in a pinctrl_hog group and since
> its pinctrl initialization is skipped, the camera clock is kept
> in GPIO functionality instead of CLK_CKO function.
> 
> - Audio stopped working on imx6qdl-wandboard and imx53-qsb for
> the same reason.
> 
> Richard Fitzgerald explains the problem:
> 
> "I see the bug. If the hog node isn't a 1st level child of the pinctrl
> parent node it will go around the for(;;) loop again but on the first
> pass I overwrite pctldev with the result of
> get_pinctrl_dev_from_of_node() so it doesn't point to the pinctrl driver
> any more."
> 
> Fix the issue by stashing the original pctldev so it doesn't
> get overwritten.
> 
> Fixes:  b89405b6102f ("pinctrl: devicetree: Fix dt_to_map_one_config handling of hogs")
> Cc: <stable@vger.kernel.org>
> Reported-by: Mika Penttilä <mika.penttila@nextfour.com>
> Reported-by: Steve Longerbeam <slongerbeam@gmail.com>
> Suggested-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> ---
>   drivers/pinctrl/devicetree.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
> index b601039..c4aa411 100644
> --- a/drivers/pinctrl/devicetree.c
> +++ b/drivers/pinctrl/devicetree.c
> @@ -101,10 +101,11 @@ struct pinctrl_dev *of_pinctrl_get(struct device_node *np)
>   }
>   
>   static int dt_to_map_one_config(struct pinctrl *p,
> -				struct pinctrl_dev *pctldev,
> +				struct pinctrl_dev *hog_pctldev,
>   				const char *statename,
>   				struct device_node *np_config)
>   {
> +	struct pinctrl_dev *pctldev = NULL;
>   	struct device_node *np_pctldev;
>   	const struct pinctrl_ops *ops;
>   	int ret;
> @@ -123,8 +124,10 @@ static int dt_to_map_one_config(struct pinctrl *p,
>   			return -EPROBE_DEFER;
>   		}
>   		/* If we're creating a hog we can use the passed pctldev */
> -		if (pctldev && (np_pctldev == p->dev->of_node))
> +		if (hog_pctldev && (np_pctldev == p->dev->of_node)) {
> +			pctldev = hog_pctldev;
>   			break;
> +		}
>   		pctldev = get_pinctrl_dev_from_of_node(np_pctldev);
>   		if (pctldev)
>   			break;
> 

Thanks for creating this patch. Looks ok to me.

Reviewed-by: Richard Fitzgerald <rf@opensource.cirrus.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij June 13, 2018, 11 a.m. UTC | #3 
On Thu, Jun 7, 2018 at 6:51 PM, Fabio Estevam <festevam@gmail.com> wrote:

> From: Fabio Estevam <fabio.estevam@nxp.com>
>
> Commit b89405b6102f ("pinctrl: devicetree: Fix dt_to_map_one_config
> handling of hogs") causes the pinctrl hog pins to not get initialized
> on i.MX platforms leaving them with the IOMUX settings untouched.
>
> This causes several regressions on i.MX such as:
>
> - OV5640 camera driver can not be probed anymore on imx6qdl-sabresd
> because the camera clock pin is in a pinctrl_hog group and since
> its pinctrl initialization is skipped, the camera clock is kept
> in GPIO functionality instead of CLK_CKO function.
>
> - Audio stopped working on imx6qdl-wandboard and imx53-qsb for
> the same reason.
>
> Richard Fitzgerald explains the problem:
>
> "I see the bug. If the hog node isn't a 1st level child of the pinctrl
> parent node it will go around the for(;;) loop again but on the first
> pass I overwrite pctldev with the result of
> get_pinctrl_dev_from_of_node() so it doesn't point to the pinctrl driver
> any more."
>
> Fix the issue by stashing the original pctldev so it doesn't
> get overwritten.
>
> Fixes:  b89405b6102f ("pinctrl: devicetree: Fix dt_to_map_one_config handling of hogs")
> Cc: <stable@vger.kernel.org>
> Reported-by: Mika Penttilä <mika.penttila@nextfour.com>
> Reported-by: Steve Longerbeam <slongerbeam@gmail.com>
> Suggested-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>

Patch applied for fixes with the ACKs!

Thanks for digging into this and fixing it up!

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index b601039..c4aa411 100644
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -101,10 +101,11 @@  struct pinctrl_dev *of_pinctrl_get(struct device_node *np)
 }
 
 static int dt_to_map_one_config(struct pinctrl *p,
-				struct pinctrl_dev *pctldev,
+				struct pinctrl_dev *hog_pctldev,
 				const char *statename,
 				struct device_node *np_config)
 {
+	struct pinctrl_dev *pctldev = NULL;
 	struct device_node *np_pctldev;
 	const struct pinctrl_ops *ops;
 	int ret;
@@ -123,8 +124,10 @@  static int dt_to_map_one_config(struct pinctrl *p,
 			return -EPROBE_DEFER;
 		}
 		/* If we're creating a hog we can use the passed pctldev */
-		if (pctldev && (np_pctldev == p->dev->of_node))
+		if (hog_pctldev && (np_pctldev == p->dev->of_node)) {
+			pctldev = hog_pctldev;
 			break;
+		}
 		pctldev = get_pinctrl_dev_from_of_node(np_pctldev);
 		if (pctldev)
 			break;