Re: [PATCH]dma rules violation in the option driver

Am Dienstag, 27. März 2007 19:43 schrieb Pete Zaitcev:
> On Tue, 27 Mar 2007 14:30:52 +0200, Oliver Neukum <[email protected]> wrote:
> 
> > --- a/drivers/usb/serial/option.c     2007-03-27 13:31:04.000000000 +0200
> > +++ b/drivers/usb/serial/option.c     2007-03-27 14:21:21.000000000 +0200
> > @@ -627,14 +629,23 @@
> > +     *buf = usb_buffer_alloc(serial->dev, len, GFP_KERNEL, handle);
> >               /* Fill URB using supplied data. */
> >       usb_fill_bulk_urb(urb, serial->dev,
> >                     usb_sndbulkpipe(serial->dev, endpoint) | dir,
> > -                   buf, len, callback, ctx);
> > +                   *buf, len, callback, ctx);
> > +     urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> > +     urb->transfer_dma = *handle;
> 
> Why don't you just use kmalloc here? The needless use of usb_buffer_alloc
> always irritates me. It's as if the iommu space is free and endless.

Very well, here's a version with read buffers allocated only while the device
is in use. In addition it fixes

- a small race with setting low latency
- failure to report ENOMEM if no read URB can be allocated

	Regards
		Oliver

Signed-off-by: Oliver Neukum <[email protected]>
-----

--- a/drivers/usb/serial/option.c	2007-03-28 13:45:46.000000000 +0200
+++ b/drivers/usb/serial/option.c	2007-03-28 14:21:12.000000000 +0200
@@ -226,10 +226,11 @@
 struct option_port_private {
 	/* Input endpoints and buffer for this port */
 	struct urb *in_urbs[N_IN_URB];
-	char in_buffer[N_IN_URB][IN_BUFLEN];
+	u8 *in_buffer[N_IN_URB];
+	dma_addr_t in_dma[N_IN_URB];
 	/* Output endpoints and buffer for this port */
 	struct urb *out_urbs[N_OUT_URB];
-	char out_buffer[N_OUT_URB][OUT_BUFLEN];
+	u8 *out_buffer[N_OUT_URB];
 
 	/* Settings for the port */
 	int rts_state;	/* Handshaking pins (outputs) */
@@ -538,8 +539,10 @@
 {
 	struct option_port_private *portdata;
 	struct usb_serial *serial = port->serial;
-	int i, err;
+	dma_addr_t handle;
+	int i, err, count = 0;
 	struct urb *urb;
+	u8 *buf;
 
 	portdata = usb_get_serial_port_data(port);
 
@@ -548,17 +551,26 @@
 	/* Set some sane defaults */
 	portdata->rts_state = 1;
 	portdata->dtr_state = 1;
+	port->tty->low_latency = 1;
 
 	/* Reset low level data toggle and start reading from endpoints */
 	for (i = 0; i < N_IN_URB; i++) {
-		urb = portdata->in_urbs[i];
+		urb = portdata->in_urbs[i] = usb_alloc_urb(0, GFP_KERNEL);;
 		if (! urb)
 			continue;
-		if (urb->dev != serial->dev) {
-			dbg("%s: dev %p != %p", __FUNCTION__,
-				urb->dev, serial->dev);
+		buf = usb_buffer_alloc(serial->dev, IN_BUFLEN, GFP_KERNEL, &handle);
+		if (buf == NULL) {
+			dbg("%s: allocation of a buffer for endpoint %d failed.", __FUNCTION__, port->bulk_in_endpointAddress);
+			usb_free_urb(urb);
+			portdata->in_urbs[i] = NULL;
 			continue;
 		}
+		usb_fill_bulk_urb(urb, serial->dev,
+			usb_rcvbulkpipe(serial->dev, port->bulk_in_endpointAddress),
+			buf, IN_BUFLEN, option_indat_callback , port);
+		urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
+		urb->transfer_dma = portdata->in_dma[i] = handle;
+		count++;
 
 		/*
 		 * make sure endpoint data toggle is synchronized with the
@@ -573,6 +585,8 @@
 				urb->transfer_buffer_length);
 		}
 	}
+	if (!count)
+		return -ENOMEM;
 
 	/* Reset low level data toggle on out endpoints */
 	for (i = 0; i < N_OUT_URB; i++) {
@@ -584,7 +598,7 @@
 				usb_pipeout(urb->pipe), 0); */
 	}
 
-	port->tty->low_latency = 1;
+
 
 	option_send_setup(port);
 
@@ -593,7 +607,7 @@
 
 static inline void stop_urb(struct urb *urb)
 {
-	if (urb && urb->status == -EINPROGRESS)
+	if (urb)
 		usb_kill_urb(urb);
 }
 
@@ -613,8 +627,12 @@
 		option_send_setup(port);
 
 		/* Stop reading/writing urbs */
-		for (i = 0; i < N_IN_URB; i++)
+		for (i = 0; i < N_IN_URB; i++) {
 			stop_urb(portdata->in_urbs[i]);
+			usb_buffer_free(serial->dev, IN_BUFLEN,
+				portdata->in_buffer[i], portdata->in_dma[i]);
+			usb_free_urb(portdata->in_urbs[i]);
+		}
 		for (i = 0; i < N_OUT_URB; i++)
 			stop_urb(portdata->out_urbs[i]);
 	}
@@ -623,24 +641,31 @@
 
 /* Helper functions used by option_setup_urbs */
 static struct urb *option_setup_urb(struct usb_serial *serial, int endpoint,
-		int dir, void *ctx, char *buf, int len,
+		int dir, void *ctx, u8 **buf, int len,
 		void (*callback)(struct urb *))
 {
 	struct urb *urb;
+	u8 *temp_buf;
 
 	if (endpoint == -1)
 		return NULL;		/* endpoint not needed */
 
+	temp_buf = kmalloc(len, GFP_KERNEL);
+	if (!temp_buf)
+		return NULL;
+
 	urb = usb_alloc_urb(0, GFP_KERNEL);		/* No ISO */
 	if (urb == NULL) {
 		dbg("%s: alloc for endpoint %d failed.", __FUNCTION__, endpoint);
+		kfree(temp_buf);
 		return NULL;
 	}
+	*buf = temp_buf;
 
 		/* Fill URB using supplied data. */
 	usb_fill_bulk_urb(urb, serial->dev,
 		      usb_sndbulkpipe(serial->dev, endpoint) | dir,
-		      buf, len, callback, ctx);
+		      *buf, len, callback, ctx);
 
 	return urb;
 }
@@ -658,18 +683,11 @@
 		port = serial->port[i];
 		portdata = usb_get_serial_port_data(port);
 
-	/* Do indat endpoints first */
-		for (j = 0; j < N_IN_URB; ++j) {
-			portdata->in_urbs[j] = option_setup_urb (serial,
-                  	port->bulk_in_endpointAddress, USB_DIR_IN, port,
-                  	portdata->in_buffer[j], IN_BUFLEN, option_indat_callback);
-		}
-
 		/* outdat endpoints */
 		for (j = 0; j < N_OUT_URB; ++j) {
 			portdata->out_urbs[j] = option_setup_urb (serial,
                   	port->bulk_out_endpointAddress, USB_DIR_OUT, port,
-                  	portdata->out_buffer[j], OUT_BUFLEN, option_outdat_callback);
+                  	&portdata->out_buffer[j], OUT_BUFLEN, option_outdat_callback);
 		}
 	}
 }

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel