Skip to contentSkip to conversation

usb: dwc3: gadget: Misc transfer cancellation fixes

PATCH In review Thinh Nguyen 2 participants 17 messages Mar 5, 2020 – Mar 16, 2020

Subsystems (3)
Stable branch USB DesignWare USB3 DRD IP driver
lore
Share & tools
lore

Thinh Nguyen

author maintainer
#0 lore raw

cover usb: dwc3: gadget: Misc transfer cancellation fixes

This series contains miscellaneous fixes to dwc3's gadget. Most of them are related to transfer cancellation handling.

Thinh Nguyen (6):
usb: dwc3: gadget: Don't clear flags before transfer ended
usb: dwc3: gadget: Properly handle ClearFeature(halt)
usb: dwc3: gadget: Wrap around when skip TRBs
usb: dwc3: gadget: Give back staled requests
usb: dwc3: gadget: Remove unnecessary checks
usb: dwc3: gadget: Refactor dwc3_gadget_ep_dequeue

 drivers/usb/dwc3/gadget.c | 85 +++++++++++++++++++++++++++++++----------------
 1 file changed, 57 insertions(+), 28 deletions(-)
Signature · 1 line
2.11.0
author maintainer
#1 lore raw

PATCH 1/6 usb: dwc3: gadget: Don't clear flags before transfer ended

We track END_TRANSFER command completion. Don't clear transfer started/ended flag prematurely. Otherwise, we'd run into the problem with restarting transfer before END_TRANSFER command finishes.

Fixes 6d8a019614f3 ("usb: dwc3: gadget: check for Missed Isoc from event status")
Signed-off-by Thinh Nguyen <[email protected]>
---
 drivers/usb/dwc3/gadget.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/usb/dwc3/gadget.c +1 -3
index 1b7d2f9cb673..b87bdec84210 100644
@@ -2563,10 +2563,8 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
 
 	dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
 
-	if (stop) {
+	if (stop)
 		dwc3_stop_active_transfer(dep, true, true);
-		dep->flags = DWC3_EP_ENABLED;
-	}
 
 	/*
 	 * WORKAROUND: This is the 2nd half of U1/U2 -> U0 workaround.
Signature · 1 line
2.11.0
author maintainer
#2 lore raw

PATCH 2/6 usb: dwc3: gadget: Properly handle ClearFeature(halt)

In reply to Thinh Nguyen

DWC3 must not issue CLEAR_STALL command to control endpoints. The controller automatically clears the STALL when it receives the SETUP token. Also, when the driver receives ClearFeature(halt_ep), DWC3 must stop any active transfer from the endpoint and give back all the requests to the function drivers.

Fixes 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver")
Signed-off-by Thinh Nguyen <[email protected]>
---
 drivers/usb/dwc3/gadget.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)
drivers/usb/dwc3/gadget.c +33 -3
index b87bdec84210..21b10364b888 100644
@@ -1501,6 +1501,10 @@ static void dwc3_gadget_ep_skip_trbs(struct dwc3_ep *dep, struct dwc3_request *r
 {
 	int i;
 
+	/* If req->trb is not set, then the request has not started */
+	if (!req->trb)
+		return;
+
 	/*
 	 * If request was already started, this means we had to
 	 * stop the transfer. With that we also need to ignore

@@ -1591,6 +1595,8 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
 {
 	struct dwc3_gadget_ep_cmd_params	params;
 	struct dwc3				*dwc = dep->dwc;
+	struct dwc3_request			*req;
+	struct dwc3_request			*tmp;
 	int					ret;
 
 	if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {

@@ -1627,13 +1633,37 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
 		else
 			dep->flags |= DWC3_EP_STALL;
 	} else {
+		/*
+		 * Don't issue CLEAR_STALL command to control endpoints. The
+		 * controller automatically clears the STALL when it receives
+		 * the SETUP token.
+		 */
+		if (dep->number <= 1) {
+			dep->flags &= ~(DWC3_EP_STALL | DWC3_EP_WEDGE);
+			return 0;
+		}
 
 		ret = dwc3_send_clear_stall_ep_cmd(dep);
-		if (ret)
+		if (ret) {
 			dev_err(dwc->dev, "failed to clear STALL on %s\n",
 					dep->name);
-		else
-			dep->flags &= ~(DWC3_EP_STALL | DWC3_EP_WEDGE);
+			return ret;
+		}
+
+		dep->flags &= ~(DWC3_EP_STALL | DWC3_EP_WEDGE);
+
+		dwc3_stop_active_transfer(dep, true, true);
+
+		list_for_each_entry_safe(req, tmp, &dep->started_list, list)
+			dwc3_gadget_move_cancelled_request(req);
+
+		list_for_each_entry_safe(req, tmp, &dep->pending_list, list)
+			dwc3_gadget_move_cancelled_request(req);
+
+		if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING)) {
+			dep->flags &= ~DWC3_EP_DELAY_START;
+			dwc3_gadget_ep_cleanup_cancelled_requests(dep);
+		}
 	}
 
 	return ret;
Signature · 1 line
2.11.0
#3 lore raw

Re: [PATCH 2/6] usb: dwc3: gadget: Properly handle ClearFeature(halt)

Hi,

drivers/usb/dwc3/gadget.c int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol) · line 1640

Quoted 9 lines · Thinh Nguyen
drivers/usb/dwc3/gadget.c +5 -0
@@ -1627,13 +1633,37 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
 		else
 			dep->flags |= DWC3_EP_STALL;
 	} else {
+		/*
+		 * Don't issue CLEAR_STALL command to control endpoints. The
+		 * controller automatically clears the STALL when it receives
+		 * the SETUP token.
+		 */

we have a separate dwc3_gadget_ep0_set_halt() to handle that. How was this triggered?

Signature · 1 line
balbi
author maintainer
#4 lore raw

Re: [PATCH 2/6] usb: dwc3: gadget: Properly handle ClearFeature(halt)

Hi,

Quoted 15 lines · Felipe Balbi

Hi,

drivers/usb/dwc3/gadget.c int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol) · line 1640

Quoted 9 lines · Thinh Nguyen
drivers/usb/dwc3/gadget.c +5 -0
@@ -1627,13 +1633,37 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
  		else
  			dep->flags |= DWC3_EP_STALL;
  	} else {
+		/*
+		 * Don't issue CLEAR_STALL command to control endpoints. The
+		 * controller automatically clears the STALL when it receives
+		 * the SETUP token.
+		 */

we have a separate dwc3_gadget_ep0_set_halt() to handle that. How was this triggered?

I think it's a bit confusing here that the dwc3_gadget_ep0_set_halt() has the similar name as __dwc3_gadget_ep_set_halt(). However, that function only calls dwc3_ep0_stall_and_restart() and not handled through SET/CLEAR_FEATURE(halt) request.

If host issues SET_FEATURE(halt) or CLEAR_FEATURE(halt) to control endpoints, it still goes through __dwc3_gadget_ep_set_halt().

BR,
Thinh

#5 lore raw

Re: [PATCH 2/6] usb: dwc3: gadget: Properly handle ClearFeature(halt)

Hi,

Quoted 17 lines · Thinh Nguyen
Quoted 15 lines · Felipe Balbi

Hi,

drivers/usb/dwc3/gadget.c int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol) · line 1640

Quoted 9 lines · Thinh Nguyen
drivers/usb/dwc3/gadget.c +5 -0
@@ -1627,13 +1633,37 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
  		else
  			dep->flags |= DWC3_EP_STALL;
  	} else {
+		/*
+		 * Don't issue CLEAR_STALL command to control endpoints. The
+		 * controller automatically clears the STALL when it receives
+		 * the SETUP token.
+		 */

we have a separate dwc3_gadget_ep0_set_halt() to handle that. How was this triggered?

Quoted 7 lines

I think it's a bit confusing here that the dwc3_gadget_ep0_set_halt() has the similar name as __dwc3_gadget_ep_set_halt(). However, that function only calls dwc3_ep0_stall_and_restart() and not handled through SET/CLEAR_FEATURE(halt) request.

If host issues SET_FEATURE(halt) or CLEAR_FEATURE(halt) to control endpoints, it still goes through __dwc3_gadget_ep_set_halt().

Perhaps that should be fixed, then?

Signature · 1 line
balbi
author maintainer
#6 lore raw

Re: [PATCH 2/6] usb: dwc3: gadget: Properly handle ClearFeature(halt)

Hi,

Quoted 19 lines · Felipe Balbi

Hi,

Quoted 16 lines · Thinh Nguyen
Quoted 15 lines · Felipe Balbi

Hi,

drivers/usb/dwc3/gadget.c int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol) · line 1640

Quoted 9 lines · Thinh Nguyen
drivers/usb/dwc3/gadget.c +5 -0
@@ -1627,13 +1633,37 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
   		else
   			dep->flags |= DWC3_EP_STALL;
   	} else {
+		/*
+		 * Don't issue CLEAR_STALL command to control endpoints. The
+		 * controller automatically clears the STALL when it receives
+		 * the SETUP token.
+		 */

we have a separate dwc3_gadget_ep0_set_halt() to handle that. How was this triggered?

Quoted 9 lines
Quoted 7 lines

I think it's a bit confusing here that the dwc3_gadget_ep0_set_halt() has the similar name as __dwc3_gadget_ep_set_halt(). However, that function only calls dwc3_ep0_stall_and_restart() and not handled through SET/CLEAR_FEATURE(halt) request.

If host issues SET_FEATURE(halt) or CLEAR_FEATURE(halt) to control endpoints, it still goes through __dwc3_gadget_ep_set_halt().

Perhaps that should be fixed, then?

If we want to add a patch to make this clear, we can add a separate patch to rename dwc3_gadget_ep0_set_halt() to something along the line of dwc3_ep0_stall_and_restart().

Everything else looks fine to me. Let me know if you have any other concern.

Thanks,
Thinh

author maintainer
#7 lore raw

PATCH 3/6 usb: dwc3: gadget: Wrap around when skip TRBs

In reply to Thinh Nguyen

When skipping TRBs, we need to account for wrapping around the ring buffer and not modifying some invalid TRBs. Without this fix, dwc3 won't be able to check for available TRBs.

Cc stable <[email protected]>
Fixes 7746a8dfb3f9 ("usb: dwc3: gadget: extract dwc3_gadget_ep_skip_trbs()")
Signed-off-by Thinh Nguyen <[email protected]>
---
 drivers/usb/dwc3/gadget.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/dwc3/gadget.c +1 -1
index 21b10364b888..8c27c6ede7c4 100644
@@ -1518,7 +1518,7 @@ static void dwc3_gadget_ep_skip_trbs(struct dwc3_ep *dep, struct dwc3_request *r
 	for (i = 0; i < req->num_trbs; i++) {
 		struct dwc3_trb *trb;
 
-		trb = req->trb + i;
+		trb = &dep->trb_pool[dep->trb_dequeue];
 		trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
 		dwc3_ep_inc_deq(dep);
 	}
Signature · 1 line
2.11.0
author maintainer
#8 lore raw

PATCH 4/6 usb: dwc3: gadget: Give back staled requests

In reply to Thinh Nguyen

If a request is dequeued, the transfer is cancelled. Give back all the started requests.

In most scenarios, the function driver dequeues all requests of a transfer when there's a failure. If the function driver follows this, then it's fine. If not, then we'd be skipping TRBs at different points within the dequeue and enqueue pointers, making dequeue/enqueue pointers useless. To enforce and make sure that we're properly skipping TRBs, cancel all the started requests and give back all the cancelled requests to the function drivers.

Signed-off-by Thinh Nguyen <[email protected]>
---
 drivers/usb/dwc3/gadget.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)
drivers/usb/dwc3/gadget.c +14 -1
index 8c27c6ede7c4..42dc4973a997 100644
@@ -1553,6 +1553,11 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 
 	spin_lock_irqsave(&dwc->lock, flags);
 
+	list_for_each_entry(r, &dep->cancelled_list, list) {
+		if (r == req)
+			goto out0;
+	}
+
 	list_for_each_entry(r, &dep->pending_list, list) {
 		if (r == req)
 			break;

@@ -1564,13 +1569,21 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 				break;
 		}
 		if (r == req) {
+			struct dwc3_request *t;
+
 			/* wait until it is processed */
 			dwc3_stop_active_transfer(dep, true, true);
 
 			if (!r->trb)
 				goto out0;
 
-			dwc3_gadget_move_cancelled_request(req);
+			/*
+			 * Remove any started request if the transfer is
+			 * cancelled.
+			 */
+			list_for_each_entry_safe(r, t, &dep->started_list, list)
+				dwc3_gadget_move_cancelled_request(r);
+
 			if (dep->flags & DWC3_EP_TRANSFER_STARTED)
 				goto out0;
 			else
Signature · 1 line
2.11.0
#9 lore raw

Re: [PATCH 4/6] usb: dwc3: gadget: Give back staled requests

Hi,

Quoted 10 lines · Thinh Nguyen

If a request is dequeued, the transfer is cancelled. Give back all the started requests.

In most scenarios, the function driver dequeues all requests of a transfer when there's a failure. If the function driver follows this, then it's fine. If not, then we'd be skipping TRBs at different points within the dequeue and enqueue pointers, making dequeue/enqueue pointers useless. To enforce and make sure that we're properly skipping TRBs, cancel all the started requests and give back all the cancelled requests to the function drivers.

Which function driver is *not* cancelling transfers correctly? We can (and should) be defensive on dwc3, but let's not hide bugs on function drivers either.

Signature · 1 line
balbi
author maintainer
#10 lore raw

Re: [PATCH 4/6] usb: dwc3: gadget: Give back staled requests

Hi Felipe,

Quoted 13 lines · Felipe Balbi

Hi,

Quoted 10 lines · Thinh Nguyen

If a request is dequeued, the transfer is cancelled. Give back all the started requests.

In most scenarios, the function driver dequeues all requests of a transfer when there's a failure. If the function driver follows this, then it's fine. If not, then we'd be skipping TRBs at different points within the dequeue and enqueue pointers, making dequeue/enqueue pointers useless. To enforce and make sure that we're properly skipping TRBs, cancel all the started requests and give back all the cancelled requests to the function drivers.

Quoted 4 lines

Which function driver is *not* cancelling transfers correctly? We can (and should) be defensive on dwc3, but let's not hide bugs on function drivers either.

I didn't review all the function drivers for this. I just see a potential issue and go for a more defensive approach. What's your suggestion?

Thanks,
Thinh

#11 lore raw

Re: [PATCH 4/6] usb: dwc3: gadget: Give back staled requests

Hi,

Quoted 11 lines · Thinh Nguyen
Quoted 11 lines
Quoted 10 lines · Thinh Nguyen

If a request is dequeued, the transfer is cancelled. Give back all the started requests.

In most scenarios, the function driver dequeues all requests of a transfer when there's a failure. If the function driver follows this, then it's fine. If not, then we'd be skipping TRBs at different points within the dequeue and enqueue pointers, making dequeue/enqueue pointers useless. To enforce and make sure that we're properly skipping TRBs, cancel all the started requests and give back all the cancelled requests to the function drivers.

Quoted 8 lines
Quoted 4 lines

Which function driver is *not* cancelling transfers correctly? We can (and should) be defensive on dwc3, but let's not hide bugs on function drivers either.

I didn't review all the function drivers for this. I just see a potential issue and go for a more defensive approach. What's your suggestion?

Fair enough, that's good for my understading of why the patch was created. Is there a way to add a WARN() or something like that so we catch erroneous gadget drivers easily? Also, could you check if we need a documentation update for the gadget API with regards to this finding?

cheers

Signature · 1 line
balbi
author maintainer
#12 lore raw

Re: [PATCH 4/6] usb: dwc3: gadget: Give back staled requests

Hi,

Quoted 21 lines · Felipe Balbi

Hi,

Quoted 11 lines · Thinh Nguyen
Quoted 11 lines
Quoted 10 lines · Thinh Nguyen

If a request is dequeued, the transfer is cancelled. Give back all the started requests.

In most scenarios, the function driver dequeues all requests of a transfer when there's a failure. If the function driver follows this, then it's fine. If not, then we'd be skipping TRBs at different points within the dequeue and enqueue pointers, making dequeue/enqueue pointers useless. To enforce and make sure that we're properly skipping TRBs, cancel all the started requests and give back all the cancelled requests to the function drivers.

Quoted 7 lines
Quoted 4 lines

Which function driver is *not* cancelling transfers correctly? We can (and should) be defensive on dwc3, but let's not hide bugs on function drivers either.

I didn't review all the function drivers for this. I just see a potential issue and go for a more defensive approach. What's your suggestion?

Quoted 7 lines

Fair enough, that's good for my understading of why the patch was created. Is there a way to add a WARN() or something like that so we catch erroneous gadget drivers easily? Also, could you check if we need a documentation update for the gadget API with regards to this finding?

cheers

It's a bit difficult to add a WARN() as the function drivers can dequeue the requests in any order and any time they like. We may be able to put some contraints via documentation so we could do a WARN(), but that is also not easy.

For example, in dwc3, the current way we implement bulk transfer is that there's no concept of where one transfer starts and ends (at least for non-stream transfers). So, the function driver may queue multiple transfers but dwc3 only sees a single transfer. Dequeuing one transfer in the function driver means dequeuing all for dwc3.

Updating the documentation for this requires a bit of thoughts, and it also means we need to review all the different controller drivers. We may need to document this, but in the mean time, what do you think of this approach for now?

BR,
Thinh

author maintainer
#13 lore raw

PATCH 5/6 usb: dwc3: gadget: Remove unnecessary checks

In reply to Thinh Nguyen

Remove 2 unnecessary checks:
1) A request in the started_list must have its trb field set. So checking for req->trb is unnecessary.

2) An endpoint must have started (and have not ended) for the request to still be in the started_list. There's no point to check if the endpoint is started. We had this check because previously the driver didn't handle the endpoint's started/ended flags for END_TRANSFER command properly. See commit 9f45581f5eec ("usb: dwc3: gadget: early giveback if End Transfer already completed").

Signed-off-by Thinh Nguyen <[email protected]>
---
 drivers/usb/dwc3/gadget.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)
drivers/usb/dwc3/gadget.c +1 -8
index 42dc4973a997..cd00f2757cb5 100644
@@ -1574,9 +1574,6 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 			/* wait until it is processed */
 			dwc3_stop_active_transfer(dep, true, true);
 
-			if (!r->trb)
-				goto out0;
-
 			/*
 			 * Remove any started request if the transfer is
 			 * cancelled.

@@ -1584,10 +1581,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 			list_for_each_entry_safe(r, t, &dep->started_list, list)
 				dwc3_gadget_move_cancelled_request(r);
 
-			if (dep->flags & DWC3_EP_TRANSFER_STARTED)
-				goto out0;
-			else
-				goto out1;
+			goto out0;
 		}
 		dev_err(dwc->dev, "request %pK was not queued to %s\n",
 				request, ep->name);

@@ -1595,7 +1589,6 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 		goto out0;
 	}
 
-out1:
 	dwc3_gadget_giveback(dep, req, -ECONNRESET);
 
 out0:
Signature · 1 line
2.11.0
author maintainer
#14 lore raw

PATCH 6/6 usb: dwc3: gadget: Refactor dwc3_gadget_ep_dequeue

In reply to Thinh Nguyen

The flow from function dwc3_gadget_ep_dequeue() is not easy to follow. Refactor it for easier read. No functional change in this commit.

Signed-off-by Thinh Nguyen <[email protected]>
---
 drivers/usb/dwc3/gadget.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)
drivers/usb/dwc3/gadget.c +11 -16
index cd00f2757cb5..39c92df6e188 100644
@@ -1555,19 +1555,17 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 
 	list_for_each_entry(r, &dep->cancelled_list, list) {
 		if (r == req)
-			goto out0;
+			goto out;
 	}
 
 	list_for_each_entry(r, &dep->pending_list, list) {
-		if (r == req)
-			break;
+		if (r == req) {
+			dwc3_gadget_giveback(dep, req, -ECONNRESET);
+			goto out;
+		}
 	}
 
-	if (r != req) {
-		list_for_each_entry(r, &dep->started_list, list) {
-			if (r == req)
-				break;
-		}
+	list_for_each_entry(r, &dep->started_list, list) {
 		if (r == req) {
 			struct dwc3_request *t;
 

@@ -1581,17 +1579,14 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 			list_for_each_entry_safe(r, t, &dep->started_list, list)
 				dwc3_gadget_move_cancelled_request(r);
 
-			goto out0;
+			goto out;
 		}
-		dev_err(dwc->dev, "request %pK was not queued to %s\n",
-				request, ep->name);
-		ret = -EINVAL;
-		goto out0;
 	}
 
-	dwc3_gadget_giveback(dep, req, -ECONNRESET);
-
-out0:
+	dev_err(dwc->dev, "request %pK was not queued to %s\n",
+		request, ep->name);
+	ret = -EINVAL;
+out:
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	return ret;
Signature · 1 line
2.11.0
#15 lore raw

Re: [PATCH 6/6] usb: dwc3: gadget: Refactor dwc3_gadget_ep_dequeue

Quoted 4 lines · Thinh Nguyen

The flow from function dwc3_gadget_ep_dequeue() is not easy to follow. Refactor it for easier read. No functional change in this commit.

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

doesn't apply:

checking file drivers/usb/dwc3/gadget.c
Hunk #1 FAILED at 1555.
Hunk #2 FAILED at 1581.
2 out of 2 hunks FAILED

Signature · 1 line
balbi
author maintainer
#16 lore raw

Re: [PATCH 6/6] usb: dwc3: gadget: Refactor dwc3_gadget_ep_dequeue

Hi Felipe,

Quoted 5 lines · Felipe Balbi
Quoted 3 lines · Thinh Nguyen

The flow from function dwc3_gadget_ep_dequeue() is not easy to follow. Refactor it for easier read. No functional change in this commit.

Quoted 8 lines
Quoted 1 line
Signed-off-by Thinh Nguyen <[email protected]>

doesn't apply:

checking file drivers/usb/dwc3/gadget.c
Hunk #1 FAILED at 1555.
Hunk #2 FAILED at 1581.
2 out of 2 hunks FAILED

You'd need to pick up some dependency patches of the same series to avoid conflict.
"usb: dwc3: gadget: Give back staled requests"
"usb: dwc3: gadget: Remove unnecessary checks"

Seeing that you have some comments for one of them, it's probably not picked up. I'll resend the patches after review.

Thanks,
Thinh