usb: dwc3: gadget: Handle streams

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 <[email protected]>

---
 include/linux/usb/gadget.h | 2 ++
 1 file changed, 2 insertions(+)

include/linux/usb/gadget.h +0 -0

index e959c09a97c9..742c52f7e470 100644

include/linux/usb/gadget.h +1 -0

  * @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);

Signature · 1 line

2.11.0

Quoted 14 lines · Thinh Nguyen

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 <[email protected]>

---
  include/linux/usb/gadget.h | 2 ++
  1 file changed, 2 insertions(+)

include/linux/usb/gadget.h struct usb_ep; · line 53

Quoted 9 lines

include/linux/usb/gadget.h +1 -0

index e959c09a97c9..742c52f7e470 100644
@@ -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?

Quoted 16 lines · Thinh Nguyen

Quoted 14 lines

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 <[email protected]>

---
  include/linux/usb/gadget.h | 2 ++
  1 file changed, 2 insertions(+)

Quoted 13 lines

include/linux/usb/gadget.h struct usb_ep; · line 54

Quoted 10 lines

include/linux/usb/gadget.h +1 -0

index e959c09a97c9..742c52f7e470 100644
@@ -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.

Quoted 29 lines

Quoted 29 lines · Thinh Nguyen

Quoted 19 lines

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 <[email protected]>

---
   include/linux/usb/gadget.h | 2 ++
   1 file changed, 2 insertions(+)

include/linux/usb/gadget.h +0 -0

index e959c09a97c9..742c52f7e470 100644

include/linux/usb/gadget.h struct usb_ep; · line 54

Quoted 5 lines

include/linux/usb/gadget.h +1 -0

@@ -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?

Quoted 3 lines

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.

Quoted 10 lines

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).

Quoted 5 lines

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?

Quoted 8 lines

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.

Quoted 2 lines · Peter Chen

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.

Quoted 9 lines · Thinh Nguyen

Quoted 2 lines · Peter Chen

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?

Hi,

Quoted 11 lines · Alan Stern

Quoted 9 lines · Thinh Nguyen

Quoted 2 lines · Peter Chen

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.

Quoted 5 lines

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.

Quoted 19 lines · Thinh Nguyen

Hi,

Quoted 16 lines · Alan Stern

Quoted 9 lines · Thinh Nguyen

Quoted 2 lines · Peter Chen

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?

Quoted 5 lines

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?

Quoted 4 lines

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.

With the new usb_request->is_last field, now the function drivers can inform which request is the end of a transfer, dwc3 can program its TRBs to let the controller know when to free its resources when a transfer completes. This is required for stream transfers. The controller needs to know where one stream starts and ends to properly allocate resources for different streams.

Signed-off-by Thinh Nguyen <[email protected]>

drivers/usb/dwc3/gadget.c +39 -0

@@ -2704,6 +2707,45 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
 
 		dwc->u1u2 = 0;
 	}
+
+	return no_started_trb;
+}
+
+static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep,
+		const struct dwc3_event_depevt *event)
+{
+	dep->frame_number = event->parameters;
+}
+
+static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
+		const struct dwc3_event_depevt *event)
+{
+	int status = 0;
+
+	if (usb_endpoint_xfer_isoc(dep->endpoint.desc))
+		dwc3_gadget_endpoint_frame_from_event(dep, event);
+
+	if (event->status & DEPEVT_STATUS_BUSERR)
+		status = -ECONNRESET;
+
+	if (event->status & DEPEVT_STATUS_MISSED_ISOC)
+		status = -EXDEV;
+
+	dwc3_gadget_endpoint_trbs_complete(dep, event, status);
+}
+
+static void dwc3_gadget_endpoint_transfer_complete(struct dwc3_ep *dep,
+		const struct dwc3_event_depevt *event)
+{
+	int status = 0;
+
+	dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
+
+	if (event->status & DEPEVT_STATUS_BUSERR)
+		status = -ECONNRESET;
+
+	if (dwc3_gadget_endpoint_trbs_complete(dep, event, status))
+		dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE;
 }
 
 static void dwc3_gadget_endpoint_transfer_not_ready(struct dwc3_ep *dep,

Hi,

Quoted 7 lines · Thinh Nguyen

With the new usb_request->is_last field, now the function drivers can inform which request is the end of a transfer, dwc3 can program its TRBs to let the controller know when to free its resources when a transfer completes. This is required for stream transfers. The controller needs to know where one stream starts and ends to properly allocate resources for different streams.

Quoted 1 line

Signed-off-by Thinh Nguyen <[email protected]>

from a quick read, it looks like this can be broken down into smaller patches.

drivers/usb/dwc3/core.h struct dwc3_ep { · line 704

Quoted 10 lines

drivers/usb/dwc3/core.h +1 -0

index 7204a838ec06..b11183a715a7 100644
@@ -701,6 +701,7 @@ struct dwc3_ep {
  #define DWC3_EP_END_TRANSFER_PENDING BIT(4)
  #define DWC3_EP_PENDING_REQUEST	BIT(5)
  #define DWC3_EP_DELAY_START	BIT(6)
+#define DWC3_EP_WAIT_TRANSFER_COMPLETE	BIT(7)

this could be its own patch with proper description (and usage)

Quoted 9 lines

drivers/usb/dwc3/core.h +0 -0

  	/* This last one is specific to EP0 */
  #define DWC3_EP0_DIR_IN		BIT(31)

drivers/usb/dwc3/gadget.c +0 -0

index 865e6fbb7360..628f9d142876 100644
@@ -579,6 +579,7 @@ static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action)
 

drivers/usb/dwc3/gadget.c static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action) · line 582

Quoted 3 lines

drivers/usb/dwc3/gadget.c +1 -0

  	if (usb_ss_max_streams(comp_desc) && usb_endpoint_xfer_bulk(desc)) {
  		params.param1 |= DWC3_DEPCFG_STREAM_CAPABLE
+			| DWC3_DEPCFG_XFER_COMPLETE_EN

adding this bit for stream endpoints could be a separate commit.

Quoted 7 lines

drivers/usb/dwc3/gadget.c +0 -0

  			| DWC3_DEPCFG_STREAM_EVENT_EN;
  		dep->stream_capable = true;
  	}

@@ -917,8 +918,9 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep)
  }
 

drivers/usb/dwc3/gadget.c static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep) · line 923

Quoted 6 lines

drivers/usb/dwc3/gadget.c +3 -2

  static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
-		dma_addr_t dma, unsigned length, unsigned chain, unsigned node,
-		unsigned stream_id, unsigned short_not_ok, unsigned no_interrupt)
+		dma_addr_t dma, unsigned length, unsigned chain,
+		unsigned is_last, unsigned node, unsigned stream_id,
+		unsigned short_not_ok, unsigned no_interrupt)

if you add "is_last" as the last argument, this hunk will look smaller. Nitpicking, I know.

drivers/usb/dwc3/gadget.c static void dwc3_prepare_trbs(struct dwc3_ep *dep) · line 1237

Quoted 9 lines

drivers/usb/dwc3/gadget.c +4 -0

@@ -1223,6 +1231,10 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
 
  		if (!dwc3_calc_trbs_left(dep))
  			return;
+
+		/* Don't prepare ahead. This is not an option for DWC_usb32. */
+		if (req->request.is_last)
+			return;

this requires some better description. Why isn't it an option for dwc_usb32?

Quoted 5 lines

drivers/usb/dwc3/gadget.c +0 -0

@@ -2648,37 +2666,22 @@ static bool dwc3_gadget_ep_should_continue(struct dwc3_ep *dep)
  	return !dwc3_gadget_ep_request_completed(req);
  }
 

drivers/usb/dwc3/gadget.c static bool dwc3_gadget_ep_should_continue(struct dwc3_ep *dep) · line 2655

Quoted 5 lines

drivers/usb/dwc3/gadget.c +0 -5

-static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep,
-		const struct dwc3_event_depevt *event)
-{
-	dep->frame_number = event->parameters;
-}

moving these functions around could be a separate patch. The way it's done now takes away from review.

Quoted 46 lines

drivers/usb/dwc3/gadget.c +39 -0

@@ -2704,6 +2707,45 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
 
  		dwc->u1u2 = 0;
  	}
+
+	return no_started_trb;
+}
+
+static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep,
+		const struct dwc3_event_depevt *event)
+{
+	dep->frame_number = event->parameters;
+}
+
+static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
+		const struct dwc3_event_depevt *event)
+{
+	int status = 0;
+
+	if (usb_endpoint_xfer_isoc(dep->endpoint.desc))
+		dwc3_gadget_endpoint_frame_from_event(dep, event);
+
+	if (event->status & DEPEVT_STATUS_BUSERR)
+		status = -ECONNRESET;
+
+	if (event->status & DEPEVT_STATUS_MISSED_ISOC)
+		status = -EXDEV;
+
+	dwc3_gadget_endpoint_trbs_complete(dep, event, status);
+}
+
+static void dwc3_gadget_endpoint_transfer_complete(struct dwc3_ep *dep,
+		const struct dwc3_event_depevt *event)
+{
+	int status = 0;
+
+	dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
+
+	if (event->status & DEPEVT_STATUS_BUSERR)
+		status = -ECONNRESET;
+
+	if (dwc3_gadget_endpoint_trbs_complete(dep, event, status))
+		dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE;
  }
 

drivers/usb/dwc3/gadget.c static void dwc3_endpoint_interrupt(struct dwc3 *dwc, · line 2815

Quoted 6 lines

drivers/usb/dwc3/gadget.c +1 -0

  static void dwc3_gadget_endpoint_transfer_not_ready(struct dwc3_ep *dep,

@@ -2770,7 +2812,10 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
  		}
  		break;
  	case DWC3_DEPEVT_STREAMEVT:
+		break;

Swap these around. Keep all the "nothing to do here" cases together.

Quoted 9 lines

drivers/usb/dwc3/gadget.c static void dwc3_prepare_trbs(struct dwc3_ep *dep) · line 1237

Quoted 8 lines

drivers/usb/dwc3/gadget.c +4 -0

@@ -1223,6 +1231,10 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
 
  		if (!dwc3_calc_trbs_left(dep))
  			return;
+
+		/* Don't prepare ahead. This is not an option for DWC_usb32. */
+		if (req->request.is_last)
+			return;

this requires some better description. Why isn't it an option for dwc_usb32?

Internally, DWC_usb32 does some advance caching and burst that we should not prepare more TRB until the transfer is completed.
This doesn't apply for isoc, missed a check here. Need to apply on the next version.

Hi,

Quoted 9 lines · Thinh Nguyen

Quoted 9 lines

drivers/usb/dwc3/gadget.c static void dwc3_prepare_trbs(struct dwc3_ep *dep) · line 1237

Quoted 8 lines

drivers/usb/dwc3/gadget.c +4 -0

@@ -1223,6 +1231,10 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
 
   		if (!dwc3_calc_trbs_left(dep))
   			return;
+
+		/* Don't prepare ahead. This is not an option for DWC_usb32. */
+		if (req->request.is_last)
+			return;

this requires some better description. Why isn't it an option for dwc_usb32?

Quoted 4 lines

Internally, DWC_usb32 does some advance caching and burst that we should not prepare more TRB until the transfer is completed.
This doesn't apply for isoc, missed a check here. Need to apply on the next version.

Do you mind re-wording this statement as a comment in the code?