[PATCH] visor: Fix Oops on disconnect

[PATCH] visor: Fix Oops on disconnect

[nardelli]

[-- Attachment #1: Type: text/plain, Size: 463 bytes --]

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.



-- 
Joe Nardelli
jnardelli@infosciences.com

[-- Attachment #2: patch.treo --]
[-- Type: text/plain, Size: 8607 bytes --]

--- 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
@@ -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).
+ *
  * (06/03/2003) Judd Montgomery <judd at jpilot.org>
  *     Added support for module parameter options for untested/unknown
  *     devices.
@@ -398,7 +405,8 @@
 	
 	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 +424,19 @@
 		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 +466,8 @@
 		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 +633,18 @@
 	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 +689,9 @@
 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 +701,12 @@
 
 	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)
@@ -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));
+  
 	/* 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?
+	*/
+	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,
-		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;
 
 	return 0;
 }

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

[Greg KH]

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

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

[Pete Zaitcev]

On Thu, 20 May 2004 21:30:32 -0700
Greg KH <greg@kroah.com> wrote:

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

-	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

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

[nardelli]

Pete Zaitcev wrote:
> On Thu, 20 May 2004 21:30:32 -0700
> Greg KH <greg@kroah.com> wrote:
> 
> 
>>>-	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.
> 
> -	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.

Oops - that does look a little fishy.  I'll revisit.

> 
> -- Pete
> 


-- 
Joe Nardelli
jnardelli@infosciences.com

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

[nardelli]

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

I'll reformat/regenerate patch.

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

I'll regenerate patch.

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

It did seem like the change log was a little old :-).  The comments are at
bugzilla any way.  I'll remove it.


>>-	if (!port->read_urb) {
>>+	if ((serial->dev->descriptor.idVendor != SONY_VENDOR_ID && !port->read_urb))
>>+	{
> 
> 
> Wrong formatting style for the '{'
> 

Will reformat.

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

The patch should be happy to let devices thru visor_open with no read_urb,
except for Sonys.

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.

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

The data being returned had unitialzed data in it.  Before doing a memset, I
almost always had 23130 ports, but that changed upon kernel build.  This
resulted in trying to access ALOT of bad memory.  After adding a memset,
no random data was showing up.  If this were outside of the kernel, I suspect
that would have resulted in a segmentation violation.

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.

I would definitely prefer to do a check on the amount of data being returned.
Any suggestions?

>> 	/* 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.
> 
> 

I'll drop it.

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

I just noticed that num_ports is defined as an int in palm_os_3_probe(), and
as __u16 in visor.c.  Would you like me to set the one in palm_os_3_probe()
to a __u16?

Either way, I'll change it to look for (num_ports == 0 || num_ports > 2).
With no conection data being returned for treos, it should report 0 ports.

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

That and the memset are the meat of the patch.  Pretty simple, but it took
alot of coming up to speed on usb to try and interpret the output of usbsnoop.
There was some communication on the second (as reported by the device) bulk
out endpoint, but I couldn't figure out what that was doing.  I suspect that
some treo protocol specs would help with that, but I still haven't found any.

After more than 30 backups with jpilot, about 5 listings with pilot-xfer, and
about 5 where I disconnected the device in the middle of a transfer, I have yet
to encounter a problem.  I'd appreciate it others would test this patch after
I've made the changes described above.

> 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

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

[Alan Stern]

On Fri, 21 May 2004, nardelli wrote:

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

Alan Stern

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

[nardelli]

Alan Stern wrote:
> On Fri, 21 May 2004, nardelli wrote:
> 
> 
>>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."
> 

Sorry, I was looking at dated api docs at kernelnewbies.org.  I'll use
the size returned from usb_control_msg() instead of memset().


-- 
Joe Nardelli
jnardelli@infosciences.com

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

[Greg KH]

On Fri, May 21, 2004 at 10:48:54AM -0400, nardelli wrote:
> 
> 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.

The problem is that the "bad" Sony devices return that they have 2 ports
available, however their endpoints do not reflect this.  So I check for
a read urb to test if this really is a valid port or not.

Hm, now that I can modify the number of ports on the fly, we should just
catch this in the initialization of the device which would solve this
problem the "right way".

thanks,

greg k-h

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

[nardelli]

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

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

[jkroon]

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

[snip]

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

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

[nardelli]

jkroon@cs.up.ac.za wrote:
>>I've made all of the changes that recommended below.  If it looks like
>>I've missed anything, please indicate so.
>>
>>
> 
> 
> [snip]
> 
> 
>>>>+	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]
> 
> 

Short answer:
A warning is logged and num_ports defaults to 2.

Long answer:

Unfortunately, it does not apear that this class of device sends any kind of
connect info back in repsonse to VISOR_GET_CONNECTION_INFORMATION,
PALM_GET_EXT_CONNECTION_INFORMATION, or for that matter any request under
200 (or some similiar number - I don't remember how far I tested).

Based upon a usb packet capture under windoze, I believe that the device
is not capable of this.  I'd really like some kind of documentation on
the connection protocol, but I've come up completely empty handed in that
regard.

The packet capture available at
http://bugzilla.kernel.org/attachment.cgi?id=2924&action=view shows the
attempt to send both VISOR_GET_CONNECTION_INFORMATION (3) and 
PALM_GET_EXT_CONNECTION_INFORMATION (4) requests.  Both times nothing is
returned.

In any case, when no valid connection info is found, num_ports is initially
set to 0, a warning is logged, and num_ports defaults to 2.


-- 
Joe Nardelli
jnardelli@infosciences.com

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

[Greg KH]

On Fri, May 21, 2004 at 03:51:23PM -0400, nardelli wrote:
> 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

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

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


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

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

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

<snip>

Also, don't quote the whole previous message, that's not nice...

thanks,

greg k-h

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

[nardelli]

Greg KH wrote:
> 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?


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

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

Yep - same comment as above.

>>+	else {
>>+	        connection_info = (struct visor_connection_info *)
>>+			transfer_buffer;
> 
> 
> 
> greg k-h
> 


-- 
Joe Nardelli
jnardelli@infosciences.com

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

[Greg KH]

On Fri, May 21, 2004 at 05:44:09PM -0400, nardelli wrote:
> Greg KH wrote:
> >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?

Ok, my bad, sorry, mutt likes to wrap stuff when the mail has the
following header in it like yours did:
	Content-Type: text/plain; charset=us-ascii; format=flowed

Sorry about that, the patch is not wrapped.

greg k-h

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

[nardelli]

nardelli wrote:
> Greg KH wrote:
> 
> 
>>> @@ -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().
In this case, any attempt to read from /dev/ttyUSB0 (even if it is useless)
would result in a null pointer access violation unless there is some
form of protection around it.  Not permitting reads on this port would get
around this, but putting 'if' checks around access to read_urb is probably
much simpler.

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

Any thoughts?

-- 
Joe Nardelli
jnardelli@infosciences.com

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

[Greg KH]

On Fri, May 21, 2004 at 06:04:46PM -0400, nardelli wrote:
> 
> 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.

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

thanks,

greg k-h

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

[nardelli]

Greg KH wrote:
> On Fri, May 21, 2004 at 06:04:46PM -0400, nardelli wrote:
> 
>>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.
> 
> 

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).  The first patch that I submitted had a bug
(which I've fixed) in it 
http://marc.theaimsgroup.com/?l=linux-usb-devel&m=108515267617337&w=2
that Pete Zaitcev pointed out in visor_open() that might be confusing the 
issue.

The behavior that I intended was to allow devices that have more bulk out
endpoints than bulk in endpoints (namely treos) to still use the write only
port.  Mainly though, it was to allow for swapping endpoints to allow for
the uneven number of in and out endpoints.  This was not a problem prior to
now since the endpoints were being copied instead of swapped.


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

I bet someone really smart will figure it out - it probably just melts the ROM,
or something else useful :-)


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.


-- 
Joe Nardelli
jnardelli@infosciences.com

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

[nardelli]

nardelli wrote:
> 
> 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.



May 24 13:54:44 iscjoe kernel: cat: page allocation failure. order:0, mode:0x20
May 24 13:54:44 iscjoe kernel: Call Trace:
May 24 13:54:44 iscjoe kernel:  [__alloc_pages+850/852] __alloc_pages+0x352/0x354
May 24 13:54:44 iscjoe kernel:  [<c013e36e>] __alloc_pages+0x352/0x354
May 24 13:54:44 iscjoe kernel:  [__get_free_pages+34/63] __get_free_pages+0x22/0x3f
May 24 13:54:44 iscjoe kernel:  [<c013e392>] __get_free_pages+0x22/0x3f
May 24 13:54:44 iscjoe kernel:  [dma_alloc_coherent+77/131] dma_alloc_coherent+0x4d/0x83
May 24 13:54:44 iscjoe kernel:  [<c010c43d>] dma_alloc_coherent+0x4d/0x83
May 24 13:54:44 iscjoe kernel:  [pool_alloc_page+92/207] pool_alloc_page+0x5c/0xcf
May 24 13:54:44 iscjoe kernel:  [<c025fba8>] pool_alloc_page+0x5c/0xcf
May 24 13:54:44 iscjoe kernel:  [dma_pool_alloc+261/445] dma_pool_alloc+0x105/0x1bd
May 24 13:54:44 iscjoe kernel:  [<c025fed9>] dma_pool_alloc+0x105/0x1bd
May 24 13:54:44 iscjoe kernel:  [get_device+26/33] get_device+0x1a/0x21
May 24 13:54:44 iscjoe kernel:  [<c025cc33>] get_device+0x1a/0x21
May 24 13:54:44 iscjoe kernel:  [uhci_append_queued_urb+178/276] uhci_append_queued_urb+0xb2/0x114
May 24 13:54:44 iscjoe kernel:  [<c02dadba>] uhci_append_queued_urb+0xb2/0x114
May 24 13:54:44 iscjoe kernel:  [uhci_alloc_td+43/124] uhci_alloc_td+0x2b/0x7c
May 24 13:54:44 iscjoe kernel:  [<c02da70c>] uhci_alloc_td+0x2b/0x7c
May 24 13:54:44 iscjoe kernel:  [uhci_submit_common+360/762] uhci_submit_common+0x168/0x2fa
May 24 13:54:44 iscjoe kernel:  [<c02db8ac>] uhci_submit_common+0x168/0x2fa
May 24 13:54:44 iscjoe kernel:  [uhci_urb_enqueue+469/731] uhci_urb_enqueue+0x1d5/0x2db
May 24 13:54:44 iscjoe kernel:  [<c02dc09d>] uhci_urb_enqueue+0x1d5/0x2db
May 24 13:54:44 iscjoe kernel:  [get_device+26/33] get_device+0x1a/0x21
May 24 13:54:44 iscjoe kernel:  [<c025cc33>] get_device+0x1a/0x21
May 24 13:54:44 iscjoe kernel:  [hcd_submit_urb+287/370] hcd_submit_urb+0x11f/0x172
May 24 13:54:44 iscjoe kernel:  [<c02c8bf4>] hcd_submit_urb+0x11f/0x172
May 24 13:54:44 iscjoe kernel:  [usb_submit_urb+602/844] usb_submit_urb+0x25a/0x34c
May 24 13:54:44 iscjoe kernel:  [<c02c99b7>] usb_submit_urb+0x25a/0x34c
May 24 13:54:44 iscjoe kernel:  [__kmalloc+436/611] __kmalloc+0x1b4/0x263
May 24 13:54:44 iscjoe kernel:  [<c01439dd>] __kmalloc+0x1b4/0x263
May 24 13:54:44 iscjoe kernel:  [pg0+273019921/1068740608] visor_write+0xdf/0x25a [visor]
May 24 13:54:44 iscjoe kernel:  [<d0922411>] visor_write+0xdf/0x25a [visor]
May 24 13:54:44 iscjoe kernel:  [pg0+273274118/1068740608] serial_write+0x85/0x100 [usbserial]
May 24 13:54:44 iscjoe kernel:  [<d0960506>] serial_write+0x85/0x100 [usbserial]
May 24 13:54:44 iscjoe kernel:  [tty_default_put_char+50/52] tty_default_put_char+0x32/0x34
May 24 13:54:44 iscjoe kernel:  [<c0230c4d>] tty_default_put_char+0x32/0x34
May 24 13:54:44 iscjoe kernel:  [opost+170/469] opost+0xaa/0x1d5
May 24 13:54:44 iscjoe kernel:  [<c0231486>] opost+0xaa/0x1d5
May 24 13:54:44 iscjoe kernel:  [write_chan+395/539] write_chan+0x18b/0x21b
May 24 13:54:44 iscjoe kernel:  [<c0233b4c>] write_chan+0x18b/0x21b
May 24 13:54:44 iscjoe kernel:  [default_wake_function+0/18] default_wake_function+0x0/0x12
May 24 13:54:44 iscjoe kernel:  [<c011987e>] default_wake_function+0x0/0x12
May 24 13:54:44 iscjoe kernel:  [default_wake_function+0/18] default_wake_function+0x0/0x12
May 24 13:54:44 iscjoe kernel:  [<c011987e>] default_wake_function+0x0/0x12
May 24 13:54:44 iscjoe kernel:  [tty_write+531/786] tty_write+0x213/0x312
May 24 13:54:44 iscjoe kernel:  [<c022e05e>] tty_write+0x213/0x312
May 24 13:54:44 iscjoe kernel:  [write_chan+0/539] write_chan+0x0/0x21b
May 24 13:54:44 iscjoe kernel:  [<c02339c1>] write_chan+0x0/0x21b
May 24 13:54:44 iscjoe kernel:  [tty_write+0/786] tty_write+0x0/0x312
May 24 13:54:44 iscjoe kernel:  [<c022de4b>] tty_write+0x0/0x312
May 24 13:54:44 iscjoe kernel:  [vfs_write+162/270] vfs_write+0xa2/0x10e
May 24 13:54:44 iscjoe kernel:  [<c015a4dd>] vfs_write+0xa2/0x10e
May 24 13:54:44 iscjoe kernel:  [sys_write+63/93] sys_write+0x3f/0x5d
May 24 13:54:44 iscjoe kernel:  [<c015a5e5>] sys_write+0x3f/0x5d
May 24 13:54:44 iscjoe kernel:  [sysenter_past_esp+82/113] sysenter_past_esp+0x52/0x71
May 24 13:54:44 iscjoe kernel:  [<c0105055>] sysenter_past_esp+0x52/0x71
May 24 13:54:44 iscjoe kernel:
May 24 13:54:44 iscjoe kernel: visor ttyUSB0: visor_write - usb_submit_urb(write bulk) failed with status = -12



-- 
Joe Nardelli
jnardelli@infosciences.com

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

[Greg KH]

On Mon, May 24, 2004 at 03:38:58PM -0400, nardelli wrote:
> nardelli wrote:
> >
> >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.

Which machine dies?  The pilot or the Linux box?

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

So we ran out of memory?  Not good.

> 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

It uses it's own buffers from what I remember.

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

Expected, not all of the usb-serial drivers have to do the same thing,
as they operate on very different types of hardware.

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

Reusing the buffer and urb is _slow_.  The visor driver creates a new
buffer for every call to write.  It is entirely possible that you can
starve the kernel of memory by sending it the output of a raw device
node, as that data comes faster than the USB data can be sent.

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.

thanks,

greg k-h

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

[nardelli]

Greg KH wrote:
> 
> 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.
> 

The more I look at it, the more I agree.  I just wanted to verify that I
didn't break something.


-- 
Joe Nardelli
jnardelli@infosciences.com

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

[nardelli]

Greg KH wrote:
> On Mon, May 24, 2004 at 03:38:58PM -0400, nardelli wrote:
> 
>>nardelli wrote:
>>
>>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?
> 
> 

Oops - missed this question earlier.  It's the linux box that dies.  The
pilot makes it through without a scratch - I'm a bit shocked, as they're
not exactly the most stable device.


-- 
Joe Nardelli
jnardelli@infosciences.com

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

[Greg KH]

On Mon, May 24, 2004 at 01:20:45PM -0400, nardelli wrote:
> Greg KH wrote:
> >On Fri, May 21, 2004 at 06:04:46PM -0400, nardelli wrote:
> >
> >>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.
> >
> >
> 
> The call to open() does not fail

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.


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

But why?  Do you know some kind of protocol that this endpoint can
accept that is valid for the device?  If not, let's not mess with it.

thanks,

greg k-h

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

[nardelli]

Greg KH wrote:
> 
> 
> 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.
> 

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?



--- 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-24 16:44:03.435277632 -0400
@@ -699,7 +699,7 @@ static int palm_os_3_probe (struct usb_s
 	char *string;
 	int retval = 0;
 	int i;
-	int num_ports;
+	int num_ports = 0;
 
 	dbg("%s", __FUNCTION__);
 
@@ -721,41 +721,52 @@ 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;	
+	if (retval == sizeof(*connection_info)) {
+	        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 +898,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 +908,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;
 }



-- 
Joe Nardelli
jnardelli@infosciences.com

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

[Greg KH]

On Mon, May 24, 2004 at 05:42:54PM -0400, nardelli wrote:
> Greg KH wrote:
> >
> >
> >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.
> >
> 
> 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?

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



--- 1.110/drivers/usb/serial/visor.c	2004-05-15 08:48:18 -07:00
+++ edited/drivers/usb/serial/visor.c	2004-05-25 11:19:19 -07:00
@@ -680,7 +680,7 @@
 	char *string;
 	int retval = 0;
 	int i;
-	int num_ports;
+	int num_ports = 0;
 
 	dbg("%s", __FUNCTION__);
 
@@ -702,41 +702,50 @@
 			__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);
+	if (retval == sizeof(*connection_info)) {
+	        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;	
+		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
@@ -868,8 +877,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. */
@@ -879,31 +887,28 @@
 
 	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.
+	*/
+#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);
+	if (!swap_port)
+		return -ENOMEM;
+	COPY_PORT(swap_port, serial->port[0]);
+	COPY_PORT(serial->port[0], serial->port[1]);
+	COPY_PORT(serial->port[1], swap_port);
+	kfree(swap_port);
 
 	return 0;
 }

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

[nardelli]

Greg KH wrote:
> 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
> 
> +#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);
> +	if (!swap_port)
> +		return -ENOMEM;
> +	COPY_PORT(swap_port, serial->port[0]);
> +	COPY_PORT(serial->port[0], serial->port[1]);
> +	COPY_PORT(serial->port[1], swap_port);
> +	kfree(swap_port);
>  
>  	return 0;
>  }
> -

That's definitely less error prone as well.

BTW - I appreciate the time that you have put into this, and especially 
the constructive comments and patience that you had regarding patches 
that I submitted.

-- 
Joe Nardelli
jnardelli@infosciences.com

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

[Greg KH]

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.

Oops, one other thing, you should CC: the driver author/maintainer too,
so they know what you are trying to do.  Don't assume they will see your
patch on a mailing list :)

thanks,

greg k-h