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

[Felipe Balbi]

Hi,

Roger Quadros <rogerq@xxxxxx> writes:
> 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.

kobj->state_initialized is left set. Need to find a clean way to clear
it. As a quick test, you could memset gadget->dev to zero from usb_del_gadget_udc()

-- 
balbi