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;
+
+ /* 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;
+ 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;
- 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;
- if (desc->length == 0) {
- desc->read = 0;
-retry:
+ if (*ppos == desc->pos) {
if (test_bit(WDM_DISCONNECTING, &desc->flags)) {
rv = -ENODEV;
goto err;
}
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;
} else {
rv = wait_event_interruptible(desc->wait,
- test_bit(WDM_READ, &desc->flags));
+ (*ppos < desc->pos) ||
+ test_bit(WDM_DISCONNECTING, &desc->flags));
}
/* 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;
- 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 */
+
+ 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;
rv = cntr;
err:
- mutex_unlock(&desc->rlock);
return rv;
}
@@ -468,7 +479,7 @@ static unsigned int wdm_poll(struct file *file, struct poll_table_struct *wait)
spin_unlock_irqrestore(&desc->iuspin, flags);
goto desc_out;
}
- if (test_bit(WDM_READ, &desc->flags))
+ if (file->f_pos < desc->pos)
mask = POLLIN | POLLRDNORM;
if (desc->rerr || desc->werr)
mask |= POLLERR;
@@ -497,6 +508,9 @@ static int wdm_open(struct inode *inode, struct file *file)
desc = usb_get_intfdata(intf);
if (test_bit(WDM_DISCONNECTING, &desc->flags))
goto out;
+
+ /* don't give out any data buffered prior to opening */
+ file->f_pos = desc->pos;
file->private_data = desc;
rv = usb_autopm_get_interface(desc->intf);
@@ -634,7 +648,6 @@ next_desc:
desc = kzalloc(sizeof(struct wdm_device), GFP_KERNEL);
if (!desc)
goto out;
- mutex_init(&desc->rlock);
mutex_init(&desc->wlock);
spin_lock_init(&desc->iuspin);
init_waitqueue_head(&desc->wait);
@@ -752,17 +765,14 @@ static void wdm_disconnect(struct usb_interface *intf)
/* the spinlock makes sure no new urbs are generated in the callbacks */
spin_lock_irqsave(&desc->iuspin, flags);
set_bit(WDM_DISCONNECTING, &desc->flags);
- set_bit(WDM_READ, &desc->flags);
/* to terminate pending flushes */
clear_bit(WDM_IN_USE, &desc->flags);
spin_unlock_irqrestore(&desc->iuspin, flags);
wake_up_all(&desc->wait);
- mutex_lock(&desc->rlock);
mutex_lock(&desc->wlock);
kill_urbs(desc);
cancel_work_sync(&desc->rxwork);
mutex_unlock(&desc->wlock);
- mutex_unlock(&desc->rlock);
if (!desc->count)
cleanup(desc);
mutex_unlock(&wdm_mutex);
@@ -777,10 +787,9 @@ static int wdm_suspend(struct usb_interface *intf, pm_message_t message)
dev_dbg(&desc->intf->dev, "wdm%d_suspend\n", intf->minor);
/* if this is an autosuspend the caller does the locking */
- if (!PMSG_IS_AUTO(message)) {
- mutex_lock(&desc->rlock);
+ if (!PMSG_IS_AUTO(message))
mutex_lock(&desc->wlock);
- }
+
spin_lock_irq(&desc->iuspin);
if (PMSG_IS_AUTO(message) &&
@@ -796,10 +805,8 @@ static int wdm_suspend(struct usb_interface *intf, pm_message_t message)
kill_urbs(desc);
cancel_work_sync(&desc->rxwork);
}
- if (!PMSG_IS_AUTO(message)) {
+ if (!PMSG_IS_AUTO(message))
mutex_unlock(&desc->wlock);
- mutex_unlock(&desc->rlock);
- }
return rv;
}
@@ -837,7 +844,6 @@ static int wdm_pre_reset(struct usb_interface *intf)
{
struct wdm_device *desc = usb_get_intfdata(intf);
- mutex_lock(&desc->rlock);
mutex_lock(&desc->wlock);
kill_urbs(desc);
@@ -861,7 +867,6 @@ static int wdm_post_reset(struct usb_interface *intf)
rv = recover_from_urb_loss(desc);
mutex_unlock(&desc->wlock);
- mutex_unlock(&desc->rlock);
return 0;
}
--
1.7.8.3
--
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
