From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754781AbdCaMVh (ORCPT ); Fri, 31 Mar 2017 08:21:37 -0400 Received: from lelnx194.ext.ti.com ([198.47.27.80]:46174 "EHLO lelnx194.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752512AbdCaMVf (ORCPT ); Fri, 31 Mar 2017 08:21:35 -0400 Subject: Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode To: Felipe Balbi References: <1487250377-13653-1-git-send-email-rogerq@ti.com> <1487250377-13653-5-git-send-email-rogerq@ti.com> <87o9wlhd1j.fsf@linux.intel.com> <87d1d0l6f4.fsf@linux.intel.com> <8f7c0e29-2313-5d0a-ce5c-2c729c5f2b9d@ti.com> <87zig1j3bg.fsf@linux.intel.com> <87tw69irlf.fsf@linux.intel.com> CC: , , , Mathias Nyman From: Roger Quadros Message-ID: Date: Fri, 31 Mar 2017 15:21:25 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <87tw69irlf.fsf@linux.intel.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 31/03/17 15:00, Felipe Balbi wrote: > > Hi, > > Roger Quadros 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 >> 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 > > 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] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [ 218.314133] [] (show_stack) from [] (dump_stack+0xac/0xe0) [ 218.321716] [] (dump_stack) from [] (kobject_init+0x78/0x94) [ 218.329484] [] (kobject_init) from [] (device_initialize+0x20/0xe4) [ 218.337891] [] (device_initialize) from [] (device_register+0xc/0x18) [ 218.346502] [] (device_register) from [] (usb_add_gadget_udc_release+0x88/0x1e8 [udc_core]) [ 218.357153] [] (usb_add_gadget_udc_release [udc_core]) from [] (dwc3_gadget_init+0x278/0x6c0 [dwc3]) [ 218.368603] [] (dwc3_gadget_init [dwc3]) from [] (dwc3_mode_write+0x178/0x1c4 [dwc3]) [ 218.378668] [] (dwc3_mode_write [dwc3]) from [] (full_proxy_write+0x4c/0x64) [ 218.387897] [] (full_proxy_write) from [] (__vfs_write+0x1c/0x114) [ 218.396206] [] (__vfs_write) from [] (vfs_write+0xa0/0x168) [ 218.403877] [] (vfs_write) from [] (SyS_write+0x44/0x9c) [ 218.411276] [] (SyS_write) from [] (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] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [ 218.510996] [] (show_stack) from [] (dump_stack+0xac/0xe0) [ 218.518579] [] (dump_stack) from [] (__warn+0xd8/0x104) [ 218.525895] [] (__warn) from [] (warn_slowpath_fmt+0x34/0x44) [ 218.533756] [] (warn_slowpath_fmt) from [] (kobject_get+0x48/0x58) [ 218.542065] [] (kobject_get) from [] (klist_add_tail+0x18/0x44) [ 218.550114] [] (klist_add_tail) from [] (device_add+0x3d8/0x570) [ 218.558258] [] (device_add) from [] (usb_add_gadget_udc_release+0x88/0x1e8 [udc_core]) [ 218.568428] [] (usb_add_gadget_udc_release [udc_core]) from [] (dwc3_gadget_init+0x278/0x6c0 [dwc3]) [ 218.579872] [] (dwc3_gadget_init [dwc3]) from [] (dwc3_mode_write+0x178/0x1c4 [dwc3]) [ 218.589939] [] (dwc3_mode_write [dwc3]) from [] (full_proxy_write+0x4c/0x64) [ 218.599165] [] (full_proxy_write) from [] (__vfs_write+0x1c/0x114) [ 218.607480] [] (__vfs_write) from [] (vfs_write+0xa0/0x168) [ 218.615147] [] (vfs_write) from [] (SyS_write+0x44/0x9c) [ 218.622549] [] (SyS_write) from [] (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] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [ 218.724710] [] (show_stack) from [] (dump_stack+0xac/0xe0) [ 218.732296] [] (dump_stack) from [] (__warn+0xd8/0x104) [ 218.739605] [] (__warn) from [] (warn_slowpath_fmt+0x34/0x44) [ 218.747467] [] (warn_slowpath_fmt) from [] (kobject_get+0x24/0x58) [ 218.755778] [] (kobject_get) from [] (klist_add_tail+0x18/0x44) [ 218.763820] [] (klist_add_tail) from [] (device_add+0x3d8/0x570) [ 218.771963] [] (device_add) from [] (usb_add_gadget_udc_release+0x88/0x1e8 [udc_core]) [ 218.782128] [] (usb_add_gadget_udc_release [udc_core]) from [] (dwc3_gadget_init+0x278/0x6c0 [dwc3]) [ 218.793573] [] (dwc3_gadget_init [dwc3]) from [] (dwc3_mode_write+0x178/0x1c4 [dwc3]) [ 218.803634] [] (dwc3_mode_write [dwc3]) from [] (full_proxy_write+0x4c/0x64) [ 218.812862] [] (full_proxy_write) from [] (__vfs_write+0x1c/0x114) [ 218.821166] [] (__vfs_write) from [] (vfs_write+0xa0/0x168) [ 218.828848] [] (vfs_write) from [] (SyS_write+0x44/0x9c) [ 218.836251] [] (SyS_write) from [] (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