diff mbox

[v6,01/12] usb: hcd: Initialize hcd->flags to 0

Message ID 1459865117-7032-2-git-send-email-rogerq@ti.com
State New
Headers show

Commit Message

Roger Quadros April 5, 2016, 2:05 p.m. UTC 
When using the OTG/drd library we can call hcd_add/remove
consecutively without calling hcd_alloc in between so flags can be stale.

If the HC dies due to whatever reason then without this
patch we get the below error on next hcd_add.

[   91.494257] xhci-hcd xhci-hcd.0.auto: HC died; cleaning up
[   91.502068] hub 3-0:1.0: state 0 ports 1 chg 0000 evt 0000
[   91.510240] xhci-hcd xhci-hcd.0.auto: xHCI Host Controller
[   91.516940] xhci-hcd xhci-hcd.0.auto: new USB bus registered, assigned bus number 4
[   91.529745] usb usb4: We don't know the algorithms for LPM for this host, disabling LPM.
[   91.540637] usb usb4: New USB device found, idVendor=1d6b, idProduct=0003
[   91.757865] irq 254: nobody cared (try booting with the "irqpoll" option)
[   91.757880] CPU: 0 PID: 68 Comm: kworker/u2:2 Not tainted 4.1.4-00828-g1f0ed8c-dirty #44
[   91.757885] Hardware name: Generic AM43 (Flattened Device Tree)
[   91.757914] Workqueue: usb_otg usb_otg_work
[   91.757921] Backtrace:
[   91.757954] [<c0012af0>] (dump_backtrace) from [<c0012c8c>] (show_stack+0x18/0x1c)
[   91.757972]  r6:c089d4a4 r5:ffffffff r4:00000000 r3:ee440000
[   91.757991] [<c0012c74>] (show_stack) from [<c05f7c14>] (dump_stack+0x84/0xd0)
[   91.758008] [<c05f7b90>] (dump_stack) from [<c0084b30>] (__report_bad_irq+0x28/0xc8)
[   91.758024]  r7:00000000 r6:000000fe r5:00000000 r4:ee514c40
[   91.758037] [<c0084b08>] (__report_bad_irq) from [<c00850b0>] (note_interrupt+0x24c/0x2ac)
[   91.758052]  r6:000000fe r5:00000000 r4:ee514c40 r3:00000000
[   91.758065] [<c0084e64>] (note_interrupt) from [<c00828fc>] (handle_irq_event_percpu+0xb0/0x158)
[   91.758085]  r10:ee514c40 r9:c08ce49a r8:000000fe r7:00000000 r6:00000000 r5:00000000
[   91.758094]  r4:00000000 r3:00000000
[   91.758105] [<c008284c>] (handle_irq_event_percpu) from [<c00829e8>] (handle_irq_event+0x44/0x64)
[   91.758126]  r10:00000001 r9:ee441ab0 r8:ee441bb8 r7:c0858b4c r6:ed174280 r5:ee514ca0
[   91.758132]  r4:ee514c40
[   91.758144] [<c00829a4>] (handle_irq_event) from [<c0085970>] (handle_fasteoi_irq+0x100/0x1bc)
[   91.758159]  r6:c085dba0 r5:ee514ca0 r4:ee514c40 r3:00000000
[   91.758171] [<c0085870>] (handle_fasteoi_irq) from [<c0082058>] (generic_handle_irq+0x28/0x38)
[   91.758186]  r7:c0853d40 r6:c0858b4c r5:000000fe r4:000000fe
[   91.758197] [<c0082030>] (generic_handle_irq) from [<c00821c0>] (__handle_domain_irq+0x98/0x12c)
[   91.758207]  r4:c0853d40 r3:00000100
[   91.758219] [<c0082128>] (__handle_domain_irq) from [<c00094e0>] (gic_handle_irq+0x28/0x68)
[   91.758239]  r10:00000001 r9:ee441bb8 r8:fa240100 r7:c0858d70 r6:ee441ab0 r5:000000b8
[   91.758245]  r4:fa24010c
[   91.758264] [<c00094b8>] (gic_handle_irq) from [<c05fd540>] (__irq_svc+0x40/0x74)
[   91.758271] Exception stack(0xee441ab0 to 0xee441af8)
[   91.758280] 1aa0:                                     00000000 c08d2980 ee441ac0 00000000
[   91.758292] 1ac0: 00000008 00000089 c0858b4c c0858080 00000000 ee441bb8 00000001 ee441b3c
[   91.758301] 1ae0: 00000101 ee441af8 c02fc418 c0046a1c 20000113 ffffffff
[   91.758321]  r8:00000000 r7:ee441ae4 r6:ffffffff r5:20000113 r4:c0046a1c r3:c02fc418
[   91.758347] [<c00469a0>] (__do_softirq) from [<c0046eac>] (irq_exit+0xb8/0x104)
[   91.758367]  r10:00000001 r9:ee441bb8 r8:00000000 r7:c0853d40 r6:c0858b4c r5:00000089
[   91.758373]  r4:00000000
[   91.758386] [<c0046df4>] (irq_exit) from [<c00821c8>] (__handle_domain_irq+0xa0/0x12c)
[   91.758395]  r4:00000000 r3:00000100
[   91.758406] [<c0082128>] (__handle_domain_irq) from [<c00094e0>] (gic_handle_irq+0x28/0x68)
[   91.758426]  r10:c08e3510 r9:20000013 r8:fa240100 r7:c0858d70 r6:ee441bb8 r5:00000039
[   91.758433]  r4:fa24010c
[   91.758445] [<c00094b8>] (gic_handle_irq) from [<c05fd540>] (__irq_svc+0x40/0x74)
[   91.758450] Exception stack(0xee441bb8 to 0xee441c00)
[   91.758457] 1ba0:                                                       00000000 00000001
[   91.758468] 1bc0: 00000000 ee440000 c08e2524 0000004d 00000274 00000000 00000000 20000013
[   91.758479] 1be0: c08e3510 ee441c4c ee441b60 ee441c00 c03acfec c0080d4c 60000013 ffffffff
[   91.758499]  r8:00000000 r7:ee441bec r6:ffffffff r5:60000013 r4:c0080d4c r3:c03acfec
[   91.758524] [<c0080950>] (console_unlock) from [<c0081670>] (vprintk_emit+0x20c/0x500)
[   91.758544]  r10:ee441cc0 r9:c08d3550 r8:c08e3ea0 r7:00000000 r6:00000001 r5:0000003d
[   91.758551]  r4:c08d3550
[   91.758573] [<c0081464>] (vprintk_emit) from [<c03f6f70>] (dev_vprintk_emit+0x104/0x1ac)
[   91.758593]  r10:ee441d8c r9:0000000e r8:c07951e0 r7:00000006 r6:ee441cc0 r5:0000000d
[   91.758599]  r4:ee731068
[   91.758612] [<c03f6e6c>] (dev_vprintk_emit) from [<c03f7040>] (dev_printk_emit+0x28/0x30)
[   91.758632]  r10:00000001 r9:ee5f8410 r8:ee731000 r7:ed429000 r6:00000006 r5:ee441dc0
[   91.758638]  r4:ee731068
[   91.758651] [<c03f701c>] (dev_printk_emit) from [<c03f7098>] (__dev_printk+0x50/0x70)
[   91.758660]  r3:bf2268cc r2:c07951e0
[   91.758673] [<c03f7048>] (__dev_printk) from [<c03f70f4>] (_dev_info+0x3c/0x48)
[   91.758686]  r6:00000000 r5:ee731068 r4:ee731000
[   91.758790] [<c03f70bc>] (_dev_info) from [<bf20ec3c>] (usb_new_device+0x11c/0x518 [usbcore])
[   91.758804]  r3:00000003 r2:00001d6b r1:bf225bc4
[   91.758881] [<bf20eb20>] (usb_new_device [usbcore]) from [<bf213560>] (usb_otg_add_hcd+0x514/0x7f8 [usbcore])
[   91.758903]  r10:00000001 r9:ee5f8410 r8:ee731000 r7:000000fe r6:ed4290c8 r5:00000000
[   91.758909]  r4:ed429000
[   91.758957] [<bf21304c>] (usb_otg_add_hcd [usbcore]) from [<c047a238>] (usb_otg_start_host+0xb8/0xf8)
[   91.758978]  r10:00000000 r9:00000002 r8:00000000 r7:ee02b000 r6:ee452808 r5:ee452808
[   91.758985]  r4:ee452808
[   91.758997] [<c047a180>] (usb_otg_start_host) from [<c047a020>] (drd_set_protocol+0xac/0xd8)
[   91.759007]  r4:00000001 r3:c047a180
[   91.759018] [<c0479f74>] (drd_set_protocol) from [<c047a2ec>] (drd_set_state+0x74/0x98)
[   91.759027]  r5:ee452808 r4:00000009
[   91.759039] [<c047a278>] (drd_set_state) from [<c047a3dc>] (usb_otg_work+0xcc/0x154)
[   91.759054]  r6:ee452808 r5:ee4528b8 r4:ee452968 r3:00000000
[   91.759072] [<c047a310>] (usb_otg_work) from [<c005754c>] (process_one_work+0x128/0x340)
[   91.759087]  r6:ee02ac00 r5:ee452968 r4:ee42b900 r3:c047a310
[   91.759100] [<c0057424>] (process_one_work) from [<c00578f8>] (worker_thread+0x158/0x49c)
[   91.759120]  r10:ee42b900 r9:00000002 r8:ee02ac00 r7:00000088 r6:ee42b918 r5:ee02ac00
[   91.759127]  r4:ee02ac14
[   91.759145] [<c00577a0>] (worker_thread) from [<c005cc40>] (kthread+0xdc/0xf8)
[   91.759165]  r10:00000000 r9:00000000 r8:00000000 r7:c00577a0 r6:ee42b900 r5:ee429940
[   91.759174]  r4:00000000 r3:00000000
[   91.759190] [<c005cb64>] (kthread) from [<c000fc08>] (ret_from_fork+0x14/0x2c)
[   91.759206]  r7:00000000 r6:00000000 r5:c005cb64 r4:ee429940
[   91.759209] handlers:
[   91.759255] [<bf211b5c>] usb_hcd_irq [usbcore]
[   91.759260] Disabling IRQ #254

Signed-off-by: Roger Quadros <rogerq@ti.com>

---
 drivers/usb/core/hcd.c | 1 +
 1 file changed, 1 insertion(+)

-- 
2.5.0

Comments

Roger Quadros April 6, 2016, 6:32 a.m. UTC | #1 
On 06/04/16 09:09, Felipe Balbi wrote:
> 

> Hi,

> 

> Roger Quadros <rogerq@ti.com> writes:

>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c

>> index 2ca2cef..6b1930d 100644

>> --- a/drivers/usb/core/hcd.c

>> +++ b/drivers/usb/core/hcd.c

>> @@ -2706,6 +2706,7 @@ int usb_add_hcd(struct usb_hcd *hcd,

>>  	int retval;

>>  	struct usb_device *rhdev;

>>  

>> +	hcd->flags = 0;

> 

> seems like this would make more sense in usb_del_hcd() instead.

> 


OK, I'll move it there.

cheers,
-roger
Roger Quadros April 7, 2016, 10:40 a.m. UTC | #2 
On 07/04/16 12:42, Peter Chen wrote:
> On Wed, Apr 06, 2016 at 09:32:22AM +0300, Roger Quadros wrote:

>> On 06/04/16 09:09, Felipe Balbi wrote:

>>>

>>> Hi,

>>>

>>> Roger Quadros <rogerq@ti.com> writes:

>>>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c

>>>> index 2ca2cef..6b1930d 100644

>>>> --- a/drivers/usb/core/hcd.c

>>>> +++ b/drivers/usb/core/hcd.c

>>>> @@ -2706,6 +2706,7 @@ int usb_add_hcd(struct usb_hcd *hcd,

>>>>  	int retval;

>>>>  	struct usb_device *rhdev;

>>>>  

>>>> +	hcd->flags = 0;

>>>

> 

> I am not sure if this usb_add(remove)_hcd pair is safe and clean enough

> for start/stop host role. From my point, we may need to do like

> .probe/.remove host platform driver interface. In that case, we can make


probe and remove are meant to be called from bus layer.
I do not see a way how OTG framework can call probe/remove of HCD driver.
Some HCDs may be platform devices, some PCI, so different entities are calling
the HCD .probe hook.

> sure the clocks and regulators are off, and hcd will be zero-initialized


why can't we make that sure that is taken care of within the hcd_ops?
Why should some driver keep its regulators and clocks enabled when hcd is stopped?
It doesn't need to. If it is doing so now, it needs to be fixed.

> next time. Assume we are at gadget mode, we may not hope the vbus regulator

> is still on which is for host only. So, this part may need to implement

> by each user.

> 


Yes, correctness of this has to be taken care by each driver.

cheers,
-roger
Roger Quadros April 8, 2016, 7:16 a.m. UTC | #3 
On 08/04/16 04:01, Peter Chen wrote:
> On Thu, Apr 07, 2016 at 01:40:21PM +0300, Roger Quadros wrote:

>> On 07/04/16 12:42, Peter Chen wrote:

>>> On Wed, Apr 06, 2016 at 09:32:22AM +0300, Roger Quadros wrote:

>>>> On 06/04/16 09:09, Felipe Balbi wrote:

>>>>>

>>>>> Hi,

>>>>>

>>>>> Roger Quadros <rogerq@ti.com> writes:

>>>>>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c

>>>>>> index 2ca2cef..6b1930d 100644

>>>>>> --- a/drivers/usb/core/hcd.c

>>>>>> +++ b/drivers/usb/core/hcd.c

>>>>>> @@ -2706,6 +2706,7 @@ int usb_add_hcd(struct usb_hcd *hcd,

>>>>>>  	int retval;

>>>>>>  	struct usb_device *rhdev;

>>>>>>  

>>>>>> +	hcd->flags = 0;

>>>>>

>>>

>>> I am not sure if this usb_add(remove)_hcd pair is safe and clean enough

>>> for start/stop host role. From my point, we may need to do like

>>> .probe/.remove host platform driver interface. In that case, we can make

>>

>> probe and remove are meant to be called from bus layer.

>> I do not see a way how OTG framework can call probe/remove of HCD driver.

>> Some HCDs may be platform devices, some PCI, so different entities are calling

>> the HCD .probe hook.

>>

>>> sure the clocks and regulators are off, and hcd will be zero-initialized

>>

>> why can't we make that sure that is taken care of within the hcd_ops?

>> Why should some driver keep its regulators and clocks enabled when hcd is stopped?

>> It doesn't need to. If it is doing so now, it needs to be fixed.

>>

> 

> Well, you may misunderstand me. I mean your hcd_ops->start or ->stop

> is hard to be a general one which only calls usb_hcd_add or

> usb_hcd_remove. It needs to implement like .probe or .remove at platform

> driver, some example code like host_start and host_stop at

> drivers/usb/chipidea/host.c.

> 

The only extra thing the host_start/stop() of that driver is doing is
enabling/disabling the VBUS regulator.
In the OTG/dual role scope VBUS regulator handling has to be done via the
OTG driver using the otg ops otg_drv_vbus() and not at HCD level. Do you agree?

I don't want to complicate the OTG to HCD interface by adding new hooks there.
If HCD driver wants to do something special for OTG case it can always do that
within the struct hc_driver interface. But ideally OTG specific handling must
be done in the OTG driver. All that the HCD driver should care about is making sure
all used resources are disabled once usb_remove_hcd() is called.

cheers,
-roger
diff mbox

Patch

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 2ca2cef..6b1930d 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2706,6 +2706,7 @@  int usb_add_hcd(struct usb_hcd *hcd,
 	int retval;
 	struct usb_device *rhdev;
 
+	hcd->flags = 0;
 	if (IS_ENABLED(CONFIG_USB_PHY) && !hcd->usb_phy) {
 		struct usb_phy *phy = usb_get_phy_dev(hcd->self.controller, 0);