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

nardelli
2004-05-21T19:51:50Z

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