Re: Handling incoming ZLP in cdc-wdm

[Robert Hodaszi <robert.hodaszi@digi.com>]

> Then we have a problem. If we are servicing a read() syscall,
> we have to either return data, or, if we cannot do that
> we either sleep or return -EAGAIN depending on O_NONBLOCK.
>
> There is nothing we can do about that. This makes me think
> that the issue here is poll() rather than in wdm_read()
(Resending the mail, because Thunderbird changed format to HTML...)

Yes, exactly, that's what I'm saying. :) It's not a problem with 
wdm_read(). The problem is that wdm_poll() confuses users with returning 
available data, although there's nothing there to read, and wdm_read() 
returns with EAGAIN (correctly). That's why we don't want to set 
WDM_READ when the buffer is empty, and there's no error neither (so we 
just received a ZLP).

Returning with EAGAIN in wdm_read() is totally OK, except if cdc-wdm 
told the caller through poll that there's available data.

>
>> So what about modifying the service_interrupt_work to no simply set 
>> WDM_READ if resp_count is 0, but instead to check if there's any real 
>> message in the buffer, to not confuse consumers. Something like this:
>
> That specific proposal will not work because the issue
> is in service_interrupt_work() which can already be scheduled.
> We cannot prevent that.
>
>>     diff --git a/drivers/usb/class/cdc-wdm.c 
>> b/drivers/usb/class/cdc-wdm.c
>>     index 37873acd18f4..9037379f3603 100644
>>     --- a/drivers/usb/class/cdc-wdm.c
>>     +++ b/drivers/usb/class/cdc-wdm.c
>>     @@ -1010,7 +1010,7 @@ static void service_interrupt_work(struct
>>     work_struct *work)
>>
>>              spin_lock_irq(&desc->iuspin);
>>              service_outstanding_interrupt(desc);
>>     -       if (!desc->resp_count) {
>>     +       if (!desc->resp_count && (desc->length || desc->rerr)) {
>>                      set_bit(WDM_READ, &desc->flags);
>>                      wake_up(&desc->wait);
>
> And what happens if wdm_read() wakes up because a signal is delivered?
>
OK, I might not see the obvious, but I don't understand this now. Let me 
copy here the complete patch, not just the chunks of it. That might make 
more sense. This can be completely wrong though.

    diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
    index 86ee39db013f..9164e311a3e8 100644
    --- a/drivers/usb/class/cdc-wdm.c
    +++ b/drivers/usb/class/cdc-wdm.c
    @@ -92,7 +92,6 @@ struct wdm_device {
          u16            wMaxCommand;
          u16            wMaxPacketSize;
          __le16            inum;
    -    int            reslength;
          int            length;
          int            read;
          int            count;
    @@ -214,6 +213,11 @@ static void wdm_in_callback(struct urb *urb)
          if (desc->rerr == 0 && status != -EPIPE)
              desc->rerr = status;

    +    if (length == 0) {
    +        dev_dbg(&desc->intf->dev, "received ZLP\n");
    +        goto skip_error;
    +    }
    +
          if (length + desc->length > desc->wMaxCommand) {
              /* The buffer would overflow */
              set_bit(WDM_OVERFLOW, &desc->flags);
    @@ -222,15 +226,14 @@ static void wdm_in_callback(struct urb *urb)
              if (!test_bit(WDM_OVERFLOW, &desc->flags)) {
                  memmove(desc->ubuf + desc->length, desc->inbuf, length);
                  desc->length += length;
    -            desc->reslength = length;
              }
          }
      skip_error:

    -    if (desc->rerr) {
    +    if (desc->rerr || length == 0) {
              /*
    -         * Since there was an error, userspace may decide to not read
    -         * any data after poll'ing.
    +         * If there was a ZLP or 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.
               */
    @@ -585,15 +588,6 @@ static ssize_t wdm_read
                  goto retry;
              }

    -        if (!desc->reslength) { /* zero length read */
    -            dev_dbg(&desc->intf->dev, "zero length - clearing
    WDM_READ\n");
    -            clear_bit(WDM_READ, &desc->flags);
    -            rv = service_outstanding_interrupt(desc);
    -            spin_unlock_irq(&desc->iuspin);
    -            if (rv < 0)
    -                goto err;
    -            goto retry;
    -        }
              cntr = desc->length;
              spin_unlock_irq(&desc->iuspin);
          }
    @@ -1005,7 +999,7 @@ static void service_interrupt_work(struct
    work_struct *work)

          spin_lock_irq(&desc->iuspin);
          service_outstanding_interrupt(desc);
    -    if (!desc->resp_count) {
    +    if (!desc->resp_count && (desc->length || desc->rerr)) {
              set_bit(WDM_READ, &desc->flags);
              wake_up(&desc->wait);
          }


So with this:

    1. not setting WDM_READ on ZLP
    2. on ZLP, we just submitting another URB to receive the remaining
    responses, if resp_count != 0, so keeping the receiving alive
    3. in service_interrupt_work(), just setting the WDM_READ if we
    finished response receiving, and there's data in the buffer for the
    user to read out, or we received and error
    4. reslength is no longer needed, as that checked for ZLP-s, but we
    are handling them wdm_in_callback()


Best regards,
Robert