| Message ID | 456f864b86a72ab9490ce095d5ba3f59b39d6a09.1588025916.git.thinhn@synopsys.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc3: gadget: Handle streams | expand |
On 20-04-27 15:27:41, Thinh Nguyen wrote: > A transfer is composed of one or more usb_requests. Currently, only the > function driver knows this based on its implementation and its class > protocol. However, some usb controllers need to know this to update its > resources. For example, the DWC3 controller needs this info to update > its internal resources and initiate different streams. > > Introduce a new field is_last to usb_request to inform the controller > driver whether the request is the last of its transfer. > > Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> > --- > include/linux/usb/gadget.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h > index e959c09a97c9..742c52f7e470 100644 > --- a/include/linux/usb/gadget.h > +++ b/include/linux/usb/gadget.h > @@ -50,6 +50,7 @@ struct usb_ep; > * @short_not_ok: When reading data, makes short packets be > * treated as errors (queue stops advancing till cleanup). > * @dma_mapped: Indicates if request has been mapped to DMA (internal) > + * @is_last: Indicates if this request is the last of a transfer. Would you please describe the use case for it, and what's 'transfer' and 'request' in your use case? Peter > * @complete: Function called when request completes, so this request and > * its buffer may be re-used. The function will always be called with > * interrupts disabled, and it must not sleep. > @@ -108,6 +109,7 @@ struct usb_request { > unsigned zero:1; > unsigned short_not_ok:1; > unsigned dma_mapped:1; > + unsigned is_last:1; > > void (*complete)(struct usb_ep *ep, > struct usb_request *req); > -- > 2.11.0 >
Hi, Peter Chen wrote: > On 20-04-27 15:27:41, Thinh Nguyen wrote: >> A transfer is composed of one or more usb_requests. Currently, only the >> function driver knows this based on its implementation and its class >> protocol. However, some usb controllers need to know this to update its >> resources. For example, the DWC3 controller needs this info to update >> its internal resources and initiate different streams. >> >> Introduce a new field is_last to usb_request to inform the controller >> driver whether the request is the last of its transfer. >> >> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> >> --- >> include/linux/usb/gadget.h | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h >> index e959c09a97c9..742c52f7e470 100644 >> --- a/include/linux/usb/gadget.h >> +++ b/include/linux/usb/gadget.h >> @@ -50,6 +50,7 @@ struct usb_ep; >> * @short_not_ok: When reading data, makes short packets be >> * treated as errors (queue stops advancing till cleanup). >> * @dma_mapped: Indicates if request has been mapped to DMA (internal) >> + * @is_last: Indicates if this request is the last of a transfer. > Would you please describe the use case for it, and what's 'transfer' > and 'request' in your use case? > The transfer size is defined by a class protocol. The function driver will determine how many usb_requests are needed to fulfill that transfer. For example, in MSC, a READ/WRITE command can request for a transfer size up to (512 * 2^31-1) bytes. It'd be too large for a single usb_request, which is why the mass_storage function driver limits each request to 16KB max by default. A MSC command itself is a transfer, same with its status. This is a similar case for UASP. However, the f_tcm and the target drivers current implementation only use a single request per transfer. (This needs to be fixed, along with some other issues in f_tcm). The use case here is that DWC3x controller needs to update its resources whenever a transfer is completed. This is a requirement for streams when it needs to free up some resources for different stream transfers. The function driver must pass this information to the controller driver for streams to work properly. Potentially, another use case for this is to update the documentation on usb_dequeue_request(). By providing the concept of a transfer, we can say that dequeuing an in-flight request will cancel the transfer, and any incomplete request within the transfer must be given back to the function driver. This would make sense for controllers that use TRB ring buffer and dequeue/enqueue TRB pointers. But this idea still needs more thoughts. BR, Thinh
> > On 20-04-27 15:27:41, Thinh Nguyen wrote: > >> A transfer is composed of one or more usb_requests. Currently, only > >> the function driver knows this based on its implementation and its > >> class protocol. However, some usb controllers need to know this to > >> update its resources. For example, the DWC3 controller needs this > >> info to update its internal resources and initiate different streams. > >> > >> Introduce a new field is_last to usb_request to inform the controller > >> driver whether the request is the last of its transfer. > >> > >> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> > >> --- > >> include/linux/usb/gadget.h | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h > >> index e959c09a97c9..742c52f7e470 100644 > >> --- a/include/linux/usb/gadget.h > >> +++ b/include/linux/usb/gadget.h > >> @@ -50,6 +50,7 @@ struct usb_ep; > >> * @short_not_ok: When reading data, makes short packets be > >> * treated as errors (queue stops advancing till cleanup). > >> * @dma_mapped: Indicates if request has been mapped to DMA > >> (internal) > >> + * @is_last: Indicates if this request is the last of a transfer. > > Would you please describe the use case for it, and what's 'transfer' > > and 'request' in your use case? > > > > The transfer size is defined by a class protocol. The function driver will determine > how many usb_requests are needed to fulfill that transfer. > Thanks for your information. If 'transfer size' here is software concept, why controller needs to know? The general controller internal logic doesn't care class protocol, it only cares TRB chain software prepares. > For example, in MSC, a READ/WRITE command can request for a transfer size up > to (512 * 2^31-1) bytes. It'd be too large for a single usb_request, which is why the > mass_storage function driver limits each request to 16KB max by default. A MSC > command itself is a transfer, same with its status. > > This is a similar case for UASP. However, the f_tcm and the target drivers current > implementation only use a single request per transfer. > (This needs to be fixed, along with some other issues in f_tcm). > > The use case here is that DWC3x controller needs to update its resources > whenever a transfer is completed. This is a requirement for streams when it needs > to free up some resources for different stream transfers. The function driver must > pass this information to the controller driver for streams to work properly. > Does the controller case about stream information or the transfer information? > Potentially, another use case for this is to update the documentation on > usb_dequeue_request(). By providing the concept of a transfer, we can say that > dequeuing an in-flight request will cancel the transfer, and any incomplete request > within the transfer must be given back to the function driver. This would make > sense for controllers that use TRB ring buffer and dequeue/enqueue TRB pointers. > But this idea still needs more thoughts. > I think class driver needs to consider that, the controller driver only needs to handle request and related TRBs. Peter
Peter Chen wrote: > >>> On 20-04-27 15:27:41, Thinh Nguyen wrote: >>>> A transfer is composed of one or more usb_requests. Currently, only >>>> the function driver knows this based on its implementation and its >>>> class protocol. However, some usb controllers need to know this to >>>> update its resources. For example, the DWC3 controller needs this >>>> info to update its internal resources and initiate different streams. >>>> >>>> Introduce a new field is_last to usb_request to inform the controller >>>> driver whether the request is the last of its transfer. >>>> >>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> >>>> --- >>>> include/linux/usb/gadget.h | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h >>>> index e959c09a97c9..742c52f7e470 100644 >>>> --- a/include/linux/usb/gadget.h >>>> +++ b/include/linux/usb/gadget.h >>>> @@ -50,6 +50,7 @@ struct usb_ep; >>>> * @short_not_ok: When reading data, makes short packets be >>>> * treated as errors (queue stops advancing till cleanup). >>>> * @dma_mapped: Indicates if request has been mapped to DMA >>>> (internal) >>>> + * @is_last: Indicates if this request is the last of a transfer. >>> Would you please describe the use case for it, and what's 'transfer' >>> and 'request' in your use case? >>> >> The transfer size is defined by a class protocol. The function driver will determine >> how many usb_requests are needed to fulfill that transfer. >> > Thanks for your information. > > If 'transfer size' here is software concept, why controller needs to know? The general > controller internal logic doesn't care class protocol, it only cares TRB chain software prepares. While some controllers don't have the concept of this, DWC_usb3x does. It has a notion of starting and ending a transfer. While a transfer is started, the endpoint uses a resource. It releases that resource when the transfer completes. So far, dwc3 implemented in such a way that bulk transfers are always in-progress and don't complete. That's fine so far, but it's not the case with streams. > >> For example, in MSC, a READ/WRITE command can request for a transfer size up >> to (512 * 2^31-1) bytes. It'd be too large for a single usb_request, which is why the >> mass_storage function driver limits each request to 16KB max by default. A MSC >> command itself is a transfer, same with its status. >> >> This is a similar case for UASP. However, the f_tcm and the target drivers current >> implementation only use a single request per transfer. >> (This needs to be fixed, along with some other issues in f_tcm). >> >> The use case here is that DWC3x controller needs to update its resources >> whenever a transfer is completed. This is a requirement for streams when it needs >> to free up some resources for different stream transfers. The function driver must >> pass this information to the controller driver for streams to work properly. >> > Does the controller case about stream information or the transfer information? For streams, each stransfer has a stream ID, and each transfer uses a resource. > >> Potentially, another use case for this is to update the documentation on >> usb_dequeue_request(). By providing the concept of a transfer, we can say that >> dequeuing an in-flight request will cancel the transfer, and any incomplete request >> within the transfer must be given back to the function driver. This would make >> sense for controllers that use TRB ring buffer and dequeue/enqueue TRB pointers. >> But this idea still needs more thoughts. >> > > I think class driver needs to consider that, the controller driver only needs to handle > request and related TRBs. It maybe true for some controllers, but DWC_usb3x needs this information. BR, Thinh
On Thu, 30 Apr 2020, Thinh Nguyen wrote: > Peter Chen wrote: > > If 'transfer size' here is software concept, why controller needs to know? The general > > controller internal logic doesn't care class protocol, it only cares TRB chain software prepares. > > While some controllers don't have the concept of this, DWC_usb3x does. > It has a notion of starting and ending a transfer. While a transfer is > started, the endpoint uses a resource. It releases that resource when > the transfer completes. So far, dwc3 implemented in such a way that bulk > transfers are always in-progress and don't complete. That's fine so far, > but it's not the case with streams. This is peculiar. I haven't heard of any other controller doing this. What does the controller use this resource for? Would anything go wrong if you told the controller that each transfer was a single usb_request? Alan Stern
Hi, Alan Stern wrote: > On Thu, 30 Apr 2020, Thinh Nguyen wrote: > >> Peter Chen wrote: >>> If 'transfer size' here is software concept, why controller needs to know? The general >>> controller internal logic doesn't care class protocol, it only cares TRB chain software prepares. >> While some controllers don't have the concept of this, DWC_usb3x does. >> It has a notion of starting and ending a transfer. While a transfer is >> started, the endpoint uses a resource. It releases that resource when >> the transfer completes. So far, dwc3 implemented in such a way that bulk >> transfers are always in-progress and don't complete. That's fine so far, >> but it's not the case with streams. > This is peculiar. I haven't heard of any other controller doing this. > > What does the controller use this resource for? Would anything go > wrong if you told the controller that each transfer was a single > usb_request? It's no problem. Each transfer can be a single request. Just set the request->is_last. (Refer to [patch 2/5] for f_tcm). The issue here is that the controller needs to know when a stream completes so it can start on a different stream. In the controller driver, this is done by setting a certain control bit in the TRB indicating the last TRB of a transfer. This knowledge can only come from the function driver, which is why we need this "is_last" field. BR, Thinh
On Thu, 30 Apr 2020, Thinh Nguyen wrote: > Hi, > > Alan Stern wrote: > > On Thu, 30 Apr 2020, Thinh Nguyen wrote: > > > >> Peter Chen wrote: > >>> If 'transfer size' here is software concept, why controller needs to know? The general > >>> controller internal logic doesn't care class protocol, it only cares TRB chain software prepares. > >> While some controllers don't have the concept of this, DWC_usb3x does. > >> It has a notion of starting and ending a transfer. While a transfer is > >> started, the endpoint uses a resource. It releases that resource when > >> the transfer completes. So far, dwc3 implemented in such a way that bulk > >> transfers are always in-progress and don't complete. That's fine so far, > >> but it's not the case with streams. > > This is peculiar. I haven't heard of any other controller doing this. > > > > What does the controller use this resource for? Would anything go > > wrong if you told the controller that each transfer was a single > > usb_request? > > It's no problem. Each transfer can be a single request. Just set the > request->is_last. (Refer to [patch 2/5] for f_tcm). > > The issue here is that the controller needs to know when a stream > completes so it can start on a different stream. In the controller Why does it need to know this? Why can't it start on a different stream at any time? > driver, this is done by setting a certain control bit in the TRB > indicating the last TRB of a transfer. This knowledge can only come from > the function driver, which is why we need this "is_last" field. What's wrong with just assuming _every_ usb_request is the last one of its transfer? Then you wouldn't have to add a new flag or change the existing function drivers. Alan Stern
Alan Stern wrote: > On Thu, 30 Apr 2020, Thinh Nguyen wrote: > >> Hi, >> >> Alan Stern wrote: >>> On Thu, 30 Apr 2020, Thinh Nguyen wrote: >>> >>>> Peter Chen wrote: >>>>> If 'transfer size' here is software concept, why controller needs to know? The general >>>>> controller internal logic doesn't care class protocol, it only cares TRB chain software prepares. >>>> While some controllers don't have the concept of this, DWC_usb3x does. >>>> It has a notion of starting and ending a transfer. While a transfer is >>>> started, the endpoint uses a resource. It releases that resource when >>>> the transfer completes. So far, dwc3 implemented in such a way that bulk >>>> transfers are always in-progress and don't complete. That's fine so far, >>>> but it's not the case with streams. >>> This is peculiar. I haven't heard of any other controller doing this. >>> >>> What does the controller use this resource for? Would anything go >>> wrong if you told the controller that each transfer was a single >>> usb_request? >> It's no problem. Each transfer can be a single request. Just set the >> request->is_last. (Refer to [patch 2/5] for f_tcm). >> >> The issue here is that the controller needs to know when a stream >> completes so it can start on a different stream. In the controller > Why does it need to know this? Why can't it start on a different > stream at any time? By design, whenever the controller needs to start on a different stream, it will issue a START_TRANSFER command along with a stream ID. It cannot issue START_TRANSFER command again until the previous transfer completes or ended. > >> driver, this is done by setting a certain control bit in the TRB >> indicating the last TRB of a transfer. This knowledge can only come from >> the function driver, which is why we need this "is_last" field. > What's wrong with just assuming _every_ usb_request is the last one of > its transfer? Then you wouldn't have to add a new flag or change the > existing function drivers. That will affect performance. The driver will then need to service multiple interrupts and restart the transfer multiple times. BR, Thinh
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index e959c09a97c9..742c52f7e470 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -50,6 +50,7 @@ struct usb_ep; * @short_not_ok: When reading data, makes short packets be * treated as errors (queue stops advancing till cleanup). * @dma_mapped: Indicates if request has been mapped to DMA (internal) + * @is_last: Indicates if this request is the last of a transfer. * @complete: Function called when request completes, so this request and * its buffer may be re-used. The function will always be called with * interrupts disabled, and it must not sleep. @@ -108,6 +109,7 @@ struct usb_request { unsigned zero:1; unsigned short_not_ok:1; unsigned dma_mapped:1; + unsigned is_last:1; void (*complete)(struct usb_ep *ep, struct usb_request *req);
A transfer is composed of one or more usb_requests. Currently, only the function driver knows this based on its implementation and its class protocol. However, some usb controllers need to know this to update its resources. For example, the DWC3 controller needs this info to update its internal resources and initiate different streams. Introduce a new field is_last to usb_request to inform the controller driver whether the request is the last of its transfer. Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> --- include/linux/usb/gadget.h | 2 ++ 1 file changed, 2 insertions(+)