Re: [PATCH 1/3] usb: dwc3: gadget: Prevent losing events in event cache

2017-04-11 Thread Janusz Dziedzic 
On 10 April 2017 at 20:43, John Youn <john.y...@synopsys.com> wrote:
> On 04/08/2017 12:38 PM, Janusz Dziedzic wrote:
>> 2017-04-08 1:57 GMT+02:00 Thinh Nguyen <thinh.ngu...@synopsys.com>:
>>> The dwc3 driver can overwite its previous events if its top half IRQ
>>> handler gets invoked again before processing the events in the cache. We
>>> see this as a hang in the file transfer and the host will attempt to
>>> reset the device. This shouldn't be possible in the dwc3 implementation
>>> if the HW and the SW are in sync. However, through testing and reading
>>> the pcie trace, the top half IRQ handler occasionally still gets invoked
>>> one more time after HW interrupt deassertion. We suspect that there is a
>>> small detection delay in the SW.
>>>
>>> Top half IRQ handler deasserts the interrupt line after it gets the
>>> event count by writing DWC3_GEVNTSIZ_INTMASK to DWC3_GEVNTSIZ. There's a
>>> small window for a new event coming in between reading the event count
>>> and the interrupt deassertion. More generally, we will see 0 event
>>> count, which should not affect anything.
>>>
>>> To avoid this issue, we ensure that the events in the cache are
>>> processed before checking for the new ones. The bottom half IRQ will
>>> unmask the DWC3_GEVNTSIZ_INTMASK once it finishes processing the events.
>>> Top half IRQ handler needs to check whether the DWC3_GEVNTSIZ_INTMASK is
>>> still set before storing the new events. It also should return
>>> IRQ_HANDLED if the mask is still set since IRQ handler was invoked for
>>> our usb interrupt.
>>>
>>> Signed-off-by: Thinh Nguyen <thi...@synopsys.com>
>>> ---
>>>  drivers/usb/dwc3/gadget.c | 5 -
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 4dc80729ae11..93d98fb7215e 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -3045,6 +3045,10 @@ static irqreturn_t dwc3_check_event_buf(struct 
>>> dwc3_event_buffer *evt)
>>> return IRQ_HANDLED;
>>> }
>>>
>>> +   reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0));
>>> +   if (reg & DWC3_GEVNTSIZ_INTMASK)
>>> +   return IRQ_HANDLED;
>>> +
>>> count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
>>> count &= DWC3_GEVNTCOUNT_MASK;
>>> if (!count)
>>> @@ -3054,7 +3058,6 @@ static irqreturn_t dwc3_check_event_buf(struct 
>>> dwc3_event_buffer *evt)
>>> evt->flags |= DWC3_EVENT_PENDING;
>>>
>>> /* Mask interrupt */
>>> -   reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0));
>>> reg |= DWC3_GEVNTSIZ_INTMASK;
>>> dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0), reg);
>>>
>>> --
>>> 2.11.0
>>>
>> This seems like a workaround, while when we mask interrupts we should
>> not get IRQ before BH will done. Current driver implementation base on
>> this.
>> We are using 2.60a and didn't see problem you describe. Is it specyfic
>> for some HW revision?
>
> Doesn't seem to be. We can try other HW versions.
>
>>
>> I can imagine we can have the "last" interrupt and you will return
>> IRQ_HANDLED. Will HW issue this interrupt once again? If not will we
>> loose this IRQ (eg. disconnect event)?
>>
>
> Yes it will re-assert. The interrupt line will remain asserted as long
> events remain in the buffer and it is not masked. So when we
> eventually unmask, the interrupt will be reasserted again immediately.
>
>> BTW, other option here could be using (like Baolin propose some time
>> ago) dwc->lock in top half while this is held for BH.
>> Question how long spin_lock() will be held in top half...
>> I am not sure what option is the best.
>
> That won't help in this case since you can still have two calls top
> top-half before a call to bottom. The lock only prevents the two from
> stepping on each other, but that should already be the case without
> needing the lock.
>
Really can we have two TH calls before BH?
Interesting case :)

You suggest we have something like this:
dwc3 IRQ
   kernel irq_mask
  dwc3 TH
 mask dwc3 interrupts
 get evt->count, evt->cache
 write to hw (count)
 return WAKE_THREAD
   kernel irq_unmask
a) Before BH start we get another dwc3 IRQ?
b) CPU0 starts with BH while we get dwc3 IRQ on CPU1?

Re: [PATCH 3/3] usb: dwc3: ep0: improve handling of unaligned OUT requests

2017-04-10 Thread Janusz Dziedzic 
On 7 April 2017 at 13:36, Felipe Balbi  wrote:
> Just like we did for all other endpoint types, let's rely on a chained
> TRB pointing to ep0_bounce_addr in order to align transfer size. This
> will make the code simpler.
>
> Signed-off-by: Felipe Balbi 
> ---
>  drivers/usb/dwc3/core.h   |  6 +
>  drivers/usb/dwc3/ep0.c| 65 
> +--
>  drivers/usb/dwc3/gadget.c | 50 
>  3 files changed, 29 insertions(+), 92 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 2a2673767ccd..e1958f6237af 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -43,7 +43,7 @@
>  #define DWC3_PULL_UP_TIMEOUT   500 /* ms */
>  #define DWC3_ZLP_BUF_SIZE  1024/* size of a superspeed bulk */
>  #define DWC3_BOUNCE_SIZE   1024/* size of a superspeed bulk */
> -#define DWC3_EP0_BOUNCE_SIZE   512
> +#define DWC3_EP0_SETUP_SIZE512
>  #define DWC3_ENDPOINTS_NUM 32
>  #define DWC3_XHCI_RESOURCES_NUM2
>
> @@ -763,12 +763,10 @@ struct dwc3_scratchpad_array {
>   * struct dwc3 - representation of our controller
>   * @drd_work - workqueue used for role swapping
>   * @ep0_trb: trb which is used for the ctrl_req
> - * @ep0_bounce: bounce buffer for ep0
>   * @zlp_buf: used when request->zero is set
>   * @setup_buf: used while precessing STD USB requests
>   * @ep0_trb: dma address of ep0_trb
>   * @ep0_usb_req: dummy req used while handling STD USB requests
> - * @ep0_bounce_addr: dma address of ep0_bounce
>   * @scratch_addr: dma address of scratchbuf
>   * @ep0_in_setup: one control transfer is completed and enter setup phase
>   * @lock: for synchronizing
> @@ -866,13 +864,11 @@ struct dwc3 {
> struct work_struct  drd_work;
> struct dwc3_trb *ep0_trb;
> void*bounce;
> -   void*ep0_bounce;
> void*zlp_buf;
> void*scratchbuf;
> u8  *setup_buf;
> dma_addr_t  ep0_trb_addr;
> dma_addr_t  bounce_addr;
> -   dma_addr_t  ep0_bounce_addr;
> dma_addr_t  scratch_addr;
> struct dwc3_request ep0_usb_req;
> struct completion   ep0_in_setup;
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 3ba2309c837f..04249243e4d3 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -836,7 +836,6 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
> struct usb_request  *ur;
> struct dwc3_trb *trb;
> struct dwc3_ep  *ep0;
> -   unsignedtransfer_size = 0;
> unsignedmaxp;
> unsignedremaining_ur_length;
> void*buf;
> @@ -849,9 +848,7 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
> ep0 = dwc->eps[0];
>
> dwc->ep0_next_event = DWC3_EP0_NRDY_STATUS;
> -
> trb = dwc->ep0_trb;
> -
> trace_dwc3_complete_trb(ep0, trb);
>
> r = next_request(>pending_list);
> @@ -872,39 +869,17 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
> remaining_ur_length = ur->length;
>
> length = trb->size & DWC3_TRB_SIZE_MASK;
> -
> maxp = ep0->endpoint.maxpacket;
> +   transferred = ur->length - length;
> +   ur->actual += transferred;
>
> if (dwc->ep0_bounced) {
> -   /*
> -* Handle the first TRB before handling the bounce buffer if
> -* the request length is greater than the bounce buffer size
> -*/
> -   if (ur->length > DWC3_EP0_BOUNCE_SIZE) {
> -   transfer_size = ALIGN(ur->length - maxp, maxp);
> -   transferred = transfer_size - length;
> -   buf = (u8 *)buf + transferred;
> -   ur->actual += transferred;
> -   remaining_ur_length -= transferred;
> -
> -   trb++;
> -   length = trb->size & DWC3_TRB_SIZE_MASK;
> -
> -   ep0->trb_enqueue = 0;
> -   }
> -
> -   transfer_size = roundup((ur->length - transfer_size),
> -   maxp);
> -
> -   transferred = min_t(u32, remaining_ur_length,
> -   transfer_size - length);
> -   memcpy(buf, dwc->ep0_bounce, transferred);
> -   } else {
> -   transferred = ur->length - length;
> +   trb++;
> +   trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
> +   ep0->trb_enqueue = 0;
> +   dwc->ep0_bounced = false;
> }
>
> -   ur->actual += transferred;
> -
> if ((epnum & 1) && ur->actual <

Re: [PATCH 1/3] usb: dwc3: gadget: Prevent losing events in event cache

2017-04-08 Thread Janusz Dziedzic 
2017-04-08 1:57 GMT+02:00 Thinh Nguyen :
> The dwc3 driver can overwite its previous events if its top half IRQ
> handler gets invoked again before processing the events in the cache. We
> see this as a hang in the file transfer and the host will attempt to
> reset the device. This shouldn't be possible in the dwc3 implementation
> if the HW and the SW are in sync. However, through testing and reading
> the pcie trace, the top half IRQ handler occasionally still gets invoked
> one more time after HW interrupt deassertion. We suspect that there is a
> small detection delay in the SW.
>
> Top half IRQ handler deasserts the interrupt line after it gets the
> event count by writing DWC3_GEVNTSIZ_INTMASK to DWC3_GEVNTSIZ. There's a
> small window for a new event coming in between reading the event count
> and the interrupt deassertion. More generally, we will see 0 event
> count, which should not affect anything.
>
> To avoid this issue, we ensure that the events in the cache are
> processed before checking for the new ones. The bottom half IRQ will
> unmask the DWC3_GEVNTSIZ_INTMASK once it finishes processing the events.
> Top half IRQ handler needs to check whether the DWC3_GEVNTSIZ_INTMASK is
> still set before storing the new events. It also should return
> IRQ_HANDLED if the mask is still set since IRQ handler was invoked for
> our usb interrupt.
>
> Signed-off-by: Thinh Nguyen 
> ---
>  drivers/usb/dwc3/gadget.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 4dc80729ae11..93d98fb7215e 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -3045,6 +3045,10 @@ static irqreturn_t dwc3_check_event_buf(struct 
> dwc3_event_buffer *evt)
> return IRQ_HANDLED;
> }
>
> +   reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0));
> +   if (reg & DWC3_GEVNTSIZ_INTMASK)
> +   return IRQ_HANDLED;
> +
> count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
> count &= DWC3_GEVNTCOUNT_MASK;
> if (!count)
> @@ -3054,7 +3058,6 @@ static irqreturn_t dwc3_check_event_buf(struct 
> dwc3_event_buffer *evt)
> evt->flags |= DWC3_EVENT_PENDING;
>
> /* Mask interrupt */
> -   reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0));
> reg |= DWC3_GEVNTSIZ_INTMASK;
> dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0), reg);
>
> --
> 2.11.0
>
This seems like a workaround, while when we mask interrupts we should
not get IRQ before BH will done. Current driver implementation base on
this.
We are using 2.60a and didn't see problem you describe. Is it specyfic
for some HW revision?

I can imagine we can have the "last" interrupt and you will return
IRQ_HANDLED. Will HW issue this interrupt once again? If not will we
loose this IRQ (eg. disconnect event)?

BTW, other option here could be using (like Baolin propose some time
ago) dwc->lock in top half while this is held for BH.
Question how long spin_lock() will be held in top half...
I am not sure what option is the best.

BR
Janusz
--
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: host: xhci: stalled endpoint ring not cleared on empty td_list

2017-03-30 Thread Janusz Dziedzic 
On 30 March 2017 at 11:23, Greg KH <g...@kroah.com> wrote:
> On Thu, Mar 30, 2017 at 11:11:34AM +0200, Janusz Dziedzic wrote:
>> On 30 March 2017 at 10:53, Greg KH <g...@kroah.com> wrote:
>> > On Thu, Mar 30, 2017 at 10:29:41AM +0200, Christian Gromm wrote:
>> >>
>> >>
>> >> On 03/27/2017 11:31 AM, Felipe Balbi wrote:
>> >> >
>> >> > Hi,
>> >> >
>> >> > Christian Gromm <christian.gr...@microchip.com> writes:
>> >> > > we observe an issue with a td_list running empty and an
>> >> > > endpoint being stalled at the same time on
>> >> > >
>> >> > > Linux ihu-low 4.1.27-abl #1 SMP PREEMPT Mon Mar 20 13:51:51 CET 2017
>> >> > > x86_64 x86_64 x86_64 GNU/Linux.
>> >> >
>> >> > this is one old kernel. Please upgrade to v4.11-rc4 or v4.10 and try 
>> >> > again.
>> >> >
>> >>
>> >> Unfortunately, the system is Yocto based and cannot be updated to 4.10+.
>> >>
>> >> We applied the following patches to have the xhci code at 4.1.39
>> >>
>> >> $ git log --oneline v4.1.27..v4.1.39 -- drivers/usb/host/xhci-*
>> >> c068da4 xhci: fix deadlock at host remove by running watchdog correctly
>> >> 8e77b80 usb: xhci: apply XHCI_PME_STUCK_QUIRK to Intel Apollo Lake
>> >> 18ee106 usb: xhci: hold lock over xhci_abort_cmd_ring()
>> >> af7f5bf xhci: Handle command completion and timeout race
>> >> adae871 usb: host: xhci: Fix possible wild pointer when handling abort
>> >> command
>> >> 073dd4e usb: xhci: fix possible wild pointer
>> >> 4b6ac34 xhci: free xhci virtual devices with leaf nodes first
>> >> 605a696 xhci: workaround for hosts missing CAS bit
>> >> c23a6dc xhci: add restart quirk for Intel Wildcatpoint PCH
>> >> 3b244a6 xhci: fix usb2 resume timing and races.
>> >> 631f063 xhci: fix null pointer dereference in stop command timeout 
>> >> function
>> >> e1052fb usb: xhci: Fix panic if disconnect
>> >> 23c50b2 xhci: always handle "Command Ring Stopped" events
>> >> 95cb83b USB: xhci: Add broken streams quirk for Frescologic device id 1009
>> >> 0c3f25d usb: xhci-plat: properly handle probe deferral for devm_clk_get()
>> >> e78c8a5 xhci: Fix handling timeouted commands on hosts in weird states.
>> >>
>> >>
>> >> The issue is still there. Any other recommendation?
>> >
>> > Use a newer kernel, 4.1 is not supported at all.  If you are stuck with
>> > that kernel, go get support from the company that you are paying for
>> > support from for that kernel release.
>> >
>> Maybe we should start thinking about backporting USB stack -
>> https://backports.wiki.kernel.org.
>> In that case we could use newest USB stack with any older kernel.
>
> No, you can use a newer kernel please.  Backporting stuff is horrid,
> what is keeping you from just using a new kernel?  Nothing will break,
> unless you have out-of-tree patches (and if you do, that's your issue,
> not ours...)
>
I am lucky while I can work with current kernel. So this is not my problem :)

But, before I worked with android products (mostly 3.x kernels) where
kernel update was not an option and I always dream about USB backports
- instead of fixing old USB stack with a lot of hacks already
included. Maybe in the future someone will decide to backport also USB
stack ...

BR
Janusz

> thanks,
>
> 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: usb: host: xhci: stalled endpoint ring not cleared on empty td_list

2017-03-30 Thread Janusz Dziedzic 
On 30 March 2017 at 10:53, Greg KH  wrote:
> On Thu, Mar 30, 2017 at 10:29:41AM +0200, Christian Gromm wrote:
>>
>>
>> On 03/27/2017 11:31 AM, Felipe Balbi wrote:
>> >
>> > Hi,
>> >
>> > Christian Gromm  writes:
>> > > we observe an issue with a td_list running empty and an
>> > > endpoint being stalled at the same time on
>> > >
>> > > Linux ihu-low 4.1.27-abl #1 SMP PREEMPT Mon Mar 20 13:51:51 CET 2017
>> > > x86_64 x86_64 x86_64 GNU/Linux.
>> >
>> > this is one old kernel. Please upgrade to v4.11-rc4 or v4.10 and try again.
>> >
>>
>> Unfortunately, the system is Yocto based and cannot be updated to 4.10+.
>>
>> We applied the following patches to have the xhci code at 4.1.39
>>
>> $ git log --oneline v4.1.27..v4.1.39 -- drivers/usb/host/xhci-*
>> c068da4 xhci: fix deadlock at host remove by running watchdog correctly
>> 8e77b80 usb: xhci: apply XHCI_PME_STUCK_QUIRK to Intel Apollo Lake
>> 18ee106 usb: xhci: hold lock over xhci_abort_cmd_ring()
>> af7f5bf xhci: Handle command completion and timeout race
>> adae871 usb: host: xhci: Fix possible wild pointer when handling abort
>> command
>> 073dd4e usb: xhci: fix possible wild pointer
>> 4b6ac34 xhci: free xhci virtual devices with leaf nodes first
>> 605a696 xhci: workaround for hosts missing CAS bit
>> c23a6dc xhci: add restart quirk for Intel Wildcatpoint PCH
>> 3b244a6 xhci: fix usb2 resume timing and races.
>> 631f063 xhci: fix null pointer dereference in stop command timeout function
>> e1052fb usb: xhci: Fix panic if disconnect
>> 23c50b2 xhci: always handle "Command Ring Stopped" events
>> 95cb83b USB: xhci: Add broken streams quirk for Frescologic device id 1009
>> 0c3f25d usb: xhci-plat: properly handle probe deferral for devm_clk_get()
>> e78c8a5 xhci: Fix handling timeouted commands on hosts in weird states.
>>
>>
>> The issue is still there. Any other recommendation?
>
> Use a newer kernel, 4.1 is not supported at all.  If you are stuck with
> that kernel, go get support from the company that you are paying for
> support from for that kernel release.
>
Maybe we should start thinking about backporting USB stack -
https://backports.wiki.kernel.org.
In that case we could use newest USB stack with any older kernel.

BR
Janusz

> good luck!
>
> 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
--
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: dwc2: pci: Fix error handling in dwc2_pci_probe

2017-02-17 Thread Janusz Dziedzic 
On 17 February 2017 at 14:01, Vardan Mikayelyan
 wrote:
> Move usb_phy_generic_register() function call to the top, to simplify
> error handling.
>
> Replace kzalloc() with devm_kzalloc().
>
> After platform_device_add(), if we error out, we must do
> platform_device_unregister(), which also does the put. So lets move
> devm_kzalloc() to simplify error handling and avoid
> calling of platform_device_unregister().
>
> Reviewed-by: Wei Yongjun 
> Signed-off-by: Vardan Mikayelyan 
> ---
>  drivers/usb/dwc2/pci.c | 26 +-
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/dwc2/pci.c b/drivers/usb/dwc2/pci.c
> index fdeb8c7..7696131 100644
> --- a/drivers/usb/dwc2/pci.c
> +++ b/drivers/usb/dwc2/pci.c
> @@ -104,10 +104,17 @@ static int dwc2_pci_probe(struct pci_dev *pci,
>
> pci_set_master(pci);
>
> +   phy = usb_phy_generic_register();
> +   if (IS_ERR(phy)) {
> +   dev_err(dev, "error registering generic PHY (%ld)\n",
> +   PTR_ERR(phy));
> +   return PTR_ERR(phy);
> +   }
> +
> dwc2 = platform_device_alloc("dwc2", PLATFORM_DEVID_AUTO);
> if (!dwc2) {
> dev_err(dev, "couldn't allocate dwc2 device\n");
> -   return -ENOMEM;
> +   goto err;
> }
>
> memset(res, 0x00, sizeof(struct resource) * ARRAY_SIZE(res));
> @@ -124,32 +131,25 @@ static int dwc2_pci_probe(struct pci_dev *pci,
> ret = platform_device_add_resources(dwc2, res, ARRAY_SIZE(res));
> if (ret) {
> dev_err(dev, "couldn't add resources to dwc2 device\n");
> -   return ret;
> +   goto err;
> }
>
> dwc2->dev.parent = dev;
>
> -   phy = usb_phy_generic_register();
> -   if (IS_ERR(phy)) {
> -   dev_err(dev, "error registering generic PHY (%ld)\n",
> -   PTR_ERR(phy));
> -   return PTR_ERR(phy);
> -   }
> -
> ret = dwc2_pci_quirks(pci, dwc2);
> if (ret)
> goto err;
>
> +   glue = devm_kzalloc(dev, sizeof(*glue), GFP_KERNEL);
> +   if (!glue)
> +   goto err;
> +

Seems you can also remove kfree(glue) ...

> ret = platform_device_add(dwc2);
> if (ret) {
> dev_err(dev, "failed to register dwc2 device\n");
> goto err;
> }
>
> -   glue = kzalloc(sizeof(*glue), GFP_KERNEL);
> -   if (!glue)
> -   return -ENOMEM;
> -
> glue->phy = phy;
> glue->dwc2 = dwc2;
> pci_set_drvdata(pci, glue);
> --
> 1.9.1
>
> --
> 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
--
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 v2 28/37] usb: host: xhci: combine event TRB completion debugging messages

2017-01-24 Thread Janusz Dziedzic 
On 23 January 2017 at 13:20, Mathias Nyman
 wrote:
> From: Felipe Balbi 
>
> If we just provide a helper to convert completion code to string, we can
> combine all debugging messages into a single print.
>
> [keep the old debug messages, for warn and grep -Mathias]
> Signed-off-by: Felipe Balbi 
> Signed-off-by: Mathias Nyman 
> ---
>  drivers/usb/host/xhci.h | 80 
> +
>  1 file changed, 80 insertions(+)
>
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index aa63e38..ebdd920 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1097,6 +1097,86 @@ struct xhci_transfer_event {
>  #define COMP_SECONDARY_BANDWIDTH_ERROR 35
>  #define COMP_SPLIT_TRANSACTION_ERROR   36
>
> +static inline const char *xhci_trb_comp_code_string(u8 status)
> +{

BTW, maybe in the future we should use enum for status
and next:

#define C2S(x) case x: return #x
static inline const char *xhci_trb_comp_code_string(enum
xhci_trb_comp_code code)
{
switch(code) {
C2S(COMP_INVALID);
C2S(COMP_SUCCESS);
...
   default:
return "Unknown!!";
}
}
#undef C2S

After that in dbgmsg/trace we will have same enum and we don't need
translate msg->enum while debugging.

> +   switch (status) {
> +   case COMP_INVALID:
> +   return "Invalid";
> +   case COMP_SUCCESS:
> +   return "Success";
> +   case COMP_DATA_BUFFER_ERROR:
> +   return "Data Buffer Error";
> +   case COMP_BABBLE_DETECTED_ERROR:
> +   return "Babble Detected";
> +   case COMP_USB_TRANSACTION_ERROR:
> +   return "USB Transaction Error";
> +   case COMP_TRB_ERROR:
> +   return "TRB Error";
> +   case COMP_STALL_ERROR:
> +   return "Stall Error";
> +   case COMP_RESOURCE_ERROR:
> +   return "Resource Error";
> +   case COMP_BANDWIDTH_ERROR:
> +   return "Bandwidth Error";
> +   case COMP_NO_SLOTS_AVAILABLE_ERROR:
> +   return "No Slots Available Error";
> +   case COMP_INVALID_STREAM_TYPE_ERROR:
> +   return "Invalid Stream Type Error";
> +   case COMP_SLOT_NOT_ENABLED_ERROR:
> +   return "Slot Not Enabled Error";
> +   case COMP_ENDPOINT_NOT_ENABLED_ERROR:
> +   return "Endpoint Not Enabled Error";
> +   case COMP_SHORT_PACKET:
> +   return "Short Packet";
> +   case COMP_RING_UNDERRUN:
> +   return "Ring Underrun";
> +   case COMP_RING_OVERRUN:
> +   return "Ring Overrun";
> +   case COMP_VF_EVENT_RING_FULL_ERROR:
> +   return "VF Event Ring Full Error";
> +   case COMP_PARAMETER_ERROR:
> +   return "Parameter Error";
> +   case COMP_BANDWIDTH_OVERRUN_ERROR:
> +   return "Bandwidth Overrun Error";
> +   case COMP_CONTEXT_STATE_ERROR:
> +   return "Context State Error";
> +   case COMP_NO_PING_RESPONSE_ERROR:
> +   return "No Ping Response Error";
> +   case COMP_EVENT_RING_FULL_ERROR:
> +   return "Event Ring Full Error";
> +   case COMP_INCOMPATIBLE_DEVICE_ERROR:
> +   return "Incompatible Device Error";
> +   case COMP_MISSED_SERVICE_ERROR:
> +   return "Missed Service Error";
> +   case COMP_COMMAND_RING_STOPPED:
> +   return "Command Ring Stopped";
> +   case COMP_COMMAND_ABORTED:
> +   return "Command Aborted";
> +   case COMP_STOPPED:
> +   return "Stopped";
> +   case COMP_STOPPED_LENGTH_INVALID:
> +   return "Stopped - Length Invalid";
> +   case COMP_STOPPED_SHORT_PACKET:
> +   return "Stopped - Short Packet";
> +   case COMP_MAX_EXIT_LATENCY_TOO_LARGE_ERROR:
> +   return "Max Exit Latency Too Large Error";
> +   case COMP_ISOCH_BUFFER_OVERRUN:
> +   return "Isoch Buffer Overrun";
> +   case COMP_EVENT_LOST_ERROR:
> +   return "Event Lost Error";
> +   case COMP_UNDEFINED_ERROR:
> +   return "Undefined Error";
> +   case COMP_INVALID_STREAM_ID_ERROR:
> +   return "Invalid Stream ID Error";
> +   case COMP_SECONDARY_BANDWIDTH_ERROR:
> +   return "Secondary Bandwidth Error";
> +   case COMP_SPLIT_TRANSACTION_ERROR:
> +   return "Split Transaction Error";
> +   default:
> +   return "Unknown!!";
> +   }
> +}
> +
>  struct xhci_link_trb {
> /* 64-bit segment pointer*/
> __le64 segment_ptr;
> --
> 1.9.1
>
> --
> 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
--
To

Re: [PATCH] usb: dwc3: gadget: Avoid race between dwc3 interrupt handler and irq thread handler

2016-12-28 Thread Janusz Dziedzic 
2016-12-27 12:05 GMT+01:00 Felipe Balbi <ba...@kernel.org>:
> Hi,
>
> Lu Baolu <baolu...@linux.intel.com> writes:
>> On 12/26/2016 04:01 PM, Baolin Wang wrote:
>>> On some platfroms(like x86 platform), when one core is running the USB 
>>> gadget
>>> irq thread handler by dwc3_thread_interrupt(), meanwhile another core also 
>>> can
>>> respond other interrupts from dwc3 controller and modify the event buffer by
>>> dwc3_interrupt() function, that will cause getting the wrong event count in
>>> irq thread handler to make the USB function abnormal.
>>>
>>> We should add spin_lock/unlock() in dwc3_check_event_buf() to avoid this 
>>> race.
>>
>> Why not spin_lock_irq ones? This lock seems to be used in both
>> normal and interrupt threads. Or, I missed anything?
>
> this is top half handler. Interrupts are already disabled.
>
BTW,
We don't use spin_lock in top half handler.
Maybe we should/can switch all spin_lock_irqsave() to simple
spin_lock() in the thread/callbacks?
Or there is a reason to use irqsave() version?

BR
Janusz

> --
> balbi



-- 
Janusz Dziedzic
--
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: dwc3: gadget: Avoid race between dwc3 interrupt handler and irq thread handler

2016-12-28 Thread Janusz Dziedzic 
2016-12-27 13:16 GMT+01:00 Baolin Wang <baolin.w...@linaro.org>:
> Hi,
>
> On 27 December 2016 at 19:11, Felipe Balbi <ba...@kernel.org> wrote:
>>
>> Hi,
>>
>> Baolin Wang <baolin.w...@linaro.org> writes:
>>> Hi,
>>>
>>> On 27 December 2016 at 18:52, Janusz Dziedzic <janusz.dzied...@gmail.com> 
>>> wrote:
>>>> 2016-12-26 9:01 GMT+01:00 Baolin Wang <baolin.w...@linaro.org>:
>>>>> On some platfroms(like x86 platform), when one core is running the USB 
>>>>> gadget
>>>>> irq thread handler by dwc3_thread_interrupt(), meanwhile another core 
>>>>> also can
>>>>> respond other interrupts from dwc3 controller and modify the event buffer 
>>>>> by
>>>>> dwc3_interrupt() function, that will cause getting the wrong event count 
>>>>> in
>>>>> irq thread handler to make the USB function abnormal.
>>>>>
>>>>> We should add spin_lock/unlock() in dwc3_check_event_buf() to avoid this 
>>>>> race.
>>>>>
>>>> Interesting, I always think we mask interrupt in dwc3_interrupt() by 
>>>> setting
>>>> DWC3_GEVNTSIZ_INTMASK
>>>> And unmask interrupt when we end dwc3_thread_interrupt().
>>>>
>>>> So, we shouldn't get any IRQ from HW during dwc3_thread_interrupt(),
>>>> or I miss something?
>>>> Do you have some traces that indicate this masking will not work correctly?
>>>
>>> Yes, but we just masked the interrupts described in DEVTEN register,
>>> and we did not mask all the interrupts, like the endpoint command
>>> complete event, transfer complete event and so on, so we can still get
>>> interrupts.
>>
>> not true, we masked interrupts for the entire event buffer:
>
> Yes, you are right and I missed that. I should reproduce this problem
> and analyse the real reason.
>
>>
>>> static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt)
>>> {
>>>   struct dwc3 *dwc = evt->dwc;
>>>   u32 count;
>>>   u32 reg;
>>>
>>>   if (pm_runtime_suspended(dwc->dev)) {
>>>   pm_runtime_get(dwc->dev);
>>>   disable_irq_nosync(dwc->irq_gadget);
>>>   dwc->pending_events = true;
>>>   return IRQ_HANDLED;
>>>   }
>>>
>>>   count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
>>>   count &= DWC3_GEVNTCOUNT_MASK;
>>>   if (!count)
>>>   return IRQ_NONE;
>>>
>>>   evt->count = count;
>>>   evt->flags |= DWC3_EVENT_PENDING;
>>>
>>>   /* Mask interrupt */
>>>   reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0));
>>>   reg |= DWC3_GEVNTSIZ_INTMASK;
>>
>> See here ?!?
>>
>>>   dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0), reg);
>>>
>>>   return IRQ_WAKE_THREAD;
>>> }
>>
>>>> BTW, what value you get when problem occured, 0xFFFC?
>>>
>>> Yes, something like this, the event count become huge.
>>
Probably you have little bit different code than current community
version (depends how your PM works).

This is possible when we write:
dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), 0);
And after that
dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), 4);

After that we will get 0xFFFC (-4).

Possible races:
1) dwc3_event_buffers_setup/dwc3_event_buffers_cleanup - write 0
2) dwc3_thread - write 4

While [1] could be called in PM work or UM context (init in Android
case) spin_lock_irqsave() will only disable local irqs and still we
could get IRQ on different core, next update evt->count and run
thread...

So, seems your patch will solve this.

I am not sure this problem could be also visible in community current version.

Anyway, thanks for handling this.

BR
Janusz
>> please send us tracepoint data. You probably need to compress
>> it. Something like 256k of trace data is probably enough, so:
>>
>> # mkdir -p /t
>> # mount -t tracefs none /t
>> # cd /t
>> # echo 256 > buffer_size_kb
>> # echo 1 > events/dwc3/enable
>> # echo 0 > events/dwc3/dwc3_readl/enable
>> # echo 0 > events/dwc3/dwc3_writel/enable
>>
>> (reproduce)
>>
>> # cp /t/trace /path/to/non-volatile/media/trace.txt
>
> Okay, I try to do that. Thanks.
>
> --
> Baolin.wang
> Best Regards



-- 
Janusz Dziedzic
--
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: dwc3: gadget: Avoid race between dwc3 interrupt handler and irq thread handler

2016-12-27 Thread Janusz Dziedzic 
2016-12-26 9:01 GMT+01:00 Baolin Wang <baolin.w...@linaro.org>:
> On some platfroms(like x86 platform), when one core is running the USB gadget
> irq thread handler by dwc3_thread_interrupt(), meanwhile another core also can
> respond other interrupts from dwc3 controller and modify the event buffer by
> dwc3_interrupt() function, that will cause getting the wrong event count in
> irq thread handler to make the USB function abnormal.
>
> We should add spin_lock/unlock() in dwc3_check_event_buf() to avoid this race.
>
Interesting, I always think we mask interrupt in dwc3_interrupt() by setting
DWC3_GEVNTSIZ_INTMASK
And unmask interrupt when we end dwc3_thread_interrupt().

So, we shouldn't get any IRQ from HW during dwc3_thread_interrupt(),
or I miss something?
Do you have some traces that indicate this masking will not work correctly?

BTW, what value you get when problem occured, 0xFFFC?

BR
Janusz

> Signed-off-by: Baolin Wang <baolin.w...@linaro.org>
> ---
>  drivers/usb/dwc3/gadget.c |6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 6785595..1a1e1f4 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2894,10 +2894,13 @@ static irqreturn_t dwc3_check_event_buf(struct 
> dwc3_event_buffer *evt)
> return IRQ_HANDLED;
> }
>
> +   spin_lock(>lock);
> count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
> count &= DWC3_GEVNTCOUNT_MASK;
> -   if (!count)
> +   if (!count) {
> +   spin_unlock(>lock);
> return IRQ_NONE;
> +   }
>
> evt->count = count;
> evt->flags |= DWC3_EVENT_PENDING;
> @@ -2914,6 +2917,7 @@ static irqreturn_t dwc3_check_event_buf(struct 
> dwc3_event_buffer *evt)
> memcpy(evt->cache, evt->buf, count - amount);
>
> dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count);
> +   spin_unlock(>lock);
>
> return IRQ_WAKE_THREAD;
>  }
> --
> 1.7.9.5
>
> --
> 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



-- 
Janusz Dziedzic
--
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: skip interrupt when ep disabled

2016-12-08 Thread Janusz Dziedzic 
In case EP disabled pass only EPCPLT command
to be handled. In other case we could hit
Bug like below.

BUG: unable to handle kernel NULL pointer dereference at 0003
IP:
[] dwc3_thread_interrupt+0x11c8/0x1790

while dep->endpoint.desc is NULL.

Signed-off-by: Janusz Dziedzic <januszx.dzied...@linux.intel.com>
---
 drivers/usb/dwc3/gadget.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 6785595..d9d20b6 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2232,9 +2232,14 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
 
dep = dwc->eps[epnum];
 
-   if (!(dep->flags & DWC3_EP_ENABLED) &&
-   !(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
-   return;
+   if (!(dep->flags & DWC3_EP_ENABLED)) {
+   if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
+   return;
+
+   /* Handle only EPCMDCMPLT when EP disabled */
+   if (event->endpoint_event != DWC3_DEPEVT_EPCMDCMPLT)
+   return;
+   }
 
if (epnum == 0 || epnum == 1) {
dwc3_ep0_interrupt(dwc, event);
-- 
1.9.1

--
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 3/4] usb: dwc3: gadget: use evt->cache for processing events

2016-11-15 Thread Janusz Dziedzic 
On 15 November 2016 at 12:23, Felipe Balbi  wrote:
> From: John Youn 
>
> Let's start copying events from evt->buf to
> evt->cache and use evt->cache for processing events.
>
> A follow-up patch will be added to clear events in
> the top-half handler in order to bring IRQ line low
> as soon as possible.
>
> Signed-off-by: John Youn 
> Signed-off-by: Felipe Balbi 
> ---
>  drivers/usb/dwc3/gadget.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 304653fd9223..0481cb7d7142 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2821,7 +2821,7 @@ static irqreturn_t dwc3_process_event_buf(struct 
> dwc3_event_buffer *evt)
> while (left > 0) {
> union dwc3_event event;
>
> -   event.raw = *(u32 *) (evt->buf + evt->lpos);
> +   event.raw = *(u32 *) (evt->cache + evt->lpos);
>
> dwc3_process_event_entry(dwc, );
>
> @@ -2869,6 +2869,7 @@ static irqreturn_t dwc3_thread_interrupt(int irq, void 
> *_evt)
>  static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt)
>  {
> struct dwc3 *dwc = evt->dwc;
> +   u32 amount;
> u32 count;
> u32 reg;
>
> @@ -2892,6 +2893,12 @@ static irqreturn_t dwc3_check_event_buf(struct 
> dwc3_event_buffer *evt)
> reg |= DWC3_GEVNTSIZ_INTMASK;
> dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0), reg);
>
> +   amount = min(count, evt->length - evt->lpos);
> +   memcpy(evt->cache + evt->lpos, evt->buf + evt->lpos, amount);
> +
Why not just:
memcpy(evt->cache, evt->buf + evt->lpos, count)
And always handle evt->cache[0 ... count] in thread_interrupt.

> +   if (amount < count)
> +   memcpy(evt->cache, evt->buf, count - amount);
> +
> return IRQ_WAKE_THREAD;
>  }
>
> --
> 2.10.1
>
> --
> 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
--
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 v2 3/5] usb: dwc3: gadget: Write the event count in interrupt

2016-11-09 Thread Janusz Dziedzic 
On 9 November 2016 at 09:05, Felipe Balbi  wrote:
>
> Hi,
>
> John Youn  writes:
>>> +   dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count);
>>> +
> After that evt->buf[lpos, lpos + count] seems goes back to HW, so
> thread should not rely on this?
>
> Or I miss something?

 Hi,

 Yes, you're right. That's a possibility and to be safe we can cache
 those.

 We're relying on the same mechanism that keeps the event buffer from
 becoming full. Since it is just as (un)likely a possibility. That's
 why you must size the event buffer appropriately taking into account
 your system's latency, etc.

 And if the event buffer becomes full, that indicates something really
 wrong happened in the controller. You shouldn't be getting 100's of
 events at a time.

 But yes, we can address the overwriting issue.

 Either:

 1) Cache all incoming events. Requires double the event buffer space.

 2) Cache just one event and write back only '4' during hard
interrupt. Then in threaded interrupt read the one event from
cache, and process the remaining events from buffer as before.
Doesn't require a large cache, but a bit messier.

 Any other thoughts or ideas?
>>>
>>> do you really need to clear at least one event for this?
>>>
>>
>> Unfortunately yes. That's how the workaround works. We need to write
>> this during IMOD to de-assert the interrupt since the mask bit doesn't
>> work.
>
> okay. Then we should cache and clear a single event.
>
Cache all incoming events looks better for me, while we don't need
care of Vendor Test Event (12Bytes) here - and we will always handle
this correctly and can add simple implementation for that in bottom
half.
In case of single event cache, VTE handling will be much harder/ugly
... - while we have to check if cache 4 or 12 bytes.
Anyway is the VTE case at all? We don't support this currently and
don't have an issues ...

BR
Janusz

>> We could do this only for 3.00a as well.
>
> if we do this only for 3.00a then the code will look odd :-) It
> shouldn't cause any problems for any other revisions
>
> --
> balbi
--
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 5/5] usb: dwc3: warn on once when no trbs

2016-11-09 Thread Janusz Dziedzic 
Seems last time we hit few issues where
we get trb_left = 0, mainly because of
HWO bit still set in previous TRB.
Add warn on once to catch/fix such
problems much faster.

Signed-off-by: Janusz Dziedzic <januszx.dzied...@linux.intel.com>
---
 drivers/usb/dwc3/gadget.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index e40d58e..153491e 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -938,6 +938,7 @@ static struct dwc3_trb *dwc3_ep_prev_trb(struct dwc3_ep 
*dep, u8 index)
 static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep)
 {
struct dwc3_trb *tmp;
+   struct dwc3 *dwc = dep->dwc;
u8  trbs_left;
 
/*
@@ -949,7 +950,8 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep)
 */
if (dep->trb_enqueue == dep->trb_dequeue) {
tmp = dwc3_ep_prev_trb(dep, dep->trb_enqueue);
-   if (tmp->ctrl & DWC3_TRB_CTRL_HWO)
+   if (dev_WARN_ONCE(dwc->dev, tmp->ctrl & DWC3_TRB_CTRL_HWO,
+ "%s No TRBS left\n", dep->name))
return 0;
 
return DWC3_TRB_NUM - 1;
-- 
1.9.1

--
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/5] usb: dwc3: decrement queued_requests

2016-11-09 Thread Janusz Dziedzic 
In case we will fail to STARTTRANSFER we should
also decrement queued_requests.

Signed-off-by: Janusz Dziedzic <januszx.dzied...@linux.intel.com>
---
 drivers/usb/dwc3/gadget.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index a9c1d75..840e312 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1085,6 +1085,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep 
*dep, u16 cmd_param)
 * here and stop, unmap, free and del each of the linked
 * requests instead of what we do now.
 */
+   dep->queued_requests--;
dwc3_gadget_giveback(dep, req, ret);
return ret;
}
-- 
1.9.1

--
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/5] usb: dwc3: clean TRB if STARTTRANSFER fail

2016-11-09 Thread Janusz Dziedzic 
In case STARTTRANSFER will fail, clean TRB.
Seems HW in such case don't clean HWO bit.
So, without this cleanup prev_trb still have
HWO bit set.

In my case (without patch), after first START failed:
- dep->enqueue == 1
- dep->dequeue == 1
- prev_trb still have HWO set
- left_trb() == 0
No way to send more data.

Signed-off-by: Janusz Dziedzic <januszx.dzied...@linux.intel.com>
---
 drivers/usb/dwc3/gadget.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 840e312..19bea3b 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1085,6 +1085,8 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep 
*dep, u16 cmd_param)
 * here and stop, unmap, free and del each of the linked
 * requests instead of what we do now.
 */
+   if (req->trb)
+   memset(req->trb, 0, sizeof(struct dwc3_trb));
dep->queued_requests--;
dwc3_gadget_giveback(dep, req, ret);
return ret;
-- 
1.9.1

--
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 4/5] usb: dwc3: isoc clean DWC3_EP_PENDING_REQUEST flag

2016-11-09 Thread Janusz Dziedzic 
After we kick_transfer we should clean
DWC3_EP_PENDING_REQUEST endpoint flag.

Signed-off-by: Janusz Dziedzic <januszx.dzied...@linux.intel.com>
---
 drivers/usb/dwc3/gadget.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 0cd98c0..e40d58e 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1196,6 +1196,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, 
struct dwc3_request *req)
 
cur_uf = __dwc3_gadget_get_frame(dwc);
__dwc3_gadget_start_isoc(dwc, dep, cur_uf);
+   dep->flags &= ~DWC3_EP_PENDING_REQUEST;
}
}
return 0;
-- 
1.9.1

--
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/5] usb: dwc3: fix post-increment

2016-11-09 Thread Janusz Dziedzic 
Use pre-increment and set -ETIMEDOUT correctly.

Signed-off-by: Janusz Dziedzic <januszx.dzied...@linux.intel.com>
---
 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 19bea3b..0cd98c0 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -217,7 +217,7 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc, 
unsigned cmd, u32 param)
ret = -EINVAL;
break;
}
-   } while (timeout--);
+   } while (--timeout);
 
if (!timeout) {
ret = -ETIMEDOUT;
-- 
1.9.1

--
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 v2 3/5] usb: dwc3: gadget: Write the event count in interrupt

2016-11-04 Thread Janusz Dziedzic 
On 4 November 2016 at 07:41, Janusz Dziedzic <janusz.dzied...@tieto.com> wrote:
> On 4 November 2016 at 02:31, John Youn <johny...@synopsys.com> wrote:
>>
>> Since we are saving the event count and handling the events in the
>> threaded interrupt handler, we can write and clear out the eventcount in
>> the hard interrupt handler itself.
>>
>> This behavior will be required for IP 3.00a cores that need to use
>> interrupt moderation as a workaround for an RTL issue were the interrupt
>> line cannot be masked between the hard/soft interrupt handler.
>>
>> Signed-off-by: John Youn <johny...@synopsys.com>
>> ---
>>  drivers/usb/dwc3/gadget.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index a9c1d75..ac9eb39 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2877,8 +2877,6 @@ static irqreturn_t dwc3_process_event_buf(struct 
>> dwc3_event_buffer *evt)
>>  */
>> evt->lpos = (evt->lpos + 4) % DWC3_EVENT_BUFFERS_SIZE;
>> left -= 4;
>> -
>> -   dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), 4);
>> }
>>
>> evt->count = 0;
>> @@ -2928,6 +2926,8 @@ static irqreturn_t dwc3_check_event_buf(struct 
>> dwc3_event_buffer *evt)
>> evt->count = count;
>> evt->flags |= DWC3_EVENT_PENDING;
>>
Shouldn't you cache (somewhere):
u32 events[evt->buf + evt->lpos  evt->buf + evt->lpos + count]

before you will giveback this to  HW (to be sure HW will not overwrite
this in case lot of events)?

>> +   dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count);
>> +
After that evt->buf[lpos, lpos + count] seems goes back to HW, so
thread should not rely on this?

Or I miss something?

>> /* Mask interrupt */
>> reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0));
>> reg |= DWC3_GEVNTSIZ_INTMASK;
>> --
>
> Hello,
>
> Are we sure this will work fine with 2.60a?
>
> Some time ago I have similar code (introduce event_pop) and move
> dwc3_write(dwc->regs, DWC3_GEVNTCOUNT(0), 4)
> before
> dwc3_process_event_entry()
>
> And have some issues ...
> Didn't work correctly in my case.
>
> BR
> Janusz
--
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 v2 3/5] usb: dwc3: gadget: Write the event count in interrupt

2016-11-04 Thread Janusz Dziedzic 
On 4 November 2016 at 02:31, John Youn  wrote:
>
> Since we are saving the event count and handling the events in the
> threaded interrupt handler, we can write and clear out the eventcount in
> the hard interrupt handler itself.
>
> This behavior will be required for IP 3.00a cores that need to use
> interrupt moderation as a workaround for an RTL issue were the interrupt
> line cannot be masked between the hard/soft interrupt handler.
>
> Signed-off-by: John Youn 
> ---
>  drivers/usb/dwc3/gadget.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index a9c1d75..ac9eb39 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2877,8 +2877,6 @@ static irqreturn_t dwc3_process_event_buf(struct 
> dwc3_event_buffer *evt)
>  */
> evt->lpos = (evt->lpos + 4) % DWC3_EVENT_BUFFERS_SIZE;
> left -= 4;
> -
> -   dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), 4);
> }
>
> evt->count = 0;
> @@ -2928,6 +2926,8 @@ static irqreturn_t dwc3_check_event_buf(struct 
> dwc3_event_buffer *evt)
> evt->count = count;
> evt->flags |= DWC3_EVENT_PENDING;
>
> +   dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count);
> +
> /* Mask interrupt */
> reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0));
> reg |= DWC3_GEVNTSIZ_INTMASK;
> --

Hello,

Are we sure this will work fine with 2.60a?

Some time ago I have similar code (introduce event_pop) and move
dwc3_write(dwc->regs, DWC3_GEVNTCOUNT(0), 4)
before
dwc3_process_event_entry()

And have some issues ...
Didn't work correctly in my case.

BR
Janusz
--
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: gadget: f_hid add super speed support

2016-11-03 Thread Janusz Dziedzic 
Add super speed descriptors to f_hid.

Signed-off-by: Janusz Dziedzic <januszx.dzied...@linux.intel.com>
---
 drivers/usb/gadget/function/f_hid.c | 67 -
 1 file changed, 66 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_hid.c 
b/drivers/usb/gadget/function/f_hid.c
index e2966f8..7abd70b 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -98,6 +98,60 @@ static inline struct f_hidg *func_to_hidg(struct 
usb_function *f)
/*.desc[0].wDescriptorLenght= DYNAMIC */
 };
 
+/* Super-Speed Support */
+
+static struct usb_endpoint_descriptor hidg_ss_in_ep_desc = {
+   .bLength= USB_DT_ENDPOINT_SIZE,
+   .bDescriptorType= USB_DT_ENDPOINT,
+   .bEndpointAddress   = USB_DIR_IN,
+   .bmAttributes   = USB_ENDPOINT_XFER_INT,
+   /*.wMaxPacketSize   = DYNAMIC */
+   .bInterval  = 4, /* FIXME: Add this field in the
+ * HID gadget configuration?
+ * (struct hidg_func_descriptor)
+ */
+};
+
+static struct usb_ss_ep_comp_descriptor hidg_ss_in_comp_desc = {
+   .bLength= sizeof(hidg_ss_in_comp_desc),
+   .bDescriptorType= USB_DT_SS_ENDPOINT_COMP,
+
+   /* .bMaxBurst   = 0, */
+   /* .bmAttributes= 0, */
+   /* .wBytesPerInterval   = DYNAMIC */
+};
+
+static struct usb_endpoint_descriptor hidg_ss_out_ep_desc = {
+   .bLength= USB_DT_ENDPOINT_SIZE,
+   .bDescriptorType= USB_DT_ENDPOINT,
+   .bEndpointAddress   = USB_DIR_OUT,
+   .bmAttributes   = USB_ENDPOINT_XFER_INT,
+   /*.wMaxPacketSize   = DYNAMIC */
+   .bInterval  = 4, /* FIXME: Add this field in the
+ * HID gadget configuration?
+ * (struct hidg_func_descriptor)
+ */
+};
+
+static struct usb_ss_ep_comp_descriptor hidg_ss_out_comp_desc = {
+   .bLength= sizeof(hidg_ss_out_comp_desc),
+   .bDescriptorType= USB_DT_SS_ENDPOINT_COMP,
+
+   /* .bMaxBurst   = 0, */
+   /* .bmAttributes= 0, */
+   /* .wBytesPerInterval   = DYNAMIC */
+};
+
+static struct usb_descriptor_header *hidg_ss_descriptors[] = {
+   (struct usb_descriptor_header *)_interface_desc,
+   (struct usb_descriptor_header *)_desc,
+   (struct usb_descriptor_header *)_ss_in_ep_desc,
+   (struct usb_descriptor_header *)_ss_in_comp_desc,
+   (struct usb_descriptor_header *)_ss_out_ep_desc,
+   (struct usb_descriptor_header *)_ss_out_comp_desc,
+   NULL,
+};
+
 /* High-Speed Support */
 
 static struct usb_endpoint_descriptor hidg_hs_in_ep_desc = {
@@ -624,8 +678,14 @@ static int hidg_bind(struct usb_configuration *c, struct 
usb_function *f)
/* set descriptor dynamic values */
hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;
+   hidg_ss_in_ep_desc.wMaxPacketSize = cpu_to_le16(hidg->report_length);
+   hidg_ss_in_comp_desc.wBytesPerInterval =
+   cpu_to_le16(hidg->report_length);
hidg_hs_in_ep_desc.wMaxPacketSize = cpu_to_le16(hidg->report_length);
hidg_fs_in_ep_desc.wMaxPacketSize = cpu_to_le16(hidg->report_length);
+   hidg_ss_out_ep_desc.wMaxPacketSize = cpu_to_le16(hidg->report_length);
+   hidg_ss_out_comp_desc.wBytesPerInterval =
+   cpu_to_le16(hidg->report_length);
hidg_hs_out_ep_desc.wMaxPacketSize = cpu_to_le16(hidg->report_length);
hidg_fs_out_ep_desc.wMaxPacketSize = cpu_to_le16(hidg->report_length);
/*
@@ -641,8 +701,13 @@ static int hidg_bind(struct usb_configuration *c, struct 
usb_function *f)
hidg_hs_out_ep_desc.bEndpointAddress =
hidg_fs_out_ep_desc.bEndpointAddress;
 
+   hidg_ss_in_ep_desc.bEndpointAddress =
+   hidg_fs_in_ep_desc.bEndpointAddress;
+   hidg_ss_out_ep_desc.bEndpointAddress =
+   hidg_fs_out_ep_desc.bEndpointAddress;
+
status = usb_assign_descriptors(f, hidg_fs_descriptors,
-   hidg_hs_descriptors, NULL, NULL);
+   hidg_hs_descriptors, hidg_ss_descriptors, NULL);
if (status)
goto fail;
 
-- 
1.9.1

--
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: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically

2016-10-13 Thread Janusz Dziedzic 
On 13 October 2016 at 12:41, Baolin Wang  wrote:
> On 13 October 2016 at 17:49, Felipe Balbi  wrote:
>>
>> Hi,
>>
>> Baolin Wang  writes:
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 1783406..ca2ae5b 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -241,6 +241,9 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, 
> unsigned cmd,
>   int susphy = false;
>   int ret = -EINVAL;
>
> + if (!dwc->pullups_connected)
> + return -ESHUTDOWN;
> +
>>
>> you skip trace_dwc3_gadget_ep_cmd()
>
> Yes, we did not need trace here since we did not send out the command.
>
 What in such case: enumeration will not work and this will be because
 this ESHUTDOWN or wrong pullups_connected usage. Without a trace you
 will not know where the problem is.
 In my opinion this trace could be useful.
>>>
>>> We have returned the '-ESHUTDOWN' error number for user to know what
>>> happened.
>>
>> No, this is actually not good and Janusz has a very valid point
>> here. When we're debugging something in dwc3, we want to rely on
>> tracepoints to tell us what's going on. I don't want to have to add more
>> debugging messages to print out that ESHUTDOWN error just so I can
>> figure out what's going on. You should be patching this in a way that
>> the tracepoint is still printed out properly.
>
> Fine. Will fix this in next version.
>

BTW, did you test this patch with device mode?
Seems in my setup this fail - DWC3_DEPCMD_SETEPCONFIG for ep0out and
gadget_start() failed (enumeration fail).
Don't we need to queue ep0 cmds before pullup?

BR
Janusz
>>
>> Keep in mind that you should be setting ret to -ESHUTDOWN and make sure
>> the trace is printed. Same patch should, then, patch trace.h to handle
>> -ESHUTDOWN as an error case, i.e. following hunk should be added:
>>
>> diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
>> index d93780e84f07..70b783f0507d 100644
>> --- a/drivers/usb/dwc3/debug.h
>> +++ b/drivers/usb/dwc3/debug.h
>> @@ -319,6 +319,8 @@ static inline const char *dwc3_ep_cmd_status_string(int 
>> status)
>> switch (status) {
>> case -ETIMEDOUT:
>> return "Timed Out";
>> +   case -ESHUTDOWN:
>> +   return "Shut Down";
>> case 0:
>> return "Successful";
>> case DEPEVT_TRANSFER_NO_RESOURCE:
>>
>> --
>> balbi
>
>
>
> --
> Baolin.wang
> Best Regards
--
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: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically

2016-10-13 Thread Janusz Dziedzic 
On 13 October 2016 at 10:21, Baolin Wang <baolin.w...@linaro.org> wrote:
> Hi,
>
> On 13 October 2016 at 16:16, Janusz Dziedzic <janusz.dzied...@tieto.com> 
> wrote:
>> On 13 October 2016 at 09:37, Baolin Wang <baolin.w...@linaro.org> wrote:
>>> Hi,
>>>
>>> On 13 October 2016 at 15:06, Felipe Balbi <ba...@kernel.org> wrote:
>>>>
>>>> Hi,
>>>>
>>>> Baolin Wang <baolin.w...@linaro.org> writes:
>>>>> When system has stpped the gadget, we should avoid queuing any requests
>>>>
>>>> queueing is *not* a problem. Starting is. In fact, that's what your
>>>> patch is doing.
>>>
>>> OK.
>>>
>>>>
>>>>> which will cause tranfer failed. Thus adding some disconnect checking to
>>>>^^^
>>>>transfer
>>>
>>> Sorry for spelling mistake, will fix it.
>>>
>>>>
>>>>> avoid this situation.
>>>>>
>>>>> Signed-off-by: Baolin Wang <baolin.w...@linaro.org>
>>>>> ---
>>>>> Changes since v2:
>>>>>  - Move disconnect checking into dwc3_send_gadget_ep_cmd().
>>>>>  - Rename completion name and issue complete() at one place.
>>>>>  - Move completion initialization into dwc3_gadget_init().
>>>>>
>>>>> Changes since v1:
>>>>>  - Split into 2 separate ptaches.
>>>>>  - Choose complete mechanism instead of polling.
>>>>> ---
>>>>>  drivers/usb/dwc3/gadget.c |3 +++
>>>>>  1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>>> index 1783406..ca2ae5b 100644
>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>> @@ -241,6 +241,9 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, 
>>>>> unsigned cmd,
>>>>>   int susphy = false;
>>>>>   int ret = -EINVAL;
>>>>>
>>>>> + if (!dwc->pullups_connected)
>>>>> + return -ESHUTDOWN;
>>>>> +
>>
>> you skip trace_dwc3_gadget_ep_cmd()
>
> Yes, we did not need trace here since we did not send out the command.
>
What in such case: enumeration will not work and this will be because
this ESHUTDOWN or wrong pullups_connected usage. Without a trace you
will not know where the problem is.
In my opinion this trace could be useful.

BR
Janusz

>>
>> BR
>> Janusz
>>
>>>>>   /*
>>>>>* Synopsys Databook 2.60a states, on section 6.3.2.5.[1-8], that if
>>>>>* we're issuing an endpoint command, we must check if
>>>>> --
>>>>> 1.7.9.5
>>>>>
>>>>> --
>>>>> 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
>>>>
>>>> --
>>>> balbi
>>>
>>>
>>>
>>> --
>>> Baolin.wang
>>> Best Regards
>>> --
>>> 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
>
>
>
> --
> Baolin.wang
> Best Regards
--
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: [RESEND PATCH v3 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically

2016-10-13 Thread Janusz Dziedzic 
On 13 October 2016 at 09:37, Baolin Wang  wrote:
> Hi,
>
> On 13 October 2016 at 15:06, Felipe Balbi  wrote:
>>
>> Hi,
>>
>> Baolin Wang  writes:
>>> When system has stpped the gadget, we should avoid queuing any requests
>>
>> queueing is *not* a problem. Starting is. In fact, that's what your
>> patch is doing.
>
> OK.
>
>>
>>> which will cause tranfer failed. Thus adding some disconnect checking to
>>^^^
>>transfer
>
> Sorry for spelling mistake, will fix it.
>
>>
>>> avoid this situation.
>>>
>>> Signed-off-by: Baolin Wang 
>>> ---
>>> Changes since v2:
>>>  - Move disconnect checking into dwc3_send_gadget_ep_cmd().
>>>  - Rename completion name and issue complete() at one place.
>>>  - Move completion initialization into dwc3_gadget_init().
>>>
>>> Changes since v1:
>>>  - Split into 2 separate ptaches.
>>>  - Choose complete mechanism instead of polling.
>>> ---
>>>  drivers/usb/dwc3/gadget.c |3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 1783406..ca2ae5b 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -241,6 +241,9 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, 
>>> unsigned cmd,
>>>   int susphy = false;
>>>   int ret = -EINVAL;
>>>
>>> + if (!dwc->pullups_connected)
>>> + return -ESHUTDOWN;
>>> +

you skip trace_dwc3_gadget_ep_cmd()

BR
Janusz

>>>   /*
>>>* Synopsys Databook 2.60a states, on section 6.3.2.5.[1-8], that if
>>>* we're issuing an endpoint command, we must check if
>>> --
>>> 1.7.9.5
>>>
>>> --
>>> 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
>>
>> --
>> balbi
>
>
>
> --
> Baolin.wang
> Best Regards
> --
> 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
--
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 11/22] usb: dwc3: gadget: clear LST from previous TRB on Update Transfer

2016-05-27 Thread Janusz Dziedzic 
On 19 May 2016 at 09:08, Felipe Balbi  wrote:
>
> Hi,
>
> Paul Zimmerman  writes:
>> Felipe Balbi  writes:
>>
>>> If we're going to issue a Update Transfer command,
>>> let's clear LST bit from previous TRB. This will let
>>> us continue processing TRBs and convert previous IRQ
>>> into XferInProgress, instead of XferComplete.
>>>
>>> Signed-off-by: Felipe Balbi 
>>> ---
>>>  drivers/usb/dwc3/gadget.c | 32 +++-
>>>  1 file changed, 23 insertions(+), 9 deletions(-)
>>
>> < snip >
>>
>>> +/*
>>> + * When trying to issue Update Transfer, we can remove LST bit from
>>> + * previous TRB and avoid a XferComplete
>>> + */
>>> +if (!starting) {
>>> +trb = >trb_pool[dep->trb_enqueue - 1];
>>> +if (trb->ctrl & DWC3_TRB_CTRL_HWO)
>>> +trb->ctrl &= ~DWC3_TRB_CTRL_LST;
>>
>> Hi Felipe,
>>
>> This violates the DWC USB3 programming model. Once the HWO bit has been set
>> and the transfer started, the host is not allowed to modify any of the
>> fields in the TRB until the controller clears the HWO bit, or the transfer
>> completes or is halted.
>

With this patch and rndis interface and highspeed I have (tested using iperf):
275Mbps for TCP for both directions (305Mbps for UDP).

Without patch:
145Mbps for TCP

This is quite interesting, while I understand we violate programming
model here, but still have correct TCP traffic (also during MTP tests
didn't hit any issue)/
And the improvement is also nice. Maybe HW should add something like
APPEND_TRB which will set/clear this flag
correctly in the HW, while HW know if this is already safe.
This mean we still can improve dwc3 driver and get similar TP we have
with this "wrong" patch.

BR
Janusz

> oh that's right. Just found it on section 8.2.3.2. I'll drop this patch
> from the queue. Thanks for the note :-)
>
> Hope you're doing ok, Paul
>
> cheers
>
> --
> balbi
--
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