Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

From: Roger Quadros
Date: Fri Mar 31 2017 - 08:21:46 EST




On 31/03/17 15:00, Felipe Balbi wrote:
>
> Hi,
>
> Roger Quadros <rogerq@xxxxxx> writes:
>>>>> Your first implementation could be just that. Refactoring what needs to
>>>>> be refactored, then patching "mode" debugfs to work properly in that
>>>>> case. Only add otg.c/drd.c after "mode" debugfs file is stable, because
>>>>> then you know what needs to be taken into consideration.
>>>>>
>>>>> Just to be clear, I'm not saying we should *ONLY* get the debugfs
>>>>> interface for v4.12, I'm saying you should start with that and get that
>>>>> stable and working properly (make an infinite loop constantly changing
>>>>> modes and keep it running over the weekend) before you add support for
>>>>> OTG interrupts, which could come in the same series ;-)
>>>>>
>>>>
>>>> Just to clarify debugfs mode behaviour.
>>>>
>>>> Currently it is just changing PRTCAPDIR. What we need to do is that if
>>>> dr_mode == "otg", then we call dwc3_host/gadget_init/exit() accordingly as well.
>>>>
>>>> Does this make sense?
>>>
>>> it does.
>>>
>>
>> OK. Below is a patch that allows us to use debugfs/mode to do the role switch.
>> Switching from device to host worked fine but I get the following error when
>> switching from host to device.
>>
>> https://hastebin.com/liluqosewe.xml
>>
>> cheers,
>> -roger
>>
>> ---
>> From 50c49f18474b388d10533eb9f6d04f454fabf687 Mon Sep 17 00:00:00 2001
>> From: Roger Quadros <rogerq@xxxxxx>
>> Date: Fri, 31 Mar 2017 12:54:13 +0300
>> Subject: [PATCH] usb: dwc3: make role-switching work with debugfs/mode
>>
>> If dr_mode == "otg", we start by default in PERIPHERAL mode.
>> Keep track of current role in "current_dr_role" whenever dwc3_set_mode()
>> is called.
>>
>> When debugfs/mode is changed AND we're in dual-role mode,
>> handle the switch by stopping and starting the respective
>> host/gadget controllers.
>>
>> Signed-off-by: Roger Quadros <rogerq@xxxxxx>
>
> I'm assuming you also plan on breaking this down further ;-)

Did you mean I must split this patch into smaller ones?

>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 369bab1..e2d36ba 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -108,6 +108,8 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
>> reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
>> reg |= DWC3_GCTL_PRTCAPDIR(mode);
>> dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>> +
>> + dwc->current_dr_role = mode;
>> }
>>
>> u32 dwc3_core_fifo_space(struct dwc3_ep *dep, u8 type)
>> @@ -862,13 +864,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>> }
>> break;
>> case USB_DR_MODE_OTG:
>> - ret = dwc3_host_init(dwc);
>> - if (ret) {
>> - if (ret != -EPROBE_DEFER)
>> - dev_err(dev, "failed to initialize host\n");
>> - return ret;
>> - }
>> -
>> + /* start in peripheral role by default */
>> + dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>> ret = dwc3_gadget_init(dwc);
>> if (ret) {
>> if (ret != -EPROBE_DEFER)
>> @@ -894,8 +891,11 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc)
>> dwc3_host_exit(dwc);
>> break;
>> case USB_DR_MODE_OTG:
>> - dwc3_host_exit(dwc);
>> - dwc3_gadget_exit(dwc);
>> + /* role might have changed since start */
>> + if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE)
>> + dwc3_gadget_exit(dwc);
>> + else if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
>> + dwc3_host_exit(dwc);
>
> how about patching the respective exit/init functions with something
> like:
>
> if (dwc->current_dr_role != $my_expected_role)
> return 0;
>
> then you can call them without any checks.

OK.

>
>> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
>> index 31926dd..a101b14 100644
>> --- a/drivers/usb/dwc3/debugfs.c
>> +++ b/drivers/usb/dwc3/debugfs.c
>> @@ -327,19 +327,54 @@ static ssize_t dwc3_mode_write(struct file *file,
>> return -EFAULT;
>>
>> if (!strncmp(buf, "host", 4))
>> - mode |= DWC3_GCTL_PRTCAP_HOST;
>> + mode = DWC3_GCTL_PRTCAP_HOST;
>>
>> if (!strncmp(buf, "device", 6))
>> - mode |= DWC3_GCTL_PRTCAP_DEVICE;
>> + mode = DWC3_GCTL_PRTCAP_DEVICE;
>>
>> if (!strncmp(buf, "otg", 3))
>> - mode |= DWC3_GCTL_PRTCAP_OTG;
>> + mode = DWC3_GCTL_PRTCAP_OTG;
>>
>> - if (mode) {
>> - spin_lock_irqsave(&dwc->lock, flags);
>> - dwc3_set_mode(dwc, mode);
>> - spin_unlock_irqrestore(&dwc->lock, flags);
>> + if (!mode)
>> + return -EINVAL;
>> +
>> + if (mode == dwc->current_dr_role)
>> + goto exit;
>> +
>> + /* prevent role switching if we're not dual-role */
>> + if (dwc->dr_mode != USB_DR_MODE_OTG)
>> + return -EINVAL;
>> +
>> + /* stop old role */
>> + if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
>
> is this your bug? This switch statement only executes when we're in host
> mode. This means that when you switch to peripheral, you don't exit
> host. Then when you switch back from peripheral to host, you're going to
> add the same platform_device again. We're going to have TWO xHCI
> platform device with the exact same name. When you finally switch again
> from host to device, then you have issues.
>
> Can you confirm?

That was a bug but I still see the issue although only when a mass storage
device was plugged in.

I see this other new issue when not using a mass storage device.

root@rockdesk:/sys/kernel/debug/48890000.usb# echo device > mode
[ 218.226104] xhci-hcd xhci-hcd.1.auto: remove, state 4
[ 218.231822] usb usb4: USB disconnect, device number 1
[ 218.246973] xhci-hcd xhci-hcd.1.auto: USB bus 4 deregistered
[ 218.252961] xhci-hcd xhci-hcd.1.auto: remove, state 4
[ 218.258347] usb usb3: USB disconnect, device number 1
[ 218.265858] xhci-hcd xhci-hcd.1.auto: USB bus 3 deregistered
[ 218.274312] dwc3 48890000.usb: changing max_speed on rev 5533202a
[ 218.282108] kobject (ed120208): tried to init an initialized object, something is seriously wrong.
[ 218.291553] CPU: 1 PID: 2025 Comm: bash Not tainted 4.11.0-rc4-00004-g559b2c9 #1285
[ 218.299590] Hardware name: Generic DRA74X (Flattened Device Tree)
[ 218.306002] [<c01101ec>] (unwind_backtrace) from [<c010c328>] (show_stack+0x10/0x14)
[ 218.314133] [<c010c328>] (show_stack) from [<c04b46b8>] (dump_stack+0xac/0xe0)
[ 218.321716] [<c04b46b8>] (dump_stack) from [<c04b5dc8>] (kobject_init+0x78/0x94)
[ 218.329484] [<c04b5dc8>] (kobject_init) from [<c0570c98>] (device_initialize+0x20/0xe4)
[ 218.337891] [<c0570c98>] (device_initialize) from [<c0573280>] (device_register+0xc/0x18)
[ 218.346502] [<c0573280>] (device_register) from [<bf298ce8>] (usb_add_gadget_udc_release+0x88/0x1e8 [udc_core])
[ 218.357153] [<bf298ce8>] (usb_add_gadget_udc_release [udc_core]) from [<bf2c6d68>] (dwc3_gadget_init+0x278/0x6c0 [dwc3])
[ 218.368603] [<bf2c6d68>] (dwc3_gadget_init [dwc3]) from [<bf2cab54>] (dwc3_mode_write+0x178/0x1c4 [dwc3])
[ 218.378668] [<bf2cab54>] (dwc3_mode_write [dwc3]) from [<c04557c4>] (full_proxy_write+0x4c/0x64)
[ 218.387897] [<c04557c4>] (full_proxy_write) from [<c02b29d8>] (__vfs_write+0x1c/0x114)
[ 218.396206] [<c02b29d8>] (__vfs_write) from [<c02b421c>] (vfs_write+0xa0/0x168)
[ 218.403877] [<c02b421c>] (vfs_write) from [<c02b50b4>] (SyS_write+0x44/0x9c)
[ 218.411276] [<c02b50b4>] (SyS_write) from [<c0107880>] (ret_fast_syscall+0x0/0x1c)
[ 218.419347] ------------[ cut here ]------------
[ 218.424208] WARNING: CPU: 1 PID: 2025 at lib/kobject.c:597 kobject_get+0x48/0x58
[ 218.432018] kobject: '(null)' (ed379018): is not initialized, yet kobject_get() is being called.
[ 218.441272] Modules linked in: usb_f_ss_lb g_zero libcomposite xhci_plat_hcd xhci_hcd usbcore dwc3 udc_core evdev snd_soc_davinci_mcasp usb_common snd_soc_edma snd_soc_simple_card m25p80 snd_soc_tlv320aic3x e
[ 218.488436] CPU: 1 PID: 2025 Comm: bash Not tainted 4.11.0-rc4-00004-g559b2c9 #1285
[ 218.496470] Hardware name: Generic DRA74X (Flattened Device Tree)
[ 218.502870] [<c01101ec>] (unwind_backtrace) from [<c010c328>] (show_stack+0x10/0x14)
[ 218.510996] [<c010c328>] (show_stack) from [<c04b46b8>] (dump_stack+0xac/0xe0)
[ 218.518579] [<c04b46b8>] (dump_stack) from [<c0136cf0>] (__warn+0xd8/0x104)
[ 218.525895] [<c0136cf0>] (__warn) from [<c0136d50>] (warn_slowpath_fmt+0x34/0x44)
[ 218.533756] [<c0136d50>] (warn_slowpath_fmt) from [<c04b5e38>] (kobject_get+0x48/0x58)
[ 218.542065] [<c04b5e38>] (kobject_get) from [<c0800ef0>] (klist_add_tail+0x18/0x44)
[ 218.550114] [<c0800ef0>] (klist_add_tail) from [<c05730dc>] (device_add+0x3d8/0x570)
[ 218.558258] [<c05730dc>] (device_add) from [<bf298ce8>] (usb_add_gadget_udc_release+0x88/0x1e8 [udc_core])
[ 218.568428] [<bf298ce8>] (usb_add_gadget_udc_release [udc_core]) from [<bf2c6d68>] (dwc3_gadget_init+0x278/0x6c0 [dwc3])
[ 218.579872] [<bf2c6d68>] (dwc3_gadget_init [dwc3]) from [<bf2cab54>] (dwc3_mode_write+0x178/0x1c4 [dwc3])
[ 218.589939] [<bf2cab54>] (dwc3_mode_write [dwc3]) from [<c04557c4>] (full_proxy_write+0x4c/0x64)
[ 218.599165] [<c04557c4>] (full_proxy_write) from [<c02b29d8>] (__vfs_write+0x1c/0x114)
[ 218.607480] [<c02b29d8>] (__vfs_write) from [<c02b421c>] (vfs_write+0xa0/0x168)
[ 218.615147] [<c02b421c>] (vfs_write) from [<c02b50b4>] (SyS_write+0x44/0x9c)
[ 218.622549] [<c02b50b4>] (SyS_write) from [<c0107880>] (ret_fast_syscall+0x0/0x1c)
[ 218.630543] ---[ end trace 9b9aa5ff9aaa9cf9 ]---
[ 218.635433] ------------[ cut here ]------------
[ 218.640321] WARNING: CPU: 1 PID: 2025 at lib/refcount.c:114 kobject_get+0x24/0x58
[ 218.648232] refcount_t: increment on 0; use-after-free.
[ 218.653718] Modules linked in: usb_f_ss_lb g_zero libcomposite xhci_plat_hcd xhci_hcd usbcore dwc3 udc_core evdev snd_soc_davinci_mcasp usb_common snd_soc_edma snd_soc_simple_card m25p80 snd_soc_tlv320aic3x e
[ 218.700873] CPU: 1 PID: 2025 Comm: bash Tainted: G W 4.11.0-rc4-00004-g559b2c9 #1285
[ 218.710180] Hardware name: Generic DRA74X (Flattened Device Tree)
[ 218.716583] [<c01101ec>] (unwind_backtrace) from [<c010c328>] (show_stack+0x10/0x14)
[ 218.724710] [<c010c328>] (show_stack) from [<c04b46b8>] (dump_stack+0xac/0xe0)
[ 218.732296] [<c04b46b8>] (dump_stack) from [<c0136cf0>] (__warn+0xd8/0x104)
[ 218.739605] [<c0136cf0>] (__warn) from [<c0136d50>] (warn_slowpath_fmt+0x34/0x44)
[ 218.747467] [<c0136d50>] (warn_slowpath_fmt) from [<c04b5e14>] (kobject_get+0x24/0x58)
[ 218.755778] [<c04b5e14>] (kobject_get) from [<c0800ef0>] (klist_add_tail+0x18/0x44)
[ 218.763820] [<c0800ef0>] (klist_add_tail) from [<c05730dc>] (device_add+0x3d8/0x570)
[ 218.771963] [<c05730dc>] (device_add) from [<bf298ce8>] (usb_add_gadget_udc_release+0x88/0x1e8 [udc_core])
[ 218.782128] [<bf298ce8>] (usb_add_gadget_udc_release [udc_core]) from [<bf2c6d68>] (dwc3_gadget_init+0x278/0x6c0 [dwc3])
[ 218.793573] [<bf2c6d68>] (dwc3_gadget_init [dwc3]) from [<bf2cab54>] (dwc3_mode_write+0x178/0x1c4 [dwc3])
[ 218.803634] [<bf2cab54>] (dwc3_mode_write [dwc3]) from [<c04557c4>] (full_proxy_write+0x4c/0x64)
[ 218.812862] [<c04557c4>] (full_proxy_write) from [<c02b29d8>] (__vfs_write+0x1c/0x114)
[ 218.821166] [<c02b29d8>] (__vfs_write) from [<c02b421c>] (vfs_write+0xa0/0x168)
[ 218.828848] [<c02b421c>] (vfs_write) from [<c02b50b4>] (SyS_write+0x44/0x9c)
[ 218.836251] [<c02b50b4>] (SyS_write) from [<c0107880>] (ret_fast_syscall+0x0/0x1c)
[ 218.844240] ---[ end trace 9b9aa5ff9aaa9cfa ]---
[ 218.850118] zero gadget: Gadget Zero, version: Cinco de Mayo 2008
[ 218.856622] zero gadget: zero ready
[ 218.861206] omap_l3_noc 44000000.ocp: L3 application error: target 5 mod:1 (unclearable)
[ 218.869779] omap_l3_noc 44000000.ocp: L3 debug error: target 5 mod:1 (unclearable)

>
>> + switch (dwc->current_dr_role) {
>> + case DWC3_GCTL_PRTCAP_HOST:
>> + dwc3_host_exit(dwc);
>> + break;
>> + case DWC3_GCTL_PRTCAP_DEVICE:
>> + dwc3_gadget_exit(dwc);
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + /* switch PRTCAP mode. updates current_dr_role */
>> + spin_lock_irqsave(&dwc->lock, flags);
>> + dwc3_set_mode(dwc, mode);
>> + spin_unlock_irqrestore(&dwc->lock, flags);
>> +
>> + /* start new role */
>> + switch (dwc->current_dr_role) {
>> + case DWC3_GCTL_PRTCAP_HOST:
>> + dwc3_host_init(dwc);
>> + break;
>> + case DWC3_GCTL_PRTCAP_DEVICE:
>> + dwc3_gadget_init(dwc);
>> + break;
>> + default:
>> + break;
>> }
>> +exit:
>> return count;
>> }
>>
>> --
>> 2.7.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

cheers,
-roger