otg/ulpi: bail out on read errors

PATCH New Wolfram Sang 3 participants 10 messages Jun 15, 2010 – Jun 16, 2010

Subsystems (3)

USB Arm i.MX and Layerscape USB ehci driver

lore

Share & tools

lore

Wolfram Sang

author

2010-06-15 10:34 UTC #0 lore raw

PATCH 1/2 otg/ulpi: bail out on read errors

otg_read may return errnos, so bail out correctly to prevent bogus ID-numbers.

Signed-off-by Wolfram Sang <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc Daniel Mack <[email protected]>

Cc Greg KH <[email protected]>

---
 drivers/usb/otg/ulpi.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

drivers/usb/otg/ulpi.c +11 -6

index b1b3469..d331b22 100644
@@ -59,12 +59,17 @@ static int ulpi_set_flags(struct otg_transceiver *otg)
 
 static int ulpi_init(struct otg_transceiver *otg)
 {
-	int i, vid, pid;
-
-	vid = (otg_io_read(otg, ULPI_VENDOR_ID_HIGH) << 8) |
-	       otg_io_read(otg, ULPI_VENDOR_ID_LOW);
-	pid = (otg_io_read(otg, ULPI_PRODUCT_ID_HIGH) << 8) |
-	       otg_io_read(otg, ULPI_PRODUCT_ID_LOW);
+	int i, vid, pid, ret;
+	u32 ulpi_id = 0;
+
+	for (i = 0; i < 4; i++) {
+		ret = otg_io_read(otg, ULPI_PRODUCT_ID_HIGH - i);
+		if (ret < 0)
+			return ret;
+		ulpi_id = (ulpi_id << 8) | ret;
+	}
+	vid = ulpi_id & 0xffff;
+	pid = ulpi_id >> 16;
 
 	pr_info("ULPI transceiver vendor/product ID 0x%04x/0x%04x\n", vid, pid);

Signature · 1 line

1.7.1

Wolfram Sang

author

2010-06-15 10:34 UTC #1 lore raw

PATCH 2/2 ehci-mxc: bail out on transceiver problems

The old code registered the hcd even if there were no transceivers detected, leading to oopses like this if we try to probe a non-existant ULPI:

[    2.730000] mxc-ehci mxc-ehci.0: unable to init transceiver
[    2.740000] timeout polling for ULPI device
[    2.740000] timeout polling for ULPI device
[    2.750000] mxc-ehci mxc-ehci.0: unable to enable vbus on transceiver
[    2.750000] mxc-ehci mxc-ehci.0: Freescale On-Chip EHCI Host Controller
[    2.760000] mxc-ehci mxc-ehci.0: new USB bus registered, assigned bus number 2
[    2.770000] Unhandled fault: external abort on non-linefetch (0x808) at 0xc4876184
[    2.770000] Internal error: : 808 [#1] PREEMPT
[    2.770000] last sysfs file:
[    2.770000] Modules linked in:
[    2.770000] CPU: 0    Not tainted  (2.6.33.5 #5)
[    2.770000] PC is at ehci_hub_control+0x4d4/0x8f8
[    2.770000] LR is at ehci_mxc_setup+0xbc/0xdc
[    2.770000] pc : [<c0196dfc>]    lr : [<c019bc8c>]    psr: 00000093
[    2.770000] sp : c3815e40  ip : 00000001  fp : 60000013
[    2.770000] r10: c4876184  r9 : 00000000  r8 : c3814000
[    2.770000] r7 : c391d2cc  r6 : 00000001  r5 : 00000001  r4 : 00000000
[    2.770000] r3 : 80000000  r2 : 00000007  r1 : 80000000  r0 : c4876184
[    2.770000] Flags: nzcv  IRQs off  FIQs on  Mode SVC_32  ISA ARM Segment kernel
[    2.770000] Control: 0005317f  Table: a0004000  DAC: 00000017
[    2.770000] Process swapper (pid: 1, stack limit = 0xc3814270)

...

Signed-off-by Wolfram Sang <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc Daniel Mack <[email protected]>

Cc Greg KH <[email protected]>

Cc stable at kernel.org

---
 drivers/usb/host/ehci-mxc.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

drivers/usb/host/ehci-mxc.c +10 -3

index 544ccfd..bd40277 100644
@@ -207,10 +207,17 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
 	/* Initialize the transceiver */
 	if (pdata->otg) {
 		pdata->otg->io_priv = hcd->regs + ULPI_VIEWPORT_OFFSET;
-		if (otg_init(pdata->otg) != 0)
-			dev_err(dev, "unable to init transceiver\n");
-		else if (otg_set_vbus(pdata->otg, 1) != 0)
+		ret = otg_init(pdata->otg);
+		if (ret) {
+			dev_err(dev, "unable to init transceiver, probably missing\n");
+			ret = -ENODEV;
+			goto err_add;
+		}
+		ret = otg_set_vbus(pdata->otg, 1);
+		if (ret) {
 			dev_err(dev, "unable to enable vbus on transceiver\n");
+			goto err_add;
+		}
 	}
 
 	priv->hcd = hcd;

Signature · 1 line

1.7.1

Daniel Mack

2010-06-15 10:43 UTC #2 lore raw

PATCH 2/2 ehci-mxc: bail out on transceiver problems

Quoted 3 lines · Wolfram Sang

The old code registered the hcd even if there were no transceivers detected, leading to oopses like this if we try to probe a non-existant ULPI:

Hmm. I'm aware that there was a missing bail in this function, but actually, I had hardware which didn't properly detect the ULPI chip but still worked fine. There has been quite some discussion here about that, and eventually I decided to not make this a hard error as it didn't really harm.

Quoted 13 lines

[    2.730000] mxc-ehci mxc-ehci.0: unable to init transceiver
[    2.740000] timeout polling for ULPI device
[    2.740000] timeout polling for ULPI device
[    2.750000] mxc-ehci mxc-ehci.0: unable to enable vbus on transceiver
[    2.750000] mxc-ehci mxc-ehci.0: Freescale On-Chip EHCI Host Controller
[    2.760000] mxc-ehci mxc-ehci.0: new USB bus registered, assigned bus number 2
[    2.770000] Unhandled fault: external abort on non-linefetch (0x808) at 0xc4876184
[    2.770000] Internal error: : 808 [#1] PREEMPT
[    2.770000] last sysfs file:
[    2.770000] Modules linked in:
[    2.770000] CPU: 0    Not tainted  (2.6.33.5 #5)
[    2.770000] PC is at ehci_hub_control+0x4d4/0x8f8
[    2.770000] LR is at ehci_mxc_setup+0xbc/0xdc

Quoted 8 lines

[    2.770000] pc : [<c0196dfc>]    lr : [<c019bc8c>]    psr: 00000093
[    2.770000] sp : c3815e40  ip : 00000001  fp : 60000013
[    2.770000] r10: c4876184  r9 : 00000000  r8 : c3814000
[    2.770000] r7 : c391d2cc  r6 : 00000001  r5 : 00000001  r4 : 00000000
[    2.770000] r3 : 80000000  r2 : 00000007  r1 : 80000000  r0 : c4876184
[    2.770000] Flags: nzcv  IRQs off  FIQs on  Mode SVC_32  ISA ARM Segment kernel
[    2.770000] Control: 0005317f  Table: a0004000  DAC: 00000017
[    2.770000] Process swapper (pid: 1, stack limit = 0xc3814270)

But that, of course, changes everthing. If you happen to see such Ooopses, your patch should go in, and I will check the boards again and see if they still fail to probe the ULPI. Eventually, we might need some kind of quirks field in the platform data :(

Thanks,
Daniel

Quoted 2 lines

Signed-off-by Wolfram Sang <[email protected]>
Cc Sascha Hauer <[email protected]>

Acked-by Daniel Mack <[email protected]>

Quoted 34 lines

Cc Greg KH <[email protected]>

Cc stable at kernel.org

---
 drivers/usb/host/ehci-mxc.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

drivers/usb/host/ehci-mxc.c +10 -3

index 544ccfd..bd40277 100644
@@ -207,10 +207,17 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
 	/* Initialize the transceiver */
 	if (pdata->otg) {
 		pdata->otg->io_priv = hcd->regs + ULPI_VIEWPORT_OFFSET;
-		if (otg_init(pdata->otg) != 0)
-			dev_err(dev, "unable to init transceiver\n");
-		else if (otg_set_vbus(pdata->otg, 1) != 0)
+		ret = otg_init(pdata->otg);
+		if (ret) {
+			dev_err(dev, "unable to init transceiver, probably missing\n");
+			ret = -ENODEV;
+			goto err_add;
+		}
+		ret = otg_set_vbus(pdata->otg, 1);
+		if (ret) {
 			dev_err(dev, "unable to enable vbus on transceiver\n");
+			goto err_add;
+		}
 	}
 
 	priv->hcd = hcd;

Signature · 1 line

1.7.1

Wolfram Sang

author

2010-06-15 11:03 UTC #3 lore raw

PATCH 2/2 ehci-mxc: bail out on transceiver problems

Quoted 10 lines · Daniel Mack

Quoted 3 lines · Wolfram Sang
The old code registered the hcd even if there were no transceivers detected, leading to oopses like this if we try to probe a non-existant ULPI:
Hmm. I'm aware that there was a missing bail in this function, but actually, I had hardware which didn't properly detect the ULPI chip but still worked fine. There has been quite some discussion here about that, and eventually I decided to not make this a hard error as it didn't really harm.

My use case is that we have two very similar boards, one uses just the second host controller, the other one uses also OTG. Of course, we'd like to have just one kernel, so we wanted to check if probing for OTG fails gracefully enough, if the ULPI is not present. It did not, so this is what my patch intends to fix. I think, checking why your boards fail to do the init, is the cleaner way to go in the long run.

Regards,

Wolfram

Signature · 9 lines

Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100615/c48eff7c/attachment.sig>

Daniel Mack

2010-06-15 11:12 UTC #4 lore raw

PATCH 2/2 ehci-mxc: bail out on transceiver problems

Quoted 12 lines · Wolfram Sang

Quoted 10 lines · Daniel Mack
Quoted 3 lines · Wolfram Sang
The old code registered the hcd even if there were no transceivers detected, leading to oopses like this if we try to probe a non-existant ULPI:
Hmm. I'm aware that there was a missing bail in this function, but actually, I had hardware which didn't properly detect the ULPI chip but still worked fine. There has been quite some discussion here about that, and eventually I decided to not make this a hard error as it didn't really harm.

Quoted 6 lines

My use case is that we have two very similar boards, one uses just the second host controller, the other one uses also OTG. Of course, we'd like to have just one kernel, so we wanted to check if probing for OTG fails gracefully enough, if the ULPI is not present. It did not, so this is what my patch intends to fix. I think, checking why your boards fail to do the init, is the cleaner way to go in the long run.

There's a design flaw in Freescale's MX31 reference designs which lets the nCS pin float. And many board designers blindly copied that flaw over to their own designs.

See

http://marc.info/?l=linux-usb&m=125985151110906&w=4

So we might need quirks handling here.

Daniel

Philippe Rétornaz

2010-06-15 11:47 UTC #5 lore raw

PATCH 2/2 ehci-mxc: bail out on transceiver problems

In reply to Daniel Mack

Le mardi, 15 juin 2010 12.43:42, Daniel Mack a ?crit :

Quoted 35 lines · Wolfram Sang

Quoted 3 lines
The old code registered the hcd even if there were no transceivers detected, leading to oopses like this if we try to probe a non-existant ULPI:
Hmm. I'm aware that there was a missing bail in this function, but actually, I had hardware which didn't properly detect the ULPI chip but still worked fine. There has been quite some discussion here about that, and eventually I decided to not make this a hard error as it didn't really harm.
Quoted 20 lines
[    2.730000] mxc-ehci mxc-ehci.0: unable to init transceiver
[    2.740000] timeout polling for ULPI device
[    2.740000] timeout polling for ULPI device
[    2.750000] mxc-ehci mxc-ehci.0: unable to enable vbus on transceiver
[    2.750000] mxc-ehci mxc-ehci.0: Freescale On-Chip EHCI Host
Controller [ 2.760000] mxc-ehci mxc-ehci.0: new USB bus registered,
assigned bus number 2 [ 2.770000] Unhandled fault: external abort on
non-linefetch (0x808) at 0xc4876184 [ 2.770000] Internal error: : 808
[#1] PREEMPT
[    2.770000] last sysfs file:
[    2.770000] Modules linked in:
[    2.770000] CPU: 0    Not tainted  (2.6.33.5 #5)
[    2.770000] PC is at ehci_hub_control+0x4d4/0x8f8
[    2.770000] LR is at ehci_mxc_setup+0xbc/0xdc
[    2.770000] pc : [<c0196dfc>]    lr : [<c019bc8c>]    psr: 00000093
[    2.770000] sp : c3815e40  ip : 00000001  fp : 60000013
[    2.770000] r10: c4876184  r9 : 00000000  r8 : c3814000
[    2.770000] r7 : c391d2cc  r6 : 00000001  r5 : 00000001  r4 : 00000000
[    2.770000] r3 : 80000000  r2 : 00000007  r1 : 80000000  r0 : c4876184
[    2.770000] Flags: nzcv  IRQs off  FIQs on  Mode SVC_32  ISA ARM
Quoted 3 lines
Segment kernel [ 2.770000] Control: 0005317f Table: a0004000 DAC:
00000017 [ 2.770000] Process swapper (pid: 1, stack limit =
0xc3814270)

Quoted 5 lines

But that, of course, changes everthing. If you happen to see such Ooopses, your patch should go in, and I will check the boards again and see if they still fail to probe the ULPI. Eventually, we might need some kind of quirks field in the platform data :(

Have you tried my imx31 EHCI controller init patch applied ?
http://lists.infradead.org/pipermail/linux-arm-kernel/2010-May/014729.html

Btw, it has still not be applied, anybody knows why ?

Regards,

Philippe

Daniel Mack

2010-06-15 11:53 UTC #6 lore raw

PATCH 2/2 ehci-mxc: bail out on transceiver problems

Quoted 36 lines · Philippe R?tornaz

Le mardi, 15 juin 2010 12.43:42, Daniel Mack a ?crit :
Quoted 35 lines · Wolfram Sang
Quoted 3 lines
The old code registered the hcd even if there were no transceivers detected, leading to oopses like this if we try to probe a non-existant ULPI:
Hmm. I'm aware that there was a missing bail in this function, but actually, I had hardware which didn't properly detect the ULPI chip but still worked fine. There has been quite some discussion here about that, and eventually I decided to not make this a hard error as it didn't really harm.
Quoted 23 lines
[    2.730000] mxc-ehci mxc-ehci.0: unable to init transceiver
[    2.740000] timeout polling for ULPI device
[    2.740000] timeout polling for ULPI device
[    2.750000] mxc-ehci mxc-ehci.0: unable to enable vbus on transceiver
[    2.750000] mxc-ehci mxc-ehci.0: Freescale On-Chip EHCI Host
Controller [ 2.760000] mxc-ehci mxc-ehci.0: new USB bus registered,
assigned bus number 2 [ 2.770000] Unhandled fault: external abort on
non-linefetch (0x808) at 0xc4876184 [ 2.770000] Internal error: : 808
[#1] PREEMPT
[    2.770000] last sysfs file:
[    2.770000] Modules linked in:
[    2.770000] CPU: 0    Not tainted  (2.6.33.5 #5)
[    2.770000] PC is at ehci_hub_control+0x4d4/0x8f8
[    2.770000] LR is at ehci_mxc_setup+0xbc/0xdc
[    2.770000] pc : [<c0196dfc>]    lr : [<c019bc8c>]    psr: 00000093
[    2.770000] sp : c3815e40  ip : 00000001  fp : 60000013
[    2.770000] r10: c4876184  r9 : 00000000  r8 : c3814000
[    2.770000] r7 : c391d2cc  r6 : 00000001  r5 : 00000001  r4 : 00000000
[    2.770000] r3 : 80000000  r2 : 00000007  r1 : 80000000  r0 : c4876184
[    2.770000] Flags: nzcv  IRQs off  FIQs on  Mode SVC_32  ISA ARM
Segment kernel [ 2.770000] Control: 0005317f Table: a0004000 DAC:
00000017 [ 2.770000] Process swapper (pid: 1, stack limit =
0xc3814270)

Quoted 8 lines

Quoted 5 lines
But that, of course, changes everthing. If you happen to see such Ooopses, your patch should go in, and I will check the boards again and see if they still fail to probe the ULPI. Eventually, we might need some kind of quirks field in the platform data :(
Have you tried my imx31 EHCI controller init patch applied ?
http://lists.infradead.org/pipermail/linux-arm-kernel/2010-May/014729.html

No, I haven't yet, as I'm not currently working on such a board. But I will once I get back to that issue. Thanks for the reminder :)

Daniel

Wolfram Sang

author

2010-06-16 01:44 UTC #7 lore raw

PATCH 2/2 ehci-mxc: bail out on transceiver problems

In reply to Daniel Mack

Quoted 10 lines · Daniel Mack

Quoted 3 lines · Wolfram Sang
The old code registered the hcd even if there were no transceivers detected, leading to oopses like this if we try to probe a non-existant ULPI:
Hmm. I'm aware that there was a missing bail in this function, but actually, I had hardware which didn't properly detect the ULPI chip but still worked fine. There has been quite some discussion here about that, and eventually I decided to not make this a hard error as it didn't really harm.

Hmm, so, do you think this patch is stable-material after all?

Signature · 9 lines

Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100616/bd0e517d/attachment.sig>

Daniel Mack

2010-06-16 05:35 UTC #8 lore raw

PATCH 2/2 ehci-mxc: bail out on transceiver problems

Quoted 6 lines · Wolfram Sang

Quoted 5 lines · Daniel Mack
Quoted 3 lines · Wolfram Sang
The old code registered the hcd even if there were no transceivers detected, leading to oopses like this if we try to probe a non-existant ULPI:

Quoted 7 lines

Quoted 5 lines
Hmm. I'm aware that there was a missing bail in this function, but actually, I had hardware which didn't properly detect the ULPI chip but still worked fine. There has been quite some discussion here about that, and eventually I decided to not make this a hard error as it didn't really harm.
Hmm, so, do you think this patch is stable-material after all?

Hard to say. It might break existing board support, which would be a regression. OTOH, it fixes an Oops. Don't know really ...

Daniel

Daniel Mack

2010-06-15 10:39 UTC #9 lore raw

PATCH 1/2 otg/ulpi: bail out on read errors

In reply to Wolfram Sang

Quoted 5 lines · Wolfram Sang

otg_read may return errnos, so bail out correctly to prevent bogus ID-numbers.
Signed-off-by Wolfram Sang <[email protected]>
Cc Sascha Hauer <[email protected]>

Acked-by Daniel Mack <[email protected]>

Quoted 36 lines

Cc Greg KH <[email protected]>

---
 drivers/usb/otg/ulpi.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

drivers/usb/otg/ulpi.c +11 -6

index b1b3469..d331b22 100644
@@ -59,12 +59,17 @@ static int ulpi_set_flags(struct otg_transceiver *otg)
 
 static int ulpi_init(struct otg_transceiver *otg)
 {
-	int i, vid, pid;
-
-	vid = (otg_io_read(otg, ULPI_VENDOR_ID_HIGH) << 8) |
-	       otg_io_read(otg, ULPI_VENDOR_ID_LOW);
-	pid = (otg_io_read(otg, ULPI_PRODUCT_ID_HIGH) << 8) |
-	       otg_io_read(otg, ULPI_PRODUCT_ID_LOW);
+	int i, vid, pid, ret;
+	u32 ulpi_id = 0;
+
+	for (i = 0; i < 4; i++) {
+		ret = otg_io_read(otg, ULPI_PRODUCT_ID_HIGH - i);
+		if (ret < 0)
+			return ret;
+		ulpi_id = (ulpi_id << 8) | ret;
+	}
+	vid = ulpi_id & 0xffff;
+	pid = ulpi_id >> 16;
 
 	pr_info("ULPI transceiver vendor/product ID 0x%04x/0x%04x\n", vid, pid);

Signature · 1 line

1.7.1