LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/2] usb: dwc2: fix the uncompleted requests problem
@ 2015-01-05  9:31 Robert Baldyga
  2015-01-05  9:31 ` [PATCH 1/2] Revert "usb: dwc2: gadget: kill requests with 'force' in s3c_hsotg_udc_stop()" Robert Baldyga
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Robert Baldyga @ 2015-01-05  9:31 UTC (permalink / raw)
  To: balbi, paulz; +Cc: gregkh, linux-usb, linux-kernel, Robert Baldyga

Hi Felipe,

I prepared this patchset in response to our discussion:
https://lkml.org/lkml/2014/12/29/193

It reverts of unproper patch and adds working solution of the problem
described in commit messages.

Best regards,
Robert Baldyga

Robert Baldyga (2):
  Revert "usb: dwc2: gadget: kill requests with 'force' in
    s3c_hsotg_udc_stop()"
  drivers: usb: dwc2: remove 'force' parameter from kill_all_requests()

 drivers/usb/dwc2/gadget.c | 31 ++++++++++---------------------
 1 file changed, 10 insertions(+), 21 deletions(-)

-- 
1.9.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] Revert "usb: dwc2: gadget: kill requests with 'force' in s3c_hsotg_udc_stop()"
  2015-01-05  9:31 [PATCH 0/2] usb: dwc2: fix the uncompleted requests problem Robert Baldyga
@ 2015-01-05  9:31 ` Robert Baldyga
  2015-01-05  9:31 ` [PATCH 2/2] drivers: usb: dwc2: remove 'force' parameter from kill_all_requests() Robert Baldyga
  2015-01-20  6:12 ` [PATCH 0/2] usb: dwc2: fix the uncompleted requests problem Robert Baldyga
  2 siblings, 0 replies; 7+ messages in thread
From: Robert Baldyga @ 2015-01-05  9:31 UTC (permalink / raw)
  To: balbi, paulz; +Cc: gregkh, linux-usb, linux-kernel, Robert Baldyga

This reverts commit 62f4f0651ce8ef966a0e5b6db6a7a524c268fdd2.

This patch is incomplete solution. It won't work without changes in udc-core,
which were considered as unacceptable (discussion [1]).

[1] https://lkml.org/lkml/2014/12/12/360

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/dwc2/gadget.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 7924200..200168e 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2567,7 +2567,7 @@ error:
  * s3c_hsotg_ep_disable - disable given endpoint
  * @ep: The endpoint to disable.
  */
-static int s3c_hsotg_ep_disable_force(struct usb_ep *ep, bool force)
+static int s3c_hsotg_ep_disable(struct usb_ep *ep)
 {
 	struct s3c_hsotg_ep *hs_ep = our_ep(ep);
 	struct dwc2_hsotg *hsotg = hs_ep->parent;
@@ -2588,7 +2588,7 @@ static int s3c_hsotg_ep_disable_force(struct usb_ep *ep, bool force)
 
 	spin_lock_irqsave(&hsotg->lock, flags);
 	/* terminate all requests with shutdown */
-	kill_all_requests(hsotg, hs_ep, -ESHUTDOWN, force);
+	kill_all_requests(hsotg, hs_ep, -ESHUTDOWN, false);
 
 	hsotg->fifo_map &= ~(1<<hs_ep->fifo_index);
 	hs_ep->fifo_index = 0;
@@ -2609,10 +2609,6 @@ static int s3c_hsotg_ep_disable_force(struct usb_ep *ep, bool force)
 	return 0;
 }
 
-static int s3c_hsotg_ep_disable(struct usb_ep *ep)
-{
-	return s3c_hsotg_ep_disable_force(ep, false);
-}
 /**
  * on_list - check request is on the given endpoint
  * @ep: The endpoint to check.
@@ -2928,7 +2924,7 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget)
 
 	/* all endpoints should be shutdown */
 	for (ep = 1; ep < hsotg->num_of_eps; ep++)
-		s3c_hsotg_ep_disable_force(&hsotg->eps[ep].ep, true);
+		s3c_hsotg_ep_disable(&hsotg->eps[ep].ep);
 
 	spin_lock_irqsave(&hsotg->lock, flags);
 
-- 
1.9.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/2] drivers: usb: dwc2: remove 'force' parameter from kill_all_requests()
  2015-01-05  9:31 [PATCH 0/2] usb: dwc2: fix the uncompleted requests problem Robert Baldyga
  2015-01-05  9:31 ` [PATCH 1/2] Revert "usb: dwc2: gadget: kill requests with 'force' in s3c_hsotg_udc_stop()" Robert Baldyga
@ 2015-01-05  9:31 ` Robert Baldyga
  2015-01-27 15:42   ` Felipe Balbi
  2015-01-20  6:12 ` [PATCH 0/2] usb: dwc2: fix the uncompleted requests problem Robert Baldyga
  2 siblings, 1 reply; 7+ messages in thread
From: Robert Baldyga @ 2015-01-05  9:31 UTC (permalink / raw)
  To: balbi, paulz; +Cc: gregkh, linux-usb, linux-kernel, Robert Baldyga

This patch fixes in simpler way the bug described in [1] and [2]. It
looks like DWC2 is the only UDC driver that doesn't force usb requests
to complete in ep_disable() function. This causes described problem,
because we have no guarantee that all requests will be completed before
unbind of usb function.

To fix this problem we force all requests of disabled endpoint to complete.
Also currently running request is not handled. This allowed to simplify
code of kill_all_requests() function, because 'force' parameter is always
set to true, so we don't need it anymore.

In s3c_hsotg_rx_data() we change function used to print message when active
request is NULL from dev_warn() to dev_dbg(), because such situation is
harmless for driver and now it can take place during normal endpoint
disabling.

[1] https://lkml.org/lkml/2014/12/9/283
[2] https://lkml.org/lkml/2014/12/12/360

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/dwc2/gadget.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 200168e..fee0ad5 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -1305,7 +1305,7 @@ static void s3c_hsotg_rx_data(struct dwc2_hsotg *hsotg, int ep_idx, int size)
 		u32 epctl = readl(hsotg->regs + DOEPCTL(ep_idx));
 		int ptr;
 
-		dev_warn(hsotg->dev,
+		dev_dbg(hsotg->dev,
 			 "%s: FIFO %d bytes on ep%d but no req (DXEPCTl=0x%08x)\n",
 			 __func__, size, ep_idx, epctl);
 
@@ -1988,30 +1988,23 @@ static void s3c_hsotg_irq_enumdone(struct dwc2_hsotg *hsotg)
  * @hsotg: The device state.
  * @ep: The endpoint the requests may be on.
  * @result: The result code to use.
- * @force: Force removal of any current requests
  *
  * Go through the requests on the given endpoint and mark them
  * completed with the given result code.
  */
 static void kill_all_requests(struct dwc2_hsotg *hsotg,
 			      struct s3c_hsotg_ep *ep,
-			      int result, bool force)
+			      int result)
 {
 	struct s3c_hsotg_req *req, *treq;
 	unsigned size;
 
-	list_for_each_entry_safe(req, treq, &ep->queue, queue) {
-		/*
-		 * currently, we can't do much about an already
-		 * running request on an in endpoint
-		 */
-
-		if (ep->req == req && ep->dir_in && !force)
-			continue;
+	ep->req = NULL;
 
+	list_for_each_entry_safe(req, treq, &ep->queue, queue)
 		s3c_hsotg_complete_request(hsotg, ep, req,
 					   result);
-	}
+
 	if (!hsotg->dedicated_fifos)
 		return;
 	size = (readl(hsotg->regs + DTXFSTS(ep->index)) & 0xffff) * 4;
@@ -2036,7 +2029,7 @@ void s3c_hsotg_disconnect(struct dwc2_hsotg *hsotg)
 
 	hsotg->connected = 0;
 	for (ep = 0; ep < hsotg->num_of_eps; ep++)
-		kill_all_requests(hsotg, &hsotg->eps[ep], -ESHUTDOWN, true);
+		kill_all_requests(hsotg, &hsotg->eps[ep], -ESHUTDOWN);
 
 	call_gadget(hsotg, disconnect);
 }
@@ -2334,7 +2327,7 @@ irq_retry:
 				       msecs_to_jiffies(200))) {
 
 				kill_all_requests(hsotg, &hsotg->eps[0],
-							  -ECONNRESET, true);
+							  -ECONNRESET);
 
 				s3c_hsotg_core_init_disconnected(hsotg);
 				s3c_hsotg_core_connect(hsotg);
@@ -2588,7 +2581,7 @@ static int s3c_hsotg_ep_disable(struct usb_ep *ep)
 
 	spin_lock_irqsave(&hsotg->lock, flags);
 	/* terminate all requests with shutdown */
-	kill_all_requests(hsotg, hs_ep, -ESHUTDOWN, false);
+	kill_all_requests(hsotg, hs_ep, -ESHUTDOWN);
 
 	hsotg->fifo_map &= ~(1<<hs_ep->fifo_index);
 	hs_ep->fifo_index = 0;
-- 
1.9.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/2] usb: dwc2: fix the uncompleted requests problem
  2015-01-05  9:31 [PATCH 0/2] usb: dwc2: fix the uncompleted requests problem Robert Baldyga
  2015-01-05  9:31 ` [PATCH 1/2] Revert "usb: dwc2: gadget: kill requests with 'force' in s3c_hsotg_udc_stop()" Robert Baldyga
  2015-01-05  9:31 ` [PATCH 2/2] drivers: usb: dwc2: remove 'force' parameter from kill_all_requests() Robert Baldyga
@ 2015-01-20  6:12 ` Robert Baldyga
  2 siblings, 0 replies; 7+ messages in thread
From: Robert Baldyga @ 2015-01-20  6:12 UTC (permalink / raw)
  To: balbi, paulz; +Cc: gregkh, linux-usb, linux-kernel

Hi Felipe,

I can't see these patches in your tree. I see only "usb: dwc2: gadget:
kill requests with 'force' in s3c_hsotg_udc_stop()", which doesn't fix
the bug completely. I was sure that my the final patch was in your tree,
so what happened? Can you apply these patches to fix this problem?

On 01/05/2015 10:31 AM, Robert Baldyga wrote:
> Hi Felipe,
> 
> I prepared this patchset in response to our discussion:
> https://lkml.org/lkml/2014/12/29/193
> 
> It reverts of unproper patch and adds working solution of the problem
> described in commit messages.
> 
> Best regards,
> Robert Baldyga
> 
> Robert Baldyga (2):
>   Revert "usb: dwc2: gadget: kill requests with 'force' in
>     s3c_hsotg_udc_stop()"
>   drivers: usb: dwc2: remove 'force' parameter from kill_all_requests()
> 
>  drivers/usb/dwc2/gadget.c | 31 ++++++++++---------------------
>  1 file changed, 10 insertions(+), 21 deletions(-)
> 

Best regards,
Robert Baldyga

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] drivers: usb: dwc2: remove 'force' parameter from kill_all_requests()
  2015-01-05  9:31 ` [PATCH 2/2] drivers: usb: dwc2: remove 'force' parameter from kill_all_requests() Robert Baldyga
@ 2015-01-27 15:42   ` Felipe Balbi
  2015-01-28  7:18     ` Robert Baldyga
  0 siblings, 1 reply; 7+ messages in thread
From: Felipe Balbi @ 2015-01-27 15:42 UTC (permalink / raw)
  To: Robert Baldyga; +Cc: balbi, paulz, gregkh, linux-usb, linux-kernel

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

On Mon, Jan 05, 2015 at 10:31:35AM +0100, Robert Baldyga wrote:
> This patch fixes in simpler way the bug described in [1] and [2]. It
> looks like DWC2 is the only UDC driver that doesn't force usb requests
> to complete in ep_disable() function. This causes described problem,
> because we have no guarantee that all requests will be completed before
> unbind of usb function.
> 
> To fix this problem we force all requests of disabled endpoint to complete.
> Also currently running request is not handled. This allowed to simplify
> code of kill_all_requests() function, because 'force' parameter is always
> set to true, so we don't need it anymore.
> 
> In s3c_hsotg_rx_data() we change function used to print message when active
> request is NULL from dev_warn() to dev_dbg(), because such situation is
> harmless for driver and now it can take place during normal endpoint
> disabling.
> 
> [1] https://lkml.org/lkml/2014/12/9/283
> [2] https://lkml.org/lkml/2014/12/12/360
> 
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>

this fails to apply:

checking file drivers/usb/dwc2/gadget.c
Hunk #1 succeeded at 2602 (offset 35 lines).
Hunk #2 FAILED at 2588.
Hunk #3 succeeded at 2645 (offset 36 lines).
Hunk #4 FAILED at 2924.
2 out of 4 hunks FAILED

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] drivers: usb: dwc2: remove 'force' parameter from kill_all_requests()
  2015-01-27 15:42   ` Felipe Balbi
@ 2015-01-28  7:18     ` Robert Baldyga
  2015-01-28 14:08       ` Felipe Balbi
  0 siblings, 1 reply; 7+ messages in thread
From: Robert Baldyga @ 2015-01-28  7:18 UTC (permalink / raw)
  To: balbi; +Cc: paulz, gregkh, linux-usb, linux-kernel

On 01/27/2015 04:42 PM, Felipe Balbi wrote:
> On Mon, Jan 05, 2015 at 10:31:35AM +0100, Robert Baldyga wrote:
>> This patch fixes in simpler way the bug described in [1] and [2]. It
>> looks like DWC2 is the only UDC driver that doesn't force usb requests
>> to complete in ep_disable() function. This causes described problem,
>> because we have no guarantee that all requests will be completed before
>> unbind of usb function.
>>
>> To fix this problem we force all requests of disabled endpoint to complete.
>> Also currently running request is not handled. This allowed to simplify
>> code of kill_all_requests() function, because 'force' parameter is always
>> set to true, so we don't need it anymore.
>>
>> In s3c_hsotg_rx_data() we change function used to print message when active
>> request is NULL from dev_warn() to dev_dbg(), because such situation is
>> harmless for driver and now it can take place during normal endpoint
>> disabling.
>>
>> [1] https://lkml.org/lkml/2014/12/9/283
>> [2] https://lkml.org/lkml/2014/12/12/360
>>
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> 
> this fails to apply:
> 
> checking file drivers/usb/dwc2/gadget.c
> Hunk #1 succeeded at 2602 (offset 35 lines).
> Hunk #2 FAILED at 2588.
> Hunk #3 succeeded at 2645 (offset 36 lines).
> Hunk #4 FAILED at 2924.
> 2 out of 4 hunks FAILED
> 

Isn't this patch already in linux-next? I see it's also in your 'next'
branch.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] drivers: usb: dwc2: remove 'force' parameter from kill_all_requests()
  2015-01-28  7:18     ` Robert Baldyga
@ 2015-01-28 14:08       ` Felipe Balbi
  0 siblings, 0 replies; 7+ messages in thread
From: Felipe Balbi @ 2015-01-28 14:08 UTC (permalink / raw)
  To: Robert Baldyga; +Cc: balbi, paulz, gregkh, linux-usb, linux-kernel

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

On Wed, Jan 28, 2015 at 08:18:30AM +0100, Robert Baldyga wrote:
> On 01/27/2015 04:42 PM, Felipe Balbi wrote:
> > On Mon, Jan 05, 2015 at 10:31:35AM +0100, Robert Baldyga wrote:
> >> This patch fixes in simpler way the bug described in [1] and [2]. It
> >> looks like DWC2 is the only UDC driver that doesn't force usb requests
> >> to complete in ep_disable() function. This causes described problem,
> >> because we have no guarantee that all requests will be completed before
> >> unbind of usb function.
> >>
> >> To fix this problem we force all requests of disabled endpoint to complete.
> >> Also currently running request is not handled. This allowed to simplify
> >> code of kill_all_requests() function, because 'force' parameter is always
> >> set to true, so we don't need it anymore.
> >>
> >> In s3c_hsotg_rx_data() we change function used to print message when active
> >> request is NULL from dev_warn() to dev_dbg(), because such situation is
> >> harmless for driver and now it can take place during normal endpoint
> >> disabling.
> >>
> >> [1] https://lkml.org/lkml/2014/12/9/283
> >> [2] https://lkml.org/lkml/2014/12/12/360
> >>
> >> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> > 
> > this fails to apply:
> > 
> > checking file drivers/usb/dwc2/gadget.c
> > Hunk #1 succeeded at 2602 (offset 35 lines).
> > Hunk #2 FAILED at 2588.
> > Hunk #3 succeeded at 2645 (offset 36 lines).
> > Hunk #4 FAILED at 2924.
> > 2 out of 4 hunks FAILED
> > 
> 
> Isn't this patch already in linux-next? I see it's also in your 'next'
> branch.

if it's there, good.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2015-01-29  1:31 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-05  9:31 [PATCH 0/2] usb: dwc2: fix the uncompleted requests problem Robert Baldyga
2015-01-05  9:31 ` [PATCH 1/2] Revert "usb: dwc2: gadget: kill requests with 'force' in s3c_hsotg_udc_stop()" Robert Baldyga
2015-01-05  9:31 ` [PATCH 2/2] drivers: usb: dwc2: remove 'force' parameter from kill_all_requests() Robert Baldyga
2015-01-27 15:42   ` Felipe Balbi
2015-01-28  7:18     ` Robert Baldyga
2015-01-28 14:08       ` Felipe Balbi
2015-01-20  6:12 ` [PATCH 0/2] usb: dwc2: fix the uncompleted requests problem Robert Baldyga

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).