Re: [PATCH v2 4/6] usb: dwc3: gadget: add remaining sg entries to ring
Hi, John Youn writes: >> Felipe Balbi writes: On your testing/next, I see considerable slow down and file corruption in mass storage. After bisecting, this patch seems to be the first one that shows problems. The device fails even enumeration here. I haven't looked in detail at the changes but, do you have any ideas? I have attached driver logs. >>> >>> g_mass_storage doesn't use sglists, so I find this to be unlikely. I'll >>> test again but I didn't notice any problems on my side. >> >> Few things: >> >> Host sent you a few Resets which I don't know why they're there: >> >> irq/16-dwc3-2849 [002] d..245.257835: dwc3_event: event >> (0101): Reset [U0] >> >> [...] >> >> irq/16-dwc3-2849 [002] d..245.352847: dwc3_event: event >> (0101): Reset [U0] >> >> [...] >> >> irq/16-dwc3-2849 [002] d..245.257835: dwc3_event: event >> (0101): Reset [U0] >> >> [...] >> >> 4 requests per endpoint, why so few? To test throughput, I've been using >> 96 requests per endpoint :-p > > Any trick to get it to do that? We haven't really used the > g_mass_storage for performance testing yet. Change to CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS 115 config USB_GADGET_STORAGE_NUM_BUFFERS 116 int "Number of storage pipeline buffers" 117 range 2 256 118 default 2 119 help 120Usually 2 buffers are enough to establish a good buffering 121pipeline. The number may be increased in order to compensate 122for a bursty VFS behaviour. For instance there may be CPU wake up 123latencies that makes the VFS to appear bursty in a system with 124an CPU on-demand governor. Especially if DMA is doing IO to 125offload the CPU. In this case the CPU will go into power 126save often and spin up occasionally to move data within VFS. 127If selecting USB_GADGET_DEBUG_FILES this value may be set by 128a module parameter as well. 129If unsure, say 2. -- balbi signature.asc Description: PGP signature
Re: [PATCH v2 4/6] usb: dwc3: gadget: add remaining sg entries to ring
On 8/25/2016 12:45 AM, Felipe Balbi wrote: > > Hi, > > Felipe Balbi writes: >>> On your testing/next, I see considerable slow down and file >>> corruption in mass storage. >>> >>> After bisecting, this patch seems to be the first one that shows >>> problems. The device fails even enumeration here. >>> >>> I haven't looked in detail at the changes but, do you have any ideas? >>> >>> I have attached driver logs. >> >> g_mass_storage doesn't use sglists, so I find this to be unlikely. I'll >> test again but I didn't notice any problems on my side. > > Few things: > > Host sent you a few Resets which I don't know why they're there: > > irq/16-dwc3-2849 [002] d..245.257835: dwc3_event: event (0101): > Reset [U0] > > [...] > > irq/16-dwc3-2849 [002] d..245.352847: dwc3_event: event (0101): > Reset [U0] > > [...] > > irq/16-dwc3-2849 [002] d..245.257835: dwc3_event: event (0101): > Reset [U0] > > [...] > > 4 requests per endpoint, why so few? To test throughput, I've been using > 96 requests per endpoint :-p Any trick to get it to do that? We haven't really used the g_mass_storage for performance testing yet. Regards, John -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/6] usb: dwc3: gadget: add remaining sg entries to ring
Hi, Felipe Balbi writes: > Felipe Balbi writes: >> Felipe Balbi writes: On your testing/next, I see considerable slow down and file corruption in mass storage. After bisecting, this patch seems to be the first one that shows problems. The device fails even enumeration here. I haven't looked in detail at the changes but, do you have any ideas? I have attached driver logs. >>> >>> g_mass_storage doesn't use sglists, so I find this to be unlikely. I'll >>> test again but I didn't notice any problems on my side. >> >> Few things: >> >> Host sent you a few Resets which I don't know why they're there: >> >> irq/16-dwc3-2849 [002] d..245.257835: dwc3_event: event >> (0101): Reset [U0] >> >> [...] >> >> irq/16-dwc3-2849 [002] d..245.352847: dwc3_event: event >> (0101): Reset [U0] >> >> [...] >> >> irq/16-dwc3-2849 [002] d..245.257835: dwc3_event: event >> (0101): Reset [U0] >> >> [...] >> >> 4 requests per endpoint, why so few? To test throughput, I've been using >> 96 requests per endpoint :-p >> >> file-storage-2848 [003] ...145.319500: dwc3_alloc_request: ep1in: >> req 8ba68ead9ce8 length 0/0 zsI ==> 0 >> file-storage-2848 [003] ...145.319505: dwc3_alloc_request: ep1out: >> req 8ba68eadad68 length 0/0 zsI ==> 0 >> file-storage-2848 [003] ...145.319510: dwc3_alloc_request: ep1in: >> req 8ba68ead9ef8 length 0/0 zsI ==> 0 >> file-storage-2848 [003] ...145.319514: dwc3_alloc_request: ep1out: >> req 8ba68ead98c8 length 0/0 zsI ==> 0 >> file-storage-2848 [003] ...145.319519: dwc3_alloc_request: ep1in: >> req 8ba68eadb9c8 length 0/0 zsI ==> 0 >> file-storage-2848 [003] ...145.319524: dwc3_alloc_request: ep1out: >> req 8ba68eadb5a8 length 0/0 zsI ==> 0 >> file-storage-2848 [003] ...145.319528: dwc3_alloc_request: ep1in: >> req 8ba68eadb188 length 0/0 zsI ==> 0 >> file-storage-2848 [003] ...145.319533: dwc3_alloc_request: ep1out: >> req 8ba68ead9ad8 length 0/0 zsI ==> 0 >> >> In fact, there are no bulk ep transfers on your log. Why's that? What is >> the host doing? > > I can reproduce this here and I'm now looking at it. Here's an incremental patch to update $subject. It's working fine on SKL. diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 27cf4d74db06..37a86522fa88 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1980,6 +1980,9 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep, return __dwc3_gadget_kick_transfer(dep, 0); dwc3_gadget_giveback(dep, req, status); + + if (ret) + break; } /* I have force-pushed testing/next with this update too. -- balbi signature.asc Description: PGP signature
Re: [PATCH v2 4/6] usb: dwc3: gadget: add remaining sg entries to ring
Hi, Felipe Balbi writes: > Felipe Balbi writes: >>> On your testing/next, I see considerable slow down and file >>> corruption in mass storage. >>> >>> After bisecting, this patch seems to be the first one that shows >>> problems. The device fails even enumeration here. >>> >>> I haven't looked in detail at the changes but, do you have any ideas? >>> >>> I have attached driver logs. >> >> g_mass_storage doesn't use sglists, so I find this to be unlikely. I'll >> test again but I didn't notice any problems on my side. > > Few things: > > Host sent you a few Resets which I don't know why they're there: > > irq/16-dwc3-2849 [002] d..245.257835: dwc3_event: event (0101): > Reset [U0] > > [...] > > irq/16-dwc3-2849 [002] d..245.352847: dwc3_event: event (0101): > Reset [U0] > > [...] > > irq/16-dwc3-2849 [002] d..245.257835: dwc3_event: event (0101): > Reset [U0] > > [...] > > 4 requests per endpoint, why so few? To test throughput, I've been using > 96 requests per endpoint :-p > > file-storage-2848 [003] ...145.319500: dwc3_alloc_request: ep1in: > req 8ba68ead9ce8 length 0/0 zsI ==> 0 > file-storage-2848 [003] ...145.319505: dwc3_alloc_request: ep1out: > req 8ba68eadad68 length 0/0 zsI ==> 0 > file-storage-2848 [003] ...145.319510: dwc3_alloc_request: ep1in: > req 8ba68ead9ef8 length 0/0 zsI ==> 0 > file-storage-2848 [003] ...145.319514: dwc3_alloc_request: ep1out: > req 8ba68ead98c8 length 0/0 zsI ==> 0 > file-storage-2848 [003] ...145.319519: dwc3_alloc_request: ep1in: > req 8ba68eadb9c8 length 0/0 zsI ==> 0 > file-storage-2848 [003] ...145.319524: dwc3_alloc_request: ep1out: > req 8ba68eadb5a8 length 0/0 zsI ==> 0 > file-storage-2848 [003] ...145.319528: dwc3_alloc_request: ep1in: > req 8ba68eadb188 length 0/0 zsI ==> 0 > file-storage-2848 [003] ...145.319533: dwc3_alloc_request: ep1out: > req 8ba68ead9ad8 length 0/0 zsI ==> 0 > > In fact, there are no bulk ep transfers on your log. Why's that? What is > the host doing? I can reproduce this here and I'm now looking at it. -- balbi signature.asc Description: PGP signature
Re: [PATCH v2 4/6] usb: dwc3: gadget: add remaining sg entries to ring
Hi, Felipe Balbi writes: >> On your testing/next, I see considerable slow down and file >> corruption in mass storage. >> >> After bisecting, this patch seems to be the first one that shows >> problems. The device fails even enumeration here. >> >> I haven't looked in detail at the changes but, do you have any ideas? >> >> I have attached driver logs. > > g_mass_storage doesn't use sglists, so I find this to be unlikely. I'll > test again but I didn't notice any problems on my side. Few things: Host sent you a few Resets which I don't know why they're there: irq/16-dwc3-2849 [002] d..245.257835: dwc3_event: event (0101): Reset [U0] [...] irq/16-dwc3-2849 [002] d..245.352847: dwc3_event: event (0101): Reset [U0] [...] irq/16-dwc3-2849 [002] d..245.257835: dwc3_event: event (0101): Reset [U0] [...] 4 requests per endpoint, why so few? To test throughput, I've been using 96 requests per endpoint :-p file-storage-2848 [003] ...145.319500: dwc3_alloc_request: ep1in: req 8ba68ead9ce8 length 0/0 zsI ==> 0 file-storage-2848 [003] ...145.319505: dwc3_alloc_request: ep1out: req 8ba68eadad68 length 0/0 zsI ==> 0 file-storage-2848 [003] ...145.319510: dwc3_alloc_request: ep1in: req 8ba68ead9ef8 length 0/0 zsI ==> 0 file-storage-2848 [003] ...145.319514: dwc3_alloc_request: ep1out: req 8ba68ead98c8 length 0/0 zsI ==> 0 file-storage-2848 [003] ...145.319519: dwc3_alloc_request: ep1in: req 8ba68eadb9c8 length 0/0 zsI ==> 0 file-storage-2848 [003] ...145.319524: dwc3_alloc_request: ep1out: req 8ba68eadb5a8 length 0/0 zsI ==> 0 file-storage-2848 [003] ...145.319528: dwc3_alloc_request: ep1in: req 8ba68eadb188 length 0/0 zsI ==> 0 file-storage-2848 [003] ...145.319533: dwc3_alloc_request: ep1out: req 8ba68ead9ad8 length 0/0 zsI ==> 0 In fact, there are no bulk ep transfers on your log. Why's that? What is the host doing? -- balbi signature.asc Description: PGP signature
Re: [PATCH v2 4/6] usb: dwc3: gadget: add remaining sg entries to ring
Hi, John Youn writes: > Hi Felipe, > > On 8/15/2016 4:02 AM, Felipe Balbi wrote: >> Upon transfer completion after a full ring, let's >> add more TRBs to our ring in order to complete our >> request successfully. >> >> Signed-off-by: Felipe Balbi >> --- >> drivers/usb/dwc3/gadget.c | 36 >> 1 file changed, 24 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 90b3d7965136..6483991c8013 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -888,14 +888,13 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep) >> static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep, >> struct dwc3_request *req, unsigned int trbs_left) >> { >> -struct usb_request *request = &req->request; >> -struct scatterlist *sg = request->sg; >> +struct scatterlist *sg = req->sg; >> struct scatterlist *s; >> unsigned intlength; >> dma_addr_t dma; >> int i; >> >> -for_each_sg(sg, s, request->num_mapped_sgs, i) { >> +for_each_sg(sg, s, req->num_pending_sgs, i) { >> unsigned chain = true; >> >> length = sg_dma_len(s); >> @@ -945,7 +944,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep) >> return; >> >> list_for_each_entry_safe(req, n, &dep->pending_list, list) { >> -if (req->request.num_mapped_sgs > 0) >> +if (req->num_pending_sgs > 0) >> dwc3_prepare_one_trb_sg(dep, req, trbs_left--); >> else >> dwc3_prepare_one_trb_linear(dep, req, trbs_left--); >> @@ -1071,6 +1070,9 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, >> struct dwc3_request *req) >> if (ret) >> return ret; >> >> +req->sg = req->request.sg; >> +req->num_pending_sgs= req->request.num_mapped_sgs; >> + >> list_add_tail(&req->list, &dep->pending_list); >> >> if (usb_endpoint_xfer_isoc(dep->endpoint.desc) && >> @@ -1935,22 +1937,30 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, >> struct dwc3_ep *dep, >> int ret; >> >> list_for_each_entry_safe(req, n, &dep->started_list, list) { >> - >> +unsigned length; >> +unsigned actual; >> int chain; >> >> -chain = req->request.num_mapped_sgs > 0; >> +length = req->request.length; >> +chain = req->num_pending_sgs > 0; >> if (chain) { >> -struct scatterlist *sg = req->request.sg; >> +struct scatterlist *sg = req->sg; >> struct scatterlist *s; >> +unsigned int pending = req->num_pending_sgs; >> unsigned int i; >> >> -for_each_sg(sg, s, req->request.num_mapped_sgs, i) { >> +for_each_sg(sg, s, pending, i) { >> trb = &dep->trb_pool[dep->trb_dequeue]; >> count += trb->size & DWC3_TRB_SIZE_MASK; >> dwc3_ep_inc_deq(dep); >> >> +req->sg = sg_next(s); >> +req->num_pending_sgs--; >> + >> ret = __dwc3_cleanup_done_trbs(dwc, dep, req, >> trb, >> event, status, chain); >> +if (ret) >> +break; >> } >> } else { >> trb = &dep->trb_pool[dep->trb_dequeue]; >> @@ -1968,11 +1978,13 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, >> struct dwc3_ep *dep, >> * should receive and we simply bounce the request back to the >> * gadget driver for further processing. >> */ >> -req->request.actual += req->request.length - count; >> -dwc3_gadget_giveback(dep, req, status); >> +actual = length - req->request.actual; >> +req->request.actual = actual; >> >> -if (ret) >> -break; >> +if (ret && chain && (actual < length) && req->num_pending_sgs) >> +return __dwc3_gadget_kick_transfer(dep, 0); >> + >> +dwc3_gadget_giveback(dep, req, status); >> } >> >> /* >> > > On your testing/next, I see considerable slow down and file > corruption in mass storage. > > After bisecting, this patch seems to be the first one that shows > problems. The device fails even enumeration here. > > I haven't looked in detail at the changes but, do you have any ideas? > > I have attached driver logs. g_mass_storage doesn't use sglists, so I find this to be unlikely. I'll test again but I didn't notice any problems on my side. -- balbi signature.asc Description: PGP signature
Re: [PATCH v2 4/6] usb: dwc3: gadget: add remaining sg entries to ring
Hi Felipe, On 8/15/2016 4:02 AM, Felipe Balbi wrote: > Upon transfer completion after a full ring, let's > add more TRBs to our ring in order to complete our > request successfully. > > Signed-off-by: Felipe Balbi > --- > drivers/usb/dwc3/gadget.c | 36 > 1 file changed, 24 insertions(+), 12 deletions(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 90b3d7965136..6483991c8013 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -888,14 +888,13 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep) > static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep, > struct dwc3_request *req, unsigned int trbs_left) > { > - struct usb_request *request = &req->request; > - struct scatterlist *sg = request->sg; > + struct scatterlist *sg = req->sg; > struct scatterlist *s; > unsigned intlength; > dma_addr_t dma; > int i; > > - for_each_sg(sg, s, request->num_mapped_sgs, i) { > + for_each_sg(sg, s, req->num_pending_sgs, i) { > unsigned chain = true; > > length = sg_dma_len(s); > @@ -945,7 +944,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep) > return; > > list_for_each_entry_safe(req, n, &dep->pending_list, list) { > - if (req->request.num_mapped_sgs > 0) > + if (req->num_pending_sgs > 0) > dwc3_prepare_one_trb_sg(dep, req, trbs_left--); > else > dwc3_prepare_one_trb_linear(dep, req, trbs_left--); > @@ -1071,6 +1070,9 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, > struct dwc3_request *req) > if (ret) > return ret; > > + req->sg = req->request.sg; > + req->num_pending_sgs= req->request.num_mapped_sgs; > + > list_add_tail(&req->list, &dep->pending_list); > > if (usb_endpoint_xfer_isoc(dep->endpoint.desc) && > @@ -1935,22 +1937,30 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, > struct dwc3_ep *dep, > int ret; > > list_for_each_entry_safe(req, n, &dep->started_list, list) { > - > + unsigned length; > + unsigned actual; > int chain; > > - chain = req->request.num_mapped_sgs > 0; > + length = req->request.length; > + chain = req->num_pending_sgs > 0; > if (chain) { > - struct scatterlist *sg = req->request.sg; > + struct scatterlist *sg = req->sg; > struct scatterlist *s; > + unsigned int pending = req->num_pending_sgs; > unsigned int i; > > - for_each_sg(sg, s, req->request.num_mapped_sgs, i) { > + for_each_sg(sg, s, pending, i) { > trb = &dep->trb_pool[dep->trb_dequeue]; > count += trb->size & DWC3_TRB_SIZE_MASK; > dwc3_ep_inc_deq(dep); > > + req->sg = sg_next(s); > + req->num_pending_sgs--; > + > ret = __dwc3_cleanup_done_trbs(dwc, dep, req, > trb, > event, status, chain); > + if (ret) > + break; > } > } else { > trb = &dep->trb_pool[dep->trb_dequeue]; > @@ -1968,11 +1978,13 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, > struct dwc3_ep *dep, >* should receive and we simply bounce the request back to the >* gadget driver for further processing. >*/ > - req->request.actual += req->request.length - count; > - dwc3_gadget_giveback(dep, req, status); > + actual = length - req->request.actual; > + req->request.actual = actual; > > - if (ret) > - break; > + if (ret && chain && (actual < length) && req->num_pending_sgs) > + return __dwc3_gadget_kick_transfer(dep, 0); > + > + dwc3_gadget_giveback(dep, req, status); > } > > /* > On your testing/next, I see considerable slow down and file corruption in mass storage. After bisecting, this patch seems to be the first one that shows problems. The device fails even enumeration here. I haven't looked in detail at the changes but, do you have any ideas? I have attached driver logs. Regards, John log.tar.gz Description: application/gzip
[PATCH v2 4/6] usb: dwc3: gadget: add remaining sg entries to ring
Upon transfer completion after a full ring, let's add more TRBs to our ring in order to complete our request successfully. Signed-off-by: Felipe Balbi --- drivers/usb/dwc3/gadget.c | 36 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 90b3d7965136..6483991c8013 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -888,14 +888,13 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep) static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep, struct dwc3_request *req, unsigned int trbs_left) { - struct usb_request *request = &req->request; - struct scatterlist *sg = request->sg; + struct scatterlist *sg = req->sg; struct scatterlist *s; unsigned intlength; dma_addr_t dma; int i; - for_each_sg(sg, s, request->num_mapped_sgs, i) { + for_each_sg(sg, s, req->num_pending_sgs, i) { unsigned chain = true; length = sg_dma_len(s); @@ -945,7 +944,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep) return; list_for_each_entry_safe(req, n, &dep->pending_list, list) { - if (req->request.num_mapped_sgs > 0) + if (req->num_pending_sgs > 0) dwc3_prepare_one_trb_sg(dep, req, trbs_left--); else dwc3_prepare_one_trb_linear(dep, req, trbs_left--); @@ -1071,6 +1070,9 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) if (ret) return ret; + req->sg = req->request.sg; + req->num_pending_sgs= req->request.num_mapped_sgs; + list_add_tail(&req->list, &dep->pending_list); if (usb_endpoint_xfer_isoc(dep->endpoint.desc) && @@ -1935,22 +1937,30 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep, int ret; list_for_each_entry_safe(req, n, &dep->started_list, list) { - + unsigned length; + unsigned actual; int chain; - chain = req->request.num_mapped_sgs > 0; + length = req->request.length; + chain = req->num_pending_sgs > 0; if (chain) { - struct scatterlist *sg = req->request.sg; + struct scatterlist *sg = req->sg; struct scatterlist *s; + unsigned int pending = req->num_pending_sgs; unsigned int i; - for_each_sg(sg, s, req->request.num_mapped_sgs, i) { + for_each_sg(sg, s, pending, i) { trb = &dep->trb_pool[dep->trb_dequeue]; count += trb->size & DWC3_TRB_SIZE_MASK; dwc3_ep_inc_deq(dep); + req->sg = sg_next(s); + req->num_pending_sgs--; + ret = __dwc3_cleanup_done_trbs(dwc, dep, req, trb, event, status, chain); + if (ret) + break; } } else { trb = &dep->trb_pool[dep->trb_dequeue]; @@ -1968,11 +1978,13 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep, * should receive and we simply bounce the request back to the * gadget driver for further processing. */ - req->request.actual += req->request.length - count; - dwc3_gadget_giveback(dep, req, status); + actual = length - req->request.actual; + req->request.actual = actual; - if (ret) - break; + if (ret && chain && (actual < length) && req->num_pending_sgs) + return __dwc3_gadget_kick_transfer(dep, 0); + + dwc3_gadget_giveback(dep, req, status); } /* -- 2.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html