Oliver & others:
I obtained the modem and was able to reproduce the bug. It's very easy.
Also, I turned this around with our customer. I found that the original
patch had a bug where I tried to be too clever. The patch copies the
logics from my 2.4 usbserial code, but not exactly. This is what
write_room does in 2.4:
retval = 0;
if (!port->write_busy && port->write_backlog == 0)
retval = port->bulk_out_size;
return retval;
My first acm patch did this:
+ return acm_wb_is_avail(acm) ? acm->writesize : 0;
This is what it does now:
+ return (acm->write_ready && acm_wb_is_avail(acm)) ? acm->writesize : 0;
Meh. Should've just copied known-working code...
Aside from correcting the above, I moved this patch to a newer codebase
(it needed small merging because of some structure renaming), and applied
all changes from review. It continues to advance to new buffer
unconditionally, because I'm convinced that it's the best way and
it's known to be correct.
I have tested the patch and it fixes the missing LF problem completely.
Greg, Oliver, I think this should be applied.
-- Pete
diff -urp -X dontdiff linux-2.6.12-rc1/drivers/usb/class/cdc-acm.c
linux-2.6.12-rc1-lem/drivers/usb/class/cdc-acm.c
--- linux-2.6.12-rc1/drivers/usb/class/cdc-acm.c 2005-03-18
17:12:12.000000000 -0800
+++ linux-2.6.12-rc1-lem/drivers/usb/class/cdc-acm.c 2005-04-18
13:53:26.000000000 -0700
@@ -106,6 +106,111 @@ static int acm_ctrl_msg(struct acm *acm,
acm_ctrl_msg(acm, USB_CDC_REQ_SEND_BREAK, ms, NULL, 0)
/*
+ * Write buffer management.
+ * All of these assume proper locks taken by the caller.
+ */
+
+static int acm_wb_alloc(struct acm *acm)
+{
+ int i, wbn;
+ struct acm_wb *wb;
+
+ wbn = acm->write_current;
+ i = 0;
+ for (;;) {
+ wb = &acm->wb[wbn];
+ if (!wb->use) {
+ wb->use = 1;
+ return wbn;
+ }
+ wbn = (wbn + 1) % ACM_NWB;
+ if (++i >= ACM_NWB)
+ return -1;
+ }
+}
+
+static void acm_wb_free(struct acm *acm, int wbn)
+{
+ acm->wb[wbn].use = 0;
+}
+
+static int acm_wb_is_avail(struct acm *acm)
+{
+ int i, n;
+
+ n = 0;
+ for (i = 0; i < ACM_NWB; i++) {
+ if (!acm->wb[i].use)
+ n++;
+ }
+ return n;
+}
+
+static inline int acm_wb_is_used(struct acm *acm, int wbn)
+{
+ return acm->wb[wbn].use;
+}
+
+/*
+ * Finish write.
+ */
+static void acm_write_done(struct acm *acm)
+{
+ unsigned long flags;
+ int wbn;
+
+ spin_lock_irqsave(&acm->write_lock, flags);
+ acm->write_ready = 1;
+ wbn = acm->write_current;
+ acm_wb_free(acm, wbn);
+ acm->write_current = (wbn + 1) % ACM_NWB;
+ spin_unlock_irqrestore(&acm->write_lock, flags);
+}
+
+/*
+ * Poke write.
+ */
+static int acm_write_start(struct acm *acm)
+{
+ unsigned long flags;
+ int wbn;
+ struct acm_wb *wb;
+ int rc;
+
+ spin_lock_irqsave(&acm->write_lock, flags);
+ if (!acm->dev) {
+ spin_unlock_irqrestore(&acm->write_lock, flags);
+ return -ENODEV;
+ }
+
+ if (!acm->write_ready) {
+ spin_unlock_irqrestore(&acm->write_lock, flags);
+ return 0; /* A white lie */
+ }
+
+ wbn = acm->write_current;
+ if (!acm_wb_is_used(acm, wbn)) {
+ spin_unlock_irqrestore(&acm->write_lock, flags);
+ return 0;
+ }
+ wb = &acm->wb[wbn];
+
+ acm->write_ready = 0;
+ spin_unlock_irqrestore(&acm->write_lock, flags);
+
+ acm->writeurb->transfer_buffer = wb->buf;
+ acm->writeurb->transfer_dma = wb->dmah;
+ acm->writeurb->transfer_buffer_length = wb->len;
+ acm->writeurb->dev = acm->dev;
+
+ if ((rc = usb_submit_urb(acm->writeurb, GFP_ATOMIC)) < 0) {
+ dbg("usb_submit_urb(write bulk) failed: %d", rc);
+ acm_write_done(acm);
+ }
+ return rc;
+}
+
+/*
* Interrupt handlers for various ACM device responses
*/
@@ -237,17 +342,13 @@ static void acm_rx_tasklet(unsigned long
static void acm_write_bulk(struct urb *urb, struct pt_regs *regs)
{
struct acm *acm = (struct acm *)urb->context;
- dbg("Entering acm_write_bulk with status %d\n", urb->status);
-
- if (!ACM_READY(acm))
- goto out;
- if (urb->status)
- dbg("nonzero write bulk status received: %d", urb->status);
+ dbg("Entering acm_write_bulk with status %d\n", urb->status);
- schedule_work(&acm->work);
-out:
- acm->ready_for_write = 1;
+ acm_write_done(acm);
+ acm_write_start(acm);
+ if (ACM_READY(acm))
+ schedule_work(&acm->work);
}
static void acm_softint(void *private)
@@ -351,32 +452,33 @@ static int acm_tty_write(struct tty_stru
{
struct acm *acm = tty->driver_data;
int stat;
+ unsigned long flags;
+ int wbn;
+ struct acm_wb *wb;
+
dbg("Entering acm_tty_write to write %d bytes,\n", count);
if (!ACM_READY(acm))
return -EINVAL;
- if (!acm->ready_for_write)
- return 0;
if (!count)
return 0;
- count = (count > acm->writesize) ? acm->writesize : count;
+ spin_lock_irqsave(&acm->write_lock, flags);
+ if ((wbn = acm_wb_alloc(acm)) < 0) {
+ spin_unlock_irqrestore(&acm->write_lock, flags);
+ acm_write_start(acm);
+ return 0;
+ }
+ wb = &acm->wb[wbn];
+ count = (count > acm->writesize) ? acm->writesize : count;
dbg("Get %d bytes...", count);
- memcpy(acm->write_buffer, buf, count);
- dbg(" Successfully copied.\n");
+ memcpy(wb->buf, buf, count);
+ wb->len = count;
+ spin_unlock_irqrestore(&acm->write_lock, flags);
- acm->writeurb->transfer_buffer_length = count;
- acm->writeurb->dev = acm->dev;
-
- acm->ready_for_write = 0;
- stat = usb_submit_urb(acm->writeurb, GFP_ATOMIC);
- if (stat < 0) {
- dbg("usb_submit_urb(write bulk) failed");
- acm->ready_for_write = 1;
+ if ((stat = acm_write_start(acm)) < 0)
return stat;
- }
-
return count;
}
@@ -385,7 +487,11 @@ static int acm_tty_write_room(struct tty
struct acm *acm = tty->driver_data;
if (!ACM_READY(acm))
return -EINVAL;
- return !acm->ready_for_write ? 0 : acm->writesize;
+ /*
+ * Do not let the line discipline to know that we have a reserve,
+ * or it might get too enthusiastic.
+ */
+ return (acm->write_ready && acm_wb_is_avail(acm)) ? acm->writesize : 0;
}
static int acm_tty_chars_in_buffer(struct tty_struct *tty)
@@ -393,7 +499,10 @@ static int acm_tty_chars_in_buffer(struc
struct acm *acm = tty->driver_data;
if (!ACM_READY(acm))
return -EINVAL;
- return !acm->ready_for_write ? acm->writeurb->transfer_buffer_length :
0;
+ /*
+ * This is inaccurate (overcounts), but it works.
+ */
+ return (ACM_NWB - acm_wb_is_avail(acm)) * acm->writesize;
}
static void acm_tty_throttle(struct tty_struct *tty)
@@ -526,6 +635,39 @@ static void acm_tty_set_termios(struct t
* USB probe and disconnect routines.
*/
+/* Little helper: write buffers free */
+static void acm_write_buffers_free(struct acm *acm)
+{
+ int i;
+ struct acm_wb *wb;
+
+ for (wb = &acm->wb[0], i = 0; i < ACM_NWB; i++, wb++) {
+ usb_buffer_free(acm->dev, acm->writesize, wb->buf, wb->dmah);
+ }
+}
+
+/* Little helper: write buffers allocate */
+static int acm_write_buffers_alloc(struct acm *acm)
+{
+ int i;
+ struct acm_wb *wb;
+
+ for (wb = &acm->wb[0], i = 0; i < ACM_NWB; i++, wb++) {
+ wb->buf = usb_buffer_alloc(acm->dev, acm->writesize, GFP_KERNEL,
+ &wb->dmah);
+ if (!wb->buf) {
+ while (i != 0) {
+ --i;
+ --wb;
+ usb_buffer_free(acm->dev, acm->writesize,
+ wb->buf, wb->dmah);
+ }
+ return -ENOMEM;
+ }
+ }
+ return 0;
+}
+
static int acm_probe (struct usb_interface *intf,
const struct usb_device_id *id)
{
@@ -700,7 +842,8 @@ skip_normal_probe:
acm->bh.data = (unsigned long) acm;
INIT_WORK(&acm->work, acm_softint, acm);
spin_lock_init(&acm->throttle_lock);
- acm->ready_for_write = 1;
+ spin_lock_init(&acm->write_lock);
+ acm->write_ready = 1;
buf = usb_buffer_alloc(usb_dev, ctrlsize, GFP_KERNEL, &acm->ctrl_dma);
if (!buf) {
@@ -716,12 +859,10 @@ skip_normal_probe:
}
acm->read_buffer = buf;
- buf = usb_buffer_alloc(usb_dev, acm->writesize, GFP_KERNEL,
&acm->write_dma);
- if (!buf) {
+ if (acm_write_buffers_alloc(acm) < 0) {
dev_dbg(&intf->dev, "out of memory (write buffer alloc)\n");
goto alloc_fail4;
}
- acm->write_buffer = buf;
acm->ctrlurb = usb_alloc_urb(0, GFP_KERNEL);
if (!acm->ctrlurb) {
@@ -750,9 +891,9 @@ skip_normal_probe:
acm->readurb->transfer_dma = acm->read_dma;
usb_fill_bulk_urb(acm->writeurb, usb_dev, usb_sndbulkpipe(usb_dev,
epwrite->bEndpointAddress),
- acm->write_buffer, acm->writesize, acm_write_bulk,
acm);
+ NULL, acm->writesize, acm_write_bulk, acm);
acm->writeurb->transfer_flags |= URB_NO_FSBR | URB_NO_TRANSFER_DMA_MAP;
- acm->writeurb->transfer_dma = acm->write_dma;
+ /* acm->writeurb->transfer_dma = 0; */
dev_info(&intf->dev, "ttyACM%d: USB ACM device\n", minor);
@@ -775,7 +916,7 @@ alloc_fail7:
alloc_fail6:
usb_free_urb(acm->ctrlurb);
alloc_fail5:
- usb_buffer_free(usb_dev, acm->writesize, acm->write_buffer,
acm->write_dma);
+ acm_write_buffers_free(acm);
alloc_fail4:
usb_buffer_free(usb_dev, readsize, acm->read_buffer, acm->read_dma);
alloc_fail3:
@@ -806,7 +947,7 @@ static void acm_disconnect(struct usb_in
flush_scheduled_work(); /* wait for acm_softint */
- usb_buffer_free(usb_dev, acm->writesize, acm->write_buffer,
acm->write_dma);
+ acm_write_buffers_free(acm);
usb_buffer_free(usb_dev, acm->readsize, acm->read_buffer,
acm->read_dma);
usb_buffer_free(usb_dev, acm->ctrlsize, acm->ctrl_buffer,
acm->ctrl_dma);
diff -urp -X dontdiff linux-2.6.12-rc1/drivers/usb/class/cdc-acm.h
linux-2.6.12-rc1-lem/drivers/usb/class/cdc-acm.h
--- linux-2.6.12-rc1/drivers/usb/class/cdc-acm.h 2005-03-18
17:12:12.000000000 -0800
+++ linux-2.6.12-rc1-lem/drivers/usb/class/cdc-acm.h 2005-03-19
17:09:44.000000000 -0800
@@ -51,14 +51,34 @@
* Internal driver structures.
*/
+/*
+ * The only reason to have several buffers is to accomodate assumptions
+ * in line disciplines. They ask for empty space amount, receive our URB size,
+ * and proceed to issue several 1-character writes, assuming they will fit.
+ * The very first write takes a complete URB. Fortunately, this only happens
+ * when processing onlcr, so we only need 2 buffers.
+ */
+#define ACM_NWB 2
+struct acm_wb {
+ unsigned char *buf;
+ dma_addr_t dmah;
+ int len;
+ int use;
+};
+
struct acm {
struct usb_device *dev; /* the corresponding
usb device */
struct usb_interface *control; /* control interface */
struct usb_interface *data; /* data interface */
struct tty_struct *tty; /* the corresponding
tty */
struct urb *ctrlurb, *readurb, *writeurb; /* urbs */
- u8 *ctrl_buffer, *read_buffer, *write_buffer; /* buffers of urbs */
- dma_addr_t ctrl_dma, read_dma, write_dma; /* dma handles of
buffers */
+ u8 *ctrl_buffer, *read_buffer; /* buffers of urbs */
+ dma_addr_t ctrl_dma, read_dma; /* dma handles of
buffers */
+ struct acm_wb wb[ACM_NWB];
+ int write_current; /* current write buffer
*/
+ int write_used; /* number of non-empty
write buffers */
+ int write_ready; /* write urb is not
running */
+ spinlock_t write_lock;
struct usb_cdc_line_coding line; /* bits, stop, parity */
struct work_struct work; /* work queue entry for
line discipline waking up */
struct tasklet_struct bh; /* rx processing */
@@ -71,7 +91,6 @@ struct acm {
unsigned int minor; /* acm minor number */
unsigned char throttle; /* throttled by tty
layer */
unsigned char clocal; /* termios CLOCAL */
- unsigned char ready_for_write; /* write urb can be
used */
unsigned char resubmit_to_unthrottle; /* throtteling has
disabled the read urb */
unsigned int ctrl_caps; /* control capabilities
from the class specific header */
};
-------------------------------------------------------
This SF.Net email is sponsored by: New Crystal Reports XI.
Version 11 adds new functionality designed to reduce time involved in
creating, integrating, and deploying reporting solutions. Free runtime info,
new features, or free trial, at: http://www.businessobjects.com/devxi/728
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
