Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1457866 > unrolled thread

[PACTH v2] cdc-wdm: Clear read pipeline in case of error

Back to article view | Back to linux.kernel

Contents

  [PACTH v2] cdc-wdm: Clear read pipeline in case of error robert.foss@collabora.com - 2016-08-08 17:50 +0200
    Re: [PACTH v2] cdc-wdm: Clear read pipeline in case of error Oliver Neukum <oneukum@suse.com> - 2016-08-09 14:30 +0200
    Re: [PACTH v2] cdc-wdm: Clear read pipeline in case of error Greg KH <gregkh@linuxfoundation.org> - 2016-08-09 16:00 +0200
      Re: [PACTH v2] cdc-wdm: Clear read pipeline in case of error Robert Foss <robert.foss@collabora.com> - 2016-08-09 16:30 +0200

#1457866 — [PACTH v2] cdc-wdm: Clear read pipeline in case of error

From: Robert Foss <robert.foss@collabora.com>

Implemented queued response handling. This queue is processed every time the
WDM_READ flag is cleared.

In case of a read error, userspace may not actually read the data, since the
driver returns an error through wdm_poll. After this, the underlying device may
attempt to send us more data, but the queue is not processed. While userspace is
also blocked, because the read error is never cleared.

After this patch, we proactively process the queue on a read error. If there was
an outstanding response to handle, that will clear the error (or go through the
same logic again, if another read error occurs). If there was no outstanding
response, this will bring the queue size back to 0, unblocking a future response
from the underlying device.

Tested-by: Robert Foss <robert.foss@collabora.com>
Signed-off-by: Robert Foss <robert.foss@collabora.com>
---

Changes since v1:
- Only set a new error code it there is no previous error code

 drivers/usb/class/cdc-wdm.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 337948c..f12a006 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -154,6 +154,9 @@ static void wdm_out_callback(struct urb *urb)
 	wake_up(&desc->wait);
 }
 
+/* forward declaration */
+static int service_outstanding_interrupt(struct wdm_device *desc);
+
 static void wdm_in_callback(struct urb *urb)
 {
 	struct wdm_device *desc = urb->context;
@@ -188,7 +191,13 @@ static void wdm_in_callback(struct urb *urb)
 		}
 	}
 
-	desc->rerr = status;
+	/*
+	 * only set a new error if there is no previous error.
+	 * Errors are only cleared during read/open
+	 */
+	if (desc->rerr  == 0)
+		desc->rerr = status;
+
 	if (length + desc->length > desc->wMaxCommand) {
 		/* The buffer would overflow */
 		set_bit(WDM_OVERFLOW, &desc->flags);
@@ -201,9 +210,19 @@ static void wdm_in_callback(struct urb *urb)
 		}
 	}
 skip_error:
+	set_bit(WDM_READ, &desc->flags);
 	wake_up(&desc->wait);
 
-	set_bit(WDM_READ, &desc->flags);
+	if (desc->rerr) {
+		/*
+		 * Since there was an error, userspace may decide to not read
+		 * any data after poll'ing.
+		 * We should respond to further attempts from the device to send
+		 * data, so that we can get unstuck.
+		 */
+		service_outstanding_interrupt(desc);
+	}
+
 	spin_unlock(&desc->iuspin);
 }
 
@@ -436,17 +455,14 @@ out_free_mem:
 }
 
 /*
- * clear WDM_READ flag and possibly submit the read urb if resp_count
- * is non-zero.
+ * Submit the read urb if resp_count is non-zero.
  *
  * Called with desc->iuspin locked
  */
-static int clear_wdm_read_flag(struct wdm_device *desc)
+static int service_outstanding_interrupt(struct wdm_device *desc)
 {
 	int rv = 0;
 
-	clear_bit(WDM_READ, &desc->flags);
-
 	/* submit read urb only if the device is waiting for it */
 	if (!desc->resp_count || !--desc->resp_count)
 		goto out;
@@ -538,7 +554,8 @@ retry:
 
 		if (!desc->reslength) { /* zero length read */
 			dev_dbg(&desc->intf->dev, "%s: zero length - clearing WDM_READ\n", __func__);
-			rv = clear_wdm_read_flag(desc);
+			clear_bit(WDM_READ, &desc->flags);
+			rv = service_outstanding_interrupt(desc);
 			spin_unlock_irq(&desc->iuspin);
 			if (rv < 0)
 				goto err;
@@ -563,8 +580,10 @@ retry:
 
 	desc->length -= cntr;
 	/* in case we had outstanding data */
-	if (!desc->length)
-		clear_wdm_read_flag(desc);
+	if (!desc->length) {
+		clear_bit(WDM_READ, &desc->flags);
+		service_outstanding_interrupt(desc);
+	}
 	spin_unlock_irq(&desc->iuspin);
 	rv = cntr;
 
-- 
2.7.4

[toc] | [next] | [standalone]

#1458663

On Mon, 2016-08-08 at 11:48 -0400, robert.foss@collabora.com wrote:
> From: Robert Foss <robert.foss@collabora.com>
> 
> Implemented queued response handling. This queue is processed every time the
> WDM_READ flag is cleared.
> 
> In case of a read error, userspace may not actually read the data, since the
> driver returns an error through wdm_poll. After this, the underlying device may
> attempt to send us more data, but the queue is not processed. While userspace is
> also blocked, because the read error is never cleared.
> 
> After this patch, we proactively process the queue on a read error. If there was
> an outstanding response to handle, that will clear the error (or go through the
> same logic again, if another read error occurs). If there was no outstanding
> response, this will bring the queue size back to 0, unblocking a future response
> from the underlying device.
> 
> Tested-by: Robert Foss <robert.foss@collabora.com>
> Signed-off-by: Robert Foss <robert.foss@collabora.com>
Acked-by: Oliver Neukum <oneukum@suse.com>

[toc] | [prev] | [next] | [standalone]

#1458769

On Mon, Aug 08, 2016 at 11:48:52AM -0400, robert.foss@collabora.com wrote:
> From: Robert Foss <robert.foss@collabora.com>
> 
> Implemented queued response handling. This queue is processed every time the
> WDM_READ flag is cleared.
> 
> In case of a read error, userspace may not actually read the data, since the
> driver returns an error through wdm_poll. After this, the underlying device may
> attempt to send us more data, but the queue is not processed. While userspace is
> also blocked, because the read error is never cleared.
> 
> After this patch, we proactively process the queue on a read error. If there was
> an outstanding response to handle, that will clear the error (or go through the
> same logic again, if another read error occurs). If there was no outstanding
> response, this will bring the queue size back to 0, unblocking a future response
> from the underlying device.
> 
> Tested-by: Robert Foss <robert.foss@collabora.com>
> Signed-off-by: Robert Foss <robert.foss@collabora.com>
> Acked-by: Oliver Neukum <oneukum@suse.com>
> ---
> 
> Changes since v1:
> - Only set a new error code it there is no previous error code

This no longer applies to my tree due to a patch sent before yours for
the same driver.  Can you resync against my usb-testing brance on my
usb.git tree on git.kernel.org and resend?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1458798

On 2016-08-09 09:51 AM, Greg KH wrote:
> On Mon, Aug 08, 2016 at 11:48:52AM -0400, robert.foss@collabora.com wrote:
>> From: Robert Foss <robert.foss@collabora.com>
>>
>> Implemented queued response handling. This queue is processed every time the
>> WDM_READ flag is cleared.
>>
>> In case of a read error, userspace may not actually read the data, since the
>> driver returns an error through wdm_poll. After this, the underlying device may
>> attempt to send us more data, but the queue is not processed. While userspace is
>> also blocked, because the read error is never cleared.
>>
>> After this patch, we proactively process the queue on a read error. If there was
>> an outstanding response to handle, that will clear the error (or go through the
>> same logic again, if another read error occurs). If there was no outstanding
>> response, this will bring the queue size back to 0, unblocking a future response
>> from the underlying device.
>>
>> Tested-by: Robert Foss <robert.foss@collabora.com>
>> Signed-off-by: Robert Foss <robert.foss@collabora.com>
>> Acked-by: Oliver Neukum <oneukum@suse.com>

Thanks Oliver!

>> ---
>>
>> Changes since v1:
>> - Only set a new error code it there is no previous error code
>
> This no longer applies to my tree due to a patch sent before yours for
> the same driver.  Can you resync against my usb-testing brance on my
> usb.git tree on git.kernel.org and resend?
>

Will do!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web