Message-ID: <bcb7e78b-9518-c833-9386-4974642c9ad4@ti.com>
Date:   Fri, 31 Mar 2017 15:21:25 +0300
From:   Roger Quadros <rogerq@...com>
To:     Felipe Balbi <balbi@...nel.org>
CC:     <vivek.gautam@...eaurora.org>, <linux-usb@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        Mathias Nyman <mathias.nyman@...ux.intel.com>
Subject: Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7
 in dual-role mode



On 31/03/17 15:00, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@...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@...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@...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@...kdesk:/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@...r.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

cheers,
-roger

Powered by blists - more mailing lists