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 -- 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
