[PATCH 1/1] usb: ulpi: ulpi_init should be used in subsys_initcall

[PATCH 1/1] usb: ulpi: ulpi_init should be used in subsys_initcall

[Lu Baolu]

The intention of this change is to fix below kernel panic when
USB_ULPI_BUS was configured as buildin.

[0.746856] kernel BUG at drivers/base/driver.c:153!
[0.752418] invalid opcode: 0000 [#1] PREEMPT SMP
[0.757804] Modules linked in:
[0.893985] Call Trace:
[0.896729]  [<ffffffff81870cb1>] ? ulpi_register_driver+0x21/0x30
[0.903654]  [<ffffffff823d5cac>] tusb1210_driver_init+0x10/0x12
[0.910386]  [<ffffffff81000318>] do_one_initcall+0xd8/0x200
[0.916729]  [<ffffffff8239b094>] kernel_init_freeable+0x196/0x21e
[0.923655]  [<ffffffff81affed0>] ? rest_init+0x90/0x90
[0.929509]  [<ffffffff81affede>] kernel_init+0xe/0xf0
[0.935266]  [<ffffffff81b0f442>] ret_from_fork+0x42/0x70
[0.941315]  [<ffffffff81affed0>] ? rest_init+0x90/0x90

Reported-by: Zhuo Qiuxu <qiuxu.zhuo@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/usb/common/ulpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
index 0e6f968..01c0c04 100644
--- a/drivers/usb/common/ulpi.c
+++ b/drivers/usb/common/ulpi.c
@@ -242,7 +242,7 @@ static int __init ulpi_init(void)
 {
 	return bus_register(&ulpi_bus);
 }
-module_init(ulpi_init);
+subsys_initcall(ulpi_init);
 
 static void __exit ulpi_exit(void)
 {
-- 
2.1.4

Re: [PATCH 1/1] usb: ulpi: ulpi_init should be used in subsys_initcall

[Heikki Krogerus]

On Thu, May 21, 2015 at 01:40:43PM +0800, Lu Baolu wrote:
> The intention of this change is to fix below kernel panic when
> USB_ULPI_BUS was configured as buildin.

That is actually incorrect. Having the bus build-in does not cause
this panic..

> [0.746856] kernel BUG at drivers/base/driver.c:153!
> [0.752418] invalid opcode: 0000 [#1] PREEMPT SMP
> [0.757804] Modules linked in:
> [0.893985] Call Trace:
> [0.896729]  [<ffffffff81870cb1>] ? ulpi_register_driver+0x21/0x30
> [0.903654]  [<ffffffff823d5cac>] tusb1210_driver_init+0x10/0x12
> [0.910386]  [<ffffffff81000318>] do_one_initcall+0xd8/0x200
> [0.916729]  [<ffffffff8239b094>] kernel_init_freeable+0x196/0x21e
> [0.923655]  [<ffffffff81affed0>] ? rest_init+0x90/0x90
> [0.929509]  [<ffffffff81affede>] kernel_init+0xe/0xf0
> [0.935266]  [<ffffffff81b0f442>] ret_from_fork+0x42/0x70
> [0.941315]  [<ffffffff81affed0>] ? rest_init+0x90/0x90

This happens if the PHY drivers are build-in. If the bus is build-in,
but the drivers are modules, you are still fine.

Maybe it would have been better to explain that this is addressing an
issue with the execution sequence, and consider Sasha's patch as the
actual fix for panic.


Thanks guys,

-- 
heikki

Re: [PATCH 1/1] usb: ulpi: ulpi_init should be used in subsys_initcall

[Lu, Baolu]

On 05/21/2015 03:33 PM, Heikki Krogerus wrote:
> On Thu, May 21, 2015 at 01:40:43PM +0800, Lu Baolu wrote:
>> The intention of this change is to fix below kernel panic when
>> USB_ULPI_BUS was configured as buildin.
> That is actually incorrect. Having the bus build-in does not cause
> this panic..
>
>> [0.746856] kernel BUG at drivers/base/driver.c:153!
>> [0.752418] invalid opcode: 0000 [#1] PREEMPT SMP
>> [0.757804] Modules linked in:
>> [0.893985] Call Trace:
>> [0.896729]  [<ffffffff81870cb1>] ? ulpi_register_driver+0x21/0x30
>> [0.903654]  [<ffffffff823d5cac>] tusb1210_driver_init+0x10/0x12
>> [0.910386]  [<ffffffff81000318>] do_one_initcall+0xd8/0x200
>> [0.916729]  [<ffffffff8239b094>] kernel_init_freeable+0x196/0x21e
>> [0.923655]  [<ffffffff81affed0>] ? rest_init+0x90/0x90
>> [0.929509]  [<ffffffff81affede>] kernel_init+0xe/0xf0
>> [0.935266]  [<ffffffff81b0f442>] ret_from_fork+0x42/0x70
>> [0.941315]  [<ffffffff81affed0>] ? rest_init+0x90/0x90
> This happens if the PHY drivers are build-in. If the bus is build-in,
> but the drivers are modules, you are still fine.

I ever tried the config of bus "=y" and driver "=n". It results in
panic as well. The call trace looks like below.

1.058876]  [<ffffffff8165a6d2>] device_add+0x402/0x640
[1.064829]  [<ffffffff8165a92e>] device_register+0x1e/0x30
[1.071072]  [<ffffffff81870bdc>] ulpi_register_interface+0x14c/0x1f0
[1.078291]  [<ffffffff817c68d4>] dwc3_ulpi_init+0x24/0x60
[1.084437]  [<ffffffff817be052>] dwc3_probe+0x802/0x1650
[1.090487]  [<ffffffff8165fb54>] platform_drv_probe+0x34/0xa0
[1.097022]  [<ffffffff8165d9e9>] driver_probe_device+0x209/0x4b0
[1.103850]  [<ffffffff8165dc90>] ? driver_probe_device+0x4b0/0x4b0
[1.110871]  [<ffffffff8165dccb>] __device_attach+0x3b/0x40
[1.117114]  [<ffffffff8165b6d3>] bus_for_each_drv+0x63/0xa0
[1.123454]  [<ffffffff8165d778>] device_attach+0x98/0xb0
[1.129503]  [<ffffffff8165cb60>] bus_probe_device+0xa0/0xc0
[1.135843]  [<ffffffff8165a73f>] device_add+0x46f/0x640
[1.141799]  [<ffffffff81090fe0>] ? __insert_resource+0x60/0x150
[1.148530]  [<ffffffff8165f780>] platform_device_add+0xd0/0x2b0
[1.155260]  [<ffffffff817c7386>] dwc3_pci_probe+0xf6/0x2c0
[1.161505]  [<ffffffff8149d86c>] pci_device_probe+0x8c/0xf0
[1.167846]  [<ffffffff8165d9e9>] driver_probe_device+0x209/0x4b0
[1.174673]  [<ffffffff8165dd6b>] __driver_attach+0x9b/0xa0
[1.180917]  [<ffffffff8165dcd0>] ? __device_attach+0x40/0x40
[1.187354]  [<ffffffff8165b60b>] bus_for_each_dev+0x6b/0xb0
[1.193694]  [<ffffffff8165d2be>] driver_attach+0x1e/0x20
[1.199742]  [<ffffffff8165ce80>] bus_add_driver+0x180/0x250
[1.206086]  [<ffffffff823e453c>] ? 
ftrace_define_fields_dwc3_log_trb+0x126/0x126
[1.214474]  [<ffffffff8165e5d4>] driver_register+0x64/0xf0
[1.220718]  [<ffffffff8149c51b>] __pci_register_driver+0x4b/0x50
[1.227546]  [<ffffffff823e4555>] dwc3_pci_driver_init+0x19/0x1b
[1.234277]  [<ffffffff81000318>] do_one_initcall+0xd8/0x200
[1.240619]  [<ffffffff8239b094>] kernel_init_freeable+0x196/0x21e
[1.247545]  [<ffffffff81affbd0>] ? rest_init+0x90/0x90
[1.253399]  [<ffffffff81affbde>] kernel_init+0xe/0xf0
[1.259156]  [<ffffffff81b0f142>] ret_from_fork+0x42/0x70
[1.265205]  [<ffffffff81affbd0>] ? rest_init+0x90/0x90

>
> Maybe it would have been better to explain that this is addressing an
> issue with the execution sequence, and consider Sasha's patch as the
> actual fix for panic.

Fair enough. I will resend the patch.

>
>
> Thanks guys,
>

Thanks,
Baolu