From: Roger Quadros <rogerq@ti.com>
To: Felipe Balbi <balbi@kernel.org>
Cc: <vivek.gautam@codeaurora.org>, <linux-usb@vger.kernel.org>,
<linux-kernel@vger.kernel.org>,
Mathias Nyman <mathias.nyman@linux.intel.com>
Subject: Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode
Date: Fri, 31 Mar 2017 15:21:25 +0300 [thread overview]
Message-ID: <bcb7e78b-9518-c833-9386-4974642c9ad4@ti.com> (raw)
In-Reply-To: <87tw69irlf.fsf@linux.intel.com>
On 31/03/17 15:00, Felipe Balbi wrote:
>
> Hi,
>
> Roger Quadros <rogerq@ti.com> 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@ti.com>
>> 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@ti.com>
>
> 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@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
cheers,
-roger
next prev parent reply other threads:[~2017-03-31 12:21 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-02-16 13:06 [PATCH v2 0/4] usb: dwc3: dual-role support Roger Quadros
2017-02-16 13:06 ` [PATCH v2 1/4] usb: dwc3: core.h: add some register definitions Roger Quadros
2017-02-16 13:06 ` [PATCH v2 2/4] usb: dwc3: omap: don't miss events during suspend/resume Roger Quadros
2017-02-16 13:06 ` [PATCH v2 3/4] usb: dwc3: add dual-role support Roger Quadros
2017-03-28 11:07 ` Felipe Balbi
2017-03-29 11:33 ` Roger Quadros
2017-03-29 13:15 ` Felipe Balbi
2017-03-30 6:40 ` Roger Quadros
2017-03-30 9:27 ` Felipe Balbi
2017-04-03 5:31 ` John Youn
2017-02-16 13:06 ` [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode Roger Quadros
2017-02-23 8:34 ` Vivek Gautam
2017-02-24 0:57 ` Peter Chen
2017-02-24 3:08 ` Vivek Gautam
2017-02-24 12:02 ` Roger Quadros
2017-02-25 3:46 ` Chanwoo Choi
2017-02-25 3:50 ` Chanwoo Choi
2017-02-28 13:54 ` Vivek Gautam
2017-02-25 3:35 ` Chanwoo Choi
2017-02-28 15:17 ` Roger Quadros
2017-03-28 11:10 ` Felipe Balbi
2017-03-29 9:57 ` Roger Quadros
2017-03-29 10:32 ` Felipe Balbi
2017-03-29 12:00 ` Roger Quadros
2017-03-29 13:21 ` Felipe Balbi
2017-03-29 13:58 ` Roger Quadros
2017-03-30 9:32 ` Felipe Balbi
2017-03-30 10:11 ` Roger Quadros
2017-03-31 7:43 ` Roger Quadros
2017-03-31 7:46 ` Felipe Balbi
2017-03-31 11:50 ` Roger Quadros
2017-03-31 12:00 ` Felipe Balbi
2017-03-31 12:21 ` Roger Quadros [this message]
2017-03-31 12:58 ` Felipe Balbi
2017-03-13 8:33 ` [PATCH v2 0/4] usb: dwc3: dual-role support Roger Quadros
2017-03-28 10:27 ` Felipe Balbi
2017-03-29 9:50 ` Roger Quadros
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bcb7e78b-9518-c833-9386-4974642c9ad4@ti.com \
--to=rogerq@ti.com \
--cc=balbi@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=mathias.nyman@linux.intel.com \
--cc=vivek.gautam@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.