visor: Fix Oops on disconnect

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.

linux-2.6.6/drivers/usb/serial/visor.c +0 -0

 	/* send a get connection info request */
 	retval = usb_control_msg (serial->dev,
 				  usb_rcvctrlpipe(serial->dev, 0),

@@ -726,11 +751,20 @@
 

linux-2.6.6/drivers/usb/serial/visor.c +22 -28

+		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;

linux-2.6.6/drivers/usb/serial/visor.c line 409

Quoted 3 lines

linux-2.6.6/drivers/usb/serial/visor.c +2 -1

-	if (!port->read_urb) {
+	if ((serial->dev->descriptor.idVendor != SONY_VENDOR_ID && !port->read_urb))
+	{

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?

Quoted 9 lines · On Thu, 20 May 2004 21:30:32 -0700 Greg KH <[email protected]> wrote:

linux-2.6.6/drivers/usb/serial/visor.c line 409

Quoted 3 lines

linux-2.6.6/drivers/usb/serial/visor.c +2 -1

-	if (!port->read_urb) {
+	if ((serial->dev->descriptor.idVendor != SONY_VENDOR_ID && !port->read_urb))
+	{

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?

I know nothing about Palms, but also that part contradicted a comment.

linux-2.6.6/drivers/usb/serial/visor.c +2 -1

-	if (!port->read_urb) {
+	if ((serial->dev->descriptor.idVendor != SONY_VENDOR_ID && !port->read_urb))
+	{
 		/* this is needed for some brain dead Sony devices */

So.... the patch makes the body of the if to be used when it's NOT Sony, but the comment says that it's intended for Sony. I think it's fishy.

-- Pete

Quoted 8 lines · nardelli

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.

linux-2.6.6/drivers/usb/serial/visor.c line 20

Quoted 11 lines

linux-2.6.6/drivers/usb/serial/visor.c +6 -0

@@ -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.

linux-2.6.6/drivers/usb/serial/visor.c line 409

Quoted 3 lines

linux-2.6.6/drivers/usb/serial/visor.c +2 -1

-	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?

Quoted 5 lines

linux-2.6.6/drivers/usb/serial/visor.c +0 -0

 static int palm_os_3_probe (struct usb_serial *serial, const struct usb_device_id *id)

@@ -710,6 +728,13 @@
 		return -ENOMEM;
 	}
 

linux-2.6.6/drivers/usb/serial/visor.c line 736

Quoted 6 lines

linux-2.6.6/drivers/usb/serial/visor.c +6 -0

+	/*
+	* 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?

Quoted 5 lines

linux-2.6.6/drivers/usb/serial/visor.c +0 -0

 	/* send a get connection info request */
 	retval = usb_control_msg (serial->dev,
 				  usb_rcvctrlpipe(serial->dev, 0),

@@ -726,11 +751,20 @@
 

linux-2.6.6/drivers/usb/serial/visor.c line 759

Quoted 10 lines

linux-2.6.6/drivers/usb/serial/visor.c +6 -2

 	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.

linux-2.6.6/drivers/usb/serial/visor.c line 760

Quoted 1 line

linux-2.6.6/drivers/usb/serial/visor.c +1 -0

+	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 :)

Quoted 74 lines

linux-2.6.6/drivers/usb/serial/visor.c +30 -28

+		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;

linux-2.6.6/drivers/usb/serial/visor.c line 959

Quoted 2 lines

linux-2.6.6/drivers/usb/serial/visor.c +2 -0

+	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
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

The api for usb_control_msg says, 'If successful, it returns 0, othwise a negative error number', and I didn't see any other way to figure out how much data was being returned.

Quoted 3 lines · nardelli

The api for usb_control_msg says, 'If successful, it returns 0, othwise a negative error number', and I didn't see any other way to figure out how much data was being returned.

In the current kernel sources, the kerneldoc for usb_control_msg() says
"If successful, it returns the number of bytes transferred, otherwise a negative error number."

The old check would error out of visor_open() with -ENODEV if there was not a read_urb for any device, and there was a comment that this was needed for 'some brain dead Sony devices'. I modified this to error out only for Sony devices, instead of just a comment about them. This should not modify the behavior on Sonys, but may on others (namely treos).

I'd really like to know more about why some Sony devices do not have a read_urb, but at least for now, I did not change functionality for them.

Quoted 8 lines · nardelli

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.

linux-2.6.6/drivers/usb/serial/visor.c

Quoted 2 lines

linux-2.6.6/drivers/usb/serial/visor.c +0 -0

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:

drivers/usb/serial/visor.c +0 -0

linux-2.6.6/drivers/usb/serial/visor.c line 20

Quoted 13 lines

linux-2.6.6/drivers/usb/serial/visor.c +6 -0

@@ -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.

linux-2.6.6/drivers/usb/serial/visor.c line 22

Quoted 3 lines

linux-2.6.6/drivers/usb/serial/visor.c +2 -1

-	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?

linux-2.6.6/drivers/usb/serial/visor.c line 736

Quoted 11 lines

linux-2.6.6/drivers/usb/serial/visor.c +6 -0

 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?

Quoted 5 lines

linux-2.6.6/drivers/usb/serial/visor.c +0 -0

 	/* send a get connection info request */
 	retval = usb_control_msg (serial->dev,
 				  usb_rcvctrlpipe(serial->dev, 0),

@@ -726,11 +751,20 @@
 

linux-2.6.6/drivers/usb/serial/visor.c line 759

Quoted 10 lines

linux-2.6.6/drivers/usb/serial/visor.c +6 -2

 	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.

linux-2.6.6/drivers/usb/serial/visor.c line 760

Quoted 1 line

linux-2.6.6/drivers/usb/serial/visor.c +1 -0

+	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 :)

Quoted 66 lines

linux-2.6.6/drivers/usb/serial/visor.c +22 -28

+		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;

linux-2.6.6/drivers/usb/serial/visor.c line 959

Quoted 10 lines

linux-2.6.6/drivers/usb/serial/visor.c +10 -0

+	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
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

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

Quoted 7 lines

Review

Quoted 2 lines

(quoted diff) +1 -0

+	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 :)

Quoted 4 lines

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

[snip]

Quoted 6 lines

Quoted 6 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	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 :)

What happens if num_ports == 0? Not that hardware should ever report that.

[snip]

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

(quoted diff) +14 -9

 	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;

}

(quoted diff) +11 -5

@@ -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)

(quoted diff) +0 -25

@@ -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;

On Fri, May 21, 2004 at 03:51:23PM -0400, nardelli wrote:

Patch is line-wrapped, so I can't apply it :(

Review static void visor_close (struct usb_seri · line 464

Quoted 7 lines

(quoted diff) +2 -1

@@ -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);

I really do not think these extra checks for read_urb all of the place need to be added. We take care of it in the open() call, right?

Review static void visor_close (struct usb_seri

Quoted 4 lines

(quoted diff) +4 -0

+	else if (retval != sizeof(*connection_info)) {
+		/* real invalid connection info handling is below */
+		num_ports = 0;
+	}

Change this to a "if" instead of a "else if".
Actually just set num_ports to 0 at the beginning of the function, and then just check for a valud retval and do the code below...

Review

Quoted 3 lines

(quoted diff) +3 -0

+	else {
+	        connection_info = (struct visor_connection_info *)
+			transfer_buffer;

greg k-h

Quoted 12 lines · Greg KH

Review static void visor_close (struct usb_seri · line 464

Quoted 7 lines

(quoted diff) +2 -1

@@ -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);

I really do not think these extra checks for read_urb all of the place need to be added. We take care of it in the open() call, right?

Yes - less clutter and more efficient too.

Maybe I spoke too soon here. We have 1 bulk in, 2 bulk out, and 1 interrupt in endpoint, which by the logic in usb-serial, translates to 2 ports. Only one of those ports can have a read_urb associated with it, unless we want to do some really fancy juggling. This means that we're going to have a port that does not have a valid read_urb associated with it, even after open().

I'm at a loss why this device has an uneven number of bulk in and bulk out endpoints.

Quoted 5 lines · nardelli

Maybe I spoke too soon here. We have 1 bulk in, 2 bulk out, and 1 interrupt in endpoint, which by the logic in usb-serial, translates to 2 ports. Only one of those ports can have a read_urb associated with it, unless we want to do some really fancy juggling. This means that we're going to have a port that does not have a valid read_urb associated with it, even after open().

But the call to open() fails, which prevents any of the other tty calls from happening on that port. That's why we don't need to make that check anywhere else.

Quoted 2 lines

I'm at a loss why this device has an uneven number of bulk in and bulk out endpoints.

Stupid hardware engineers? Who really knows...

One more question - my system does not really like it when I redirect /dev/urandom to either the first or second port. I know that it obviosuly makes no sense to do such a thing, but is it expected that there should be no problems associated with this. I'm not finished testing, so I'm not sure how severe a problem develops. I'll report in when I know more about this.

Quoted 11 lines · nardelli

One more question - my system does not really like it when I redirect /dev/urandom
to either the first or second port. I know that it obviosuly makes no sense to
do such a thing, but is it expected that there should be no problems associated
with this. I'm not finished testing, so I'm not sure how severe a problem develops.
I'll report in when I know more about this.

Here's some preliminary info:

1) Whether writing to the 1st or 2nd port, the machine hangs pretty badly after catting /dev/urandom for more than 1 second or two. This continues even after catting has stopped, and the device has been disconnected. This smells like some type of resource leak, probably memory, to me.

2) I've included an Oops when writing to the 1st serial port, showing some difficulty allocating memory.

3) After looking at visor_write(), I was a bit surprised to see it allocating its own urb and buffer, while I thought it would be using the ones that were allocated in usb-serial.usb_serial_probe(). After looking at other serial devices that use usb_submit_urb() in their write() functions, I found that the following used the buffers/urb allocated for them:

cyberjack, digi_acceleport, generic, io_ti, ipaq, ir-usb, keyspan_pda, kobil_sct, mct_u232, omninet, pl2303, safe_serial

while I found that the following created their own (some for each write):

empeg, ftdi_sio, keyspan, kl5kusb105, visor, whiteheat

I'm not so sure about:
io_edgeport

Is this expected behavior, or am I just missing something here?

It would seem like reusing the buffer and urb would be advantageous, but there may be more issues here than I am aware of.

But this is a different problem from the one you originally set out to fix, how about sending a new patch for the treo disconnect problem, and then we can work on the next issues.

Quoted 2 lines · nardelli

nardelli wrote:

Quoted 4 lines

1) Whether writing to the 1st or 2nd port, the machine hangs pretty badly after catting /dev/urandom for more than 1 second or two. This continues even after catting has stopped, and the device has been disconnected. This smells like some type of resource leak, probably memory, to me.

Which machine dies? The pilot or the Linux box?

Quoted 12 lines · Greg KH

Quoted 8 lines · nardelli

Maybe I spoke too soon here. We have 1 bulk in, 2 bulk out, and 1 interrupt
in endpoint, which by the logic in usb-serial, translates to 2 ports. Only
one of those ports can have a read_urb associated with it, unless we want to
do some really fancy juggling. This means that we're going to have a port that does not have a valid read_urb associated with it, even after open().

Quoted 5 lines

But the call to open() fails, which prevents any of the other tty calls from happening on that port. That's why we don't need to make that check anywhere else.

The call to open() does not fail

- with only a write_urb associated with it,
it's not a very interesting port, but data can be written to it, at least in small quantities (see below).

Today, that call does fail:

        if (!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;
        }

Let's not change that logic please.

Quoted 4 lines · Greg KH

Today, that call does fail:

Quoted 9 lines

       if (!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;
       }

Let's not change that logic please.

OK.

Here's another patch. The differences between this one and the last are:

1) Removal of extra checks to verify that port->read_urb is valid
2) Modified num_ports initialization and return value checking in palm_os_3_probe

I made this patch against 2.6.6. Would you prefer it against 2.6.7-rc1?

On Mon, May 24, 2004 at 05:42:54PM -0400, nardelli wrote:

Nah, this is good enough. I've tweaked the patch a bit to keep from creating a big structure on the stack, and reduced the copy port logic to something a bit more readable and applied this version. I'll send it off to Linus in a day or so.

Thanks a lot for your work on this.

greg k-h

drivers/usb/serial/visor.c +9 -0

+#define COPY_PORT(dest, src)						\
+	dest->read_urb = src->read_urb;					\
+	dest->bulk_in_endpointAddress = src->bulk_in_endpointAddress;	\
+	dest->bulk_in_buffer = src->bulk_in_buffer;			\
+	dest->interrupt_in_urb = src->interrupt_in_urb;			\
+	dest->interrupt_in_endpointAddress = src->interrupt_in_endpointAddress;	\
+	dest->interrupt_in_buffer = src->interrupt_in_buffer;
+
+	swap_port = kmalloc(sizeof(*swap_port), GFP_KERNEL);

Quoted 6 lines · Greg KH

On Fri, May 21, 2004 at 03:51:23PM -0400, nardelli wrote:

Patch is line-wrapped, so I can't apply it :(

Hmmm... I couldn't see the linewrap in the original I sent, or in test ones that I did. Probably my mail tool, but then it is getting late on a Friday, which probably means that it is me.

To aid in diagnosing where I'm goofing up, could you point out a spot where it is linewrapping?

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.