Am Freitag, 20. Januar 2012, 01:50:01 schrieb Bjørn Mork:
> This changes a number of fundamental parts of the design:
> - use a shared ring buffer for all read data
> - use per file position counter to map each open file into the buffer
> - no read locking at all! reading is just a matter of copying data
> from the ring buffer
> - using an ever-increasing file position counter mapping into the
> buffer
> - try hard to let every client start reading at a message boundary
> to support packetized management protocols better
>
> Signed-off-by: Bjørn Mork <bjorn@xxxxxxx>
> ---
> This is more of a request for comments thing at the moment. It changes
> a lot in one go. But there really isn't much that can be done in
> steps here. Once you put the ring buffer there then the read locking
> goes away.
>
> FWIW, this Works For Me, both using the AT command based real Device
> Management interfaces on the Ericsson F3507g modem, and using the
> QMI command based CDC ECM lookalike interface on the Huawei E392
> modem
>
>
> drivers/usb/class/cdc-wdm.c | 137 ++++++++++++++++++++++---------------------
> 1 files changed, 71 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
> index b76696b..bd0cb5f 100644
> --- a/drivers/usb/class/cdc-wdm.c
> +++ b/drivers/usb/class/cdc-wdm.c
> @@ -49,7 +49,7 @@ MODULE_DEVICE_TABLE (usb, wdm_ids);
> #define WDM_IN_USE 1
> #define WDM_DISCONNECTING 2
> #define WDM_RESULT 3
> -#define WDM_READ 4
> +/* unused 4 */
> #define WDM_INT_STALL 5
> #define WDM_POLL_RUNNING 6
> #define WDM_RESPONDING 7
> @@ -80,20 +80,48 @@ struct wdm_device {
>
> unsigned long flags;
> u16 bufsize;
> - int reslength;
> - int length;
> - int read;
> int count;
> struct mutex wlock;
> - struct mutex rlock;
> wait_queue_head_t wait;
> struct work_struct rxwork;
> int werr;
> int rerr;
> +
> + loff_t pos; /* virtual "file position" mapped into ubuf */
> + loff_t valid; /* maintain boundary for packetized protocols! */
> };
>
> static struct usb_driver wdm_driver;
>
> +/* caller must hold desc->iulock spinlock */
> +static ssize_t wdm_copy_to_ubuf(struct wdm_device *desc, u8 *data, size_t len)
> +{
> + int rv = -EFAULT;
> + off_t offset;
> + size_t n;
> +
> + if (len > desc->bufsize)
> + goto err;
Why bother? Either you can deal with a full buffer or not. Even in this
case why not take as much data as you can?
> +
> + /* buffer offset */
> + offset = desc->pos % desc->bufsize;
> +
> + n = min((size_t)desc->bufsize - offset, len);
> + memcpy(desc->ubuf + offset, data, n);
> +
> + /* wrap around? */
> + if (n < len) {
> + memcpy(desc->ubuf, data + n, len - n);
> + desc->valid = desc->pos; /* saving last sane boundary */
> + }
> +
> + /* move current position */
> + desc->pos += len;
And what keeps this from overflowing?
> + rv = len;
> +err:
> + return rv;
> +}
> +
> /* --- callbacks --- */
> static void wdm_out_callback(struct urb *urb)
> {
> @@ -141,13 +169,11 @@ static void wdm_in_callback(struct urb *urb)
> }
>
> desc->rerr = status;
> - desc->reslength = urb->actual_length;
> - memmove(desc->ubuf + desc->length, desc->inbuf, desc->reslength);
> - desc->length += desc->reslength;
> + if (urb->actual_length > 0)
> + wdm_copy_to_ubuf(desc, urb->transfer_buffer, urb->actual_length);
> skip_error:
> wake_up(&desc->wait);
>
> - set_bit(WDM_READ, &desc->flags);
> spin_unlock(&desc->iuspin);
> }
>
> @@ -206,7 +232,6 @@ static void wdm_int_callback(struct urb *urb)
> }
>
> spin_lock(&desc->iuspin);
> - clear_bit(WDM_READ, &desc->flags);
> set_bit(WDM_RESPONDING, &desc->flags);
> if (!test_bit(WDM_DISCONNECTING, &desc->flags)
> && !test_bit(WDM_SUSPENDING, &desc->flags)) {
> @@ -360,30 +385,34 @@ static ssize_t wdm_read
> {
> int rv, cntr = 0;
> int i = 0;
> + size_t len, offset;
> struct wdm_device *desc = file->private_data;
>
> + /* cannot read more than a full buffer */
> + if (count > desc->bufsize)
> + count = desc->bufsize;
Again, why bother? Either you can deal with requests to read more than
available or you cannot.
>
> - rv = mutex_lock_interruptible(&desc->rlock); /*concurrent reads */
> - if (rv < 0)
> - return -ERESTARTSYS;
> + /*
> + * if we are more than a bufsize behind, then we've inevitably
> + * lost data - nothing to do but to do a forced llseek to the
> + * last known boundary
> + */
> + if (*ppos + desc->bufsize < desc->pos)
> + *ppos = desc->valid;
This races with the callback
>
> - if (desc->length == 0) {
> - desc->read = 0;
> -retry:
> + if (*ppos == desc->pos) {
> if (test_bit(WDM_DISCONNECTING, &desc->flags)) {
> rv = -ENODEV;
> goto err;
> }
> i++;
What is the function of i?
> if (file->f_flags & O_NONBLOCK) {
> - if (!test_bit(WDM_READ, &desc->flags)) {
> - rv = cntr ? cntr : -EAGAIN;
> - goto err;
> - }
> - rv = 0;
> + rv = -EAGAIN;
> + goto err;
No way. If you remove the loop, you must be able to handle read errors
in the O_NONBLOCK case. You cannot unconditionally return -EAGAIN
> } else {
> rv = wait_event_interruptible(desc->wait,
> - test_bit(WDM_READ, &desc->flags));
> + (*ppos < desc->pos) ||
> + test_bit(WDM_DISCONNECTING, &desc->flags));
Again, this is wrong.
1. You fail to test for an error condition as a reason for a wakeup
2. As you test for a disconnection here, you must be able to handle it
3. As you have removed the loop and you are rightly keeping the interruptible
sleep, you must be able to handle signals.
> }
>
> /* may have happened while we slept */
> @@ -397,50 +426,32 @@ retry:
> goto err;
> }
>
> - spin_lock_irq(&desc->iuspin);
> -
> - if (desc->rerr) { /* read completed, error happened */
> - desc->rerr = 0;
No, you cannot simply remove unsetting the error.
And you need to hold the spinlock to handle the race with the callback.
> - spin_unlock_irq(&desc->iuspin);
> + /* read completed, error happened */
> + if (desc->rerr) {
> rv = -EIO;
> goto err;
> }
> - /*
> - * recheck whether we've lost the race
> - * against the completion handler
> - */
> - if (!test_bit(WDM_READ, &desc->flags)) { /* lost race */
> - spin_unlock_irq(&desc->iuspin);
> - goto retry;
> - }
> - if (!desc->reslength) { /* zero length read */
> - spin_unlock_irq(&desc->iuspin);
> - goto retry;
> - }
> - clear_bit(WDM_READ, &desc->flags);
> - spin_unlock_irq(&desc->iuspin);
> }
>
> - cntr = count > desc->length ? desc->length : count;
> - rv = copy_to_user(buffer, desc->ubuf, cntr);
> + offset = *ppos % desc->bufsize; /* buffer offset */
> + len = desc->pos - *ppos; /* data length */
> + cntr = min(count, len); /* read length */
> + len = min((int)(desc->bufsize - offset), cntr); /* copy length */
So you will happily give the same data to multiple readers?
And what happens if the callback arrives here? You'll deliver
partially old and partially new data.
> +
> + rv = copy_to_user(buffer, desc->ubuf + offset, len);
> + if (len < cntr) /* wrap around? */
> + rv += copy_to_user(buffer + len, desc->ubuf, cntr - len);
> +
> if (rv > 0) {
> rv = -EFAULT;
> goto err;
> }
>
> - for (i = 0; i < desc->length - cntr; i++)
> - desc->ubuf[i] = desc->ubuf[i + cntr];
> -
> - spin_lock_irq(&desc->iuspin);
> - desc->length -= cntr;
> - spin_unlock_irq(&desc->iuspin);
> - /* in case we had outstanding data */
> - if (!desc->length)
> - clear_bit(WDM_READ, &desc->flags);
> + /* move file position */
> + *ppos += cntr;
And this races in case of multiple readers.
> rv = cntr;
>
> err:
> - mutex_unlock(&desc->rlock);
> return rv;
> }
>
I am afraid this needs some serious rework. It is currently in no shape
for inclusion.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
