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

[Greg KH <greg@kroah.com>]

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