| Message ID | 20200107071441.480-1-ran.wang_1@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc3: gadget: Fix controller get stuck when kicking extra transfer in wrong case | expand |
Hi, On 1/7/2020 12:44 PM, Ran Wang wrote: > According to original commit c96e6725db9d6a ("usb: dwc3: gadget: Correct the > logic for queuing sgs"), we would only kick off another transfer in case of > req->num_pending_sgs > 0. > The commit 8c7d4b7b3d43 ("usb: dwc3: gadget: Fix logical condition") fixes the commit f38e35dd84e2 (usb: dwc3: gadget: split dwc3_gadget_ep_cleanup_completed_requests()). > However, current logic will do this as long as req->remaining > 0, this will > include the case of non-sgs (both dwc3_gadget_ep_request_completed(req) and > req->num_pending_sgs are 0) that we did not want to. > > Without this fix, we observed dwc3 got stuck on Layerscape plaftorms (such as > LS1088ARDB) when enabling gadget (mass storage function) as below: > Similar issue was reported by Thinh after my fix, and he has submitted a patch for the same. You can refer the discussion https://patchwork.kernel.org/patch/11292087/. > [ 27.923959] Mass Storage Function, version: 2009/09/11 > [ 27.929115] LUN: removable file: (no medium) > [ 27.933432] LUN: file: /run/media/sda1/419/test > [ 27.937963] Number of LUNs=1 > [ 27.941042] g_mass_storage gadget: Mass Storage Gadget, version: 2009/09/11 > [ 27.948019] g_mass_storage gadget: userspace failed to provide iSerialNumber > [ 27.955069] g_mass_storage gadget: g_mass_storage ready > [ 28.411188] g_mass_storage gadget: super-speed config #1: Linux File-Backed Storage > [ 48.319766] g_mass_storage gadget: super-speed config #1: Linux File-Backed Storage > [ 68.320794] g_mass_storage gadget: super-speed config #1: Linux File-Backed Storage > [ 88.319898] g_mass_storage gadget: super-speed config #1: Linux File-Backed Storage > [ 108.320808] g_mass_storage gadget: super-speed config #1: Linux File-Backed Storage > [ 128.323419] g_mass_storage gadget: super-speed config #1: Linux File-Backed Storage > [ 148.320857] g_mass_storage gadget: super-speed config #1: Linux File-Backed Storage > [ 148.362023] g_mass_storage gadget: super-speed config #0: unconfigured > > Fixes: 8c7d4b7b3d43 ("usb: dwc3: gadget: Fix logical condition") > > Cc: stable@vger.kernel.org > Signed-off-by: Ran Wang <ran.wang_1@nxp.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 0c960a9..5b0f02f 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2491,7 +2491,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
Hi Tejas, On Tuesday, January 07, 2020 18:16, Tejas Joglekar wrote: > Hi, > On 1/7/2020 12:44 PM, Ran Wang wrote: > > According to original commit c96e6725db9d6a ("usb: dwc3: gadget: > > Correct the logic for queuing sgs"), we would only kick off another > > transfer in case of > > req->num_pending_sgs > 0. > > > The commit 8c7d4b7b3d43 ("usb: dwc3: gadget: Fix logical condition") fixes > the commit f38e35dd84e2 (usb: dwc3: gadget: split > dwc3_gadget_ep_cleanup_completed_requests()). Yes, actually I have tried to dig the history of this implementation as much as I can. and the commit I mentioned looks like the first one to add this function. > > However, current logic will do this as long as req->remaining > 0, > > this will include the case of non-sgs (both > > dwc3_gadget_ep_request_completed(req) and > > req->num_pending_sgs are 0) that we did not want to. > > > > Without this fix, we observed dwc3 got stuck on Layerscape plaftorms > > (such as > > LS1088ARDB) when enabling gadget (mass storage function) as below: > > > Similar issue was reported by Thinh after my fix, and he has submitted a > patch for the same. You can refer the discussion > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch > work.kernel.org%2Fpatch%2F11292087%2F&data=02%7C01%7Cran.wan > g_1%40nxp.com%7C97345f38c19849e5a82608d7935a8d31%7C686ea1d3bc2b > 4c6fa92cd99c5c301635%7C0%7C0%7C637139889495749889&sdata=tcV6 > FWjqvYYO7BCByACqAg%2FB%2B%2Fz3NC%2B20%2BIEVuyfJc4%3D&reser > ved=0. Thanks for this information. I will follow that thread. Regards, Ran > > [ 27.923959] Mass Storage Function, version: 2009/09/11 > > [ 27.929115] LUN: removable file: (no medium) > > [ 27.933432] LUN: file: /run/media/sda1/419/test > > [ 27.937963] Number of LUNs=1 > > [ 27.941042] g_mass_storage gadget: Mass Storage Gadget, version: > 2009/09/11 > > [ 27.948019] g_mass_storage gadget: userspace failed to provide > iSerialNumber > > [ 27.955069] g_mass_storage gadget: g_mass_storage ready > > [ 28.411188] g_mass_storage gadget: super-speed config #1: Linux File- > Backed Storage > > [ 48.319766] g_mass_storage gadget: super-speed config #1: Linux File- > Backed Storage > > [ 68.320794] g_mass_storage gadget: super-speed config #1: Linux File- > Backed Storage > > [ 88.319898] g_mass_storage gadget: super-speed config #1: Linux File- > Backed Storage > > [ 108.320808] g_mass_storage gadget: super-speed config #1: Linux > > File-Backed Storage [ 128.323419] g_mass_storage gadget: super-speed > > config #1: Linux File-Backed Storage [ 148.320857] g_mass_storage > > gadget: super-speed config #1: Linux File-Backed Storage [ > > 148.362023] g_mass_storage gadget: super-speed config #0: unconfigured > > > > Fixes: 8c7d4b7b3d43 ("usb: dwc3: gadget: Fix logical condition") > > > > Cc: stable@vger.kernel.org > > Signed-off-by: Ran Wang <ran.wang_1@nxp.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 0c960a9..5b0f02f 100644 > > --- a/drivers/usb/dwc3/gadget.c > > +++ b/drivers/usb/dwc3/gadget.c > > @@ -2491,7 +2491,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
Hi, (Tejas, please break your lines at 80-columns. You shouldn't have to be reminded of this) Tejas Joglekar <Tejas.Joglekar@synopsys.com> writes: > Hi, > On 1/7/2020 12:44 PM, Ran Wang wrote: >> According to original commit c96e6725db9d6a ("usb: dwc3: gadget: Correct the >> logic for queuing sgs"), we would only kick off another transfer in case of >> req->num_pending_sgs > 0. >> > The commit 8c7d4b7b3d43 ("usb: dwc3: gadget: Fix logical condition") > fixes the commit f38e35dd84e2 (usb: dwc3: gadget: split > dwc3_gadget_ep_cleanup_completed_requests()). > >> However, current logic will do this as long as req->remaining > 0, this will >> include the case of non-sgs (both dwc3_gadget_ep_request_completed(req) and >> req->num_pending_sgs are 0) that we did not want to. >> >> Without this fix, we observed dwc3 got stuck on Layerscape plaftorms (such as >> LS1088ARDB) when enabling gadget (mass storage function) as below: >> > Similar issue was reported by Thinh after my fix, and he has submitted > a patch for the same. You can refer the discussion > https://patchwork.kernel.org/patch/11292087/. Based on this, I'm assuming we don't need this patch.
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 0c960a9..5b0f02f 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2491,7 +2491,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;
According to original commit c96e6725db9d6a ("usb: dwc3: gadget: Correct the logic for queuing sgs"), we would only kick off another transfer in case of req->num_pending_sgs > 0. However, current logic will do this as long as req->remaining > 0, this will include the case of non-sgs (both dwc3_gadget_ep_request_completed(req) and req->num_pending_sgs are 0) that we did not want to. Without this fix, we observed dwc3 got stuck on Layerscape plaftorms (such as LS1088ARDB) when enabling gadget (mass storage function) as below: [ 27.923959] Mass Storage Function, version: 2009/09/11 [ 27.929115] LUN: removable file: (no medium) [ 27.933432] LUN: file: /run/media/sda1/419/test [ 27.937963] Number of LUNs=1 [ 27.941042] g_mass_storage gadget: Mass Storage Gadget, version: 2009/09/11 [ 27.948019] g_mass_storage gadget: userspace failed to provide iSerialNumber [ 27.955069] g_mass_storage gadget: g_mass_storage ready [ 28.411188] g_mass_storage gadget: super-speed config #1: Linux File-Backed Storage [ 48.319766] g_mass_storage gadget: super-speed config #1: Linux File-Backed Storage [ 68.320794] g_mass_storage gadget: super-speed config #1: Linux File-Backed Storage [ 88.319898] g_mass_storage gadget: super-speed config #1: Linux File-Backed Storage [ 108.320808] g_mass_storage gadget: super-speed config #1: Linux File-Backed Storage [ 128.323419] g_mass_storage gadget: super-speed config #1: Linux File-Backed Storage [ 148.320857] g_mass_storage gadget: super-speed config #1: Linux File-Backed Storage [ 148.362023] g_mass_storage gadget: super-speed config #0: unconfigured Fixes: 8c7d4b7b3d43 ("usb: dwc3: gadget: Fix logical condition") Cc: stable@vger.kernel.org Signed-off-by: Ran Wang <ran.wang_1@nxp.com> --- drivers/usb/dwc3/gadget.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)