Am Freitag, 28. August 2009 17:25:18 schrieb Greg KH:
> On Fri, Aug 28, 2009 at 10:48:08AM -0400, Alan Stern wrote:
> > On Fri, 28 Aug 2009, Oliver Neukum wrote:
> > > Hi,
> > >
> > > if I understand this correctly, no driver is using max_in_flight_urbs.
> > > Do we want to keep this feature or not? If we want to keep it, which
> > > drivers would sensibly use it?
> >
> > It is used in the debug-cable driver (usb_debug.c). Is that reason
> > enough to keep it?
>
> Yes, the goal was to move the other drivers to start using it as well,
> to remove the duplicated logic that they have for this type of thing.
>
> So I'd like to keep it, and start using it more often if possible.
Good. But if this is to be done it better be done right.
So here's a version of it with interrupt mitigation. What
do you think?
Regards
Oliver
--
commit 17149582cc0f2c84e32af17a484a426e55b344f9
Author: Oliver Neukum <oliver@xxxxxxxxxx>
Date: Fri Aug 28 14:55:56 2009 +0200
usb: serial: provide interrupt mitigation for the multi urb pattern
diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index ce57f6a..169d00d 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -191,86 +191,117 @@ void usb_serial_generic_close(struct usb_serial_port *port)
generic_cleanup(port);
}
+static void usb_serial_generic_write_mult_callback(struct urb *urb)
+{
+ struct usb_serial_transfer *cluster = urb->context;
+ struct usb_serial_port *port;
+ int status = urb->status;
+ int i;
+ int size;
+
+ if (status == -ECONNRESET)
+ return;
+
+ port = cluster->port;
+ size = cluster->size;
+
+ if (status < 0)
+ for(i = 0; i < size; i++)
+ usb_unlink_urb(cluster->urb[i]);
+
+ for(i = 0; i < size; i++) {
+ kfree(cluster->urb[i]->transfer_buffer);
+ usb_free_urb(cluster->urb[i]);
+ }
+
+ kfree(cluster);
+
+ spin_lock(&port->lock);
+ port->urbs_in_flight -= size;
+ spin_unlock(&port->lock);
+
+ usb_serial_port_softint(port);
+}
+
static int usb_serial_multi_urb_write(struct tty_struct *tty,
struct usb_serial_port *port, const unsigned char *buf, int count)
{
+ struct usb_serial_transfer *cluster;
+ char *buffer;
unsigned long flags;
- struct urb *urb;
- unsigned char *buffer;
- int status;
- int towrite;
- int bwrite = 0;
+ int error = -ENOMEM;
+ int num_urbs, i, write;
dbg("%s - port %d", __func__, port->number);
- if (count == 0)
- dbg("%s - write request of 0 bytes", __func__);
+ num_urbs = count / port->bulk_out_size +
+ (count % port->bulk_out_size ? 1 : 0);
- while (count > 0) {
- towrite = (count > port->bulk_out_size) ?
- port->bulk_out_size : count;
- spin_lock_irqsave(&port->lock, flags);
- if (port->urbs_in_flight >
- port->serial->type->max_in_flight_urbs) {
- spin_unlock_irqrestore(&port->lock, flags);
- dbg("%s - write limit hit\n", __func__);
- return bwrite;
- }
- port->tx_bytes_flight += towrite;
- port->urbs_in_flight++;
- spin_unlock_irqrestore(&port->lock, flags);
+ spin_lock_irqsave(&port->lock, flags);
+ if (num_urbs + port->urbs_in_flight > port->serial->type->max_in_flight_urbs)
+ num_urbs = port->serial->type->max_in_flight_urbs - port->urbs_in_flight;
- buffer = kmalloc(towrite, GFP_ATOMIC);
- if (!buffer) {
- dev_err(&port->dev,
- "%s ran out of kernel memory for urb ...\n", __func__);
- goto error_no_buffer;
- }
+ port->urbs_in_flight += num_urbs;
+ spin_unlock_irqrestore(&port->lock, flags);
- urb = usb_alloc_urb(0, GFP_ATOMIC);
- if (!urb) {
- dev_err(&port->dev, "%s - no more free urbs\n",
- __func__);
- goto error_no_urb;
- }
+ cluster = kzalloc(sizeof(struct usb_serial_transfer) + num_urbs * sizeof(struct urb *),
+ GFP_ATOMIC);
+ if (!cluster)
+ goto out;
+ cluster->size = num_urbs;
+ cluster->port = port;
+
+ for(i = 0; i < num_urbs; i++) {
+ cluster->urb[i] = usb_alloc_urb(0, GFP_ATOMIC);
+ if (!cluster->urb[i])
+ goto out_free;
+ }
- /* Copy data */
- memcpy(buffer, buf + bwrite, towrite);
- usb_serial_debug_data(debug, &port->dev, __func__,
- towrite, buffer);
- /* fill the buffer and send it */
- usb_fill_bulk_urb(urb, port->serial->dev,
+ for(i = 0; i < num_urbs; i++) {
+ buffer = kmalloc(port->bulk_out_size, GFP_ATOMIC);
+ if (!buffer)
+ goto out_buffers;
+ write = i == num_urbs - 1 ? port->bulk_out_size :
+ (count % port->bulk_out_size ?
+ count % port->bulk_out_size :
+ port->bulk_out_size);
+ usb_fill_bulk_urb(cluster->urb[i],
+ port->serial->dev,
usb_sndbulkpipe(port->serial->dev,
- port->bulk_out_endpointAddress),
- buffer, towrite,
- usb_serial_generic_write_bulk_callback, port);
+ port->bulk_out_endpointAddress),
+ buffer, write,
+ usb_serial_generic_write_mult_callback, cluster);
+ if (i != num_urbs - 1)
+ cluster->urb[i]->transfer_flags |= URB_NO_INTERRUPT;
+ memcpy(buffer, buf + i * port->bulk_out_size, write);
+ }
- status = usb_submit_urb(urb, GFP_ATOMIC);
- if (status) {
- dev_err(&port->dev,
- "%s - failed submitting write urb, error %d\n",
- __func__, status);
- goto error;
- }
+ for(i = 0; i < num_urbs; i++) {
+ error = usb_submit_urb(cluster->urb[i], GFP_ATOMIC);
+ if (error < 0)
+ goto out_kill;
+ }
- /* This urb is the responsibility of the host driver now */
- usb_free_urb(urb);
- dbg("%s write: %d", __func__, towrite);
- count -= towrite;
- bwrite += towrite;
+ return count;
+
+out_kill:
+ for (--i; i >= 0; i--) {
+ usb_unlink_urb(cluster->urb[i]);
+ usb_free_urb(cluster->urb[i]);
}
- return bwrite;
-error:
- usb_free_urb(urb);
-error_no_urb:
- kfree(buffer);
-error_no_buffer:
+out_buffers:
+ for(i = 0; i < num_urbs; i++)
+ kfree(cluster->urb[i]->transfer_buffer);
+out_free:
+ for(i = 0; i < num_urbs; i++)
+ usb_free_urb(cluster->urb[i]);
+ kfree(cluster);
+out:
spin_lock_irqsave(&port->lock, flags);
- port->urbs_in_flight--;
- port->tx_bytes_flight -= towrite;
+ port->urbs_in_flight -= num_urbs;
spin_unlock_irqrestore(&port->lock, flags);
- return bwrite;
+ return error;
}
int usb_serial_generic_write(struct tty_struct *tty,
@@ -473,23 +504,13 @@ EXPORT_SYMBOL_GPL(usb_serial_generic_read_bulk_callback);
void usb_serial_generic_write_bulk_callback(struct urb *urb)
{
- unsigned long flags;
struct usb_serial_port *port = urb->context;
int status = urb->status;
dbg("%s - port %d", __func__, port->number);
- if (port->serial->type->max_in_flight_urbs) {
- spin_lock_irqsave(&port->lock, flags);
- --port->urbs_in_flight;
- port->tx_bytes_flight -= urb->transfer_buffer_length;
- if (port->urbs_in_flight < 0)
- port->urbs_in_flight = 0;
- spin_unlock_irqrestore(&port->lock, flags);
- } else {
- /* Handle the case for single urb mode */
- port->write_urb_busy = 0;
- }
+
+ port->write_urb_busy = 0;
if (status) {
dbg("%s - nonzero write bulk status received: %d",
diff --git a/include/linux/usb/serial.h b/include/linux/usb/serial.h
index 0ec50ba..bcfe73f 100644
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@ -270,6 +270,12 @@ struct usb_serial_driver {
#define to_usb_serial_driver(d) \
container_of(d, struct usb_serial_driver, driver)
+struct usb_serial_transfer {
+ int size;
+ struct usb_serial_port *port;
+ struct urb *urb[0];
+};
+
extern int usb_serial_register(struct usb_serial_driver *driver);
extern void usb_serial_deregister(struct usb_serial_driver *driver);
extern void usb_serial_port_softint(struct usb_serial_port *port);
--
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
