Re: make a confirm for [usb: dwc3: gadget: skip Set/Clear Halt when invalid]

Re: make a confirm for [usb: dwc3: gadget: skip Set/Clear Halt when invalid]

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2856 bytes --]


Hi,

that patch is not 100% correct. You can revert it in your tree. I added
that because of a problem I found when running adb against macOS.

It's actually okay to send Clear Halt at any time, but for some reason
dwc3 was hanging when running adb against macOS.

If you can revert the patch and make sure it works against all 3 major
OSes (linux, windows and mac) I'd be really glad.

liangshengjun <liangshengjun@hisilicon.com> writes:
> Hi felipe,
>
>       I have met a case about set/clear Halt patch
> Version: linux v4.16,
> Case:  usb uvc run with bulk-mode connect to Windows 7 PC. When PC stop camera application , it would send clearHalt request to uvc device to streaming-off video transfer.
>            But with v4.16 dwc3 drivers, it would skip handling this clear Halt request , because dep->flags is not DWC3_EP_STALL status, then it causes PC restart camera application , uvc transfer fail.
>            And I have confirmed v3.18 dwc3 drivers is OK.
>
>       So how to balance for handling clear Halt without first setHalt ??
>
> PS:
> commit ffb80fc672c3a7b6afd0cefcb1524fb99917b2f3
> Author: Felipe Balbi <felipe.balbi@linux.intel.com>
> Date:   Thu Jan 19 13:38:42 2017 +0200
>
>     usb: dwc3: gadget: skip Set/Clear Halt when invalid
>
>     At least macOS seems to be sending
>     ClearFeature(ENDPOINT_HALT) to endpoints which
>     aren't Halted. This makes DWC3's CLEARSTALL command
>     time out which causes several issues for the driver.
>
>     Instead, let's just return 0 and bail out early.
>
>     Cc: <stable@vger.kernel.org>
>     Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 6faf484..0a664d8 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1379,6 +1379,9 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
>                 unsigned transfer_in_flight;
>                 unsigned started;
>
> +               if (dep->flags & DWC3_EP_STALL)
> +                       return 0;
> +
>                 if (dep->number > 1)
>                         trb = dwc3_ep_prev_trb(dep, dep->trb_enqueue);
>                 else
> @@ -1400,6 +1403,8 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
>                 else
>                         dep->flags |= DWC3_EP_STALL;
>         } else {
> +               if (!(dep->flags & DWC3_EP_STALL))
> +                       return 0;
>
>                 ret = dwc3_send_clear_stall_ep_cmd(dep);
>                 if (ret)
>
>
> Liang Shengjun
> [cid:image001.png@01D40971.9265B340]
> HISILICON TECHNOLOGIES CO., LTD.
> New R&D Center, Wuhe Road, Bantian,
> Longgang District, Shenzhen 518129 P.R. China
>

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: make a confirm for [usb: dwc3: gadget: skip Set/Clear Halt when invalid]

[Alan Stern]

On Thu, 21 Jun 2018, Felipe Balbi wrote:

> 
> Hi,
> 
> that patch is not 100% correct. You can revert it in your tree. I added
> that because of a problem I found when running adb against macOS.
> 
> It's actually okay to send Clear Halt at any time, but for some reason
> dwc3 was hanging when running adb against macOS.

Note: According to the USB spec it's okay to send Clear-Halt at any
time.  But there are plenty of devices that get upset if they receive
this message when the endpoint isn't actually halted.

Alan stern

Re: make a confirm for [usb: dwc3: gadget: skip Set/Clear Halt when invalid]

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 777 bytes --]


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
>> that patch is not 100% correct. You can revert it in your tree. I added
>> that because of a problem I found when running adb against macOS.
>> 
>> It's actually okay to send Clear Halt at any time, but for some reason
>> dwc3 was hanging when running adb against macOS.
>
> Note: According to the USB spec it's okay to send Clear-Halt at any
> time.  But there are plenty of devices that get upset if they receive
> this message when the endpoint isn't actually halted.

right. The weird thing here is that dwc3 has never suffered from this
until we ran ADB against macOS. That was the only way to get any
problems.

Without clear halt, though, we have no means for syncing data toggle.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

答复: make a confirm for [usb: dwc3: gadget: skip Set/Clear Halt when invalid]

[liangshengjun]

Hi balbi,

It means that the mainline keep  checking stall status first before handle clear-halt request?
as usb spec, it's actually okay to send Clear Halt at any time. But dwc3 core hanging with macOS adb application,
so I think there is other rootcase why dwc3 hanging ,  and current patch just for avoid this case. Right?

If someday WindonwPC/LINUX PC meet this case again liked my case, would you plan to revert it ? or other plan ?


Liang Shengjun

HISILICON TECHNOLOGIES CO., LTD.
New R&D Center, Wuhe Road, Bantian,
Longgang District, Shenzhen 518129 P.R. China

-----邮件原件-----
发件人: Felipe Balbi [mailto:felipe.balbi@linux.intel.com] 
发送时间: 2018年6月25日 15:48
收件人: Alan Stern <stern@rowland.harvard.edu>
抄送: liangshengjun <liangshengjun@hisilicon.com>; stable@vger.kernel.org; linux-usb@vger.kernel.org
主题: Re: make a confirm for [usb: dwc3: gadget: skip Set/Clear Halt when invalid]


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
>> that patch is not 100% correct. You can revert it in your tree. I 
>> added that because of a problem I found when running adb against macOS.
>> 
>> It's actually okay to send Clear Halt at any time, but for some 
>> reason
>> dwc3 was hanging when running adb against macOS.
>
> Note: According to the USB spec it's okay to send Clear-Halt at any 
> time.  But there are plenty of devices that get upset if they receive 
> this message when the endpoint isn't actually halted.

right. The weird thing here is that dwc3 has never suffered from this until we ran ADB against macOS. That was the only way to get any problems.

Without clear halt, though, we have no means for syncing data toggle.

--
balbi

Re: 答复: make a confirm for [usb: dwc3: gadget: skip Set/Clear Halt when invalid]

[Felipe Balbi]

(no top-posting!!)

liangshengjun <liangshengjun@hisilicon.com> writes:

> Hi balbi,
>
> It means that the mainline keep checking stall status first before
> handle clear-halt request?  as usb spec, it's actually okay to send
> Clear Halt at any time. But dwc3 core hanging with macOS adb
> application, so I think there is other rootcase why dwc3 hanging , and
> current patch just for avoid this case. Right?

that's correct. There's another problem happening that causes dwc3 to
hang. I guess I'll just revert that patch since it causes more problems
than solve.

> If someday WindonwPC/LINUX PC meet this case again liked my case,
> would you plan to revert it ? or other plan ?

I'll just revert it.

-- 
balbi

Re: 答复: make a confirm for [usb: dwc3: gadget: skip Set/Clear Halt when invalid]

[Thinh Nguyen]

Hi Felipe,

On 6/28/2018 11:24 PM, Felipe Balbi wrote:
> (no top-posting!!)
>
> liangshengjun <liangshengjun@hisilicon.com> writes:
>
>> Hi balbi,
>>
>> It means that the mainline keep checking stall status first before
>> handle clear-halt request?  as usb spec, it's actually okay to send
>> Clear Halt at any time. But dwc3 core hanging with macOS adb
>> application, so I think there is other rootcase why dwc3 hanging , and
>> current patch just for avoid this case. Right?
> that's correct. There's another problem happening that causes dwc3 to
> hang. I guess I'll just revert that patch since it causes more problems
> than solve.
>
>> If someday WindonwPC/LINUX PC meet this case again liked my case,
>> would you plan to revert it ? or other plan ?
> I'll just revert it.
>

Do you plan to revert the patch ffb80fc672c3 ("usb: dwc3: gadget: skip
Set/Clear Halt when invalid") for the next release?

BR,
Thinh

Re: 答复: make a confirm for [usb: dwc3: gadget: skip Set/Clear Halt when invalid]

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 919 bytes --]


Hi,

Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
>>> It means that the mainline keep checking stall status first before
>>> handle clear-halt request?  as usb spec, it's actually okay to send
>>> Clear Halt at any time. But dwc3 core hanging with macOS adb
>>> application, so I think there is other rootcase why dwc3 hanging , and
>>> current patch just for avoid this case. Right?
>> that's correct. There's another problem happening that causes dwc3 to
>> hang. I guess I'll just revert that patch since it causes more problems
>> than solve.
>>
>>> If someday WindonwPC/LINUX PC meet this case again liked my case,
>>> would you plan to revert it ? or other plan ?
>> I'll just revert it.
>>
>
> Do you plan to revert the patch ffb80fc672c3 ("usb: dwc3: gadget: skip
> Set/Clear Halt when invalid") for the next release?

I'll send a patch soon to be merged on -rc5 or so.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: make a confirm for [usb: dwc3: gadget: skip Set/Clear Halt when invalid]

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1872 bytes --]


Hi,

please don't top-post and break your emails at 80 columns

liangshengjun <liangshengjun@hisilicon.com> writes:
> Hi balbi,
>
> 	Thanks for reponse. Now I fixed this case without check STALL
> 	status when clear-Halt request.
>
> BDW, I have meet other case:
>
> Run dwc3 for uvc function, the uvc video show video overlapping when
> windows 7 restart camera app.
>
> I debug the dwc3 drivers ,found :
>
> 	1. when camera app close, the dwc3 ep_dequeue is call. Then
> 	giveback request with status: -ECONNRESET to uvc function layer.
> 	dep->trb_dequeue keep stable
>
> 	2.when camera app reopen ,the dwc3 ep_queue is call. Then kick a
> 	transfer, fetch a new trb to dwc3 core. dep->trb_enqueue
> 	increment.
>
> 	3.when one trb tranfter complete, handle event process function
> 	fetch one frb by current dep->trb_dequeue, and uvc function
> 	would get one request complete giveback, which have been
> 	dequeue.
>
> 	   But in fact, current dep->trb_dequeue pointer buffer is
> 	   "old", because stp1 have been dequeue it.
>
> 	   Current dwc3 drivers: the correct enqueue req process is " ep
> 	   enqueue > fetch a new trb by trb_enqueue > increase
> 	   trb_enqueue > pack trb to dwc3 core", Right?
>
> 	   For dequeue request process, now is " ep dequeue > stop
> 	   started_list request > giveback request with -ECONNRESET
> 	   status" Right ?
>
>   To avoid getting a older request which has been dequeued, I think
>   dequeue process necessary sync the trb_dequeue to trb_enqueue. Right?

no, this is not the right fix. I think thkis has been fixed recently,
though. Check if current mainline still has the issue. If it doesn,
collect tracepoint data (see [1] below) and let me know how you
reproduce it.

[1] https://www.kernel.org/doc/html/latest/driver-api/usb/dwc3.html#reporting-bugs

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: make a confirm for [usb: dwc3: gadget: skip Set/Clear Halt when invalid]

[liangshengjun]

Hi balbi,

	Thanks for reponse. Now I fixed this case without check STALL status when clear-Halt request.

BDW, I have meet other case:

Run dwc3 for uvc function, the uvc video show video overlapping when windows 7 restart camera app.
I debug the dwc3 drivers ,found :
	1. when camera app close, the dwc3 ep_dequeue is call. Then giveback request with status: -ECONNRESET to uvc function layer.  dep->trb_dequeue keep stable
	2.when camera app reopen ,the dwc3 ep_queue is call. Then kick a transfer, fetch a new trb to dwc3 core. dep->trb_enqueue increment.
	3.when one trb tranfter complete, handle event process function fetch one frb by current dep->trb_dequeue, and uvc function would get one request complete giveback, which have been dequeue.
	   But in fact, current dep->trb_dequeue pointer buffer is "old", because stp1 have been dequeue it.
	   Current dwc3 drivers:  the correct enqueue req process is " ep enqueue > fetch a new trb by trb_enqueue > increase trb_enqueue > pack trb to dwc3 core",  Right?
	   For dequeue request process, now is " ep dequeue > stop started_list request > giveback request with -ECONNRESET status" Right ?

  To avoid getting a older request which has been dequeued, I think dequeue process necessary sync the trb_dequeue to trb_enqueue. Right ?		



Liang Shengjun


-----邮件原件-----
发件人: Felipe Balbi [mailto:felipe.balbi@linux.intel.com] 
发送时间: 2018年6月25日 15:48
收件人: Alan Stern <stern@rowland.harvard.edu>
抄送: liangshengjun <liangshengjun@hisilicon.com>; stable@vger.kernel.org; linux-usb@vger.kernel.org
主题: Re: make a confirm for [usb: dwc3: gadget: skip Set/Clear Halt when invalid]


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
>> that patch is not 100% correct. You can revert it in your tree. I 
>> added that because of a problem I found when running adb against macOS.
>> 
>> It's actually okay to send Clear Halt at any time, but for some 
>> reason
>> dwc3 was hanging when running adb against macOS.
>
> Note: According to the USB spec it's okay to send Clear-Halt at any 
> time.  But there are plenty of devices that get upset if they receive 
> this message when the endpoint isn't actually halted.

right. The weird thing here is that dwc3 has never suffered from this until we ran ADB against macOS. That was the only way to get any problems.

Without clear halt, though, we have no means for syncing data toggle.

--
balbi