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

Greg KH
2004-05-22T02:32:20Z

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