| Message ID | 660a74249e64b3b62ca9b394584387baee67a119.1583466150.git.thinhn@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc3: gadget: Remove incomplete check | expand |
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > We only care to resume transfer for SG because the request maybe > partially completed. dwc3_gadget_ep_request_completed() doesn't check > that of a request, at least not fully. > > 1) It doesn't account for OUT direction. > 2) It doesn't account for isoc. For isoc, a request maybe completed with > partial data. I would rather fix the function for these cases instead of removing it completely. While at that, also move the req->num_pending_sgs check inside dwc3_gadget_ep_request_completed()
Hi Felipe, Felipe Balbi wrote: > Hi, > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >> We only care to resume transfer for SG because the request maybe >> partially completed. dwc3_gadget_ep_request_completed() doesn't check >> that of a request, at least not fully. >> >> 1) It doesn't account for OUT direction. >> 2) It doesn't account for isoc. For isoc, a request maybe completed with >> partial data. > I would rather fix the function for these cases instead of removing it > completely. While at that, also move the req->num_pending_sgs check > inside dwc3_gadget_ep_request_completed() > If we want to keep this function, the only thing this function does is to check req->num_pending_sgs. We'd only resume the request because there are pending TRBs from SG not completed yet. If all the TRBs of a request are completed, regardless if all the data are received/sent, we don't queue them again. Do you still want to have this function? BR, Thinh
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>> We only care to resume transfer for SG because the request maybe >>> partially completed. dwc3_gadget_ep_request_completed() doesn't check >>> that of a request, at least not fully. >>> >>> 1) It doesn't account for OUT direction. >>> 2) It doesn't account for isoc. For isoc, a request maybe completed with >>> partial data. >> I would rather fix the function for these cases instead of removing it >> completely. While at that, also move the req->num_pending_sgs check >> inside dwc3_gadget_ep_request_completed() >> > > If we want to keep this function, the only thing this function does is > to check req->num_pending_sgs. We'd only resume the request because > there are pending TRBs from SG not completed yet. If all the TRBs of a > request are completed, regardless if all the data are received/sent, we > don't queue them again. Do you still want to have this function? The function gives a name to a very specific "concept", that of a completed request. You can see that even today, the function is super simple: OUT direction is always completed, IN direction is completed when actual == length, we're just missing the pending_sgs check. So something like the hunks below. One thing I don't get from your patch is why you're completely removing the function and why isn't req->direction and actual == length not needed anymore. Could you explain? hunks: @@ -2482,7 +2482,8 @@ static int dwc3_gadget_ep_reclaim_trb_linear(struct dwc3_ep *dep, event, status, false); } -static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req) +static bool dwc3_gadget_ep_request_completed(struct dwc3_ep *dep, + struct dwc3_request *req) { /* * For OUT direction, host may send less than the setup @@ -2491,6 +2492,16 @@ static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req) if (!req->direction) return true; + /* + * If there are pending scatterlist entries, we should + * continue processing them. + */ + if (req->num_pending_sgs) + return false; + + if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) + do_something(); + return req->request.actual == req->request.length; } @@ -2515,8 +2526,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) || - req->num_pending_sgs) { + if (!dwc3_gadget_ep_request_completed(dep, req)) __dwc3_gadget_kick_transfer(dep); goto out; }
Hi, Felipe Balbi wrote: > Hi, > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>>> We only care to resume transfer for SG because the request maybe >>>> partially completed. dwc3_gadget_ep_request_completed() doesn't check >>>> that of a request, at least not fully. >>>> >>>> 1) It doesn't account for OUT direction. >>>> 2) It doesn't account for isoc. For isoc, a request maybe completed with >>>> partial data. >>> I would rather fix the function for these cases instead of removing it >>> completely. While at that, also move the req->num_pending_sgs check >>> inside dwc3_gadget_ep_request_completed() >>> >> If we want to keep this function, the only thing this function does is >> to check req->num_pending_sgs. We'd only resume the request because >> there are pending TRBs from SG not completed yet. If all the TRBs of a >> request are completed, regardless if all the data are received/sent, we >> don't queue them again. Do you still want to have this function? > The function gives a name to a very specific "concept", that of a > completed request. You can see that even today, the function is super > simple: OUT direction is always completed, IN direction is completed > when actual == length, we're just missing the pending_sgs check. So > something like the hunks below. Fair point. > > One thing I don't get from your patch is why you're completely removing > the function and why isn't req->direction and actual == length not > needed anymore. Could you explain? It's because there's no use for that function outside of checking for number of pending SGs and resume. > > hunks: > > @@ -2482,7 +2482,8 @@ static int dwc3_gadget_ep_reclaim_trb_linear(struct dwc3_ep *dep, > event, status, false); > } > > -static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req) > +static bool dwc3_gadget_ep_request_completed(struct dwc3_ep *dep, > + struct dwc3_request *req) > { > /* > * For OUT direction, host may send less than the setup > @@ -2491,6 +2492,16 @@ static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req) > if (!req->direction) > return true; > > + /* > + * If there are pending scatterlist entries, we should > + * continue processing them. > + */ > + if (req->num_pending_sgs) > + return false; > + > + if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) > + do_something(); do_something() will always return true here. > + > return req->request.actual == req->request.length; This should always be true if the request completes. By spec, bulk and interrupt endpoints data delivery are guaranteed, and the retry/error detection is done at the lower level. If by chance that the host fails to request for data multiple times at the packet level, it will issue a ClearFeature(halt_ep) request to the endpoint. This will trigger dwc3 to stop the endpoint and cancel the transfer, and we still won't resume this transfer. > } > > @@ -2515,8 +2526,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) || > - req->num_pending_sgs) { > + if (!dwc3_gadget_ep_request_completed(dep, req)) > __dwc3_gadget_kick_transfer(dep); > goto out; > } > BR, Thinh
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >> One thing I don't get from your patch is why you're completely removing >> the function and why isn't req->direction and actual == length not >> needed anymore. Could you explain? > > It's because there's no use for that function outside of checking for > number of pending SGs and resume. wait, huh? What about cases when user unplugs cable midtransfer? We have versions of dwc3 HW which fail to produce disconnect interrupt, right? >> @@ -2491,6 +2492,16 @@ static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req) >> if (!req->direction) >> return true; >> >> + /* >> + * If there are pending scatterlist entries, we should >> + * continue processing them. >> + */ >> + if (req->num_pending_sgs) >> + return false; >> + >> + if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) >> + do_something(); > > do_something() will always return true here. Will do "do_something", then return true or simply return true? >> return req->request.actual == req->request.length; > > This should always be true if the request completes. By spec, bulk and > interrupt endpoints data delivery are guaranteed, and the retry/error > detection is done at the lower level. If by chance that the host fails > to request for data multiple times at the packet level, it will issue a > ClearFeature(halt_ep) request to the endpoint. This will trigger dwc3 to > stop the endpoint and cancel the transfer, and we still won't resume > this transfer. we can unplug the cable at any time, even mid-transfer.
Hi Felipe, Felipe Balbi wrote: > Hi, > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>> One thing I don't get from your patch is why you're completely removing >>> the function and why isn't req->direction and actual == length not >>> needed anymore. Could you explain? >> It's because there's no use for that function outside of checking for >> number of pending SGs and resume. > wait, huh? What about cases when user unplugs cable midtransfer? We have > versions of dwc3 HW which fail to produce disconnect interrupt, right? There won't even enter this code path if disconnect event is generated or not. > >>> @@ -2491,6 +2492,16 @@ static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req) >>> if (!req->direction) >>> return true; >>> >>> + /* >>> + * If there are pending scatterlist entries, we should >>> + * continue processing them. >>> + */ >>> + if (req->num_pending_sgs) >>> + return false; >>> + >>> + if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) >>> + do_something(); >> do_something() will always return true here. > Will do "do_something", then return true or simply return true? I mean simply return true here. > >>> return req->request.actual == req->request.length; >> This should always be true if the request completes. By spec, bulk and >> interrupt endpoints data delivery are guaranteed, and the retry/error >> detection is done at the lower level. If by chance that the host fails >> to request for data multiple times at the packet level, it will issue a >> ClearFeature(halt_ep) request to the endpoint. This will trigger dwc3 to >> stop the endpoint and cancel the transfer, and we still won't resume >> this transfer. > we can unplug the cable at any time, even mid-transfer. > That's fine if there's a disconnection mid-transfer. The transfer is cancelled in that case, why would we want to kick_transfer again? Also, the controller would not generate XferInProgress event to notify TRB completion for the driver to enter this code path. The condition here is if (!request_complete()), then kick_transfer(). Let's take a look at what kick_transfer() do: kick_transfer() will prepare new TRBs and issue START_TRANSFER command or UPDATE_TRANSFER command. The endpoint is already started, and nothing is causing it to end at this point. So it should just be UPDATE_TRANSFER command. UPDATE_TRANSFER command tells the controller to update its TRB cache because there will be new TRBs prepared for the request. If this is non-SG/non-chained TRB request, then there's only 1 TRB per request for IN endpoints. If that TRB is completed, that means that the request is completed. There's no reason to issue kick_transfer() again. When the function driver queues a new request, then there will be new TRBs to prepare and then the driver can kick_transfer() again. So, this condition to check if request_complete() is only valid for a request with multiple chained TRBs. Since we can only check for IN direction, the chained TRB setup related to OUT direction to fit MaxPacketSize does not apply here. What left is chained TRBs for SG. In this case, we do want to kick_transfer again. This may happen when we run out of TRBs and we have to wait for available TRBs. When there are available TRBs and still pending SGs, then we want to prepare the rest of the SG entries to finish the request. So kick_transfer() makes sense here. Let me know if there's something not clear. Thanks, Thinh
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>>> @@ -2491,6 +2492,16 @@ static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req) >>>> if (!req->direction) >>>> return true; >>>> >>>> + /* >>>> + * If there are pending scatterlist entries, we should >>>> + * continue processing them. >>>> + */ >>>> + if (req->num_pending_sgs) >>>> + return false; >>>> + >>>> + if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) >>>> + do_something(); >>> do_something() will always return true here. >> Will do "do_something", then return true or simply return true? > > I mean simply return true here. got it >>>> return req->request.actual == req->request.length; >>> This should always be true if the request completes. By spec, bulk and >>> interrupt endpoints data delivery are guaranteed, and the retry/error >>> detection is done at the lower level. If by chance that the host fails >>> to request for data multiple times at the packet level, it will issue a >>> ClearFeature(halt_ep) request to the endpoint. This will trigger dwc3 to >>> stop the endpoint and cancel the transfer, and we still won't resume >>> this transfer. >> we can unplug the cable at any time, even mid-transfer. > > That's fine if there's a disconnection mid-transfer. The transfer is > cancelled in that case, why would we want to kick_transfer again? Also, > the controller would not generate XferInProgress event to notify TRB > completion for the driver to enter this code path. d'oh! That's true > The condition here is if (!request_complete()), then kick_transfer(). > Let's take a look at what kick_transfer() do: > > kick_transfer() will prepare new TRBs and issue START_TRANSFER command > or UPDATE_TRANSFER command. The endpoint is already started, and nothing > is causing it to end at this point. So it should just be UPDATE_TRANSFER > command. UPDATE_TRANSFER command tells the controller to update its TRB > cache because there will be new TRBs prepared for the request. > > If this is non-SG/non-chained TRB request, then there's only 1 TRB per > request for IN endpoints. If that TRB is completed, that means that the > request is completed. There's no reason to issue kick_transfer() again. not entirely true for bulk. We never set LST bit; we will never complete a transfer, we continually add more TRBs. The reason for this is to amortize the cost of adding new transfers to the controller cache before it runs out of TRBs without HWO. How about we change the test to say "if I have non-started TRBs and I'm bulk (non-stream) or interrupt endpoint, kick more transfers"? > When the function driver queues a new request, then there will be new > TRBs to prepare and then the driver can kick_transfer() again. We may already have more TRBs in the pending list which may not have been started before we didn't have free TRBs to use. We just completed a TRB, might as well try to use it for more requests. > So, this condition to check if request_complete() is only valid for a > request with multiple chained TRBs. Since we can only check for IN > direction, the chained TRB setup related to OUT direction to fit > MaxPacketSize does not apply here. What left is chained TRBs for SG. In this part is clear now and you're correct. Thanks > this case, we do want to kick_transfer again. This may happen when we > run out of TRBs and we have to wait for available TRBs. When there are > available TRBs and still pending SGs, then we want to prepare the rest > of the SG entries to finish the request. So kick_transfer() makes sense > here. Right but we can run out of TRBs even in non-chained case. I remember testing this years ago by giving g_mass_storage a list of 300 requests. The reason for kicking the transfer is different, but it's beneficial anyhow.
Hi, Felipe Balbi wrote: > Hi, > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > >> The condition here is if (!request_complete()), then kick_transfer(). >> Let's take a look at what kick_transfer() do: >> >> kick_transfer() will prepare new TRBs and issue START_TRANSFER command >> or UPDATE_TRANSFER command. The endpoint is already started, and nothing >> is causing it to end at this point. So it should just be UPDATE_TRANSFER >> command. UPDATE_TRANSFER command tells the controller to update its TRB >> cache because there will be new TRBs prepared for the request. >> >> If this is non-SG/non-chained TRB request, then there's only 1 TRB per >> request for IN endpoints. If that TRB is completed, that means that the >> request is completed. There's no reason to issue kick_transfer() again. > not entirely true for bulk. We never set LST bit; we will never complete > a transfer, we continually add more TRBs. The reason for this is to > amortize the cost of adding new transfers to the controller cache before > it runs out of TRBs without HWO. Right, I was referring to "request" rather than transfer (as in a transfer may have 1 or more requests). > > How about we change the test to say "if I have non-started TRBs and I'm > bulk (non-stream) or interrupt endpoint, kick more transfers"? > >> When the function driver queues a new request, then there will be new >> TRBs to prepare and then the driver can kick_transfer() again. > We may already have more TRBs in the pending list which may not have > been started before we didn't have free TRBs to use. We just completed a > TRB, might as well try to use it for more requests. Yes we can and we should, but we didn't check that. Also it shouldn't be in the request_complete() check function as they are part of different requests. > >> So, this condition to check if request_complete() is only valid for a >> request with multiple chained TRBs. Since we can only check for IN >> direction, the chained TRB setup related to OUT direction to fit >> MaxPacketSize does not apply here. What left is chained TRBs for SG. In > this part is clear now and you're correct. Thanks > >> this case, we do want to kick_transfer again. This may happen when we >> run out of TRBs and we have to wait for available TRBs. When there are >> available TRBs and still pending SGs, then we want to prepare the rest >> of the SG entries to finish the request. So kick_transfer() makes sense >> here. > Right but we can run out of TRBs even in non-chained case. I remember > testing this years ago by giving g_mass_storage a list of 300 > requests. The reason for kicking the transfer is different, but it's > beneficial anyhow. > In this case, the check should be for if the pending_list is not empty, then kick again. diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 6a04c9afcab6..d8318de55000 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2975,14 +2975,7 @@ static int dwc3_gadget_ep_reclaim_trb_linear(struct dwc3_ep *dep, static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req) { - /* - * For OUT direction, host may send less than the setup - * length. Return true for all OUT requests. - */ - if (!req->direction) - return true; - - return req->request.actual == req->request.length; + return req->num_pending_sgs == 0; } static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, @@ -3007,7 +3000,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) || - req->num_pending_sgs) { + !list_empty(&dep->pending_list)) { __dwc3_gadget_kick_transfer(dep); goto out; } This is unlikely to happen, but it's necessary to be there. Let me know if you're ok with the change, I'll create a formal patch for it. BR, Thinh
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>> The condition here is if (!request_complete()), then kick_transfer(). >>> Let's take a look at what kick_transfer() do: >>> >>> kick_transfer() will prepare new TRBs and issue START_TRANSFER command >>> or UPDATE_TRANSFER command. The endpoint is already started, and nothing >>> is causing it to end at this point. So it should just be UPDATE_TRANSFER >>> command. UPDATE_TRANSFER command tells the controller to update its TRB >>> cache because there will be new TRBs prepared for the request. >>> >>> If this is non-SG/non-chained TRB request, then there's only 1 TRB per >>> request for IN endpoints. If that TRB is completed, that means that the >>> request is completed. There's no reason to issue kick_transfer() again. >> not entirely true for bulk. We never set LST bit; we will never complete >> a transfer, we continually add more TRBs. The reason for this is to >> amortize the cost of adding new transfers to the controller cache before >> it runs out of TRBs without HWO. > > Right, I was referring to "request" rather than transfer (as in a > transfer may have 1 or more requests). > >> >> How about we change the test to say "if I have non-started TRBs and I'm >> bulk (non-stream) or interrupt endpoint, kick more transfers"? >> >>> When the function driver queues a new request, then there will be new >>> TRBs to prepare and then the driver can kick_transfer() again. >> We may already have more TRBs in the pending list which may not have >> been started before we didn't have free TRBs to use. We just completed a >> TRB, might as well try to use it for more requests. > > Yes we can and we should, but we didn't check that. Also it shouldn't be > in the request_complete() check function as they are part of different > requests. > >> >>> So, this condition to check if request_complete() is only valid for a >>> request with multiple chained TRBs. Since we can only check for IN >>> direction, the chained TRB setup related to OUT direction to fit >>> MaxPacketSize does not apply here. What left is chained TRBs for SG. In >> this part is clear now and you're correct. Thanks >> >>> this case, we do want to kick_transfer again. This may happen when we >>> run out of TRBs and we have to wait for available TRBs. When there are >>> available TRBs and still pending SGs, then we want to prepare the rest >>> of the SG entries to finish the request. So kick_transfer() makes sense >>> here. >> Right but we can run out of TRBs even in non-chained case. I remember >> testing this years ago by giving g_mass_storage a list of 300 >> requests. The reason for kicking the transfer is different, but it's >> beneficial anyhow. >> > > In this case, the check should be for if the pending_list is not empty, > then kick again. > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 6a04c9afcab6..d8318de55000 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2975,14 +2975,7 @@ static int > dwc3_gadget_ep_reclaim_trb_linear(struct dwc3_ep *dep, > > static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req) > { > - /* > - * For OUT direction, host may send less than the setup > - * length. Return true for all OUT requests. > - */ > - if (!req->direction) > - return true; > - > - return req->request.actual == req->request.length; > + return req->num_pending_sgs == 0; > } > > static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, > @@ -3007,7 +3000,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) || > - req->num_pending_sgs) { > + !list_empty(&dep->pending_list)) { > __dwc3_gadget_kick_transfer(dep); > goto out; > } > > > This is unlikely to happen, but it's necessary to be there. > > Let me know if you're ok with the change, I'll create a formal patch for it. Looks good, we may just rename the function to dwc3_gadget_ep_should_continue() or something similar and move the !list_empty() check in there too.
Hi, Felipe Balbi wrote: > Hi, > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>>> The condition here is if (!request_complete()), then kick_transfer(). >>>> Let's take a look at what kick_transfer() do: >>>> >>>> kick_transfer() will prepare new TRBs and issue START_TRANSFER command >>>> or UPDATE_TRANSFER command. The endpoint is already started, and nothing >>>> is causing it to end at this point. So it should just be UPDATE_TRANSFER >>>> command. UPDATE_TRANSFER command tells the controller to update its TRB >>>> cache because there will be new TRBs prepared for the request. >>>> >>>> If this is non-SG/non-chained TRB request, then there's only 1 TRB per >>>> request for IN endpoints. If that TRB is completed, that means that the >>>> request is completed. There's no reason to issue kick_transfer() again. >>> not entirely true for bulk. We never set LST bit; we will never complete >>> a transfer, we continually add more TRBs. The reason for this is to >>> amortize the cost of adding new transfers to the controller cache before >>> it runs out of TRBs without HWO. >> Right, I was referring to "request" rather than transfer (as in a >> transfer may have 1 or more requests). >> >>> How about we change the test to say "if I have non-started TRBs and I'm >>> bulk (non-stream) or interrupt endpoint, kick more transfers"? >>> >>>> When the function driver queues a new request, then there will be new >>>> TRBs to prepare and then the driver can kick_transfer() again. >>> We may already have more TRBs in the pending list which may not have >>> been started before we didn't have free TRBs to use. We just completed a >>> TRB, might as well try to use it for more requests. >> Yes we can and we should, but we didn't check that. Also it shouldn't be >> in the request_complete() check function as they are part of different >> requests. >> >>>> So, this condition to check if request_complete() is only valid for a >>>> request with multiple chained TRBs. Since we can only check for IN >>>> direction, the chained TRB setup related to OUT direction to fit >>>> MaxPacketSize does not apply here. What left is chained TRBs for SG. In >>> this part is clear now and you're correct. Thanks >>> >>>> this case, we do want to kick_transfer again. This may happen when we >>>> run out of TRBs and we have to wait for available TRBs. When there are >>>> available TRBs and still pending SGs, then we want to prepare the rest >>>> of the SG entries to finish the request. So kick_transfer() makes sense >>>> here. >>> Right but we can run out of TRBs even in non-chained case. I remember >>> testing this years ago by giving g_mass_storage a list of 300 >>> requests. The reason for kicking the transfer is different, but it's >>> beneficial anyhow. >>> >> In this case, the check should be for if the pending_list is not empty, >> then kick again. >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 6a04c9afcab6..d8318de55000 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -2975,14 +2975,7 @@ static int >> dwc3_gadget_ep_reclaim_trb_linear(struct dwc3_ep *dep, >> >> static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req) >> { >> - /* >> - * For OUT direction, host may send less than the setup >> - * length. Return true for all OUT requests. >> - */ >> - if (!req->direction) >> - return true; >> - >> - return req->request.actual == req->request.length; >> + return req->num_pending_sgs == 0; >> } >> >> static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, >> @@ -3007,7 +3000,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) || >> - req->num_pending_sgs) { >> + !list_empty(&dep->pending_list)) { >> __dwc3_gadget_kick_transfer(dep); >> goto out; >> } >> >> >> This is unlikely to happen, but it's necessary to be there. >> >> Let me know if you're ok with the change, I'll create a formal patch for it. > Looks good, we may just rename the function to > dwc3_gadget_ep_should_continue() or something similar and move the > !list_empty() check in there too. > I forgot this condition skips the dwc3_gadget_giveback(). I have to split it. Let me send out the revised patches and you can review. Thanks, Thinh
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>>>> The condition here is if (!request_complete()), then kick_transfer(). >>>>> Let's take a look at what kick_transfer() do: >>>>> >>>>> kick_transfer() will prepare new TRBs and issue START_TRANSFER command >>>>> or UPDATE_TRANSFER command. The endpoint is already started, and nothing >>>>> is causing it to end at this point. So it should just be UPDATE_TRANSFER >>>>> command. UPDATE_TRANSFER command tells the controller to update its TRB >>>>> cache because there will be new TRBs prepared for the request. >>>>> >>>>> If this is non-SG/non-chained TRB request, then there's only 1 TRB per >>>>> request for IN endpoints. If that TRB is completed, that means that the >>>>> request is completed. There's no reason to issue kick_transfer() again. >>>> not entirely true for bulk. We never set LST bit; we will never complete >>>> a transfer, we continually add more TRBs. The reason for this is to >>>> amortize the cost of adding new transfers to the controller cache before >>>> it runs out of TRBs without HWO. >>> Right, I was referring to "request" rather than transfer (as in a >>> transfer may have 1 or more requests). >>> >>>> How about we change the test to say "if I have non-started TRBs and I'm >>>> bulk (non-stream) or interrupt endpoint, kick more transfers"? >>>> >>>>> When the function driver queues a new request, then there will be new >>>>> TRBs to prepare and then the driver can kick_transfer() again. >>>> We may already have more TRBs in the pending list which may not have >>>> been started before we didn't have free TRBs to use. We just completed a >>>> TRB, might as well try to use it for more requests. >>> Yes we can and we should, but we didn't check that. Also it shouldn't be >>> in the request_complete() check function as they are part of different >>> requests. >>> >>>>> So, this condition to check if request_complete() is only valid for a >>>>> request with multiple chained TRBs. Since we can only check for IN >>>>> direction, the chained TRB setup related to OUT direction to fit >>>>> MaxPacketSize does not apply here. What left is chained TRBs for SG. In >>>> this part is clear now and you're correct. Thanks >>>> >>>>> this case, we do want to kick_transfer again. This may happen when we >>>>> run out of TRBs and we have to wait for available TRBs. When there are >>>>> available TRBs and still pending SGs, then we want to prepare the rest >>>>> of the SG entries to finish the request. So kick_transfer() makes sense >>>>> here. >>>> Right but we can run out of TRBs even in non-chained case. I remember >>>> testing this years ago by giving g_mass_storage a list of 300 >>>> requests. The reason for kicking the transfer is different, but it's >>>> beneficial anyhow. >>>> >>> In this case, the check should be for if the pending_list is not empty, >>> then kick again. >>> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index 6a04c9afcab6..d8318de55000 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -2975,14 +2975,7 @@ static int >>> dwc3_gadget_ep_reclaim_trb_linear(struct dwc3_ep *dep, >>> >>> static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req) >>> { >>> - /* >>> - * For OUT direction, host may send less than the setup >>> - * length. Return true for all OUT requests. >>> - */ >>> - if (!req->direction) >>> - return true; >>> - >>> - return req->request.actual == req->request.length; >>> + return req->num_pending_sgs == 0; >>> } >>> >>> static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, >>> @@ -3007,7 +3000,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) || >>> - req->num_pending_sgs) { >>> + !list_empty(&dep->pending_list)) { >>> __dwc3_gadget_kick_transfer(dep); >>> goto out; >>> } >>> >>> >>> This is unlikely to happen, but it's necessary to be there. >>> >>> Let me know if you're ok with the change, I'll create a formal patch for it. >> Looks good, we may just rename the function to >> dwc3_gadget_ep_should_continue() or something similar and move the >> !list_empty() check in there too. >> > > I forgot this condition skips the dwc3_gadget_giveback(). I have to > split it. Let me send out the revised patches and you can review. Sure, I think patch 1 can go in during -rc. Do we need a Cc stable on it, though? Patch 2 will have to wait until v5.8.
Felipe Balbi wrote: > Hi, > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>>>>> The condition here is if (!request_complete()), then kick_transfer(). >>>>>> Let's take a look at what kick_transfer() do: >>>>>> >>>>>> kick_transfer() will prepare new TRBs and issue START_TRANSFER command >>>>>> or UPDATE_TRANSFER command. The endpoint is already started, and nothing >>>>>> is causing it to end at this point. So it should just be UPDATE_TRANSFER >>>>>> command. UPDATE_TRANSFER command tells the controller to update its TRB >>>>>> cache because there will be new TRBs prepared for the request. >>>>>> >>>>>> If this is non-SG/non-chained TRB request, then there's only 1 TRB per >>>>>> request for IN endpoints. If that TRB is completed, that means that the >>>>>> request is completed. There's no reason to issue kick_transfer() again. >>>>> not entirely true for bulk. We never set LST bit; we will never complete >>>>> a transfer, we continually add more TRBs. The reason for this is to >>>>> amortize the cost of adding new transfers to the controller cache before >>>>> it runs out of TRBs without HWO. >>>> Right, I was referring to "request" rather than transfer (as in a >>>> transfer may have 1 or more requests). >>>> >>>>> How about we change the test to say "if I have non-started TRBs and I'm >>>>> bulk (non-stream) or interrupt endpoint, kick more transfers"? >>>>> >>>>>> When the function driver queues a new request, then there will be new >>>>>> TRBs to prepare and then the driver can kick_transfer() again. >>>>> We may already have more TRBs in the pending list which may not have >>>>> been started before we didn't have free TRBs to use. We just completed a >>>>> TRB, might as well try to use it for more requests. >>>> Yes we can and we should, but we didn't check that. Also it shouldn't be >>>> in the request_complete() check function as they are part of different >>>> requests. >>>> >>>>>> So, this condition to check if request_complete() is only valid for a >>>>>> request with multiple chained TRBs. Since we can only check for IN >>>>>> direction, the chained TRB setup related to OUT direction to fit >>>>>> MaxPacketSize does not apply here. What left is chained TRBs for SG. In >>>>> this part is clear now and you're correct. Thanks >>>>> >>>>>> this case, we do want to kick_transfer again. This may happen when we >>>>>> run out of TRBs and we have to wait for available TRBs. When there are >>>>>> available TRBs and still pending SGs, then we want to prepare the rest >>>>>> of the SG entries to finish the request. So kick_transfer() makes sense >>>>>> here. >>>>> Right but we can run out of TRBs even in non-chained case. I remember >>>>> testing this years ago by giving g_mass_storage a list of 300 >>>>> requests. The reason for kicking the transfer is different, but it's >>>>> beneficial anyhow. >>>>> >>>> In this case, the check should be for if the pending_list is not empty, >>>> then kick again. >>>> >>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>> index 6a04c9afcab6..d8318de55000 100644 >>>> --- a/drivers/usb/dwc3/gadget.c >>>> +++ b/drivers/usb/dwc3/gadget.c >>>> @@ -2975,14 +2975,7 @@ static int >>>> dwc3_gadget_ep_reclaim_trb_linear(struct dwc3_ep *dep, >>>> >>>> static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req) >>>> { >>>> - /* >>>> - * For OUT direction, host may send less than the setup >>>> - * length. Return true for all OUT requests. >>>> - */ >>>> - if (!req->direction) >>>> - return true; >>>> - >>>> - return req->request.actual == req->request.length; >>>> + return req->num_pending_sgs == 0; >>>> } >>>> >>>> static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, >>>> @@ -3007,7 +3000,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) || >>>> - req->num_pending_sgs) { >>>> + !list_empty(&dep->pending_list)) { >>>> __dwc3_gadget_kick_transfer(dep); >>>> goto out; >>>> } >>>> >>>> >>>> This is unlikely to happen, but it's necessary to be there. >>>> >>>> Let me know if you're ok with the change, I'll create a formal patch for it. >>> Looks good, we may just rename the function to >>> dwc3_gadget_ep_should_continue() or something similar and move the >>> !list_empty() check in there too. >>> >> I forgot this condition skips the dwc3_gadget_giveback(). I have to >> split it. Let me send out the revised patches and you can review. > Sure, I think patch 1 can go in during -rc. Do we need a Cc stable on > it, though? > > Patch 2 will have to wait until v5.8. > Sure. I'll resend with Cc stable tag. Thanks, Thinh
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 39c92df6e188..9f46c8bc1114 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2506,18 +2506,6 @@ static int dwc3_gadget_ep_reclaim_trb_linear(struct dwc3_ep *dep, event, status, false); } -static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req) -{ - /* - * For OUT direction, host may send less than the setup - * length. Return true for all OUT requests. - */ - if (!req->direction) - return true; - - return req->request.actual == req->request.length; -} - static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, const struct dwc3_event_depevt *event, struct dwc3_request *req, int status) @@ -2539,8 +2527,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) || - req->num_pending_sgs) { + if (req->num_pending_sgs) { __dwc3_gadget_kick_transfer(dep); goto out; }
We only care to resume transfer for SG because the request maybe partially completed. dwc3_gadget_ep_request_completed() doesn't check that of a request, at least not fully. 1) It doesn't account for OUT direction. 2) It doesn't account for isoc. For isoc, a request maybe completed with partial data. Remove this check. Fixes: e0c42ce590fe ("usb: dwc3: gadget: simplify IOC handling") Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> --- drivers/usb/dwc3/gadget.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-)