* [PATCH] usb: dwc3: gadget: Fix logical condition
@ 2019-11-13 6:15 Tejas Joglekar
2019-11-22 4:00 ` Tejas Joglekar
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Tejas Joglekar @ 2019-11-13 6:15 UTC (permalink / raw)
To: Felipe Balbi, linux-usb; +Cc: John Youn, Tejas Joglekar, Thinh Nguyen, stable
This patch corrects the condition to kick the transfer without
giving back the requests when either request has remaining data
or when there are pending SGs. The && check was introduced during
spliting up the dwc3_gadget_ep_cleanup_completed_requests() function.
Fixes: f38e35dd84e2 ("usb: dwc3: gadget: split dwc3_gadget_ep_cleanup_completed_requests()")
Cc: stable@vger.kernel.org
Signed-off-by: Tejas Joglekar <joglekar@synopsys.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 86dc1db788a9..e07159e06f9a 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2485,7 +2485,7 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
req->request.actual = req->request.length - req->remaining;
- if (!dwc3_gadget_ep_request_completed(req) &&
+ if (!dwc3_gadget_ep_request_completed(req) ||
req->num_pending_sgs) {
__dwc3_gadget_kick_transfer(dep);
goto out;
--
2.11.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] usb: dwc3: gadget: Fix logical condition 2019-11-13 6:15 [PATCH] usb: dwc3: gadget: Fix logical condition Tejas Joglekar @ 2019-11-22 4:00 ` Tejas Joglekar 2019-12-02 11:30 ` Tejas Joglekar 2019-12-03 13:58 ` Felipe Balbi 2020-01-31 3:25 ` Jack Pham 2 siblings, 1 reply; 6+ messages in thread From: Tejas Joglekar @ 2019-11-22 4:00 UTC (permalink / raw) To: Felipe Balbi, linux-usb@vger.kernel.org Cc: John Youn, Thinh Nguyen, stable@vger.kernel.org, greg@kroah.com Hello Balbi, This is a critical patch fix for dwc3, can you please review ? On 11/13/2019 11:45 AM, Tejas Joglekar wrote: > This patch corrects the condition to kick the transfer without > giving back the requests when either request has remaining data > or when there are pending SGs. The && check was introduced during > spliting up the dwc3_gadget_ep_cleanup_completed_requests() function. > > Fixes: f38e35dd84e2 ("usb: dwc3: gadget: split dwc3_gadget_ep_cleanup_completed_requests()") > > Cc: stable@vger.kernel.org > Signed-off-by: Tejas Joglekar <joglekar@synopsys.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 86dc1db788a9..e07159e06f9a 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2485,7 +2485,7 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, > > req->request.actual = req->request.length - req->remaining; > > - if (!dwc3_gadget_ep_request_completed(req) && > + if (!dwc3_gadget_ep_request_completed(req) || > req->num_pending_sgs) { > __dwc3_gadget_kick_transfer(dep); > goto out; Thanks & Regards, Tejas Joglekar ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: dwc3: gadget: Fix logical condition 2019-11-22 4:00 ` Tejas Joglekar @ 2019-12-02 11:30 ` Tejas Joglekar 0 siblings, 0 replies; 6+ messages in thread From: Tejas Joglekar @ 2019-12-02 11:30 UTC (permalink / raw) To: Felipe Balbi, linux-usb@vger.kernel.org Cc: Tejas Joglekar, John Youn, Thinh Nguyen, stable@vger.kernel.org, greg@kroah.com Hello, Gentle reminder... On 11/22/2019 9:30 AM, Tejas Joglekar wrote: > Hello Balbi, > This is a critical patch fix for dwc3, can you please review ? > > On 11/13/2019 11:45 AM, Tejas Joglekar wrote: >> This patch corrects the condition to kick the transfer without >> giving back the requests when either request has remaining data >> or when there are pending SGs. The && check was introduced during >> spliting up the dwc3_gadget_ep_cleanup_completed_requests() function. >> >> Fixes: f38e35dd84e2 ("usb: dwc3: gadget: split dwc3_gadget_ep_cleanup_completed_requests()") >> >> Cc: stable@vger.kernel.org >> Signed-off-by: Tejas Joglekar <joglekar@synopsys.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 86dc1db788a9..e07159e06f9a 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -2485,7 +2485,7 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, >> >> req->request.actual = req->request.length - req->remaining; >> >> - if (!dwc3_gadget_ep_request_completed(req) && >> + if (!dwc3_gadget_ep_request_completed(req) || >> req->num_pending_sgs) { >> __dwc3_gadget_kick_transfer(dep); >> goto out; > Thanks & Regards, > > Tejas Joglekar > Thanks, Tejas Joglekar ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: dwc3: gadget: Fix logical condition 2019-11-13 6:15 [PATCH] usb: dwc3: gadget: Fix logical condition Tejas Joglekar 2019-11-22 4:00 ` Tejas Joglekar @ 2019-12-03 13:58 ` Felipe Balbi 2020-01-31 3:25 ` Jack Pham 2 siblings, 0 replies; 6+ messages in thread From: Felipe Balbi @ 2019-12-03 13:58 UTC (permalink / raw) To: Tejas Joglekar, linux-usb; +Cc: John Youn, Tejas Joglekar, Thinh Nguyen, stable [-- Attachment #1: Type: text/plain, Size: 605 bytes --] Hi, Tejas Joglekar <Tejas.Joglekar@synopsys.com> writes: > This patch corrects the condition to kick the transfer without > giving back the requests when either request has remaining data > or when there are pending SGs. The && check was introduced during > spliting up the dwc3_gadget_ep_cleanup_completed_requests() function. > > Fixes: f38e35dd84e2 ("usb: dwc3: gadget: split dwc3_gadget_ep_cleanup_completed_requests()") > > Cc: stable@vger.kernel.org > Signed-off-by: Tejas Joglekar <joglekar@synopsys.com> Indeed this is a very important fix :-) I'll queue it up. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: dwc3: gadget: Fix logical condition 2019-11-13 6:15 [PATCH] usb: dwc3: gadget: Fix logical condition Tejas Joglekar 2019-11-22 4:00 ` Tejas Joglekar 2019-12-03 13:58 ` Felipe Balbi @ 2020-01-31 3:25 ` Jack Pham 2020-01-31 8:07 ` Felipe Balbi 2 siblings, 1 reply; 6+ messages in thread From: Jack Pham @ 2020-01-31 3:25 UTC (permalink / raw) To: Tejas Joglekar; +Cc: Felipe Balbi, linux-usb, John Youn, Thinh Nguyen, stable Hi Tejas & Felipe, On Wed, Nov 13, 2019 at 11:45:16AM +0530, Tejas Joglekar wrote: > This patch corrects the condition to kick the transfer without > giving back the requests when either request has remaining data > or when there are pending SGs. The && check was introduced during > spliting up the dwc3_gadget_ep_cleanup_completed_requests() function. > > Fixes: f38e35dd84e2 ("usb: dwc3: gadget: split dwc3_gadget_ep_cleanup_completed_requests()") > > Cc: stable@vger.kernel.org > Signed-off-by: Tejas Joglekar <joglekar@synopsys.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 86dc1db788a9..e07159e06f9a 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2485,7 +2485,7 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, > > req->request.actual = req->request.length - req->remaining; > > - if (!dwc3_gadget_ep_request_completed(req) && > + if (!dwc3_gadget_ep_request_completed(req) || > req->num_pending_sgs) { > __dwc3_gadget_kick_transfer(dep); > goto out; Been staring at this for a while--I think I see a potential issue but not sure if it is or not. If this condition is true and causes an early return, the 'ret' value could be 0 which could allow the caller in cleanup_completed_requests() to continue looping over the started_list and calling cleanup_completed_request() again on the next req. But we just issued another START or UPDATE transfer command on the previous incomplete req and now the loop continued to try to reclaim the next TRB (and increment the dequeue pointer and whatnot) when it might actually be in progress. According to the code before f38e35dd84e2, list_for_each_entry_safe(req, tmp, &dep->started_list, list) { ... if (!dwc3_gadget_ep_request_completed(req) || req->num_pending_sgs) { __dwc3_gadget_kick_transfer(dep); break; } The 'goto out' used to be a 'break', which terminates the list loop. But with the refactored code, the loop can only terminate if 'ret' is non-zero. I haven't seen any real issue with the code as-is yet, but was just wondering if the 'goto out' should be replaced with a return 1? Jack -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: dwc3: gadget: Fix logical condition 2020-01-31 3:25 ` Jack Pham @ 2020-01-31 8:07 ` Felipe Balbi 0 siblings, 0 replies; 6+ messages in thread From: Felipe Balbi @ 2020-01-31 8:07 UTC (permalink / raw) To: Jack Pham, Tejas Joglekar; +Cc: linux-usb, John Youn, Thinh Nguyen, stable [-- Attachment #1: Type: text/plain, Size: 2601 bytes --] Hi, Jack Pham <jackp@codeaurora.org> writes: > Hi Tejas & Felipe, > > On Wed, Nov 13, 2019 at 11:45:16AM +0530, Tejas Joglekar wrote: >> This patch corrects the condition to kick the transfer without >> giving back the requests when either request has remaining data >> or when there are pending SGs. The && check was introduced during >> spliting up the dwc3_gadget_ep_cleanup_completed_requests() function. >> >> Fixes: f38e35dd84e2 ("usb: dwc3: gadget: split dwc3_gadget_ep_cleanup_completed_requests()") >> >> Cc: stable@vger.kernel.org >> Signed-off-by: Tejas Joglekar <joglekar@synopsys.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 86dc1db788a9..e07159e06f9a 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -2485,7 +2485,7 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, >> >> req->request.actual = req->request.length - req->remaining; >> >> - if (!dwc3_gadget_ep_request_completed(req) && >> + if (!dwc3_gadget_ep_request_completed(req) || >> req->num_pending_sgs) { >> __dwc3_gadget_kick_transfer(dep); >> goto out; > > Been staring at this for a while--I think I see a potential issue but > not sure if it is or not. > > If this condition is true and causes an early return, the 'ret' value > could be 0 which could allow the caller in cleanup_completed_requests() > to continue looping over the started_list and calling > cleanup_completed_request() again on the next req. But we just issued > another START or UPDATE transfer command on the previous incomplete req > and now the loop continued to try to reclaim the next TRB (and increment > the dequeue pointer and whatnot) when it might actually be in progress. > > According to the code before f38e35dd84e2, > > list_for_each_entry_safe(req, tmp, &dep->started_list, list) { > > ... > if (!dwc3_gadget_ep_request_completed(req) || > req->num_pending_sgs) { > __dwc3_gadget_kick_transfer(dep); > break; > } > > The 'goto out' used to be a 'break', which terminates the list loop. But > with the refactored code, the loop can only terminate if 'ret' is > non-zero. ret is initialized properly by dwc3_gadget_ep_reclaim*(). That goto is correct. > I haven't seen any real issue with the code as-is yet, but was just > wondering if the 'goto out' should be replaced with a return 1? let us know if you find any problems -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-01-31 8:08 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-11-13 6:15 [PATCH] usb: dwc3: gadget: Fix logical condition Tejas Joglekar 2019-11-22 4:00 ` Tejas Joglekar 2019-12-02 11:30 ` Tejas Joglekar 2019-12-03 13:58 ` Felipe Balbi 2020-01-31 3:25 ` Jack Pham 2020-01-31 8:07 ` Felipe Balbi
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.