Re: [PATCH 2/3] usb: dwc3: gadget: Don't skip updating remaining data

2018-08-02 Thread Thinh Nguyen
On 8/2/2018 12:46 AM, Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen  writes:
>
> 
>
>>> These patches will not fix the issue.
>>>
>> What do you think of this fix?
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index f452ab708ffc..338f7ab8a8b4 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2277,8 +2277,10 @@ static int
>> dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>>  * with one TRB pending in the ring. We need to manually clear
>> HWO bit
>>  * from that TRB.
>>  */
>> -   if ((req->zero || req->unaligned) && (trb->ctrl & 
>> DWC3_TRB_CTRL_HWO)) {
>> -   trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
>> +   if ((req->zero || req->unaligned) && !chain) {
>> +   if (trb->ctrl & DWC3_TRB_CTRL_HWO)
>> +   trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
>> +
>> return 1;
>> }
> This is a rathher minimal fix. I like it. So this together with the one
> I wrote for the TRB type, right? Can you send this one as a proper patch
> and add the correct Cc stable and Fixes tags?
>
Yes. Can you create an official patch for that TRB type issue you found?

Thanks,

Thinh

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH] usb: dwc3: gadget: Properly check last unaligned/zero chain TRB

2018-08-02 Thread Thinh Nguyen
Current check for the last extra TRB for zero and unaligned transfers
does not account for isoc OUT. The last TRB of the Buffer Descriptor for
isoc OUT transfers will be retired with HWO=0. As a result, we won't
return early. The req->remaining will be updated to include the BUFSIZ
count of the extra TRB, and the actual number of transferred bytes
calculation will be wrong.

To fix this, check whether it's a short or zero packet and the last TRB
chain bit to return early.

Cc: sta...@vger.kernel.org
Fixes: c6267a51639b ("usb: dwc3: gadget: align transfers to wMaxPacketSize")
Signed-off-by: Thinh Nguyen 
---
 drivers/usb/dwc3/gadget.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 032ea7d709ba..c09e4f784810 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2251,7 +2251,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct 
dwc3_ep *dep,
 * with one TRB pending in the ring. We need to manually clear HWO bit
 * from that TRB.
 */
-   if ((req->zero || req->unaligned) && (trb->ctrl & DWC3_TRB_CTRL_HWO)) {
+   if ((req->zero || req->unaligned) && !(trb->ctrl & DWC3_TRB_CTRL_CHN)) {
trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
return 1;
}
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: usb: dynamic suspend

2018-08-02 Thread Alan Stern
On Thu, 2 Aug 2018, Muni Sekhar wrote:

> I see that CONFIG_PM_RUNTIME & CONFIG_USB_SUSPEND are not enabled. Is that 
> fine?

The CONFIG_PM_RUNTIME and CONFIG_USB_SUSPEND symbols are no longer 
used.

> The device is using btusb driver. Does unloading the driver helps?

It should.

> I ran ‘lsof’ and and I see that the USB device was being held open by
> fwupd, not sure what is ‘fwupd’?

It is a firmware update daemon.

> # sudo lsof +D /dev/bus/usb
> lsof: WARNING: can't stat() fuse.gvfsd-fuse file system /run/user/1000/gvfs
>   Output information may be incomplete.
> COMMAND  PID USER   FD   TYPE DEVICE SIZE/OFF NODE NAME
> fwupd   1631 root   49u   CHR 189,67  0t0  547 /dev/bus/usb/001/068
> 
> 
> So I killed that process(fwupd), but still device is not going to SUSPEND 
> mode.
> 
> # sudo lsof +D /dev/bus/usb
> lsof: WARNING: can't stat() fuse.gvfsd-fuse file system /run/user/1000/gvfs
>   Output information may be incomplete.
> 
> 
> # cat /sys/bus/usb/devices/1-1.3/power/runtime_status
> active

Well, keeping the device might or might not prevent it from being
suspended.  It all depends on how the driver is written.  But if you
unload the driver then the device certainly ought to go into runtime
suspend.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] Fix big-endian application issue for MUSB gadget

2018-08-02 Thread Bin Liu
On Thu, Aug 02, 2018 at 11:44:00AM -0500, Bin Liu wrote:
> Hi,
> 
> On Thu, Jul 26, 2018 at 12:52:53PM +, Alexey Spirkov wrote:
> > Existing code is not applicable to big-endian machines
> > ctrlrequest fields received in USB endian - i.e. in little-endian
> > and should be converted to cpu endianness before usage.
> > 
> > Signed-off-by: Alexey Spirkov 
> > ---
> >  drivers/usb/musb/musb_gadget_ep0.c | 33 -
> >  1 file changed, 20 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/usb/musb/musb_gadget_ep0.c 
> > b/drivers/usb/musb/musb_gadget_ep0.c
> > index 91a5027..5d5c933 100644
> > --- a/drivers/usb/musb/musb_gadget_ep0.c
> > +++ b/drivers/usb/musb/musb_gadget_ep0.c
> > @@ -82,7 +82,7 @@ static int service_tx_status_request(
> > u16 tmp;
> > void __iomem*regs;
> >  
> > -   epnum = (u8) ctrlrequest->wIndex;
> > +   epnum = (u8) le16_to_cpu(ctrlrequest->wIndex);
> > if (!epnum) {
> > result[0] = 0;
> > break;
> > @@ -217,14 +217,15 @@ __acquires(musb->lock)
> > case USB_REQ_SET_ADDRESS:
> > /* change it after the status stage */
> > musb->set_address = true;
> > -   musb->address = (u8) (ctrlrequest->wValue & 0x7f);
> > +   musb->address = (u8) (le16_to_cpu(ctrlrequest->wValue) &
> > +   0x7f);
> > handled = 1;
> > break;
> >  
> > case USB_REQ_CLEAR_FEATURE:
> > switch (recip) {
> > case USB_RECIP_DEVICE:
> > -   if (ctrlrequest->wValue
> > +   if (le16_to_cpu(ctrlrequest->wValue)
> > != USB_DEVICE_REMOTE_WAKEUP)
> > break;
> > musb->may_wakeup = 0;
> > @@ -234,7 +235,7 @@ __acquires(musb->lock)
> > break;
> > case USB_RECIP_ENDPOINT:{
> > const u8epnum =
> > -   ctrlrequest->wIndex & 0x0f;
> > +   le16_to_cpu(ctrlrequest->wIndex) & 0x0f;
> > struct musb_ep  *musb_ep;
> > struct musb_hw_ep   *ep;
> > struct musb_request *request;
> > @@ -243,12 +244,14 @@ __acquires(musb->lock)
> > u16 csr;
> >  
> > if (epnum == 0 || epnum >= MUSB_C_NUM_EPS ||
> > -   ctrlrequest->wValue != USB_ENDPOINT_HALT)
> > +   le16_to_cpu(ctrlrequest->wValue)
> > +   != USB_ENDPOINT_HALT)
> > break;
> >  
> > ep = musb->endpoints + epnum;
> > regs = ep->regs;
> > -   is_in = ctrlrequest->wIndex & USB_DIR_IN;
> > +   is_in = le16_to_cpu(ctrlrequest->wIndex) &
> > +   USB_DIR_IN;
> > if (is_in)
> > musb_ep = >ep_in;
> > else
> > @@ -300,17 +303,19 @@ __acquires(musb->lock)
> > switch (recip) {
> > case USB_RECIP_DEVICE:
> > handled = 1;
> > -   switch (ctrlrequest->wValue) {
> > +   switch (le16_to_cpu(ctrlrequest->wValue)) {
> > case USB_DEVICE_REMOTE_WAKEUP:
> > musb->may_wakeup = 1;
> > break;
> > case USB_DEVICE_TEST_MODE:
> > if (musb->g.speed != USB_SPEED_HIGH)
> > goto stall;
> > -   if (ctrlrequest->wIndex & 0xff)
> > +   if (le16_to_cpu(ctrlrequest->wIndex) &
> > +   0xff)
> > goto stall;
> >  
> > -   switch (ctrlrequest->wIndex >> 8) {
> > +   switch (le16_to_cpu(ctrlrequest->wIndex)
> > +>> 8) {
> > case 1:
> > pr_debug("TEST_J\n");
> > /* TEST_J */
> > @@ -399,7 +404,7 @@ __acquires(musb->lock)
> >  
> > case 

Re: [PATCH] Fix memory issue in non-DMA mode for MUSB gadget

2018-08-02 Thread Bin Liu
On Thu, Aug 02, 2018 at 11:14:32AM -0500, Bin Liu wrote:
> Hi,
> 
> On Thu, Jul 26, 2018 at 12:52:57PM +, Alexey Spirkov wrote:
> > dma_mapping_error function unable to works in PowerPC arch
> > when MUSB do not use DMA (illegal memory access). Proposed
> > patch replace its usage to usual define for checking DMA mapping.
> > 
> > Signed-off-by: Alexey Spirkov 
> > ---
> >  drivers/usb/musb/musb_gadget.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> > index eae8b1b..3bc7c25 100644
> > --- a/drivers/usb/musb/musb_gadget.c
> > +++ b/drivers/usb/musb/musb_gadget.c
> > @@ -140,7 +140,7 @@ __acquires(ep->musb->lock)
> > ep->busy = 1;
> > spin_unlock(>lock);
> >  
> > -   if (!dma_mapping_error(>g.dev, request->dma))
> > +   if (req && is_buffer_mapped(req))
> 
> unmap_dma_buffer() checks for is_buffer_mapped(), so this line can be
> dropped.
> 
> > unmap_dma_buffer(req, musb);
> >  
> > trace_musb_req_gb(req);

Please also fix the patch subject to "usb: musb: gadget: ..." in your
v2. I prefer

"usb: musb: gadget: fix illegal dma address check in non-DMA mode"

Regards,
-Bin.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] Fix big-endian application issue for MUSB gadget

2018-08-02 Thread Bin Liu
Hi,

On Thu, Jul 26, 2018 at 12:52:53PM +, Alexey Spirkov wrote:
> Existing code is not applicable to big-endian machines
> ctrlrequest fields received in USB endian - i.e. in little-endian
> and should be converted to cpu endianness before usage.
> 
> Signed-off-by: Alexey Spirkov 
> ---
>  drivers/usb/musb/musb_gadget_ep0.c | 33 -
>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/musb/musb_gadget_ep0.c 
> b/drivers/usb/musb/musb_gadget_ep0.c
> index 91a5027..5d5c933 100644
> --- a/drivers/usb/musb/musb_gadget_ep0.c
> +++ b/drivers/usb/musb/musb_gadget_ep0.c
> @@ -82,7 +82,7 @@ static int service_tx_status_request(
>   u16 tmp;
>   void __iomem*regs;
>  
> - epnum = (u8) ctrlrequest->wIndex;
> + epnum = (u8) le16_to_cpu(ctrlrequest->wIndex);
>   if (!epnum) {
>   result[0] = 0;
>   break;
> @@ -217,14 +217,15 @@ __acquires(musb->lock)
>   case USB_REQ_SET_ADDRESS:
>   /* change it after the status stage */
>   musb->set_address = true;
> - musb->address = (u8) (ctrlrequest->wValue & 0x7f);
> + musb->address = (u8) (le16_to_cpu(ctrlrequest->wValue) &
> + 0x7f);
>   handled = 1;
>   break;
>  
>   case USB_REQ_CLEAR_FEATURE:
>   switch (recip) {
>   case USB_RECIP_DEVICE:
> - if (ctrlrequest->wValue
> + if (le16_to_cpu(ctrlrequest->wValue)
>   != USB_DEVICE_REMOTE_WAKEUP)
>   break;
>   musb->may_wakeup = 0;
> @@ -234,7 +235,7 @@ __acquires(musb->lock)
>   break;
>   case USB_RECIP_ENDPOINT:{
>   const u8epnum =
> - ctrlrequest->wIndex & 0x0f;
> + le16_to_cpu(ctrlrequest->wIndex) & 0x0f;
>   struct musb_ep  *musb_ep;
>   struct musb_hw_ep   *ep;
>   struct musb_request *request;
> @@ -243,12 +244,14 @@ __acquires(musb->lock)
>   u16 csr;
>  
>   if (epnum == 0 || epnum >= MUSB_C_NUM_EPS ||
> - ctrlrequest->wValue != USB_ENDPOINT_HALT)
> + le16_to_cpu(ctrlrequest->wValue)
> + != USB_ENDPOINT_HALT)
>   break;
>  
>   ep = musb->endpoints + epnum;
>   regs = ep->regs;
> - is_in = ctrlrequest->wIndex & USB_DIR_IN;
> + is_in = le16_to_cpu(ctrlrequest->wIndex) &
> + USB_DIR_IN;
>   if (is_in)
>   musb_ep = >ep_in;
>   else
> @@ -300,17 +303,19 @@ __acquires(musb->lock)
>   switch (recip) {
>   case USB_RECIP_DEVICE:
>   handled = 1;
> - switch (ctrlrequest->wValue) {
> + switch (le16_to_cpu(ctrlrequest->wValue)) {
>   case USB_DEVICE_REMOTE_WAKEUP:
>   musb->may_wakeup = 1;
>   break;
>   case USB_DEVICE_TEST_MODE:
>   if (musb->g.speed != USB_SPEED_HIGH)
>   goto stall;
> - if (ctrlrequest->wIndex & 0xff)
> + if (le16_to_cpu(ctrlrequest->wIndex) &
> + 0xff)
>   goto stall;
>  
> - switch (ctrlrequest->wIndex >> 8) {
> + switch (le16_to_cpu(ctrlrequest->wIndex)
> +  >> 8) {
>   case 1:
>   pr_debug("TEST_J\n");
>   /* TEST_J */
> @@ -399,7 +404,7 @@ __acquires(musb->lock)
>  
>   case USB_RECIP_ENDPOINT:{
>   const u8epnum =
> -   

Re: [PATCH] Fix memory issue in non-DMA mode for MUSB gadget

2018-08-02 Thread Bin Liu
Hi,

On Thu, Jul 26, 2018 at 12:52:57PM +, Alexey Spirkov wrote:
> dma_mapping_error function unable to works in PowerPC arch
> when MUSB do not use DMA (illegal memory access). Proposed
> patch replace its usage to usual define for checking DMA mapping.
> 
> Signed-off-by: Alexey Spirkov 
> ---
>  drivers/usb/musb/musb_gadget.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> index eae8b1b..3bc7c25 100644
> --- a/drivers/usb/musb/musb_gadget.c
> +++ b/drivers/usb/musb/musb_gadget.c
> @@ -140,7 +140,7 @@ __acquires(ep->musb->lock)
>   ep->busy = 1;
>   spin_unlock(>lock);
>  
> - if (!dma_mapping_error(>g.dev, request->dma))
> + if (req && is_buffer_mapped(req))

unmap_dma_buffer() checks for is_buffer_mapped(), so this line can be
dropped.

>   unmap_dma_buffer(req, musb);
>  
>   trace_musb_req_gb(req);

Regards,
-Bin.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: usb: dynamic suspend

2018-08-02 Thread Muni Sekhar
On Thu, Aug 2, 2018 at 8:21 PM, Alan Stern  wrote:
> On Thu, 2 Aug 2018, Muni Sekhar wrote:
>
>> On Thu, Aug 2, 2018 at 7:08 PM, Alan Stern  wrote:
>> > On Thu, 2 Aug 2018, Muni Sekhar wrote:
>> >
>> >> [ Please keep me in CC as I'm not subscribed to the list]
>> >>
>> >>
>> >> Hello All,
>> >>
>> >>
>> >> I’ve an Ubuntu 16.04 LTS PC with 4.4.0-124-generic kernel version.
>> >>
>> >> I  want to test the dynamic suspend for the below mentioned connected
>> >> USB device.
>> >>
>> >> Bus 003 Device 028: ID 0cf3:e300 Atheros Communications, Inc.
>> >>
>> >>
>> >> First I’d like to know the required configuration(kernel built) of the
>> >> kernel for supporting the dynamic suspend for USB.
>> >>
>> >> My kernel had been built with the below mentioned configuration:
>> >> CONFIG_SUSPEND=y
>> >> CONFIG_HIBERNATION=y
>> >> CONFIG_PM=y
>> >>
>> >> Any other kernel configuration required for supporting the dynamic 
>> >> suspend?
>> >
>> > No.
>> >
>> >> How can I suspend a particular USB device?
>> >
>> > You have to do something like:
>> >
>> > echo auto >/sys/bus/usb/devices/.../power/control
>> >
>> > where the "..." is filled in with the appropriate name for the
>> > particular device.  There are some programs, like powertop, which will
>> > do this for you automatically.
>> Thank you Alan for this information. I wrote 'auto' to power/control,
>> but still device not going to suspend mode(power/runtime_status is
>> 'active'). Any other factors needs to be considered?
>
> Yes, there are other factors.  They depend on how device's driver is
> written.
>
> I don't know exactly what kind of device you're talking about, what
> driver it uses, or what the driver's requirements are.  Since the
> vendor is Atheros, I guess it's a wifi card.  Perhaps turning wifi off
> will allow the device to be suspended; you'll probably have to check
> with the driver's author or maintainer to find out for certain.

I see that CONFIG_PM_RUNTIME & CONFIG_USB_SUSPEND are not enabled. Is that fine?

The device is using btusb driver. Does unloading the driver helps?

T:  Bus=01 Lev=02 Prnt=02 Port=02 Cnt=01 Dev#= 68 Spd=12   MxCh= 0
D:  Ver= 1.10 Cls=e0(wlcon) Sub=01 Prot=01 MxPS=64 #Cfgs=  1
P:  Vendor=0cf3 ProdID=e300 Rev= 0.01
C:* #Ifs= 2 Cfg#= 1 Atr=e0 MxPwr=100mA
I:* If#= 0 Alt= 0 #EPs= 3 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
E:  Ad=81(I) Atr=03(Int.) MxPS=  16 Ivl=1ms
E:  Ad=82(I) Atr=02(Bulk) MxPS=  64 Ivl=0ms
E:  Ad=02(O) Atr=02(Bulk) MxPS=  64 Ivl=0ms
I:* If#= 1 Alt= 0 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
E:  Ad=83(I) Atr=01(Isoc) MxPS=   0 Ivl=1ms
E:  Ad=03(O) Atr=01(Isoc) MxPS=   0 Ivl=1ms
I:  If#= 1 Alt= 1 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
E:  Ad=83(I) Atr=01(Isoc) MxPS=   9 Ivl=1ms
E:  Ad=03(O) Atr=01(Isoc) MxPS=   9 Ivl=1ms
I:  If#= 1 Alt= 2 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
E:  Ad=83(I) Atr=01(Isoc) MxPS=  17 Ivl=1ms
E:  Ad=03(O) Atr=01(Isoc) MxPS=  17 Ivl=1ms
I:  If#= 1 Alt= 3 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
E:  Ad=83(I) Atr=01(Isoc) MxPS=  25 Ivl=1ms
E:  Ad=03(O) Atr=01(Isoc) MxPS=  25 Ivl=1ms
I:  If#= 1 Alt= 4 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
E:  Ad=83(I) Atr=01(Isoc) MxPS=  33 Ivl=1ms
E:  Ad=03(O) Atr=01(Isoc) MxPS=  33 Ivl=1ms
I:  If#= 1 Alt= 5 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
E:  Ad=83(I) Atr=01(Isoc) MxPS=  49 Ivl=1ms
E:  Ad=03(O) Atr=01(Isoc) MxPS=  49 Ivl=1ms

I ran ‘lsof’ and and I see that the USB device was being held open by
fwupd, not sure what is ‘fwupd’?

# sudo lsof +D /dev/bus/usb
lsof: WARNING: can't stat() fuse.gvfsd-fuse file system /run/user/1000/gvfs
  Output information may be incomplete.
COMMAND  PID USER   FD   TYPE DEVICE SIZE/OFF NODE NAME
fwupd   1631 root   49u   CHR 189,67  0t0  547 /dev/bus/usb/001/068


So I killed that process(fwupd), but still device is not going to SUSPEND mode.

# sudo lsof +D /dev/bus/usb
lsof: WARNING: can't stat() fuse.gvfsd-fuse file system /run/user/1000/gvfs
  Output information may be incomplete.


# cat /sys/bus/usb/devices/1-1.3/power/runtime_status
active


>
> Alan Stern
>
>> Here is the my log:
>>
>> # cat /sys/bus/usb/devices/1-1.3/idVendor
>> 0cf3
>> # cat /sys/bus/usb/devices/1-1.3/idProduct
>> e300
>> # cat /sys/bus/usb/devices/1-1.3/power/control
>> on
>> # cat /sys/bus/usb/devices/1-1.3/power/runtime_status
>> active
>> # cat /sys/bus/usb/devices/1-1.3/power/runtime_suspended_time
>> 0
>> # echo auto > /sys/bus/usb/devices/1-1.3/power/control
>> # cat /sys/bus/usb/devices/1-1.3/power/control
>> auto
>> # cat /sys/bus/usb/devices/1-1.3/power/runtime_status
>> active
>



-- 
Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: usb: dynamic suspend

2018-08-02 Thread Alan Stern
On Thu, 2 Aug 2018, Muni Sekhar wrote:

> On Thu, Aug 2, 2018 at 7:08 PM, Alan Stern  wrote:
> > On Thu, 2 Aug 2018, Muni Sekhar wrote:
> >
> >> [ Please keep me in CC as I'm not subscribed to the list]
> >>
> >>
> >> Hello All,
> >>
> >>
> >> I’ve an Ubuntu 16.04 LTS PC with 4.4.0-124-generic kernel version.
> >>
> >> I  want to test the dynamic suspend for the below mentioned connected
> >> USB device.
> >>
> >> Bus 003 Device 028: ID 0cf3:e300 Atheros Communications, Inc.
> >>
> >>
> >> First I’d like to know the required configuration(kernel built) of the
> >> kernel for supporting the dynamic suspend for USB.
> >>
> >> My kernel had been built with the below mentioned configuration:
> >> CONFIG_SUSPEND=y
> >> CONFIG_HIBERNATION=y
> >> CONFIG_PM=y
> >>
> >> Any other kernel configuration required for supporting the dynamic suspend?
> >
> > No.
> >
> >> How can I suspend a particular USB device?
> >
> > You have to do something like:
> >
> > echo auto >/sys/bus/usb/devices/.../power/control
> >
> > where the "..." is filled in with the appropriate name for the
> > particular device.  There are some programs, like powertop, which will
> > do this for you automatically.
> Thank you Alan for this information. I wrote 'auto' to power/control,
> but still device not going to suspend mode(power/runtime_status is
> 'active'). Any other factors needs to be considered?

Yes, there are other factors.  They depend on how device's driver is
written.

I don't know exactly what kind of device you're talking about, what
driver it uses, or what the driver's requirements are.  Since the
vendor is Atheros, I guess it's a wifi card.  Perhaps turning wifi off
will allow the device to be suspended; you'll probably have to check
with the driver's author or maintainer to find out for certain.

Alan Stern

> Here is the my log:
> 
> # cat /sys/bus/usb/devices/1-1.3/idVendor
> 0cf3
> # cat /sys/bus/usb/devices/1-1.3/idProduct
> e300
> # cat /sys/bus/usb/devices/1-1.3/power/control
> on
> # cat /sys/bus/usb/devices/1-1.3/power/runtime_status
> active
> # cat /sys/bus/usb/devices/1-1.3/power/runtime_suspended_time
> 0
> # echo auto > /sys/bus/usb/devices/1-1.3/power/control
> # cat /sys/bus/usb/devices/1-1.3/power/control
> auto
> # cat /sys/bus/usb/devices/1-1.3/power/runtime_status
> active

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers

2018-08-02 Thread Vincent Pelletier
On Thu, 2 Aug 2018 00:45:14 +, "He, Bo"  wrote:
> Your patch fix the issue BUG: scheduling while atomic:

Yes, although from my understanding of Felipe's answer, the actual bug
is the "scheduling" part (sleeping in dwc3 UDC) rather than the
"atomic" part.

So my patch addresses, still if my understanding is correct, the wrong
half of the problem, and even introduced the regression you identified.
Hence my uncertainty...
-- 
Vincent Pelletier
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: usb: dynamic suspend

2018-08-02 Thread Muni Sekhar
On Thu, Aug 2, 2018 at 7:08 PM, Alan Stern  wrote:
> On Thu, 2 Aug 2018, Muni Sekhar wrote:
>
>> [ Please keep me in CC as I'm not subscribed to the list]
>>
>>
>> Hello All,
>>
>>
>> I’ve an Ubuntu 16.04 LTS PC with 4.4.0-124-generic kernel version.
>>
>> I  want to test the dynamic suspend for the below mentioned connected
>> USB device.
>>
>> Bus 003 Device 028: ID 0cf3:e300 Atheros Communications, Inc.
>>
>>
>> First I’d like to know the required configuration(kernel built) of the
>> kernel for supporting the dynamic suspend for USB.
>>
>> My kernel had been built with the below mentioned configuration:
>> CONFIG_SUSPEND=y
>> CONFIG_HIBERNATION=y
>> CONFIG_PM=y
>>
>> Any other kernel configuration required for supporting the dynamic suspend?
>
> No.
>
>> How can I suspend a particular USB device?
>
> You have to do something like:
>
> echo auto >/sys/bus/usb/devices/.../power/control
>
> where the "..." is filled in with the appropriate name for the
> particular device.  There are some programs, like powertop, which will
> do this for you automatically.
Thank you Alan for this information. I wrote 'auto' to power/control,
but still device not going to suspend mode(power/runtime_status is
'active'). Any other factors needs to be considered?
Here is the my log:

# cat /sys/bus/usb/devices/1-1.3/idVendor
0cf3
# cat /sys/bus/usb/devices/1-1.3/idProduct
e300
# cat /sys/bus/usb/devices/1-1.3/power/control
on
# cat /sys/bus/usb/devices/1-1.3/power/runtime_status
active
# cat /sys/bus/usb/devices/1-1.3/power/runtime_suspended_time
0
# echo auto > /sys/bus/usb/devices/1-1.3/power/control
# cat /sys/bus/usb/devices/1-1.3/power/control
auto
# cat /sys/bus/usb/devices/1-1.3/power/runtime_status
active


>
>> How do I verify whether the device went to suspended mode or
>> not(without connecting the USB analyzer)?
>
> cat /sys/bus/usb/devices/.../power/runtime_status
>
> will tell you whether or not the device is currently suspended.
>
> Alan Stern
>



-- 
Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: usb: dynamic suspend

2018-08-02 Thread Alan Stern
On Thu, 2 Aug 2018, Muni Sekhar wrote:

> [ Please keep me in CC as I'm not subscribed to the list]
> 
> 
> Hello All,
> 
> 
> I’ve an Ubuntu 16.04 LTS PC with 4.4.0-124-generic kernel version.
> 
> I  want to test the dynamic suspend for the below mentioned connected
> USB device.
> 
> Bus 003 Device 028: ID 0cf3:e300 Atheros Communications, Inc.
> 
> 
> First I’d like to know the required configuration(kernel built) of the
> kernel for supporting the dynamic suspend for USB.
> 
> My kernel had been built with the below mentioned configuration:
> CONFIG_SUSPEND=y
> CONFIG_HIBERNATION=y
> CONFIG_PM=y
> 
> Any other kernel configuration required for supporting the dynamic suspend?

No.

> How can I suspend a particular USB device?

You have to do something like:

echo auto >/sys/bus/usb/devices/.../power/control

where the "..." is filled in with the appropriate name for the 
particular device.  There are some programs, like powertop, which will 
do this for you automatically.

> How do I verify whether the device went to suspended mode or
> not(without connecting the USB analyzer)?

cat /sys/bus/usb/devices/.../power/runtime_status

will tell you whether or not the device is currently suspended.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 3/3] arm64: dts: rockchip: enable dwc2-based otg controller on px30-evb

2018-08-02 Thread Heiko Stuebner
Enable the newly added controller on the px30 evaluation board.

Signed-off-by: Heiko Stuebner 
---
 arch/arm64/boot/dts/rockchip/px30-evb.dts | 4 
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/px30-evb.dts 
b/arch/arm64/boot/dts/rockchip/px30-evb.dts
index c74aa910a631..263d7f3dbc44 100644
--- a/arch/arm64/boot/dts/rockchip/px30-evb.dts
+++ b/arch/arm64/boot/dts/rockchip/px30-evb.dts
@@ -206,6 +206,10 @@
status = "okay";
 };
 
+_otg {
+   status = "okay";
+};
+
 _host0_ehci {
status = "okay";
 };
-- 
2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 0/3] add dwc2 support for px30

2018-08-02 Thread Heiko Stuebner
This is split out from the series adding the px30 support, due to
the dwc2 binding change not having landed yet in a maintainer tree.

The Acked binding change should go through some usb tree, while
I'll pick up the dts changes once that has happened.

The binding itself is unchanged.


Heiko Stuebner (2):
  arm64: dts: rockchip: add dwc2 otg controller on px30
  arm64: dts: rockchip: enable dwc2-based otg controller on px30-evb

Liang Chen (1):
  dt-bindings: usb: dwc2: add description for px30

 Documentation/devicetree/bindings/usb/dwc2.txt |  1 +
 arch/arm64/boot/dts/rockchip/px30-evb.dts  |  4 
 arch/arm64/boot/dts/rockchip/px30.dtsi | 16 
 3 files changed, 21 insertions(+)

-- 
2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 1/3] dt-bindings: usb: dwc2: add description for px30

2018-08-02 Thread Heiko Stuebner
From: Liang Chen 

This patch adds the compatible of dwc2 for PX30 SoCs.

Signed-off-by: Liang Chen 
Acked-by: Rob Herring 
Signed-off-by: Heiko Stuebner 
---
unchanged from it being part of the general px30 series

 Documentation/devicetree/bindings/usb/dwc2.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt 
b/Documentation/devicetree/bindings/usb/dwc2.txt
index 46da5f184460..6dc3c4a34483 100644
--- a/Documentation/devicetree/bindings/usb/dwc2.txt
+++ b/Documentation/devicetree/bindings/usb/dwc2.txt
@@ -6,6 +6,7 @@ Required properties:
   - brcm,bcm2835-usb: The DWC2 USB controller instance in the BCM2835 SoC.
   - hisilicon,hi6220-usb: The DWC2 USB controller instance in the hi6220 SoC.
   - rockchip,rk3066-usb: The DWC2 USB controller instance in the rk3066 Soc;
+  - "rockchip,px30-usb", "rockchip,rk3066-usb", "snps,dwc2": for px30 Soc;
   - "rockchip,rk3188-usb", "rockchip,rk3066-usb", "snps,dwc2": for rk3188 Soc;
   - "rockchip,rk3288-usb", "rockchip,rk3066-usb", "snps,dwc2": for rk3288 Soc;
   - "lantiq,arx100-usb": The DWC2 USB controller instance in Lantiq ARX SoCs;
-- 
2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 2/3] arm64: dts: rockchip: add dwc2 otg controller on px30

2018-08-02 Thread Heiko Stuebner
Add the node for the dwc2-based otg controller on the px30 soc.

Signed-off-by: Heiko Stuebner 
---
 arch/arm64/boot/dts/rockchip/px30.dtsi | 16 
 1 file changed, 16 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/px30.dtsi 
b/arch/arm64/boot/dts/rockchip/px30.dtsi
index dc3b22ca9a0e..fed782194a94 100644
--- a/arch/arm64/boot/dts/rockchip/px30.dtsi
+++ b/arch/arm64/boot/dts/rockchip/px30.dtsi
@@ -702,6 +702,22 @@
<1>, <2>;
};
 
+   usb20_otg: usb@ff30 {
+   compatible = "rockchip,px30-usb", "rockchip,rk3066-usb",
+"snps,dwc2";
+   reg = <0x0 0xff30 0x0 0x4>;
+   interrupts = ;
+   clocks = < HCLK_OTG>;
+   clock-names = "otg";
+   dr_mode = "otg";
+   g-np-tx-fifo-size = <16>;
+   g-rx-fifo-size = <280>;
+   g-tx-fifo-size = <256 128 128 64 32 16>;
+   g-use-dma;
+   power-domains = < PX30_PD_USB>;
+   status = "disabled";
+   };
+
usb_host0_ehci: usb@ff34 {
compatible = "generic-ehci";
reg = <0x0 0xff34 0x0 0x1>;
-- 
2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: broken TRIM support for JMS578 in uas mode

2018-08-02 Thread Johannes Thumshirn
On Thu, Aug 02, 2018 at 02:24:16PM +0200, Oliver Neukum wrote:
> On Mo, 2018-07-30 at 14:43 +0300, Mailing Lists wrote:
> > I cannot issue TRIM commands to SSD behind a JMS578-based sata to
> > usb-c adapter. Tried with Fedora 28 kernel and with latest
> > 4.18.0-0.rc6.git0.1.vanilla.knurd.1.fc28.x86_64
> > lsblk -D shows that discard is not enabled, but the SSD has this
> > capability (see below)
> 
> Hi Johannes,
> 
> is there a way to signal the SCSI layer that a trim for a device should
> be attempted, even if it claims not to support it?

Generally speaking no. If the USB Adapter doesn't translate the
Command it doesn't translate it.

Byte,
Johannes
-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: broken TRIM support for JMS578 in uas mode

2018-08-02 Thread Oliver Neukum
On Mo, 2018-07-30 at 14:43 +0300, Mailing Lists wrote:
> I cannot issue TRIM commands to SSD behind a JMS578-based sata to
> usb-c adapter. Tried with Fedora 28 kernel and with latest
> 4.18.0-0.rc6.git0.1.vanilla.knurd.1.fc28.x86_64
> lsblk -D shows that discard is not enabled, but the SSD has this
> capability (see below)

Hi Johannes,

is there a way to signal the SCSI layer that a trim for a device should
be attempted, even if it claims not to support it?

Regards
Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


usb: dynamic suspend

2018-08-02 Thread Muni Sekhar
[ Please keep me in CC as I'm not subscribed to the list]


Hello All,


I’ve an Ubuntu 16.04 LTS PC with 4.4.0-124-generic kernel version.

I  want to test the dynamic suspend for the below mentioned connected
USB device.

Bus 003 Device 028: ID 0cf3:e300 Atheros Communications, Inc.


First I’d like to know the required configuration(kernel built) of the
kernel for supporting the dynamic suspend for USB.

My kernel had been built with the below mentioned configuration:
CONFIG_SUSPEND=y
CONFIG_HIBERNATION=y
CONFIG_PM=y

Any other kernel configuration required for supporting the dynamic suspend?

How can I suspend a particular USB device?

How do I verify whether the device went to suspended mode or
not(without connecting the USB analyzer)?



-- 
Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: fusb302 type-c chip driver supply cutting out

2018-08-02 Thread Adam Thomson
On 01 August 2018 19:38, Tim Harvey wrote:

> On Tue, Jul 31, 2018 at 2:22 AM Adam Thomson
>  wrote:
> >
> > On 27 July 2018 17:41, Tim Harvey wrote:
> >
> > Adding Guenter to the thread.
> >
> > > Greetings,
> > >
> > > I have a custom design with a Fairchild FUSB302 Type-C chip driver
> > > that I'm testing with Linux 4.17 and a BTI AC-60TC 60W charger. For
> > > this design we are using Type-C as a power/charger input only - no
> > > USB2/3 is connected. The board consumes approximately 12W typical
> > > which doesn't leave enough to charge the battery in a timely fashion
> > > unless switching up to a higher voltage via USB-PD.
> > >
> > > I'm finding that when there is no fusb302 driver enabled the charger
> > > puts out 5V@3A as expected but when I enable the fusb302 driver the
> > > charger cuts out momentarily when the fusb302 driver comes up on its
> > > way to switching up to 20V which causes the board to reboot.
> > >
> > > Here's my dts config for a 20V@3A 60W capable sink:
> > >
> > > fusb302@0x24 {
> > > compatible = "fcs,fusb302";
> > > pinctrl-names = "default";
> > > pinctrl-0 = <_usbpd>;
> > > reg = <0x24>;
> > > interrupt-parent = <>;
> > > interrupts = <10 IRQ_TYPE_EDGE_FALLING>;
> > > fcs,max-sink-microvolt = <2000>;
> > > fcs,max-sink-microamp = <300>;
> > > fcs,max-sink-microwatt = <6000>;
> > > };
> > >
> > >
> > > Here's what I see from the kernel while booting without a battery:
> > > [6.122858] typec_fusb302 1-0024: 1-0024 supply vbus not found,
> > > using dummy regulator
> > > ^ supply cuts out momentarily at this point causing board reboot
> > >
> > > Here's what I see from /sys/kernel/debug/tcpm when I add a battery on
> > > the system with enough charge to keep it powered during the VUSB cut:
> > > [6.134543] Setting voltage/current limit 0 mV 0 mA
> > > [6.134960] polarity 0
> > > [6.135304] Requesting mux mode 0, usb-role 0, orientation 0
> > > [6.136271] state change INVALID_STATE -> SNK_UNATTACHED
> > > [6.136334] CC1: 0 -> 0, CC2: 0 -> 0 [state SNK_UNATTACHED,
> > > polarity 0, disconnected]
> > > [6.136358] 1-0024: registered
> > > [6.144183] Setting voltage/current limit 0 mV 0 mA
> > > [6.144452] polarity 0
> > > [6.144586] Requesting mux mode 0, usb-role 0, orientation 0
> > > [6.145592] cc:=0
> > > [6.164039] pending state change PORT_RESET -> PORT_RESET_WAIT_OFF @
> 100
> > > ms
> > > [6.176918] VBUS off
> > > ^ this appears to be where the charger first cuts out
> >
> > The port is being reset here to return the port to a known state as part of
> > initialisation. It then seems that the Source/Charger is resetting it's VBUS
> > down to 0V for ~1s which is most likely it performing a hard reset.
> 
> Is this normal? I'm not clear why a Source/Charger would reset its VBUS.
> 
> >
> > >
> > > [6.273374] state change PORT_RESET -> PORT_RESET_WAIT_OFF [delayed 100
> > > ms]
> > > [6.273394] state change PORT_RESET_WAIT_OFF -> SNK_UNATTACHED
> > > [6.273403] Start DRP toggling
> > > [6.297315] CC1: 0 -> 0, CC2: 0 -> 5 [state DRP_TOGGLING, polarity
> > > 0, connected]
> > > [6.297327] state change DRP_TOGGLING -> SNK_ATTACH_WAIT
> > > [6.297529] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @
> 200
> > > ms
> > > [6.409960] VBUS on
> > > [6.513218] state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED [delayed
> 200
> > > ms]
> > > [6.513236] state change SNK_DEBOUNCED -> SNK_ATTACHED
> > > [6.513244] polarity 1
> > > [6.513251] Requesting mux mode 1, usb-role 2, orientation 2
> > > [6.515121] state change SNK_ATTACHED -> SNK_STARTUP
> > > [6.515418] state change SNK_STARTUP -> SNK_DISCOVERY
> > > [6.515428] Setting voltage/current limit 5000 mV 3000 mA
> > > [6.515462] vbus=0 charge:=1
> > > [6.515536] state change SNK_DISCOVERY -> SNK_WAIT_CAPABILITIES
> > > [6.530767] pending state change SNK_WAIT_CAPABILITIES ->
> > > HARD_RESET_SEND @ 240 ms
> > > [6.567379] PD RX, header: 0x6161 [1]
> > > [6.567407]  PDO 0: type 0, 5000 mV, 3000 mA [E]
> > > [6.567417]  PDO 1: type 0, 9000 mV, 3000 mA []
> > > [6.567427]  PDO 2: type 0, 12000 mV, 3000 mA []
> > > [6.567436]  PDO 3: type 0, 15000 mV, 3000 mA []
> > > [6.567444]  PDO 4: type 0, 18000 mV, 3000 mA []
> > > [6.567452]  PDO 5: type 0, 2 mV, 3000 mA []
> > > [6.567459] state change SNK_WAIT_CAPABILITIES ->
> > > SNK_NEGOTIATE_CAPABILITIES
> > > [6.567643] cc=0 cc1=0 cc2=5 vbus=0 vconn=sink polarity=1
> > > [6.567657] Requesting PDO 5: 2 mV, 3000 mA
> > > [6.567665] PD TX, header: 0x1042
> > > [6.580208] PD TX complete, status: 0
> > > [6.580539] pending state change SNK_NEGOTIATE_CAPABILITIES ->
> > > HARD_RESET_SEND @ 60 ms
> > > [6.653644] state change 

Re: [PATCH] usb: ehci-sh: convert to SPDX identifiers

2018-08-02 Thread Kuninori Morimoto


Hi Greg

> > From: Kuninori Morimoto 
> > 
> > Signed-off-by: Kuninori Morimoto 
> > ---
> >  include/linux/platform_data/ehci-sh.h | 14 +-
> >  1 file changed, 1 insertion(+), 13 deletions(-)
> 
> I can not take patches without any changelog text at all, sorry.

OK, will post v2 with changelog tomorrow

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] usb: ehci-sh: convert to SPDX identifiers

2018-08-02 Thread Greg Kroah-Hartman
On Thu, Aug 02, 2018 at 01:46:40AM +, Kuninori Morimoto wrote:
> 
> From: Kuninori Morimoto 
> 
> Signed-off-by: Kuninori Morimoto 
> ---
>  include/linux/platform_data/ehci-sh.h | 14 +-
>  1 file changed, 1 insertion(+), 13 deletions(-)

I can not take patches without any changelog text at all, sorry.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/3] usb: dwc3: gadget: Don't skip updating remaining data

2018-08-02 Thread Felipe Balbi

Hi,

Thinh Nguyen  writes:



>> These patches will not fix the issue.
>>
>
> What do you think of this fix?
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index f452ab708ffc..338f7ab8a8b4 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2277,8 +2277,10 @@ static int
> dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>  * with one TRB pending in the ring. We need to manually clear
> HWO bit
>  * from that TRB.
>  */
> -   if ((req->zero || req->unaligned) && (trb->ctrl & DWC3_TRB_CTRL_HWO)) 
> {
> -   trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
> +   if ((req->zero || req->unaligned) && !chain) {
> +   if (trb->ctrl & DWC3_TRB_CTRL_HWO)
> +   trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
> +
> return 1;
> }

This is a rathher minimal fix. I like it. So this together with the one
I wrote for the TRB type, right? Can you send this one as a proper patch
and add the correct Cc stable and Fixes tags?

thanks

-- 
balbi


signature.asc
Description: PGP signature


Re: [PATCH 2/3] usb: dwc3: gadget: Don't skip updating remaining data

2018-08-02 Thread Felipe Balbi

Hi,

Thinh Nguyen  writes:

 1. When a TRB receives whose CSP bit is 0 and CHN bit is 1 receives a
 short packet, the chained TRBs that follow it are not written back
 (e.g. the BUFSIZ and HWO fields remain the same as the
 software-prepared value)

 2. In the case of an OUT endpoint, if the CHN bit is set (and CSP is
 also set), and a short packet is received, the core retires the TRB in
 progress and skip past the TRB where CHN=0, accumulating the ISP and
 IOC bits from each TRB. If ISP or IOC is set in any TRB, the core
 generates an XferInProgress event. Hardware does not set the HWO bit
 to 0 in skipped TRBs. If the endpoint type is isochronous, the CHN=0
 TRB will also be retired and its buffer size field updated with the
 total number of bytes remaining in the BD.

 Note that at most we have confirmation that SIZE will be updated in
 case of Isoc endpoints, but there's nothing there about HWO, so I
 expected it to remain set according to the rest of the text.

 There's one possibility that "TRB will also be *retired*" means that
 it's not skipped, which would mean that HWO is, indeed, set to 0. To
>
> Right. And the programming guide also says that for isoc OUT transfer,
> - Any non-first TRBs with CHN=1 that had not already been retired will
> not be written back. HWO will still be 1, and software can reclaim them
> for another transfer.
> - The last TRB of the Buffer Descriptor will be retired with:
> * HWO = 0.
> * BUFSIZ = The total remaining buffer size of the Buffer Descriptor.
>  
> So we know that the last isoc TRB will have CHN=0 and HWO=0.

yeah, only now I understood the actual problem. req->length is 1, so
reading BUFSIZ from last TRB will, actually, cause the problem.

> static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
> struct dwc3_request *req, struct dwc3_trb *trb,
> const struct dwc3_event_depevt *event, int status, int chain)
> {
> unsigned intcount;
>
> dwc3_ep_inc_deq(dep);
>
> trace_dwc3_complete_trb(dep, trb);
>
> /*
>  * If we're in the middle of series of chained TRBs and we
>  * receive a short transfer along the way, DWC3 will skip
>  * through all TRBs including the last TRB in the chain (the
>  * where CHN bit is zero. DWC3 will also avoid clearing HWO
>  * bit and SW has to do it manually.
>  *
>  * We're going to do that here to avoid problems of HW trying
>  * to use bogus TRBs for transfers.
>  */
> if (chain && (trb->ctrl & DWC3_TRB_CTRL_HWO))
> trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
>
> /*
>  * If we're dealing with unaligned size OUT transfer, we will be left
>  * with one TRB pending in the ring. We need to manually clear HWO bit
>  * from that TRB.
>  */
> if ((req->zero || req->unaligned) && (trb->ctrl & DWC3_TRB_CTRL_HWO)) {
>   ^^
>>>
> This tries to check for the last TRB and return early. This works in normal
> cases and it stops incrementing the req->remaining. But for isoc OUT
> transfers,
> this case won't hit due to reason mention previously.
> <<
>
> trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
> return 1;
> }
>
> count = trb->size & DWC3_TRB_SIZE_MASK;
> req->remaining += count;
>  ^^^
>>>
> So, your proposals will still update the req->remaining with the BUFSIZ
> count of
> the last TRB.
> <<
>
>
> if ((trb->ctrl & DWC3_TRB_CTRL_HWO) && status != -ESHUTDOWN)
> return 1;
>
> if (event->status & DEPEVT_STATUS_SHORT && !chain)
> return 1;
>
> if (event->status & DEPEVT_STATUS_IOC)
> return 1;
>
> return 0;
> }
>
>
>
> static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
> const struct dwc3_event_depevt *event,
> struct dwc3_request *req, int status)
> {
> 
> req->request.actual = req->request.length - req->remaining;
> ^^
>>>
> And the calculation is off because req->remaining includes the remaining
> of the
> last TRB BUFSIZ count.
> <<
>  
> }
>
> I included the traces. Hopefully it will go out to the mailing list.
> Thanks for making the patches, and of course I will test them. :)

I'll read your traces shortly and reply again.

-- 
balbi


signature.asc
Description: PGP signature


I have you not received the first message I sent you this morning. We need to talk about something and I'm sure you would like to hear this information.

2018-08-02 Thread Robert Wilson