Re: usb_ep_{dis,en}able() calling context (was: Re: [PATCH 2/3] usb: dwc3: gadget: wait for End Transfer to complete)

[Baolin Wang]

Hi,

On 19 October 2016 at 18:09, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>>
>> We should not check the DWC3_EP_ENABLED flag, since we will clear all
>> flags in __dwc3_gadget_ep_disable() after setting
>> DWC3_EP_END_TRANSFER_PENDING flags in dwc3_stop_active_transfer().
>
> good catch.
>
>>
>>> +
>>> +   if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>>> +   continue;
>>
>> Ditto.
>
> yeah, END_TRANSFER_PENDING should only be cleared by command completion,
> so ep_disable needs to be patched.
>
>>> +
>>> +   wait_event_lock_irq(dep->wait_end_transfer,
>>> +   !(dep->flags & 
>>> DWC3_EP_END_TRANSFER_PENDING),
>>> +   dwc->lock);
>>> +   }
>>>  }
>>>
>>>  static int dwc3_gadget_stop(struct usb_gadget *g)
>>> @@ -2171,6 +2189,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
>>>  {
>>> struct dwc3_ep  *dep;
>>> u8  epnum = event->endpoint_number;
>>> +   u8  cmd;
>>
>> Here we should add below modification, or the event can not be handled.
>>
>> - if (!(dep->flags & DWC3_EP_ENABLED))
>> + if (!(dep->flags & DWC3_EP_ENABLED) &&
>> + !(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>> return;
>
> this seems unnecessary to me. The only interrupt with a disabled
> endpoint should be due to END_TRANSFER cmd completion. In fact, I'm now
> thinking that maybe usb_ep_disable() should also block until EndTransfer
> completes.
>
> This would means replacing wake_up() with wake_up_all()
>
>>> I'll run some tests with this in a couple hours.
>>
>> I would like to send out new version based on my original patch
>> according to your suggestion, or you can send out new version I can
>> help to test. Thanks.
>
> From ce24ab50d57a18287a99ea776e9bdc7d5cfd282c Mon Sep 17 00:00:00 2001
> From: Baolin Wang 
> Date: Thu, 13 Oct 2016 14:09:47 +0300
> Subject: [PATCH] usb: dwc3: gadget: wait for End Transfer to complete
>
> Instead of just delaying for 100us, we should
> actually wait for End Transfer Command Complete
> interrupt before moving on. Note that this should
> only be done if we're dealing with one of the core
> revisions that actually require the interrupt before
> moving on.
>
> [ felipe.ba...@linux.intel.com: minor improvements ]
>
> NYET-Signed-off-by: Baolin Wang 
> Signed-off-by: Felipe Balbi 
> ---
>  drivers/usb/dwc3/core.h   |  8 
>  drivers/usb/dwc3/gadget.c | 40 +---
>  2 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 5fc437021ac7..0cb3b78d28b7 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include 
>  #include 
> @@ -505,6 +506,7 @@ struct dwc3_event_buffer {
>   * @endpoint: usb endpoint
>   * @pending_list: list of pending requests for this endpoint
>   * @started_list: list of started requests on this endpoint
> + * @wait_end_transfer: wait_queue_head_t for waiting on End Transfer complete
>   * @lock: spinlock for endpoint request queue traversal
>   * @regs: pointer to first endpoint register
>   * @trb_pool: array of transaction buffers
> @@ -530,6 +532,8 @@ struct dwc3_ep {
> struct list_headpending_list;
> struct list_headstarted_list;
>
> +   wait_queue_head_t   wait_end_transfer;
> +
> spinlock_t  lock;
> void __iomem*regs;
>
> @@ -546,6 +550,7 @@ struct dwc3_ep {
>  #define DWC3_EP_BUSY   (1 << 4)
>  #define DWC3_EP_PENDING_REQUEST(1 << 5)
>  #define DWC3_EP_MISSED_ISOC(1 << 6)
> +#define DWC3_EP_END_TRANSFER_PENDING (1 << 7)
>
> /* This last one is specific to EP0 */
>  #define DWC3_EP0_DIR_IN(1 << 31)
> @@ -1050,6 +1055,9 @@ struct dwc3_event_depevt {
>  #define DEPEVT_TRANSFER_BUS_EXPIRY 2
>
> u32 parameters:16;
> +
> +/* For Command Complete Events */
> +#define DEPEVT_PARAMETER_CMD(n) (((n) & (0xf << 8)) >> 8)
>  } __packed;
>
>  /**
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 3b53a5714df4..bc985f8571c6 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -598,6 +598,8 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
> reg |= DWC3_DALEPENA_EP(dep->number);
> dwc3_writel(dwc->regs, DWC3_DALEPENA, reg);
>
> +   init_waitqueue_head(>wait_end_transfer);
> +
> if (usb_endpoint_xfer_control(desc))
> return 0;
>
> @@ -772,6 +774,10 @@ static int dwc3_gadget_ep_disable(struct usb_ep *ep)
> dep->name))
> return 0;
>
> 

Re: usb_ep_{dis,en}able() calling context (was: Re: [PATCH 2/3] usb: dwc3: gadget: wait for End Transfer to complete)

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>> Baolin Wang  writes:
>>> Sure. The problem I met was, when we change the USB function with
>>> configfs frequently, sometimes it will hang the system to crash. The
>>> reason is,  we will start end transfer command when disable the
>>> endpoint, but sometimes the end transfer command complete event comes
>>> after we have freed the gadget irq (since the xHCI will share the same
>>> interrupt number with gadget, thus when free the gadget irq, it will
>>> not shutdown this gadget irq line), it will trigger the interrupt line
>>> all the time.
>>
>> okay, thanks for describing it again. Another option would be to only
>> wait_for_completion() before calling free_irq() is any endpoint has
>> END_TRANSFER_PENDING flag set. Something like below:
>
> I tested below patch, but it failed and I still met the kernel crash
> with changing USB function.
>
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 5fc437021ac7..0cb3b78d28b7 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -26,6 +26,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include 
>>  #include 
>> @@ -505,6 +506,7 @@ struct dwc3_event_buffer {
>>   * @endpoint: usb endpoint
>>   * @pending_list: list of pending requests for this endpoint
>>   * @started_list: list of started requests on this endpoint
>> + * @wait_end_transfer: wait_queue_head_t for waiting on End Transfer 
>> complete
>>   * @lock: spinlock for endpoint request queue traversal
>>   * @regs: pointer to first endpoint register
>>   * @trb_pool: array of transaction buffers
>> @@ -530,6 +532,8 @@ struct dwc3_ep {
>> struct list_headpending_list;
>> struct list_headstarted_list;
>>
>> +   wait_queue_head_t   wait_end_transfer;
>> +
>> spinlock_t  lock;
>> void __iomem*regs;
>>
>> @@ -546,6 +550,7 @@ struct dwc3_ep {
>>  #define DWC3_EP_BUSY   (1 << 4)
>>  #define DWC3_EP_PENDING_REQUEST(1 << 5)
>>  #define DWC3_EP_MISSED_ISOC(1 << 6)
>> +#define DWC3_EP_END_TRANSFER_PENDING (1 << 7)
>>
>> /* This last one is specific to EP0 */
>>  #define DWC3_EP0_DIR_IN(1 << 31)
>> @@ -1050,6 +1055,9 @@ struct dwc3_event_depevt {
>>  #define DEPEVT_TRANSFER_BUS_EXPIRY 2
>>
>> u32 parameters:16;
>> +
>> +/* For Command Complete Events */
>> +#define DEPEVT_PARAMETER_CMD(n) (((n) & (0xf << 8)) >> 8)
>>  } __packed;
>>
>>  /**
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 3b53a5714df4..5929a75b3455 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -598,6 +598,8 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
>> reg |= DWC3_DALEPENA_EP(dep->number);
>> dwc3_writel(dwc->regs, DWC3_DALEPENA, reg);
>>
>> +   init_waitqueue_head(>wait_end_transfer);
>> +
>> if (usb_endpoint_xfer_control(desc))
>> return 0;
>>
>> @@ -1783,12 +1785,28 @@ static int dwc3_gadget_start(struct usb_gadget *g,
>>
>>  static void __dwc3_gadget_stop(struct dwc3 *dwc)
>>  {
>> +   int epnum;
>> +
>> if (pm_runtime_suspended(dwc->dev))
>> return;
>>
>> dwc3_gadget_disable_irq(dwc);
>> __dwc3_gadget_ep_disable(dwc->eps[0]);
>> __dwc3_gadget_ep_disable(dwc->eps[1]);
>> +
>> +   for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
>> +   struct dwc3_ep  *dep = dwc->eps[epnum];
>> +
>> +   if (!(dep->flags & DWC3_EP_ENABLED))
>> +   continue;
>
> We should not check the DWC3_EP_ENABLED flag, since we will clear all
> flags in __dwc3_gadget_ep_disable() after setting
> DWC3_EP_END_TRANSFER_PENDING flags in dwc3_stop_active_transfer().

good catch.

>
>> +
>> +   if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>> +   continue;
>
> Ditto.

yeah, END_TRANSFER_PENDING should only be cleared by command completion,
so ep_disable needs to be patched.

>> +
>> +   wait_event_lock_irq(dep->wait_end_transfer,
>> +   !(dep->flags & DWC3_EP_END_TRANSFER_PENDING),
>> +   dwc->lock);
>> +   }
>>  }
>>
>>  static int dwc3_gadget_stop(struct usb_gadget *g)
>> @@ -2171,6 +2189,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
>>  {
>> struct dwc3_ep  *dep;
>> u8  epnum = event->endpoint_number;
>> +   u8  cmd;
>
> Here we should add below modification, or the event can not be handled.
>
> - if (!(dep->flags & DWC3_EP_ENABLED))
> + if (!(dep->flags & DWC3_EP_ENABLED) &&
> + !(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
> return;

this seems unnecessary to me. The only interrupt with a disabled
endpoint should be due 

Re: usb_ep_{dis,en}able() calling context (was: Re: [PATCH 2/3] usb: dwc3: gadget: wait for End Transfer to complete)

[Baolin Wang]

Hi,

On 18 October 2016 at 16:21, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>> Sure. The problem I met was, when we change the USB function with
>> configfs frequently, sometimes it will hang the system to crash. The
>> reason is,  we will start end transfer command when disable the
>> endpoint, but sometimes the end transfer command complete event comes
>> after we have freed the gadget irq (since the xHCI will share the same
>> interrupt number with gadget, thus when free the gadget irq, it will
>> not shutdown this gadget irq line), it will trigger the interrupt line
>> all the time.
>
> okay, thanks for describing it again. Another option would be to only
> wait_for_completion() before calling free_irq() is any endpoint has
> END_TRANSFER_PENDING flag set. Something like below:

I tested below patch, but it failed and I still met the kernel crash
with changing USB function.

>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 5fc437021ac7..0cb3b78d28b7 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include 
>  #include 
> @@ -505,6 +506,7 @@ struct dwc3_event_buffer {
>   * @endpoint: usb endpoint
>   * @pending_list: list of pending requests for this endpoint
>   * @started_list: list of started requests on this endpoint
> + * @wait_end_transfer: wait_queue_head_t for waiting on End Transfer complete
>   * @lock: spinlock for endpoint request queue traversal
>   * @regs: pointer to first endpoint register
>   * @trb_pool: array of transaction buffers
> @@ -530,6 +532,8 @@ struct dwc3_ep {
> struct list_headpending_list;
> struct list_headstarted_list;
>
> +   wait_queue_head_t   wait_end_transfer;
> +
> spinlock_t  lock;
> void __iomem*regs;
>
> @@ -546,6 +550,7 @@ struct dwc3_ep {
>  #define DWC3_EP_BUSY   (1 << 4)
>  #define DWC3_EP_PENDING_REQUEST(1 << 5)
>  #define DWC3_EP_MISSED_ISOC(1 << 6)
> +#define DWC3_EP_END_TRANSFER_PENDING (1 << 7)
>
> /* This last one is specific to EP0 */
>  #define DWC3_EP0_DIR_IN(1 << 31)
> @@ -1050,6 +1055,9 @@ struct dwc3_event_depevt {
>  #define DEPEVT_TRANSFER_BUS_EXPIRY 2
>
> u32 parameters:16;
> +
> +/* For Command Complete Events */
> +#define DEPEVT_PARAMETER_CMD(n) (((n) & (0xf << 8)) >> 8)
>  } __packed;
>
>  /**
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 3b53a5714df4..5929a75b3455 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -598,6 +598,8 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
> reg |= DWC3_DALEPENA_EP(dep->number);
> dwc3_writel(dwc->regs, DWC3_DALEPENA, reg);
>
> +   init_waitqueue_head(>wait_end_transfer);
> +
> if (usb_endpoint_xfer_control(desc))
> return 0;
>
> @@ -1783,12 +1785,28 @@ static int dwc3_gadget_start(struct usb_gadget *g,
>
>  static void __dwc3_gadget_stop(struct dwc3 *dwc)
>  {
> +   int epnum;
> +
> if (pm_runtime_suspended(dwc->dev))
> return;
>
> dwc3_gadget_disable_irq(dwc);
> __dwc3_gadget_ep_disable(dwc->eps[0]);
> __dwc3_gadget_ep_disable(dwc->eps[1]);
> +
> +   for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
> +   struct dwc3_ep  *dep = dwc->eps[epnum];
> +
> +   if (!(dep->flags & DWC3_EP_ENABLED))
> +   continue;

We should not check the DWC3_EP_ENABLED flag, since we will clear all
flags in __dwc3_gadget_ep_disable() after setting
DWC3_EP_END_TRANSFER_PENDING flags in dwc3_stop_active_transfer().

> +
> +   if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
> +   continue;

Ditto.

> +
> +   wait_event_lock_irq(dep->wait_end_transfer,
> +   !(dep->flags & DWC3_EP_END_TRANSFER_PENDING),
> +   dwc->lock);
> +   }
>  }
>
>  static int dwc3_gadget_stop(struct usb_gadget *g)
> @@ -2171,6 +2189,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
>  {
> struct dwc3_ep  *dep;
> u8  epnum = event->endpoint_number;
> +   u8  cmd;

Here we should add below modification, or the event can not be handled.

- if (!(dep->flags & DWC3_EP_ENABLED))
+ if (!(dep->flags & DWC3_EP_ENABLED) &&
+ !(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
return;

>
> dep = dwc->eps[epnum];
>
> @@ -2215,8 +2234,15 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
> return;
> }
> break;
> -   case DWC3_DEPEVT_RXTXFIFOEVT:
> case DWC3_DEPEVT_EPCMDCMPLT:
> +   cmd = 

Re: usb_ep_{dis,en}able() calling context (was: Re: [PATCH 2/3] usb: dwc3: gadget: wait for End Transfer to complete)

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>> >> 8<
>> >> From f3fa94f3171709f787a30e3c5ce69a668960b66e Mon Sep 17 00:00:00 2001
>> >> From: Felipe Balbi 
>> >> Date: Thu, 13 Oct 2016 14:09:47 +0300
>> >> Subject: [PATCH v2] usb: dwc3: gadget: wait for End Transfer to 
>> >> complete
>> >>
>> >> Instead of just delaying for 100us, we should
>> >> actually wait for End Transfer Command Complete
>> >> interrupt before moving on. Note that this should
>> >> only be done if we're dealing with one of the core
>> >> revisions that actually require the interrupt before
>> >> moving on.
>> >>
>> >> Reported-by: Baolin Wang 
>> >> Signed-off-by: Felipe Balbi 
>> >
>> > From my testing, there are still some problems caused by the nested
>> > lock, at lease I found 2 functions will issue the usb_ep_disable()
>> > function with spinlock:
>>
>> Thanks for testing. Seems like I really need to revisit locking in the
>> entire gadget API. In either case, we need to have a larger discussion
>> about this situation.
>>
>> IMO, usb_ep_disable() and usb_ep_enable() should only be callable from
>> process context, but that has never been a requirement before. Still,
>> it's not far-fetched to assume that a controller (such as dwc3, but
>> probably others) might sleep while cancelling a transfer because they
>> need to wait for an Interrupt.
>>
>> In fact, we know of two controllers that need this: dwc3 and dwc3.1.
>
> It's not clear that this should be the deciding factor.  That is, does
> usb_ep_disable() need to wait until all the endpoint's outstanding
> transfers have been completed before it returns?  I don't think it
> does.

 not all, no. And, frankly, this is really only a problem if we're
 unregistering a gadget while there are still pending transfers.

 The original problem statement is that we unregister a gadget, issue End
 Transfer, but CmdCompletion for the EndTransfer comes way too
 late. Baolin, can you clarify again what happens when the IRQ comes
 late?
>>>
>>> Sure. The problem I met was, when we change the USB function with
>>> configfs frequently, sometimes it will hang the system to crash. The
>>> reason is,  we will start end transfer command when disable the
>>> endpoint, but sometimes the end transfer command complete event comes
>>> after we have freed the gadget irq (since the xHCI will share the same
>>> interrupt number with gadget, thus when free the gadget irq, it will
>>> not shutdown this gadget irq line), it will trigger the interrupt line
>>> all the time.
>>
>> okay, thanks for describing it again. Another option would be to only
>> wait_for_completion() before calling free_irq() is any endpoint has
>> END_TRANSFER_PENDING flag set. Something like below:
>
> Yeah, that is what my original patch did, but you did one better
> patch, I will test it too.

That's true. I'll give you proper credit in the commit log since you
originated the idea.

-- 
balbi


signature.asc
Description: PGP signature

Re: usb_ep_{dis,en}able() calling context (was: Re: [PATCH 2/3] usb: dwc3: gadget: wait for End Transfer to complete)

[Peter Chen]

On Tue, Oct 18, 2016 at 10:19:38AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Alan Stern  writes:
> >> Baolin Wang  writes:
> >> >> Felipe Balbi  writes:
> >> >>> Instead of just delaying for 100us, we should
> >> >>> actually wait for End Transfer Command Complete
> >> >>> interrupt before moving on. Note that this should
> >> >>> only be done if we're dealing with one of the core
> >> >>> revisions that actually require the interrupt before
> >> >>> moving on.
> >> >>>
> >> >>> Reported-by: Baolin Wang 
> >> >>> Signed-off-by: Felipe Balbi 
> >> >>
> >> >> I've updated this one in order to fix the comment we had about delaying
> >> >> 100us. No further changes were made, only the comment. Here it is:
> >> >>
> >> >> 8<
> >> >> From f3fa94f3171709f787a30e3c5ce69a668960b66e Mon Sep 17 00:00:00 2001
> >> >> From: Felipe Balbi 
> >> >> Date: Thu, 13 Oct 2016 14:09:47 +0300
> >> >> Subject: [PATCH v2] usb: dwc3: gadget: wait for End Transfer to complete
> >> >>
> >> >> Instead of just delaying for 100us, we should
> >> >> actually wait for End Transfer Command Complete
> >> >> interrupt before moving on. Note that this should
> >> >> only be done if we're dealing with one of the core
> >> >> revisions that actually require the interrupt before
> >> >> moving on.
> >> >>
> >> >> Reported-by: Baolin Wang 
> >> >> Signed-off-by: Felipe Balbi 
> >> >
> >> > From my testing, there are still some problems caused by the nested
> >> > lock, at lease I found 2 functions will issue the usb_ep_disable()
> >> > function with spinlock:
> >> 
> >> Thanks for testing. Seems like I really need to revisit locking in the
> >> entire gadget API. In either case, we need to have a larger discussion
> >> about this situation.
> >> 
> >> IMO, usb_ep_disable() and usb_ep_enable() should only be callable from
> >> process context, but that has never been a requirement before. Still,
> >> it's not far-fetched to assume that a controller (such as dwc3, but
> >> probably others) might sleep while cancelling a transfer because they
> >> need to wait for an Interrupt.
> >> 
> >> In fact, we know of two controllers that need this: dwc3 and dwc3.1.
> >
> > It's not clear that this should be the deciding factor.  That is, does 
> > usb_ep_disable() need to wait until all the endpoint's outstanding 
> > transfers have been completed before it returns?  I don't think it 
> > does.
> 
> not all, no. And, frankly, this is really only a problem if we're
> unregistering a gadget while there are still pending transfers.
> 
> The original problem statement is that we unregister a gadget, issue End
> Transfer, but CmdCompletion for the EndTransfer comes way too
> late. Baolin, can you clarify again what happens when the IRQ comes
> late?
> 

Per my understanding, if there are pending transfers, the gadget driver
needs to call usb_ep_dequeue to cancel transfers first, then call
usb_ep_disable.

-- 

Best Regards,
Peter Chen
--
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_ep_{dis,en}able() calling context (was: Re: [PATCH 2/3] usb: dwc3: gadget: wait for End Transfer to complete)

[Baolin Wang]

On 18 October 2016 at 16:21, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>>> Alan Stern  writes:
> Baolin Wang  writes:
> >> Felipe Balbi  writes:
> >>> Instead of just delaying for 100us, we should
> >>> actually wait for End Transfer Command Complete
> >>> interrupt before moving on. Note that this should
> >>> only be done if we're dealing with one of the core
> >>> revisions that actually require the interrupt before
> >>> moving on.
> >>>
> >>> Reported-by: Baolin Wang 
> >>> Signed-off-by: Felipe Balbi 
> >>
> >> I've updated this one in order to fix the comment we had about delaying
> >> 100us. No further changes were made, only the comment. Here it is:
> >>
> >> 8<
> >> From f3fa94f3171709f787a30e3c5ce69a668960b66e Mon Sep 17 00:00:00 2001
> >> From: Felipe Balbi 
> >> Date: Thu, 13 Oct 2016 14:09:47 +0300
> >> Subject: [PATCH v2] usb: dwc3: gadget: wait for End Transfer to 
> >> complete
> >>
> >> Instead of just delaying for 100us, we should
> >> actually wait for End Transfer Command Complete
> >> interrupt before moving on. Note that this should
> >> only be done if we're dealing with one of the core
> >> revisions that actually require the interrupt before
> >> moving on.
> >>
> >> Reported-by: Baolin Wang 
> >> Signed-off-by: Felipe Balbi 
> >
> > From my testing, there are still some problems caused by the nested
> > lock, at lease I found 2 functions will issue the usb_ep_disable()
> > function with spinlock:
>
> Thanks for testing. Seems like I really need to revisit locking in the
> entire gadget API. In either case, we need to have a larger discussion
> about this situation.
>
> IMO, usb_ep_disable() and usb_ep_enable() should only be callable from
> process context, but that has never been a requirement before. Still,
> it's not far-fetched to assume that a controller (such as dwc3, but
> probably others) might sleep while cancelling a transfer because they
> need to wait for an Interrupt.
>
> In fact, we know of two controllers that need this: dwc3 and dwc3.1.

 It's not clear that this should be the deciding factor.  That is, does
 usb_ep_disable() need to wait until all the endpoint's outstanding
 transfers have been completed before it returns?  I don't think it
 does.
>>>
>>> not all, no. And, frankly, this is really only a problem if we're
>>> unregistering a gadget while there are still pending transfers.
>>>
>>> The original problem statement is that we unregister a gadget, issue End
>>> Transfer, but CmdCompletion for the EndTransfer comes way too
>>> late. Baolin, can you clarify again what happens when the IRQ comes
>>> late?
>>
>> Sure. The problem I met was, when we change the USB function with
>> configfs frequently, sometimes it will hang the system to crash. The
>> reason is,  we will start end transfer command when disable the
>> endpoint, but sometimes the end transfer command complete event comes
>> after we have freed the gadget irq (since the xHCI will share the same
>> interrupt number with gadget, thus when free the gadget irq, it will
>> not shutdown this gadget irq line), it will trigger the interrupt line
>> all the time.
>
> okay, thanks for describing it again. Another option would be to only
> wait_for_completion() before calling free_irq() is any endpoint has
> END_TRANSFER_PENDING flag set. Something like below:

Yeah, that is what my original patch did, but you did one better
patch, I will test it too.

>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 5fc437021ac7..0cb3b78d28b7 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include 
>  #include 
> @@ -505,6 +506,7 @@ struct dwc3_event_buffer {
>   * @endpoint: usb endpoint
>   * @pending_list: list of pending requests for this endpoint
>   * @started_list: list of started requests on this endpoint
> + * @wait_end_transfer: wait_queue_head_t for waiting on End Transfer complete
>   * @lock: spinlock for endpoint request queue traversal
>   * @regs: pointer to first endpoint register
>   * @trb_pool: array of transaction buffers
> @@ -530,6 +532,8 @@ struct dwc3_ep {
> struct list_headpending_list;
> struct list_headstarted_list;
>
> +   wait_queue_head_t   wait_end_transfer;
> +
> spinlock_t  lock;
> void __iomem*regs;
>
> @@ -546,6 +550,7 @@ struct 

Re: usb_ep_{dis,en}able() calling context (was: Re: [PATCH 2/3] usb: dwc3: gadget: wait for End Transfer to complete)

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>> Alan Stern  writes:
 Baolin Wang  writes:
 >> Felipe Balbi  writes:
 >>> Instead of just delaying for 100us, we should
 >>> actually wait for End Transfer Command Complete
 >>> interrupt before moving on. Note that this should
 >>> only be done if we're dealing with one of the core
 >>> revisions that actually require the interrupt before
 >>> moving on.
 >>>
 >>> Reported-by: Baolin Wang 
 >>> Signed-off-by: Felipe Balbi 
 >>
 >> I've updated this one in order to fix the comment we had about delaying
 >> 100us. No further changes were made, only the comment. Here it is:
 >>
 >> 8<
 >> From f3fa94f3171709f787a30e3c5ce69a668960b66e Mon Sep 17 00:00:00 2001
 >> From: Felipe Balbi 
 >> Date: Thu, 13 Oct 2016 14:09:47 +0300
 >> Subject: [PATCH v2] usb: dwc3: gadget: wait for End Transfer to complete
 >>
 >> Instead of just delaying for 100us, we should
 >> actually wait for End Transfer Command Complete
 >> interrupt before moving on. Note that this should
 >> only be done if we're dealing with one of the core
 >> revisions that actually require the interrupt before
 >> moving on.
 >>
 >> Reported-by: Baolin Wang 
 >> Signed-off-by: Felipe Balbi 
 >
 > From my testing, there are still some problems caused by the nested
 > lock, at lease I found 2 functions will issue the usb_ep_disable()
 > function with spinlock:

 Thanks for testing. Seems like I really need to revisit locking in the
 entire gadget API. In either case, we need to have a larger discussion
 about this situation.

 IMO, usb_ep_disable() and usb_ep_enable() should only be callable from
 process context, but that has never been a requirement before. Still,
 it's not far-fetched to assume that a controller (such as dwc3, but
 probably others) might sleep while cancelling a transfer because they
 need to wait for an Interrupt.

 In fact, we know of two controllers that need this: dwc3 and dwc3.1.
>>>
>>> It's not clear that this should be the deciding factor.  That is, does
>>> usb_ep_disable() need to wait until all the endpoint's outstanding
>>> transfers have been completed before it returns?  I don't think it
>>> does.
>>
>> not all, no. And, frankly, this is really only a problem if we're
>> unregistering a gadget while there are still pending transfers.
>>
>> The original problem statement is that we unregister a gadget, issue End
>> Transfer, but CmdCompletion for the EndTransfer comes way too
>> late. Baolin, can you clarify again what happens when the IRQ comes
>> late?
>
> Sure. The problem I met was, when we change the USB function with
> configfs frequently, sometimes it will hang the system to crash. The
> reason is,  we will start end transfer command when disable the
> endpoint, but sometimes the end transfer command complete event comes
> after we have freed the gadget irq (since the xHCI will share the same
> interrupt number with gadget, thus when free the gadget irq, it will
> not shutdown this gadget irq line), it will trigger the interrupt line
> all the time.

okay, thanks for describing it again. Another option would be to only
wait_for_completion() before calling free_irq() is any endpoint has
END_TRANSFER_PENDING flag set. Something like below:

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 5fc437021ac7..0cb3b78d28b7 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -505,6 +506,7 @@ struct dwc3_event_buffer {
  * @endpoint: usb endpoint
  * @pending_list: list of pending requests for this endpoint
  * @started_list: list of started requests on this endpoint
+ * @wait_end_transfer: wait_queue_head_t for waiting on End Transfer complete
  * @lock: spinlock for endpoint request queue traversal
  * @regs: pointer to first endpoint register
  * @trb_pool: array of transaction buffers
@@ -530,6 +532,8 @@ struct dwc3_ep {
struct list_headpending_list;
struct list_headstarted_list;
 
+   wait_queue_head_t   wait_end_transfer;
+
spinlock_t  lock;
void __iomem*regs;
 
@@ -546,6 +550,7 @@ struct dwc3_ep {
 #define DWC3_EP_BUSY   (1 << 4)
 #define DWC3_EP_PENDING_REQUEST(1 << 5)
 #define DWC3_EP_MISSED_ISOC(1 << 6)
+#define DWC3_EP_END_TRANSFER_PENDING (1 << 7)
 
/* This last one is specific to EP0 */
 #define DWC3_EP0_DIR_IN(1 << 31)
@@ -1050,6 +1055,9 @@ struct 

Re: usb_ep_{dis,en}able() calling context (was: Re: [PATCH 2/3] usb: dwc3: gadget: wait for End Transfer to complete)

[Baolin Wang]

Hi,

On 18 October 2016 at 15:19, Felipe Balbi  wrote:
>
> Hi,
>
> Alan Stern  writes:
>>> Baolin Wang  writes:
>>> >> Felipe Balbi  writes:
>>> >>> Instead of just delaying for 100us, we should
>>> >>> actually wait for End Transfer Command Complete
>>> >>> interrupt before moving on. Note that this should
>>> >>> only be done if we're dealing with one of the core
>>> >>> revisions that actually require the interrupt before
>>> >>> moving on.
>>> >>>
>>> >>> Reported-by: Baolin Wang 
>>> >>> Signed-off-by: Felipe Balbi 
>>> >>
>>> >> I've updated this one in order to fix the comment we had about delaying
>>> >> 100us. No further changes were made, only the comment. Here it is:
>>> >>
>>> >> 8<
>>> >> From f3fa94f3171709f787a30e3c5ce69a668960b66e Mon Sep 17 00:00:00 2001
>>> >> From: Felipe Balbi 
>>> >> Date: Thu, 13 Oct 2016 14:09:47 +0300
>>> >> Subject: [PATCH v2] usb: dwc3: gadget: wait for End Transfer to complete
>>> >>
>>> >> Instead of just delaying for 100us, we should
>>> >> actually wait for End Transfer Command Complete
>>> >> interrupt before moving on. Note that this should
>>> >> only be done if we're dealing with one of the core
>>> >> revisions that actually require the interrupt before
>>> >> moving on.
>>> >>
>>> >> Reported-by: Baolin Wang 
>>> >> Signed-off-by: Felipe Balbi 
>>> >
>>> > From my testing, there are still some problems caused by the nested
>>> > lock, at lease I found 2 functions will issue the usb_ep_disable()
>>> > function with spinlock:
>>>
>>> Thanks for testing. Seems like I really need to revisit locking in the
>>> entire gadget API. In either case, we need to have a larger discussion
>>> about this situation.
>>>
>>> IMO, usb_ep_disable() and usb_ep_enable() should only be callable from
>>> process context, but that has never been a requirement before. Still,
>>> it's not far-fetched to assume that a controller (such as dwc3, but
>>> probably others) might sleep while cancelling a transfer because they
>>> need to wait for an Interrupt.
>>>
>>> In fact, we know of two controllers that need this: dwc3 and dwc3.1.
>>
>> It's not clear that this should be the deciding factor.  That is, does
>> usb_ep_disable() need to wait until all the endpoint's outstanding
>> transfers have been completed before it returns?  I don't think it
>> does.
>
> not all, no. And, frankly, this is really only a problem if we're
> unregistering a gadget while there are still pending transfers.
>
> The original problem statement is that we unregister a gadget, issue End
> Transfer, but CmdCompletion for the EndTransfer comes way too
> late. Baolin, can you clarify again what happens when the IRQ comes
> late?

Sure. The problem I met was, when we change the USB function with
configfs frequently, sometimes it will hang the system to crash. The
reason is,  we will start end transfer command when disable the
endpoint, but sometimes the end transfer command complete event comes
after we have freed the gadget irq (since the xHCI will share the same
interrupt number with gadget, thus when free the gadget irq, it will
not shutdown this gadget irq line), it will trigger the interrupt line
all the time.

>
>>> I wonder if there are any other controllers with this
>>> requirement. Either way, We should also consider if there are any strong
>>> reasons to call usb_ep_disable() with interrupts disabled and locks held
>>> considering that UDC _must_ call ->complete() for each and every
>>> request.
>>>
>>> If we go ahead with this, it'll mean a rather large series (again) to
>>> fix all the calling semantics in every single gadget driver for both
>>> usb_ep_enable() and usb_ep_disable(). Then, making sure all UDC drivers
>>> respect the requirement, then we update documentation about the
>>> functions and add might_sleep() to their implementation in udc/core.c
>>>
>>> Anybody objects to this new requirement?
>>
>> The usual times for calling usb_ep_disable() is when the gadget driver
>> processes an altsetting or configuration change, or a reset or
>> disconnect.  The notifications for these things happen in interrupt
>> context, so it doesn't seem reasonable to require that endpoints be
>> disabled in process context.
>
> Oh, that's true. I completely forgot about altsetting change.
>
>> f_mass_storage has its own worker thread.  Mainly for other reasons,
>> but it can also use the thread to handle these events.  But it doesn't
>> seem right to require all gadget drivers to do the same thing.
>>
>> There is still an open question about how a gadget driver can change
>> altsettings, though.  A particular endpoint might be bulk in one
>> altsetting and isochronous in another, for example. 

Re: usb_ep_{dis,en}able() calling context (was: Re: [PATCH 2/3] usb: dwc3: gadget: wait for End Transfer to complete)

[Felipe Balbi]

Hi,

Alan Stern  writes:
>> Baolin Wang  writes:
>> >> Felipe Balbi  writes:
>> >>> Instead of just delaying for 100us, we should
>> >>> actually wait for End Transfer Command Complete
>> >>> interrupt before moving on. Note that this should
>> >>> only be done if we're dealing with one of the core
>> >>> revisions that actually require the interrupt before
>> >>> moving on.
>> >>>
>> >>> Reported-by: Baolin Wang 
>> >>> Signed-off-by: Felipe Balbi 
>> >>
>> >> I've updated this one in order to fix the comment we had about delaying
>> >> 100us. No further changes were made, only the comment. Here it is:
>> >>
>> >> 8<
>> >> From f3fa94f3171709f787a30e3c5ce69a668960b66e Mon Sep 17 00:00:00 2001
>> >> From: Felipe Balbi 
>> >> Date: Thu, 13 Oct 2016 14:09:47 +0300
>> >> Subject: [PATCH v2] usb: dwc3: gadget: wait for End Transfer to complete
>> >>
>> >> Instead of just delaying for 100us, we should
>> >> actually wait for End Transfer Command Complete
>> >> interrupt before moving on. Note that this should
>> >> only be done if we're dealing with one of the core
>> >> revisions that actually require the interrupt before
>> >> moving on.
>> >>
>> >> Reported-by: Baolin Wang 
>> >> Signed-off-by: Felipe Balbi 
>> >
>> > From my testing, there are still some problems caused by the nested
>> > lock, at lease I found 2 functions will issue the usb_ep_disable()
>> > function with spinlock:
>> 
>> Thanks for testing. Seems like I really need to revisit locking in the
>> entire gadget API. In either case, we need to have a larger discussion
>> about this situation.
>> 
>> IMO, usb_ep_disable() and usb_ep_enable() should only be callable from
>> process context, but that has never been a requirement before. Still,
>> it's not far-fetched to assume that a controller (such as dwc3, but
>> probably others) might sleep while cancelling a transfer because they
>> need to wait for an Interrupt.
>> 
>> In fact, we know of two controllers that need this: dwc3 and dwc3.1.
>
> It's not clear that this should be the deciding factor.  That is, does 
> usb_ep_disable() need to wait until all the endpoint's outstanding 
> transfers have been completed before it returns?  I don't think it 
> does.

not all, no. And, frankly, this is really only a problem if we're
unregistering a gadget while there are still pending transfers.

The original problem statement is that we unregister a gadget, issue End
Transfer, but CmdCompletion for the EndTransfer comes way too
late. Baolin, can you clarify again what happens when the IRQ comes
late?

>> I wonder if there are any other controllers with this
>> requirement. Either way, We should also consider if there are any strong
>> reasons to call usb_ep_disable() with interrupts disabled and locks held
>> considering that UDC _must_ call ->complete() for each and every
>> request.
>> 
>> If we go ahead with this, it'll mean a rather large series (again) to
>> fix all the calling semantics in every single gadget driver for both
>> usb_ep_enable() and usb_ep_disable(). Then, making sure all UDC drivers
>> respect the requirement, then we update documentation about the
>> functions and add might_sleep() to their implementation in udc/core.c
>> 
>> Anybody objects to this new requirement?
>
> The usual times for calling usb_ep_disable() is when the gadget driver 
> processes an altsetting or configuration change, or a reset or 
> disconnect.  The notifications for these things happen in interrupt 
> context, so it doesn't seem reasonable to require that endpoints be 
> disabled in process context.

Oh, that's true. I completely forgot about altsetting change.

> f_mass_storage has its own worker thread.  Mainly for other reasons, 
> but it can also use the thread to handle these events.  But it doesn't 
> seem right to require all gadget drivers to do the same thing.
>
> There is still an open question about how a gadget driver can change 
> altsettings, though.  A particular endpoint might be bulk in one 
> altsetting and isochronous in another, for example.  I don't see how we 
> can change the endpoint's characteristics without being allowed to 
> sleep.

Heh, seems that I ended up touching a subject that hasn't been revisited
in few years :-)

Anyway, let's see what Baolin says about the IRQ again. Now that I think
of it, free_irq() should be calling synchronize IRQ, right? So by the
time free_irq() returns, we shouldn't get any further interrupts. Also,
when the endpoint is disabled it shouldn't trigger any further
interrupts.

-- 
balbi


signature.asc
Description: PGP signature

Re: usb_ep_{dis,en}able() calling context (was: Re: [PATCH 2/3] usb: dwc3: gadget: wait for End Transfer to complete)

[Peter Chen]

On Mon, Oct 17, 2016 at 12:51:48PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen  writes:
> > On Mon, Oct 17, 2016 at 11:06:49AM +0300, Felipe Balbi wrote:
> >> 
> >> Hi Baolin,
> >> 
> >> Baolin Wang  writes:
> >> >> Felipe Balbi  writes:
> >> >>> Instead of just delaying for 100us, we should
> >> >>> actually wait for End Transfer Command Complete
> >> >>> interrupt before moving on. Note that this should
> >> >>> only be done if we're dealing with one of the core
> >> >>> revisions that actually require the interrupt before
> >> >>> moving on.
> >> >>>
> >> >>> Reported-by: Baolin Wang 
> >> >>> Signed-off-by: Felipe Balbi 
> >> >>
> >> >> I've updated this one in order to fix the comment we had about delaying
> >> >> 100us. No further changes were made, only the comment. Here it is:
> >> >>
> >> >> 8<
> >> >> From f3fa94f3171709f787a30e3c5ce69a668960b66e Mon Sep 17 00:00:00 2001
> >> >> From: Felipe Balbi 
> >> >> Date: Thu, 13 Oct 2016 14:09:47 +0300
> >> >> Subject: [PATCH v2] usb: dwc3: gadget: wait for End Transfer to complete
> >> >>
> >> >> Instead of just delaying for 100us, we should
> >> >> actually wait for End Transfer Command Complete
> >> >> interrupt before moving on. Note that this should
> >> >> only be done if we're dealing with one of the core
> >> >> revisions that actually require the interrupt before
> >> >> moving on.
> >> >>
> >> >> Reported-by: Baolin Wang 
> >> >> Signed-off-by: Felipe Balbi 
> >> >
> >> > From my testing, there are still some problems caused by the nested
> >> > lock, at lease I found 2 functions will issue the usb_ep_disable()
> >> > function with spinlock:
> >> 
> >> Thanks for testing. Seems like I really need to revisit locking in the
> >> entire gadget API. In either case, we need to have a larger discussion
> >> about this situation.
> >> 
> >> IMO, usb_ep_disable() and usb_ep_enable() should only be callable from
> >> process context, but that has never been a requirement before. Still,
> >> it's not far-fetched to assume that a controller (such as dwc3, but
> >> probably others) might sleep while cancelling a transfer because they
> >> need to wait for an Interrupt.
> >> 
> >> In fact, we know of two controllers that need this: dwc3 and dwc3.1.
> >> 
> >> I wonder if there are any other controllers with this
> >> requirement. Either way, We should also consider if there are any strong
> >> reasons to call usb_ep_disable() with interrupts disabled and locks held
> >> considering that UDC _must_ call ->complete() for each and every
> >> request.
> >> 
> >> If we go ahead with this, it'll mean a rather large series (again) to
> >> fix all the calling semantics in every single gadget driver for both
> >> usb_ep_enable() and usb_ep_disable(). Then, making sure all UDC drivers
> >> respect the requirement, then we update documentation about the
> >> functions and add might_sleep() to their implementation in udc/core.c
> >> 
> >> Anybody objects to this new requirement?
> >> 
> >
> > At chipidea driver, it might call usb_ep_disable at interrupt context,
> > eg, the bus reset interrupt. See chipidea/udc.c isr_reset_handler->
> > _gadget_stop_activity->usb_ep_disable.
> 
> That implementation is wrong. You should tell the gadget driver that
> reset happened by calling usb_gadget_udc_reset(). It's just that
> usb_gadget_udc_reset() would also need to be called with IRQs enabled
> after this rework happens.
> 

I see, the usb_ep_disable should be called from gadget driver if a bus
reset occurs. Would you consider add special handling like work queue for
dwc3's ep_disable API to wait something?

-- 

Best Regards,
Peter Chen
--
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_ep_{dis,en}able() calling context (was: Re: [PATCH 2/3] usb: dwc3: gadget: wait for End Transfer to complete)

[Alan Stern]

On Mon, 17 Oct 2016, Felipe Balbi wrote:

> Hi Baolin,
> 
> Baolin Wang  writes:
> >> Felipe Balbi  writes:
> >>> Instead of just delaying for 100us, we should
> >>> actually wait for End Transfer Command Complete
> >>> interrupt before moving on. Note that this should
> >>> only be done if we're dealing with one of the core
> >>> revisions that actually require the interrupt before
> >>> moving on.
> >>>
> >>> Reported-by: Baolin Wang 
> >>> Signed-off-by: Felipe Balbi 
> >>
> >> I've updated this one in order to fix the comment we had about delaying
> >> 100us. No further changes were made, only the comment. Here it is:
> >>
> >> 8<
> >> From f3fa94f3171709f787a30e3c5ce69a668960b66e Mon Sep 17 00:00:00 2001
> >> From: Felipe Balbi 
> >> Date: Thu, 13 Oct 2016 14:09:47 +0300
> >> Subject: [PATCH v2] usb: dwc3: gadget: wait for End Transfer to complete
> >>
> >> Instead of just delaying for 100us, we should
> >> actually wait for End Transfer Command Complete
> >> interrupt before moving on. Note that this should
> >> only be done if we're dealing with one of the core
> >> revisions that actually require the interrupt before
> >> moving on.
> >>
> >> Reported-by: Baolin Wang 
> >> Signed-off-by: Felipe Balbi 
> >
> > From my testing, there are still some problems caused by the nested
> > lock, at lease I found 2 functions will issue the usb_ep_disable()
> > function with spinlock:
> 
> Thanks for testing. Seems like I really need to revisit locking in the
> entire gadget API. In either case, we need to have a larger discussion
> about this situation.
> 
> IMO, usb_ep_disable() and usb_ep_enable() should only be callable from
> process context, but that has never been a requirement before. Still,
> it's not far-fetched to assume that a controller (such as dwc3, but
> probably others) might sleep while cancelling a transfer because they
> need to wait for an Interrupt.
> 
> In fact, we know of two controllers that need this: dwc3 and dwc3.1.

It's not clear that this should be the deciding factor.  That is, does 
usb_ep_disable() need to wait until all the endpoint's outstanding 
transfers have been completed before it returns?  I don't think it 
does.

> I wonder if there are any other controllers with this
> requirement. Either way, We should also consider if there are any strong
> reasons to call usb_ep_disable() with interrupts disabled and locks held
> considering that UDC _must_ call ->complete() for each and every
> request.
> 
> If we go ahead with this, it'll mean a rather large series (again) to
> fix all the calling semantics in every single gadget driver for both
> usb_ep_enable() and usb_ep_disable(). Then, making sure all UDC drivers
> respect the requirement, then we update documentation about the
> functions and add might_sleep() to their implementation in udc/core.c
> 
> Anybody objects to this new requirement?

The usual times for calling usb_ep_disable() is when the gadget driver 
processes an altsetting or configuration change, or a reset or 
disconnect.  The notifications for these things happen in interrupt 
context, so it doesn't seem reasonable to require that endpoints be 
disabled in process context.

f_mass_storage has its own worker thread.  Mainly for other reasons, 
but it can also use the thread to handle these events.  But it doesn't 
seem right to require all gadget drivers to do the same thing.

There is still an open question about how a gadget driver can change 
altsettings, though.  A particular endpoint might be bulk in one 
altsetting and isochronous in another, for example.  I don't see how we 
can change the endpoint's characteristics without being allowed to 
sleep.

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: usb_ep_{dis,en}able() calling context (was: Re: [PATCH 2/3] usb: dwc3: gadget: wait for End Transfer to complete)

[Felipe Balbi]

Hi,

Peter Chen  writes:
> On Mon, Oct 17, 2016 at 11:06:49AM +0300, Felipe Balbi wrote:
>> 
>> Hi Baolin,
>> 
>> Baolin Wang  writes:
>> >> Felipe Balbi  writes:
>> >>> Instead of just delaying for 100us, we should
>> >>> actually wait for End Transfer Command Complete
>> >>> interrupt before moving on. Note that this should
>> >>> only be done if we're dealing with one of the core
>> >>> revisions that actually require the interrupt before
>> >>> moving on.
>> >>>
>> >>> Reported-by: Baolin Wang 
>> >>> Signed-off-by: Felipe Balbi 
>> >>
>> >> I've updated this one in order to fix the comment we had about delaying
>> >> 100us. No further changes were made, only the comment. Here it is:
>> >>
>> >> 8<
>> >> From f3fa94f3171709f787a30e3c5ce69a668960b66e Mon Sep 17 00:00:00 2001
>> >> From: Felipe Balbi 
>> >> Date: Thu, 13 Oct 2016 14:09:47 +0300
>> >> Subject: [PATCH v2] usb: dwc3: gadget: wait for End Transfer to complete
>> >>
>> >> Instead of just delaying for 100us, we should
>> >> actually wait for End Transfer Command Complete
>> >> interrupt before moving on. Note that this should
>> >> only be done if we're dealing with one of the core
>> >> revisions that actually require the interrupt before
>> >> moving on.
>> >>
>> >> Reported-by: Baolin Wang 
>> >> Signed-off-by: Felipe Balbi 
>> >
>> > From my testing, there are still some problems caused by the nested
>> > lock, at lease I found 2 functions will issue the usb_ep_disable()
>> > function with spinlock:
>> 
>> Thanks for testing. Seems like I really need to revisit locking in the
>> entire gadget API. In either case, we need to have a larger discussion
>> about this situation.
>> 
>> IMO, usb_ep_disable() and usb_ep_enable() should only be callable from
>> process context, but that has never been a requirement before. Still,
>> it's not far-fetched to assume that a controller (such as dwc3, but
>> probably others) might sleep while cancelling a transfer because they
>> need to wait for an Interrupt.
>> 
>> In fact, we know of two controllers that need this: dwc3 and dwc3.1.
>> 
>> I wonder if there are any other controllers with this
>> requirement. Either way, We should also consider if there are any strong
>> reasons to call usb_ep_disable() with interrupts disabled and locks held
>> considering that UDC _must_ call ->complete() for each and every
>> request.
>> 
>> If we go ahead with this, it'll mean a rather large series (again) to
>> fix all the calling semantics in every single gadget driver for both
>> usb_ep_enable() and usb_ep_disable(). Then, making sure all UDC drivers
>> respect the requirement, then we update documentation about the
>> functions and add might_sleep() to their implementation in udc/core.c
>> 
>> Anybody objects to this new requirement?
>> 
>
> At chipidea driver, it might call usb_ep_disable at interrupt context,
> eg, the bus reset interrupt. See chipidea/udc.c isr_reset_handler->
> _gadget_stop_activity->usb_ep_disable.

That implementation is wrong. You should tell the gadget driver that
reset happened by calling usb_gadget_udc_reset(). It's just that
usb_gadget_udc_reset() would also need to be called with IRQs enabled
after this rework happens.

-- 
balbi


signature.asc
Description: PGP signature

Re: usb_ep_{dis,en}able() calling context (was: Re: [PATCH 2/3] usb: dwc3: gadget: wait for End Transfer to complete)

[Peter Chen]

On Mon, Oct 17, 2016 at 11:06:49AM +0300, Felipe Balbi wrote:
> 
> Hi Baolin,
> 
> Baolin Wang  writes:
> >> Felipe Balbi  writes:
> >>> Instead of just delaying for 100us, we should
> >>> actually wait for End Transfer Command Complete
> >>> interrupt before moving on. Note that this should
> >>> only be done if we're dealing with one of the core
> >>> revisions that actually require the interrupt before
> >>> moving on.
> >>>
> >>> Reported-by: Baolin Wang 
> >>> Signed-off-by: Felipe Balbi 
> >>
> >> I've updated this one in order to fix the comment we had about delaying
> >> 100us. No further changes were made, only the comment. Here it is:
> >>
> >> 8<
> >> From f3fa94f3171709f787a30e3c5ce69a668960b66e Mon Sep 17 00:00:00 2001
> >> From: Felipe Balbi 
> >> Date: Thu, 13 Oct 2016 14:09:47 +0300
> >> Subject: [PATCH v2] usb: dwc3: gadget: wait for End Transfer to complete
> >>
> >> Instead of just delaying for 100us, we should
> >> actually wait for End Transfer Command Complete
> >> interrupt before moving on. Note that this should
> >> only be done if we're dealing with one of the core
> >> revisions that actually require the interrupt before
> >> moving on.
> >>
> >> Reported-by: Baolin Wang 
> >> Signed-off-by: Felipe Balbi 
> >
> > From my testing, there are still some problems caused by the nested
> > lock, at lease I found 2 functions will issue the usb_ep_disable()
> > function with spinlock:
> 
> Thanks for testing. Seems like I really need to revisit locking in the
> entire gadget API. In either case, we need to have a larger discussion
> about this situation.
> 
> IMO, usb_ep_disable() and usb_ep_enable() should only be callable from
> process context, but that has never been a requirement before. Still,
> it's not far-fetched to assume that a controller (such as dwc3, but
> probably others) might sleep while cancelling a transfer because they
> need to wait for an Interrupt.
> 
> In fact, we know of two controllers that need this: dwc3 and dwc3.1.
> 
> I wonder if there are any other controllers with this
> requirement. Either way, We should also consider if there are any strong
> reasons to call usb_ep_disable() with interrupts disabled and locks held
> considering that UDC _must_ call ->complete() for each and every
> request.
> 
> If we go ahead with this, it'll mean a rather large series (again) to
> fix all the calling semantics in every single gadget driver for both
> usb_ep_enable() and usb_ep_disable(). Then, making sure all UDC drivers
> respect the requirement, then we update documentation about the
> functions and add might_sleep() to their implementation in udc/core.c
> 
> Anybody objects to this new requirement?
> 

At chipidea driver, it might call usb_ep_disable at interrupt context,
eg, the bus reset interrupt. See chipidea/udc.c isr_reset_handler->
_gadget_stop_activity->usb_ep_disable.

-- 

Best Regards,
Peter Chen
--
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_ep_{dis,en}able() calling context (was: Re: [PATCH 2/3] usb: dwc3: gadget: wait for End Transfer to complete)

[Felipe Balbi]

Hi Baolin,

Baolin Wang  writes:
>> Felipe Balbi  writes:
>>> Instead of just delaying for 100us, we should
>>> actually wait for End Transfer Command Complete
>>> interrupt before moving on. Note that this should
>>> only be done if we're dealing with one of the core
>>> revisions that actually require the interrupt before
>>> moving on.
>>>
>>> Reported-by: Baolin Wang 
>>> Signed-off-by: Felipe Balbi 
>>
>> I've updated this one in order to fix the comment we had about delaying
>> 100us. No further changes were made, only the comment. Here it is:
>>
>> 8<
>> From f3fa94f3171709f787a30e3c5ce69a668960b66e Mon Sep 17 00:00:00 2001
>> From: Felipe Balbi 
>> Date: Thu, 13 Oct 2016 14:09:47 +0300
>> Subject: [PATCH v2] usb: dwc3: gadget: wait for End Transfer to complete
>>
>> Instead of just delaying for 100us, we should
>> actually wait for End Transfer Command Complete
>> interrupt before moving on. Note that this should
>> only be done if we're dealing with one of the core
>> revisions that actually require the interrupt before
>> moving on.
>>
>> Reported-by: Baolin Wang 
>> Signed-off-by: Felipe Balbi 
>
> From my testing, there are still some problems caused by the nested
> lock, at lease I found 2 functions will issue the usb_ep_disable()
> function with spinlock:

Thanks for testing. Seems like I really need to revisit locking in the
entire gadget API. In either case, we need to have a larger discussion
about this situation.

IMO, usb_ep_disable() and usb_ep_enable() should only be callable from
process context, but that has never been a requirement before. Still,
it's not far-fetched to assume that a controller (such as dwc3, but
probably others) might sleep while cancelling a transfer because they
need to wait for an Interrupt.

In fact, we know of two controllers that need this: dwc3 and dwc3.1.

I wonder if there are any other controllers with this
requirement. Either way, We should also consider if there are any strong
reasons to call usb_ep_disable() with interrupts disabled and locks held
considering that UDC _must_ call ->complete() for each and every
request.

If we go ahead with this, it'll mean a rather large series (again) to
fix all the calling semantics in every single gadget driver for both
usb_ep_enable() and usb_ep_disable(). Then, making sure all UDC drivers
respect the requirement, then we update documentation about the
functions and add might_sleep() to their implementation in udc/core.c

Anybody objects to this new requirement?

-- 
balbi


signature.asc
Description: PGP signature