Re: [linux-usb-devel] [PATCH] visor: Fix Oops on disconnect

[nardelli <jnardelli@infosciences.com>]

I've made all of the changes that recommended below.  If it looks like
I've missed anything, please indicate so.



--- linux-2.6.6.old/drivers/usb/serial/visor.c	2004-05-09 22:32:27.000000000 -0400
+++ linux-2.6.6.new/drivers/usb/serial/visor.c	2004-05-21 15:02:30.938875280 -0400
@@ -398,7 +398,8 @@ static int visor_open (struct usb_serial
 	
 	dbg("%s - port %d", __FUNCTION__, port->number);
 
-	if (!port->read_urb) {
+	if (serial->dev->descriptor.idVendor == SONY_VENDOR_ID &&
+			!port->read_urb) {
 		/* this is needed for some brain dead Sony devices */
 		dev_err(&port->dev, "Device lied about number of ports, please use a lower one.\n");
 		return -ENODEV;
@@ -416,17 +417,20 @@ static int visor_open (struct usb_serial
 		port->tty->low_latency = 1;
 
 	/* Start reading from the device */
-	usb_fill_bulk_urb (port->read_urb, serial->dev,
-			   usb_rcvbulkpipe (serial->dev, 
-					    port->bulk_in_endpointAddress),
-			   port->read_urb->transfer_buffer,
-			   port->read_urb->transfer_buffer_length,
-			   visor_read_bulk_callback, port);
-	result = usb_submit_urb(port->read_urb, GFP_KERNEL);
-	if (result) {
-		dev_err(&port->dev, "%s - failed submitting read urb, error %d\n",
-			__FUNCTION__, result);
-		goto exit;
+	if (port->read_urb) {
+		usb_fill_bulk_urb (port->read_urb, serial->dev,
+			usb_rcvbulkpipe (serial->dev, 
+				port->bulk_in_endpointAddress),
+			port->read_urb->transfer_buffer,
+			port->read_urb->transfer_buffer_length,
+			visor_read_bulk_callback, port);
+		result = usb_submit_urb(port->read_urb, GFP_KERNEL);
+		if (result) {
+			dev_err(&port->dev,
+				"%s - failed submitting read urb, error %d\n",
+				__FUNCTION__, result);
+			goto exit;
+		}
 	}
 	
 	if (port->interrupt_in_urb) {
@@ -456,7 +460,8 @@ static void visor_close (struct usb_seri
 		return;
 	
 	/* shutdown our urbs */
-	usb_unlink_urb (port->read_urb);
+	if (port->read_urb)
+		usb_unlink_urb (port->read_urb);
 	if (port->interrupt_in_urb)
 		usb_unlink_urb (port->interrupt_in_urb);
 
@@ -622,15 +627,20 @@ static void visor_read_bulk_callback (st
 	bytes_in += urb->actual_length;
 
 	/* Continue trying to always read  */
-	usb_fill_bulk_urb (port->read_urb, serial->dev,
-			   usb_rcvbulkpipe (serial->dev,
-					    port->bulk_in_endpointAddress),
-			   port->read_urb->transfer_buffer,
-			   port->read_urb->transfer_buffer_length,
-			   visor_read_bulk_callback, port);
-	result = usb_submit_urb(port->read_urb, GFP_ATOMIC);
-	if (result)
-		dev_err(&port->dev, "%s - failed resubmitting read urb, error %d\n", __FUNCTION__, result);
+	if (port->read_urb) {
+		usb_fill_bulk_urb (port->read_urb, serial->dev,
+				usb_rcvbulkpipe (serial->dev,
+				port->bulk_in_endpointAddress),
+				port->read_urb->transfer_buffer,
+				port->read_urb->transfer_buffer_length,
+				visor_read_bulk_callback, port);
+		result = usb_submit_urb(port->read_urb, GFP_ATOMIC);
+		if (result)
+			dev_err(&port->dev,
+				"%s - failed resubmitting read urb, error %d\n",
+				__FUNCTION__, result);
+	}
+  
 	return;
 }
 
@@ -675,7 +685,9 @@ exit:
 static void visor_throttle (struct usb_serial_port *port)
 {
 	dbg("%s - port %d", __FUNCTION__, port->number);
-	usb_unlink_urb (port->read_urb);
+	if (port->read_urb) {
+		usb_unlink_urb (port->read_urb);
+	}
 }
 
 
@@ -685,10 +697,14 @@ static void visor_unthrottle (struct usb
 
 	dbg("%s - port %d", __FUNCTION__, port->number);
 
-	port->read_urb->dev = port->serial->dev;
-	result = usb_submit_urb(port->read_urb, GFP_ATOMIC);
-	if (result)
-		dev_err(&port->dev, "%s - failed submitting read urb, error %d\n", __FUNCTION__, result);
+	if (port->read_urb) {
+		port->read_urb->dev = port->serial->dev;
+		result = usb_submit_urb(port->read_urb, GFP_ATOMIC);
+		if (result)
+			dev_err(&port->dev,
+				"%s - failed submitting read urb, error %d\n",
+				__FUNCTION__, result);
+	}
 }
 
 static int palm_os_3_probe (struct usb_serial *serial, const struct usb_device_id *id)
@@ -721,41 +737,55 @@ static int palm_os_3_probe (struct usb_s
 			__FUNCTION__, retval);
 		goto exit;
 	}
-		
-	connection_info = (struct visor_connection_info *)transfer_buffer;
-
-	le16_to_cpus(&connection_info->num_ports);
-	num_ports = connection_info->num_ports;
-	/* handle devices that report invalid stuff here */
-	if (num_ports > 2)
-		num_ports = 2;
-	dev_info(dev, "%s: Number of ports: %d\n", serial->type->name,
-		connection_info->num_ports);
-
-	for (i = 0; i < num_ports; ++i) {
-		switch (connection_info->connections[i].port_function_id) {
-			case VISOR_FUNCTION_GENERIC:
-				string = "Generic";
-				break;
-			case VISOR_FUNCTION_DEBUGGER:
-				string = "Debugger";
-				break;
-			case VISOR_FUNCTION_HOTSYNC:
-				string = "HotSync";
-				break;
-			case VISOR_FUNCTION_CONSOLE:
-				string = "Console";
-				break;
-			case VISOR_FUNCTION_REMOTE_FILE_SYS:
-				string = "Remote File System";
-				break;
-			default:
-				string = "unknown";
-				break;	
+	else if (retval != sizeof(*connection_info)) {
+		/* real invalid connection info handling is below */
+		num_ports = 0;
+	}
+	else {
+	        connection_info = (struct visor_connection_info *)
+			transfer_buffer;
+
+		le16_to_cpus(&connection_info->num_ports);
+		num_ports = connection_info->num_ports;
+
+		for (i = 0; i < num_ports; ++i) {
+			switch (connection_info->connections[i].
+					port_function_id) {
+				case VISOR_FUNCTION_GENERIC:
+					string = "Generic";
+					break;
+				case VISOR_FUNCTION_DEBUGGER:
+					string = "Debugger";
+					break;
+				case VISOR_FUNCTION_HOTSYNC:
+					string = "HotSync";
+					break;
+				case VISOR_FUNCTION_CONSOLE:
+					string = "Console";
+					break;
+				case VISOR_FUNCTION_REMOTE_FILE_SYS:
+					string = "Remote File System";
+					break;
+				default:
+					string = "unknown";
+					break;
+			}
+			dev_info(dev, "%s: port %d, is for %s use\n",
+				serial->type->name, connection_info->
+				connections[i].port, string);
 		}
-		dev_info(dev, "%s: port %d, is for %s use\n", serial->type->name,
-			 connection_info->connections[i].port, string);
 	}
+	/*
+	* Handle devices that report invalid stuff here.
+	*/
+	if (num_ports == 0 || num_ports > 2) {
+		dev_warn (dev, "%s: No valid connect info available\n",
+			serial->type->name);
+		num_ports = 2;
+	}
+  
+	dev_info(dev, "%s: Number of ports: %d\n", serial->type->name,
+		num_ports);
 
 	/*
 	 * save off our num_ports info so that we can use it in the
@@ -887,8 +917,7 @@ static int clie_3_5_startup (struct usb_
  
 static int treo_attach (struct usb_serial *serial)
 {
-	struct usb_serial_port *port;
-	int i;
+	struct usb_serial_port swap_port;
 
 	/* Only do this endpoint hack for the Handspring devices with
 	 * interrupt in endpoints, which for now are the Treo devices. */
@@ -898,31 +927,40 @@ static int treo_attach (struct usb_seria
 
 	dbg("%s", __FUNCTION__);
 
-	/* Ok, this is pretty ugly, but these devices want to use the
-	 * interrupt endpoint as paired up with a bulk endpoint for a
-	 * "virtual serial port".  So let's force the endpoints to be
-	 * where we want them to be. */
-	for (i = serial->num_bulk_in; i < serial->num_ports; ++i) {
-		port = serial->port[i];
-		port->read_urb = serial->port[0]->read_urb;
-		port->bulk_in_endpointAddress = serial->port[0]->bulk_in_endpointAddress;
-		port->bulk_in_buffer = serial->port[0]->bulk_in_buffer;
-	}
-
-	for (i = serial->num_bulk_out; i < serial->num_ports; ++i) {
-		port = serial->port[i];
-		port->write_urb = serial->port[0]->write_urb;
-		port->bulk_out_size = serial->port[0]->bulk_out_size;
-		port->bulk_out_endpointAddress = serial->port[0]->bulk_out_endpointAddress;
-		port->bulk_out_buffer = serial->port[0]->bulk_out_buffer;
-	}
-
-	for (i = serial->num_interrupt_in; i < serial->num_ports; ++i) {
-		port = serial->port[i];
-		port->interrupt_in_urb = serial->port[0]->interrupt_in_urb;
-		port->interrupt_in_endpointAddress = serial->port[0]->interrupt_in_endpointAddress;
-		port->interrupt_in_buffer = serial->port[0]->interrupt_in_buffer;
-	}
+	/*
+	* It appears that Treos want to use the 1st interrupt endpoint to
+	* communicate with the 2nd bulk out endpoint, so let's swap the 1st
+	* and 2nd bulk in and interrupt endpoints.  Note that swapping the
+	* bulk out endpoints would break lots of apps that want to communicate
+	* on the second port.
+	*/
+	swap_port.read_urb = serial->port[0]->read_urb;
+	swap_port.bulk_in_endpointAddress = serial->port[0]->
+		bulk_in_endpointAddress;
+	swap_port.bulk_in_buffer = serial->port[0]->bulk_in_buffer;
+	swap_port.interrupt_in_urb = serial->port[0]->interrupt_in_urb;
+	swap_port.interrupt_in_endpointAddress = serial->port[0]->
+		interrupt_in_endpointAddress;
+	swap_port.interrupt_in_buffer = serial->port[0]->interrupt_in_buffer;
+ 
+	serial->port[0]->read_urb = serial->port[1]->read_urb;
+	serial->port[0]->bulk_in_endpointAddress = serial->port[1]->
+		bulk_in_endpointAddress;
+	serial->port[0]->bulk_in_buffer = serial->port[1]->bulk_in_buffer;
+	serial->port[0]->interrupt_in_urb = serial->port[1]->interrupt_in_urb;
+	serial->port[0]->interrupt_in_endpointAddress = serial->port[1]->
+		interrupt_in_endpointAddress;
+	serial->port[0]->interrupt_in_buffer = serial->port[1]->
+		interrupt_in_buffer;
+
+	serial->port[1]->read_urb = swap_port.read_urb;
+	serial->port[1]->bulk_in_endpointAddress = swap_port.
+		bulk_in_endpointAddress;
+	serial->port[1]->bulk_in_buffer = swap_port.bulk_in_buffer;
+	serial->port[1]->interrupt_in_urb = swap_port.interrupt_in_urb;
+	serial->port[1]->interrupt_in_endpointAddress = swap_port.
+		interrupt_in_endpointAddress;
+	serial->port[1]->interrupt_in_buffer = swap_port.interrupt_in_buffer;
 
 	return 0;
 }




Greg KH wrote:
> On Thu, May 20, 2004 at 07:08:56PM -0400, nardelli wrote:
> 
>>Here is a proposed patch for Oops on disconnect in the visor module.
>>For details of the problem, please see
>>http://bugzilla.kernel.org/show_bug.cgi?id=2289
>>
>>I would really appreciate it if anyone that uses this module could please
>>try this patch to make sure that it works as intended.  Also, as this is
>>the first patch that I've submitted, please feel free to be brutally
>>honest regarding content, formatting, etc.
> 
> 
> Ok, I'll be brutal, but remember, you asked :)
> 
> First off, read Documentation/SubmittingPatches and
> Documentation/CodingStyle to see the proper way to make patches, and how
> to format the code.
> 
> 
>>--- old/linux-2.6.6/drivers/usb/serial/visor.c	2004-05-09 22:32:27.000000000 -0400
>>+++ new/linux-2.6.6/drivers/usb/serial/visor.c	2004-05-20 18:04:24.000000000 -0400
> 
> 
> I need to be able to apply this patch by using a -p1 in the kernel
> directory.  So the patch should be one directory up and look like:
> 
> --- linux-2.6.6/drivers/usb/serial/visor.c	2004-05-09 22:32:27.000000000 -0400
> +++ linux-2.6.6/drivers/usb/serial/visor.c	2004-05-20 18:04:24.000000000 -0400
> 
> 
> 
>>@@ -12,6 +12,13 @@
>>  *
>>  * See Documentation/usb/usb-serial.txt for more information on using this driver
>>  *
>>+ * (5/20/2004) Joe Nardelli
>>+ *	Reduced possibility for unitialized data access in palm_os_3_probe.
>>+ *	Modified workaround for treo endpoint setup in treo_attach.
>>+ *	Removed assumptions that port->read_urb was always valid (is not true
>>+ *	for usb serial devices with more bulk out or interrupt endpoints than
>>+ *	bulk in endpoints).
> 
> 
> I'm trying not to add new stuff to the changelog at the beginning of the
> file, but this is not really a big deal.
> 
> 
>>-	if (!port->read_urb) {
>>+	if ((serial->dev->descriptor.idVendor != SONY_VENDOR_ID && !port->read_urb))
>>+	{
> 
> 
> Wrong formatting style for the '{'
> 
> Your patch says that we might not have a read_urb for the given port?
> How could that be true?  The check here in open() will catch any devices
> that this might not be correct for.  So that portion of this patch is
> not needed, right?
> 
> 
>> static int palm_os_3_probe (struct usb_serial *serial, const struct usb_device_id *id)
>>@@ -710,6 +728,13 @@
>> 		return -ENOMEM;
>> 	}
>> 
>>+	/*
>>+	* We don't know how much data gets written into transfer_buffer, so let's
>>+	* at least set it all to 0 to avoid putting random data into num_ports
>>+	* (which causes unitialized, and possibly unallocated data to be accessed)
>>+	*/
>>+	memset (transfer_buffer, 0, sizeof(*connection_info));
> 
> 
> Um, we ask for a set ammount of data, and we should get it.  But we
> should check the size of the data returned to make sure of this, right?
> 
> 
>> 	/* send a get connection info request */
>> 	retval = usb_control_msg (serial->dev,
>> 				  usb_rcvctrlpipe(serial->dev, 0),
>>@@ -726,11 +751,20 @@
>> 
>> 	le16_to_cpus(&connection_info->num_ports);
>> 	num_ports = connection_info->num_ports;
>>-	/* handle devices that report invalid stuff here */
>>-	if (num_ports > 2)
>>+	/*
>>+	* Handle devices that report invalid stuff here.  I think that this will
>>+	* work for both big and little endian architectures that do not report
>>+	* back valid connect info, but I'd still like to verify this on a big
>>+	* endian machine.  Any testers?
>>+	*/
> 
> 
> The call above to le16_to_cpus() will handle the endian issue here.
> This comment can be removed.
> 
> 
>>+	if (num_ports <= 0 || num_ports > 2) {
> 
> 
> I like the idea of this check, but you are trying to test for a negative
> value on a __u16 variable, which is always unsigned.  So that check will
> never be true :)
> 
> 
>>+		dev_warn (dev, "%s: No valid connect info available\n",
>>+			serial->type->name);
>> 		num_ports = 2;
>>+	}
>>+  
>> 	dev_info(dev, "%s: Number of ports: %d\n", serial->type->name,
>>-		connection_info->num_ports);
>>+		num_ports);
>> 
>> 	for (i = 0; i < num_ports; ++i) {
>> 		switch (connection_info->connections[i].port_function_id) {
>>@@ -887,8 +921,7 @@
>>  
>> static int treo_attach (struct usb_serial *serial)
>> {
>>-	struct usb_serial_port *port;
>>-	int i;
>>+	struct usb_serial_port swap_port;
>> 
>> 	/* Only do this endpoint hack for the Handspring devices with
>> 	 * interrupt in endpoints, which for now are the Treo devices. */
>>@@ -898,31 +931,32 @@
>> 
>> 	dbg("%s", __FUNCTION__);
>> 
>>-	/* Ok, this is pretty ugly, but these devices want to use the
>>-	 * interrupt endpoint as paired up with a bulk endpoint for a
>>-	 * "virtual serial port".  So let's force the endpoints to be
>>-	 * where we want them to be. */
>>-	for (i = serial->num_bulk_in; i < serial->num_ports; ++i) {
>>-		port = serial->port[i];
>>-		port->read_urb = serial->port[0]->read_urb;
>>-		port->bulk_in_endpointAddress = serial->port[0]->bulk_in_endpointAddress;
>>-		port->bulk_in_buffer = serial->port[0]->bulk_in_buffer;
>>-	}
>>-
>>-	for (i = serial->num_bulk_out; i < serial->num_ports; ++i) {
>>-		port = serial->port[i];
>>-		port->write_urb = serial->port[0]->write_urb;
>>-		port->bulk_out_size = serial->port[0]->bulk_out_size;
>>-		port->bulk_out_endpointAddress = serial->port[0]->bulk_out_endpointAddress;
>>-		port->bulk_out_buffer = serial->port[0]->bulk_out_buffer;
>>-	}
>>-
>>-	for (i = serial->num_interrupt_in; i < serial->num_ports; ++i) {
>>-		port = serial->port[i];
>>-		port->interrupt_in_urb = serial->port[0]->interrupt_in_urb;
>>-		port->interrupt_in_endpointAddress = serial->port[0]->interrupt_in_endpointAddress;
>>-		port->interrupt_in_buffer = serial->port[0]->interrupt_in_buffer;
>>-	}
>>+	/*
>>+	* It appears that Treos want to use the 1st interrupt endpoint to communicate
>>+	* with the 2nd bulk out endpoint, so let's swap the 1st and 2nd bulk in
>>+	* and interrupt endpoints.  Note that swapping the bulk out endpoints would
>>+	* break lots of apps that want to communicate on the second port.
>>+	*/
>>+	swap_port.read_urb = serial->port[0]->read_urb;
>>+	swap_port.bulk_in_endpointAddress = serial->port[0]->bulk_in_endpointAddress;
>>+	swap_port.bulk_in_buffer = serial->port[0]->bulk_in_buffer;
>>+	swap_port.interrupt_in_urb = serial->port[0]->interrupt_in_urb;
>>+	swap_port.interrupt_in_endpointAddress = serial->port[0]->interrupt_in_endpointAddress;
>>+	swap_port.interrupt_in_buffer = serial->port[0]->interrupt_in_buffer;
>>+ 
>>+	serial->port[0]->read_urb = serial->port[1]->read_urb;
>>+	serial->port[0]->bulk_in_endpointAddress = serial->port[1]->bulk_in_endpointAddress;
>>+	serial->port[0]->bulk_in_buffer = serial->port[1]->bulk_in_buffer;
>>+	serial->port[0]->interrupt_in_urb = serial->port[1]->interrupt_in_urb;
>>+	serial->port[0]->interrupt_in_endpointAddress = serial->port[1]->interrupt_in_endpointAddress;
>>+	serial->port[0]->interrupt_in_buffer = serial->port[1]->interrupt_in_buffer;
>>+
>>+	serial->port[1]->read_urb = swap_port.read_urb;
>>+	serial->port[1]->bulk_in_endpointAddress = swap_port.bulk_in_endpointAddress;
>>+	serial->port[1]->bulk_in_buffer = swap_port.bulk_in_buffer;
>>+	serial->port[1]->interrupt_in_urb = swap_port.interrupt_in_urb;
>>+	serial->port[1]->interrupt_in_endpointAddress = swap_port.interrupt_in_endpointAddress;
>>+	serial->port[1]->interrupt_in_buffer = swap_port.interrupt_in_buffer;
> 
> 
> Now this is the meat of the patch.  Is that all that is needed, just
> swaping the info?  That makes more sense than the hack of assigning the
> same port data to both ports.  Does this work properly on disconnect
> too?
> 
> Thanks a lot for doing this, with some cleanups I'll be glad to accept
> this patch.
> 
> thanks,
> 
> greg k-h
> 
> 
> -------------------------------------------------------
> This SF.Net email is sponsored by: Oracle 10g
> Get certified on the hottest thing ever to hit the market... Oracle 10g. 
> Take an Oracle 10g class now, and we'll give you the exam FREE.
> http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
> _______________________________________________
> linux-usb-devel@lists.sourceforge.net
> To unsubscribe, use the last form field at:
> https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
> 


-- 
Joe Nardelli
jnardelli@infosciences.com