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

Groups > linux.kernel > #1452040 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PACTH v1] cdc-wdm: Clear read pipeline in case of error robert.foss@collabora.com - 2016-07-28 20:30 +0200
    Re: [PACTH v1] cdc-wdm: Clear read pipeline in case of error Robert Foss <robert.foss@collabora.com> - 2016-08-02 16:10 +0200
      Re: [PACTH v1] cdc-wdm: Clear read pipeline in case of error Oliver Neukum <oneukum@suse.com> - 2016-08-02 16:10 +0200
        Re: [PACTH v1] cdc-wdm: Clear read pipeline in case of error Robert Foss <robert.foss@collabora.com> - 2016-08-02 16:40 +0200
          Re: [PACTH v1] cdc-wdm: Clear read pipeline in case of error Oliver Neukum <oneukum@suse.com> - 2016-08-03 12:50 +0200
            Re: [PACTH v1] cdc-wdm: Clear read pipeline in case of error Robert Foss <robert.foss@collabora.com> - 2016-08-04 19:50 +0200
              Re: [PACTH v1] cdc-wdm: Clear read pipeline in case of error Oliver Neukum <oneukum@suse.com> - 2016-08-07 11:10 +0200
                Re: [PACTH v1] cdc-wdm: Clear read pipeline in case of error Robert Foss <robert.foss@collabora.com> - 2016-08-08 16:40 +0200
    Re: [PACTH v1] cdc-wdm: Clear read pipeline in case of error Oliver Neukum <oneukum@suse.com> - 2016-08-02 16:50 +0200

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

From: Prathmesh Prabhu <pprabhu@chromium.org>

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.

Signed-off-by: Prathmesh Prabhu <pprabhu@chromium.org>
Tested-by: Robert Foss <robert.foss@collabora.com>
Signed-off-by: Robert Foss <robert.foss@collabora.com>
---
 drivers/usb/class/cdc-wdm.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 337948c..521011c 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;
@@ -201,9 +204,22 @@ 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.
+		 * Note that, this means it is no longer guaranteed that
+		 * userspace will see desc->rerr, since the device could send us
+		 * new data before userspace has a chance to see the error.
+		 */
+		service_outstanding_interrupt(desc);
+	}
+
 	spin_unlock(&desc->iuspin);
 }
 
@@ -436,17 +452,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 +551,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 +577,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]

#1454492

On 2016-08-02 08:23 AM, Oliver Neukum wrote:
> On Thu, 2016-07-28 at 14:19 -0400, robert.foss@collabora.com wrote:
>> From: Prathmesh Prabhu <pprabhu@chromium.org>
>>
>> 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.
>
> Could you explain why user space cannot just read more data?
> That will clear the error.

Userspace certainly could read more data, but for the case when 
userspace doesn't read and clear a potential an error, we still would 
like to not be stuck if the device sends more data.

I hope that answers your question, if not I'll try to be more elaborate.

>
> 	Regards
> 		Oliver
>
>

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

#1454504

On Tue, 2016-08-02 at 09:54 -0400, Robert Foss wrote:
> 
> On 2016-08-02 08:23 AM, Oliver Neukum wrote:
> > On Thu, 2016-07-28 at 14:19 -0400, robert.foss@collabora.com wrote:
> >> From: Prathmesh Prabhu <pprabhu@chromium.org>
> >>
> >> 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.
> >
> > Could you explain why user space cannot just read more data?
> > That will clear the error.
> 
> Userspace certainly could read more data, but for the case when 
> userspace doesn't read and clear a potential an error, we still would 
> like to not be stuck if the device sends more data.
> 
> I hope that answers your question, if not I'll try to be more elaborate.

Clear, but why does that require the suppression of an error condition?
errors should always be delivered.

	Regards
		Oliver

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

#1454607

On 2016-08-02 09:59 AM, Oliver Neukum wrote:
> On Tue, 2016-08-02 at 09:54 -0400, Robert Foss wrote:
>>
>> On 2016-08-02 08:23 AM, Oliver Neukum wrote:
>>> On Thu, 2016-07-28 at 14:19 -0400, robert.foss@collabora.com wrote:
>>>> From: Prathmesh Prabhu <pprabhu@chromium.org>
>>>>
>>>> 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.
>>>
>>> Could you explain why user space cannot just read more data?
>>> That will clear the error.
>>
>> Userspace certainly could read more data, but for the case when
>> userspace doesn't read and clear a potential an error, we still would
>> like to not be stuck if the device sends more data.
>>
>> I hope that answers your question, if not I'll try to be more elaborate.
>
> Clear, but why does that require the suppression of an error condition?
> errors should always be delivered.

The goal is not to clear the error condition, but that is required to 
not stay stuck.

Maintaining the error condition and not staying stuck if the device 
sends more data are mutually exclusive as far as I understand it.


Rob.

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

#1455740

On Tue, 2016-08-02 at 10:37 -0400, Robert Foss wrote:
> 
> On 2016-08-02 09:59 AM, Oliver Neukum wrote:
> > On Tue, 2016-08-02 at 09:54 -0400, Robert Foss wrote:
> >>
> >> On 2016-08-02 08:23 AM, Oliver Neukum wrote:
> >>> On Thu, 2016-07-28 at 14:19 -0400, robert.foss@collabora.com wrote:
> >>>> From: Prathmesh Prabhu <pprabhu@chromium.org>
> >>>>
> >>>> 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.
> >>>
> >>> Could you explain why user space cannot just read more data?
> >>> That will clear the error.
> >>
> >> Userspace certainly could read more data, but for the case when
> >> userspace doesn't read and clear a potential an error, we still would
> >> like to not be stuck if the device sends more data. space
> >>
> >> I hope that answers your question, if not I'll try to be more elaborate.
> >
> > Clear, but why does that require the suppression of an error condition?
> > errors should always be delivered.
> 
> The goal is not to clear the error condition, but that is required to 
> not stay stuck.

How can that depend on what we return to user space?
In the driver we can continue just ignoring errors.
Now, if user space stops reading because we reported an error,
that is the decision user space has made. We cannot ignore errors
in the kernel because we don't like what user space does when it
sees the error.

	Regards
		Oliver

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

#1456613

On 2016-08-03 06:39 AM, Oliver Neukum wrote:
> On Tue, 2016-08-02 at 10:37 -0400, Robert Foss wrote:
>>
>> On 2016-08-02 09:59 AM, Oliver Neukum wrote:
>>> On Tue, 2016-08-02 at 09:54 -0400, Robert Foss wrote:
>>>>
>>>> On 2016-08-02 08:23 AM, Oliver Neukum wrote:
>>>>> On Thu, 2016-07-28 at 14:19 -0400, robert.foss@collabora.com wrote:
>>>>>> From: Prathmesh Prabhu <pprabhu@chromium.org>
>>>>>>
>>>>>> 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.
>>>>>
>>>>> Could you explain why user space cannot just read more data?
>>>>> That will clear the error.
>>>>
>>>> Userspace certainly could read more data, but for the case when
>>>> userspace doesn't read and clear a potential an error, we still would
>>>> like to not be stuck if the device sends more data. space
>>>>
>>>> I hope that answers your question, if not I'll try to be more elaborate.
>>>
>>> Clear, but why does that require the suppression of an error condition?
>>> errors should always be delivered.
>>
>> The goal is not to clear the error condition, but that is required to
>> not stay stuck.
>
> How can that depend on what we return to user space?
> In the driver we can continue just ignoring errors.
> Now, if user space stops reading because we reported an error,
> that is the decision user space has made. We cannot ignore errors
> in the kernel because we don't like what user space does when it
> sees the error.

So perhaps the better solution is to be more intelligent about how 
desc->rerr is written to during after an error to be able to maintain 
the error condition?

>
> 	Regards
> 		Oliver
>
>

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

#1457443

On Thu, 2016-08-04 at 13:44 -0400, Robert Foss wrote:
> 
> On 2016-08-03 06:39 AM, Oliver Neukum wrote:
> > On Tue, 2016-08-02 at 10:37 -0400, Robert Foss wrote:

> > How can that depend on what we return to user space?
> > In the driver we can continue just ignoring errors.
> > Now, if user space stops reading because we reported an error,
> > that is the decision user space has made. We cannot ignore errors
> > in the kernel because we don't like what user space does when it
> > sees the error.
> 
> So perhaps the better solution is to be more intelligent about how 
> desc->rerr is written to during after an error to be able to maintain 
> the error condition?

Yes, good idea. I think an error condition should never be overwritten.
So we go to the current behaviour only if a second error before
user space has seen the3 first error arises. Would that fix your
issue?

	Regards
		Oliver

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

#1457841

On 2016-08-07 04:59 AM, Oliver Neukum wrote:
> On Thu, 2016-08-04 at 13:44 -0400, Robert Foss wrote:
>>
>> On 2016-08-03 06:39 AM, Oliver Neukum wrote:
>>> On Tue, 2016-08-02 at 10:37 -0400, Robert Foss wrote:
>
>>> How can that depend on what we return to user space?
>>> In the driver we can continue just ignoring errors.
>>> Now, if user space stops reading because we reported an error,
>>> that is the decision user space has made. We cannot ignore errors
>>> in the kernel because we don't like what user space does when it
>>> sees the error.
>>
>> So perhaps the better solution is to be more intelligent about how
>> desc->rerr is written to during after an error to be able to maintain
>> the error condition?
>
> Yes, good idea. I think an error condition should never be overwritten.
> So we go to the current behaviour only if a second error before
> user space has seen the3 first error arises. Would that fix your
> issue?

Excellent!

As long as the device is able to keep pushing data during an -EPIPE 
error condition, I think my issues would be solved.

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

#1454619

On Thu, 2016-07-28 at 14:19 -0400, robert.foss@collabora.com wrote:
> From: Prathmesh Prabhu <pprabhu@chromium.org>
> 
> 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.

Could you explain why user space cannot just read more data?
That will clear the error.

	Regards
		Oliver

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web