[PATCH 1/1] usb: chipidea: host: add own hc_driver for each hcd

[PATCH 1/1] usb: chipidea: host: add own hc_driver for each hcd

[Peter Chen]

There are several benefits for doing like this:

- hc_driver can be customized for each hcd
- Other hcd hc_driver's initialization will not affect current one.
We run out NULL pointer dereference problem when one hcd is started
by module_init, and the other is started by otg thread at SMP platform.
The reason for this problem is ehci_init_driver will do memory copy
for current uniform hc_driver, and this memory copy will do memset (as 0)
first, so when the first hcd is running usb_add_hcd, and the second
hcd may clear the uniform hc_driver's space (at ehci_init_driver),
then the first hcd will meet NULL pointer at the same time.

See below two logs:

LOG_1:
ci_hdrc ci_hdrc.0: EHCI Host Controller
ci_hdrc ci_hdrc.0: new USB bus registered, assigned bus number 1
ci_hdrc ci_hdrc.1: doesn't support gadget
Unable to handle kernel NULL pointer dereference at virtual address 00000014
pgd = 80004000
[00000014] *pgd=00000000
Internal error: Oops: 805 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 0 PID: 108 Comm: kworker/u8:2 Not tainted 3.14.38-222193-g24b2734-dirty #25
Workqueue: ci_otg ci_otg_work
task: d839ec00 ti: d8400000 task.ti: d8400000
PC is at ehci_run+0x4c/0x284
LR is at _raw_spin_unlock_irqrestore+0x28/0x54
pc : [<8041f9a0>]    lr : [<8070ea84>]    psr: 60000113
sp : d8401e30  ip : 00000000  fp : d8004400
r10: 00000001  r9 : 00000001  r8 : 00000000
r7 : 00000000  r6 : d8419940  r5 : 80dd24c0  r4 : d8419800
r3 : 8001d060  r2 : 00000000  r1 : 00000001  r0 : 00000000
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
Control: 10c53c7d  Table: 1000404a  DAC: 00000015
Process kworker/u8:2 (pid: 108, stack limit = 0xd8400238)
Stack: (0xd8401e30 to 0xd8402000)
1e20:                                     d87523c0 d8401e48 66667562 d8419800
1e40: 00000000 00000000 d8419800 00000000 00000000 00000000 d84198b0 8040fcdc
1e60: 00000000 80dd320c d8477610 d8419c00 d803d010 d8419800 00000000 00000000
1e80: d8004400 00000000 d8400008 80431494 80431374 d803d100 d803d010 d803d1ac
1ea0: 00000000 80432428 804323d4 d803d100 00000001 80435eb8 80e0d0bc d803d100
1ec0: 00000006 80436458 00000000 d803d100 80e92ec8 80436f44 d803d010 d803d100
1ee0: d83fde00 8043292c d8752710 d803d1f4 d803d010 8042ddfc 8042ddb8 d83f3b00
1f00: d803d1f4 80042b60 00000000 00000003 00000001 00000001 80054598 d83f3b00
1f20: d8004400 d83f3b18 d8004414 d8400000 80e3957b 00000089 d8004400 80043814
1f40: d839ec00 00000000 d83fcd80 d83f3b00 800436e4 00000000 00000000 00000000
1f60: 00000000 80048f34 00000000 00000000 00000000 d83f3b00 00000000 00000000
1f80: d8401f80 d8401f80 00000000 00000000 d8401f90 d8401f90 d8401fac d83fcd80
1fa0: 80048e68 00000000 00000000 8000e538 00000000 00000000 00000000 00000000
1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[<8041f9a0>] (ehci_run) from [<8040fcdc>] (usb_add_hcd+0x248/0x6e8)
[<8040fcdc>] (usb_add_hcd) from [<80431494>] (host_start+0x120/0x2e4)
[<80431494>] (host_start) from [<80432428>] (ci_otg_start_host+0x54/0xbc)
[<80432428>] (ci_otg_start_host) from [<80435eb8>] (otg_set_protocol+0xa4/0xd0)
[<80435eb8>] (otg_set_protocol) from [<80436458>] (otg_set_state+0x574/0xc58)
[<80436458>] (otg_set_state) from [<80436f44>] (otg_statemachine+0x408/0x46c)
[<80436f44>] (otg_statemachine) from [<8043292c>] (ci_otg_fsm_work+0x3c/0x190)
[<8043292c>] (ci_otg_fsm_work) from [<8042ddfc>] (ci_otg_work+0x44/0x1c4)
[<8042ddfc>] (ci_otg_work) from [<80042b60>] (process_one_work+0xf4/0x35c)
[<80042b60>] (process_one_work) from [<80043814>] (worker_thread+0x130/0x3bc)
[<80043814>] (worker_thread) from [<80048f34>] (kthread+0xcc/0xe4)
[<80048f34>] (kthread) from [<8000e538>] (ret_from_fork+0x14/0x3c)
Code: e5953018 e3530000 0a000000 e12fff33 (e5878014)

LOG_2:
ci_hdrc ci_hdrc.0: EHCI Host Controller
ci_hdrc ci_hdrc.0: new USB bus registered, assigned bus number 1
ci_hdrc ci_hdrc.1: doesn't support gadget
Unable to handle kernel NULL pointer dereference at virtual address 00000000
pgd = 80004000
[00000000] *pgd=00000000
In Online 00:00ternal e      Offline rror: Oops: 80000005 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 0 PID: 108 Comm: kworker/u8:2 Not tainted 3.14.38-02007-g24b2734-dirty #127
Workque Online 00:00ue: ci_o      Offline tg ci_otg_work
Online 00:00task: d8      Offline 39ec00 ti: d83ea000 task.ti: d83ea000
PC is at 0x0
LR is at usb_add_hcd+0x248/0x6e8
pc : [<00000000>]    lr : [<8040f644>]    psr: 60000113
sp : d83ebe60  ip : 00000000  fp : d8004400
r10: 00000001  r9 : 00000001  r8 : d85fd4b0
r7 : 00000000  r6 : 00000000  r5 : 00000000  r4 : d85fd400
r3 : 00000000  r2 : d85fd4f4  r1 : 80410178  r0 : d85fd400
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
Control: 10c53c7d  Table: 1000404a  DAC: 00000015
Process kworker/u8:2 (pid: 108, stack limit = 0xd83ea238)
Stack: (0xd83ebe60 to 0xd83ec000)
be60: 00000000 80dd920c d8654e10 d85fd800 d803e010 d85fd400 00000000 00000000
be80: d8004400 00000000 d83ea008 80430e34 80430d14 d803e100 d803e010 d803e1ac
bea0: 00000000 80431dc8 80431d74 d803e100 00000001 80435858 80e130bc d803e100
bec0: 00000006 80435df8 00000000 d803e100 80e98ec8 804368e4 d803e010 d803e100
bee0: d86e8100 804322cc d86cf050 d803e1f4 d803e010 8042d79c 8042d758 d83cf900
bf00: d803e1f4 80042b78 00000000 00000003 00000001 00000001 800545e8 d83cf900
bf20: d8004400 d83cf918 d8004414 d83ea000 80e3f57b 00000089 d8004400 8004382c
bf40: d839ec00 00000000 d8393780 d83cf900 800436fc 00000000 00000000 00000000
bf60: 00000000 80048f50 80e019f4 00000000 0000264c d83cf900 00000000 00000000
bf80: d83ebf80 d83ebf80 00000000 00000000 d83ebf90 d83ebf90 d83ebfac d8393780
bfa0: 80048e84 00000000 00000000 8000e538 00000000 00000000 00000000 00000000
bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 ee66e85d 133ebd03
[<804 Online 00:000f644>]       Offline (usb_add_hcd) from [<80430e34>] (host_start+0x120/0x2e4)
[<80430e34>] (host_start) from [<80431dc8>] (ci_otg_start_host+0x54/0xbc)
[<80431dc8>] (ci_otg_start_host) from [<80435858>] (otg_set_protocol+0xa4/0xd0)
[<80435858>] (otg_set_protocol) from [<80435df8>] (otg_set_state+0x574/0xc58)
[<80435df8>] (otg_set_state) from [<804368e4>] (otg_statemachine+0x408/0x46c)
[<804368e4>] (otg_statemachine) from [<804322cc>] (ci_otg_fsm_work+0x3c/0x190)
[<804322cc>] (ci_otg_fsm_work) from [<8042d79c>] (ci_otg_work+0x44/0x1c4)
[<8042d79c>] (ci_otg_work) from [<80042b78>] (process_one_work+0xf4/0x35c)
[<80042b78>] (process_one_work) from [<8004382c>] (worker_thread+0x130/0x3bc)
[<8004382c>] (worker_thread) from [<80048f50>] (kthread+0xcc/0xe4)
[<80048f50>] (kthread) from [<8000e538>] (ret_from_fork+0x14/0x3c)
Code: bad PC value

Cc: Jun Li <jun.li@freescale.com>
Cc: <stable@vger.kernel.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/host.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index 30c09a8..32f0bc9 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -25,6 +25,7 @@
 #include <linux/usb/hcd.h>
 #include <linux/usb/chipidea.h>
 #include <linux/regulator/consumer.h>
+#include <linux/platform_device.h>
 
 #include "../host/ehci.h"
 
@@ -32,7 +33,8 @@
 #include "bits.h"
 #include "host.h"
 
-static struct hc_driver __read_mostly ci_ehci_hc_driver;
+#define MAX_CI_NUM	8
+static struct hc_driver __read_mostly ci_ehci_hc_driver[MAX_CI_NUM];
 static int (*orig_bus_suspend)(struct usb_hcd *hcd);
 
 struct ehci_ci_priv {
@@ -93,11 +95,13 @@ static int host_start(struct ci_hdrc *ci)
 	struct ehci_hcd *ehci;
 	struct ehci_ci_priv *priv;
 	int ret;
+	struct platform_device *pdev = to_platform_device(ci->dev);
 
 	if (usb_disabled())
 		return -ENODEV;
 
-	hcd = usb_create_hcd(&ci_ehci_hc_driver, ci->dev, dev_name(ci->dev));
+	hcd = usb_create_hcd(&ci_ehci_hc_driver[pdev->id],
+			ci->dev, dev_name(ci->dev));
 	if (!hcd)
 		return -ENOMEM;
 
@@ -236,6 +240,8 @@ static int ci_ehci_bus_suspend(struct usb_hcd *hcd)
 int ci_hdrc_host_init(struct ci_hdrc *ci)
 {
 	struct ci_role_driver *rdrv;
+	struct platform_device *pdev = to_platform_device(ci->dev);
+	struct hc_driver *ci_ehci_driver = &ci_ehci_hc_driver[pdev->id];
 
 	if (!hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_HC))
 		return -ENXIO;
@@ -250,9 +256,9 @@ int ci_hdrc_host_init(struct ci_hdrc *ci)
 	rdrv->name	= "host";
 	ci->roles[CI_ROLE_HOST] = rdrv;
 
-	ehci_init_driver(&ci_ehci_hc_driver, &ehci_ci_overrides);
-	orig_bus_suspend = ci_ehci_hc_driver.bus_suspend;
-	ci_ehci_hc_driver.bus_suspend = ci_ehci_bus_suspend;
+	ehci_init_driver(ci_ehci_driver, &ehci_ci_overrides);
+	orig_bus_suspend = ci_ehci_driver->bus_suspend;
+	ci_ehci_driver->bus_suspend = ci_ehci_bus_suspend;
 
 	return 0;
 }
-- 
1.9.1

Re: [PATCH 1/1] usb: chipidea: host: add own hc_driver for each hcd

[Alan Stern]

On Fri, 17 Jul 2015, Peter Chen wrote:

> There are several benefits for doing like this:
> 
> - hc_driver can be customized for each hcd
> - Other hcd hc_driver's initialization will not affect current one.
> We run out NULL pointer dereference problem when one hcd is started
> by module_init, and the other is started by otg thread at SMP platform.
> The reason for this problem is ehci_init_driver will do memory copy
> for current uniform hc_driver, and this memory copy will do memset (as 0)
> first, so when the first hcd is running usb_add_hcd, and the second
> hcd may clear the uniform hc_driver's space (at ehci_init_driver),
> then the first hcd will meet NULL pointer at the same time.

It seems to me the real problem is that ehci_init_driver gets called at
the wrong time.  It's not supposed to be called whenever a new host
controller is initialized; rather, it should be called just once when
the driver is initialized.  This means the call should be in a
module_init routine in host.c, not in ci_hdrc_host_init.

If you need to customize the hc_driver structure for each host 
controller then yes, a separate copy is needed.  But otherwise there's 
no need to make a copy.

Alan Stern

Re: [PATCH 1/1] usb: chipidea: host: add own hc_driver for each hcd

[Peter Chen]

On Fri, Jul 17, 2015 at 09:52:26AM -0400, Alan Stern wrote:
> On Fri, 17 Jul 2015, Peter Chen wrote:
> 
> > There are several benefits for doing like this:
> > 
> > - hc_driver can be customized for each hcd
> > - Other hcd hc_driver's initialization will not affect current one.
> > We run out NULL pointer dereference problem when one hcd is started
> > by module_init, and the other is started by otg thread at SMP platform.
> > The reason for this problem is ehci_init_driver will do memory copy
> > for current uniform hc_driver, and this memory copy will do memset (as 0)
> > first, so when the first hcd is running usb_add_hcd, and the second
> > hcd may clear the uniform hc_driver's space (at ehci_init_driver),
> > then the first hcd will meet NULL pointer at the same time.
> 
> It seems to me the real problem is that ehci_init_driver gets called at
> the wrong time.  It's not supposed to be called whenever a new host
> controller is initialized; rather, it should be called just once when
> the driver is initialized.  This means the call should be in a
> module_init routine in host.c, not in ci_hdrc_host_init.
> 

But we have no platform driver dedicated for host controller, is it a
must? The core driver requests io_address, interrupt, dma_mask, etc for
both device and host driver, and device and host part (we does not take
it as a driver) should not request resources again.

Would you accept it as another use case for ehci driver?

> If you need to customize the hc_driver structure for each host 
> controller then yes, a separate copy is needed.  But otherwise there's 
> no need to make a copy.
> 
> Alan Stern
> 

-- 

Best Regards,
Peter Chen

Re: [PATCH 1/1] usb: chipidea: host: add own hc_driver for each hcd

[Peter Chen]

On Sun, Jul 19, 2015 at 10:22:47PM -0400, Alan Stern wrote:
> On Mon, 20 Jul 2015, Peter Chen wrote:
> 
> > On Fri, Jul 17, 2015 at 09:52:26AM -0400, Alan Stern wrote:
> > > On Fri, 17 Jul 2015, Peter Chen wrote:
> > > 
> > > > There are several benefits for doing like this:
> > > > 
> > > > - hc_driver can be customized for each hcd
> > > > - Other hcd hc_driver's initialization will not affect current one.
> > > > We run out NULL pointer dereference problem when one hcd is started
> > > > by module_init, and the other is started by otg thread at SMP platform.
> > > > The reason for this problem is ehci_init_driver will do memory copy
> > > > for current uniform hc_driver, and this memory copy will do memset (as 0)
> > > > first, so when the first hcd is running usb_add_hcd, and the second
> > > > hcd may clear the uniform hc_driver's space (at ehci_init_driver),
> > > > then the first hcd will meet NULL pointer at the same time.
> > > 
> > > It seems to me the real problem is that ehci_init_driver gets called at
> > > the wrong time.  It's not supposed to be called whenever a new host
> > > controller is initialized; rather, it should be called just once when
> > > the driver is initialized.  This means the call should be in a
> > > module_init routine in host.c, not in ci_hdrc_host_init.
> > > 
> > 
> > But we have no platform driver dedicated for host controller, is it a
> > must? The core driver requests io_address, interrupt, dma_mask, etc for
> > both device and host driver, and device and host part (we does not take
> > it as a driver) should not request resources again.
> 
> It doesn't matter.  There's no harm in calling ehci_init_driver, even
> if the hardware doesn't contain a host controller.  The point is that
> this call should be made only once, when the main ci_hdrc driver starts
> up.  It should not be made every time a platform driver registers a
> host controller.
> 
> > Would you accept it as another use case for ehci driver?
> 
> I don't think a new use case is needed.  All you have to do is add
> something like this to host.c:
> 
> static void ci_hdrc_host_driver_init(void)
> {
> 	ehci_init_driver(&ci_ehci_hc_driver, &ehci_ci_overrides);
> }
> module_init(ci_hdrc_host_driver_init);
> 
> Can you think of any reason this won't work?
> 

No, it will not work. The core driver's probe will run which will
call host_start to call ehci APIs before ci_hdrc_host_driver_init
is called.

-- 

Best Regards,
Peter Chen

Re: [PATCH 1/1] usb: chipidea: host: add own hc_driver for each hcd

[Alan Stern]

On Mon, 20 Jul 2015, Peter Chen wrote:

> On Fri, Jul 17, 2015 at 09:52:26AM -0400, Alan Stern wrote:
> > On Fri, 17 Jul 2015, Peter Chen wrote:
> > 
> > > There are several benefits for doing like this:
> > > 
> > > - hc_driver can be customized for each hcd
> > > - Other hcd hc_driver's initialization will not affect current one.
> > > We run out NULL pointer dereference problem when one hcd is started
> > > by module_init, and the other is started by otg thread at SMP platform.
> > > The reason for this problem is ehci_init_driver will do memory copy
> > > for current uniform hc_driver, and this memory copy will do memset (as 0)
> > > first, so when the first hcd is running usb_add_hcd, and the second
> > > hcd may clear the uniform hc_driver's space (at ehci_init_driver),
> > > then the first hcd will meet NULL pointer at the same time.
> > 
> > It seems to me the real problem is that ehci_init_driver gets called at
> > the wrong time.  It's not supposed to be called whenever a new host
> > controller is initialized; rather, it should be called just once when
> > the driver is initialized.  This means the call should be in a
> > module_init routine in host.c, not in ci_hdrc_host_init.
> > 
> 
> But we have no platform driver dedicated for host controller, is it a
> must? The core driver requests io_address, interrupt, dma_mask, etc for
> both device and host driver, and device and host part (we does not take
> it as a driver) should not request resources again.

It doesn't matter.  There's no harm in calling ehci_init_driver, even
if the hardware doesn't contain a host controller.  The point is that
this call should be made only once, when the main ci_hdrc driver starts
up.  It should not be made every time a platform driver registers a
host controller.

> Would you accept it as another use case for ehci driver?

I don't think a new use case is needed.  All you have to do is add
something like this to host.c:

static void ci_hdrc_host_driver_init(void)
{
	ehci_init_driver(&ci_ehci_hc_driver, &ehci_ci_overrides);
}
module_init(ci_hdrc_host_driver_init);

Can you think of any reason this won't work?

Alan Stern

Re: [PATCH 1/1] usb: chipidea: host: add own hc_driver for each hcd

[Alan Stern]

On Mon, 20 Jul 2015, Peter Chen wrote:

> > I don't think a new use case is needed.  All you have to do is add
> > something like this to host.c:
> > 
> > static void ci_hdrc_host_driver_init(void)
> > {
> > 	ehci_init_driver(&ci_ehci_hc_driver, &ehci_ci_overrides);

By the way, right here I forgot to include:

	orig_bus_suspend = ci_ehci_hc_driver.bus_suspend;
	ci_ehci_hc_driver.bus_suspend = ci_ehci_bus_suspend;

> > }
> > module_init(ci_hdrc_host_driver_init);
> > 
> > Can you think of any reason this won't work?
> > 
> 
> No, it will not work. The core driver's probe will run which will
> call host_start to call ehci APIs before ci_hdrc_host_driver_init
> is called.

Ah, I see the difficulty.  core.c does 
pci_register_driver(&ehci_pci_driver).  That will have to be different.
Let's change that line to this:

static int __init ci_hdrc_driver_init(void)
{
	ci_hdrc_host_driver_init();
	return platform_register_driver(&ci_hdrc_driver);
}
module_init(ci_hdrc_driver_init);

static void __exit ci_hdrc_driver_cleanup(void)
{
	platform_unregister_driver(&ci_hdrc_driver);
}
module_exit(ci_hdrc_driver_cleanup);

Then in host.c, make ci_hdrc_host_driver_init non-static.

This should work, because the core's probe routine can't be called 
before it is registered as a platform driver, and that doesn't happen 
until after ehci_init_driver is called.

Alan Stern

Re: [PATCH 1/1] usb: chipidea: host: add own hc_driver for each hcd

[Peter Chen]

On Mon, Jul 20, 2015 at 11:37:11AM -0400, Alan Stern wrote:
> On Mon, 20 Jul 2015, Peter Chen wrote:
> 
> > > I don't think a new use case is needed.  All you have to do is add
> > > something like this to host.c:
> > > 
> > > static void ci_hdrc_host_driver_init(void)
> > > {
> > > 	ehci_init_driver(&ci_ehci_hc_driver, &ehci_ci_overrides);
> 
> By the way, right here I forgot to include:
> 
> 	orig_bus_suspend = ci_ehci_hc_driver.bus_suspend;
> 	ci_ehci_hc_driver.bus_suspend = ci_ehci_bus_suspend;
> 
> > > }
> > > module_init(ci_hdrc_host_driver_init);
> > > 
> > > Can you think of any reason this won't work?
> > > 
> > 
> > No, it will not work. The core driver's probe will run which will
> > call host_start to call ehci APIs before ci_hdrc_host_driver_init
> > is called.
> 
> Ah, I see the difficulty.  core.c does 
> pci_register_driver(&ehci_pci_driver).  That will have to be different.
> Let's change that line to this:
> 
> static int __init ci_hdrc_driver_init(void)
> {
> 	ci_hdrc_host_driver_init();
> 	return platform_register_driver(&ci_hdrc_driver);
> }
> module_init(ci_hdrc_driver_init);
> 
> static void __exit ci_hdrc_driver_cleanup(void)
> {
> 	platform_unregister_driver(&ci_hdrc_driver);
> }
> module_exit(ci_hdrc_driver_cleanup);
> 
> Then in host.c, make ci_hdrc_host_driver_init non-static.
> 
> This should work, because the core's probe routine can't be called 
> before it is registered as a platform driver, and that doesn't happen 
> until after ehci_init_driver is called.
> 

Yes, this should work, thanks.

-- 

Best Regards,
Peter Chen