Re: [patch 2.6.21-rc5] usb-serial, ti_usb: fix usb configuration changing

Greg, Oleg --

I tested this in 2.6.21-rc5 and it no longer works.

The problem is a change in usb-serial.c, in usb_serial_probe().
In 2.6.20 get_free_serial() is called before the attach function.
In 2.6.21-rc5 get_free_serial() is called after the attach function.
If the attach function returns non-zero (as it does in ti_usb_3410_5052
when changing the configuration) then the device is disconnected,
which calls usb_serial_disconnect(), usb_serial_put(), destroy_serial(),
return_serial(), which NULLs out the serial table for minor number 0
(the initial value of the usb_serial minor field before get_free_serial()
is called.)  That is a problem if minor number 0 has already been allocated
to another usb serial device.

It would be easy to work around this by setting the usb_serial minor
field to -1 in create_serial() and check for that in return_serial().
I will send you a patch that does that, though I am not sure that is
the best way to fix this.  I will look at it more.

-- Al





Quoting Oleg Verych <[email protected]>:

> usb-serial, ti_usb: fix usb configuration changing
>
>  Remove bogus error messages, comments with script.
>  Use new usb-configuration interface for usb-interface drivers.
>
> Cc: Linus Torvalds <[email protected]>
> Cc: Al Borchers <[email protected]>
> Signed-off-by: Oleg Verych <[email protected]>
> ---
>  This tiny change uses new interface merged in 2.6.19 (yet author of
>  it didn't find this driver to fix).
>
>  Greg KH waits driver's author ack, while i already have it (in
>  the linux-usb list):
>
> > I tried this on 3410 and 5052 with and without firmware--worked
> > great.
> >
> > Please
> >
> > 1) Change return value to 1, rather than 0xO1E and 0xA1B.
> >    (Clever, but it will just confuse readers.)
>
> Here i was using any values greater than zero, and not negative, i.e.
> errors.
>
> > 2) Drop (void) cast on usb_driver_set_configuration.
>
> I didn't check return status of it, because it doesn't show real
> progress in config. changing (see description of that function).
>
> > 3) Resend the patch with a signed-off line.
> >
> > Thanks for doing this--so glad we can get rid of the hotplug
> > script!
> >
> > -- Al
>
>  While they are very busy with another things, i thought they could
>  spend a few tens of minutes (in last 2 or so months) to have this
>  driver better-looking.
>
> ,-*- this patch -*-
> |usb 2-2: new full speed USB device using ohci_hcd and address 6
> |usb 2-2: configuration #1 chosen from 1 choice
> |ti_usb_3410_5052 2-2:1.0: TI USB 3410 1 port adapter converter detected
> |usb 2-2: reset full speed USB device using ohci_hcd and address 6
> - *compare start* -
> |usb 2-2: device firmware changed
> |usb 2-2: USB disconnect, address 6
> |ti_usb_3410_5052 2-2:1.0: device disconnected
> |usb 2-2: new full speed USB device using ohci_hcd and address 7
> |usb 2-2: configuration #1 chosen from 2 choices
> |ti_usb_3410_5052 2-2:1.0: TI USB 3410 1 port adapter converter detected
> |usb 2-2: setting configuration #2 (activating)
> |ti_usb_3410_5052 2-2:1.0: device disconnected
> |ti_usb_3410_5052 2-2:2.0: TI USB 3410 1 port adapter converter detected
> |usb 2-2: TI USB 3410 1 port adapter converter now attached to ttyUSB0
> `-*-
>
>  Below is original behavior and without udev scripting, note that even
>  today udev package in most distros doesn't have support of this and
>  based on this interface devices (very ugly with dups of dev. IDs and
>  same silly script).
>
> ,-*- from compare start point -*-
> |usb 3-3: device firmware changed
> |ti_usb_3410_5052: probe of 3-3:1.0 failed with error -5
> |usb 3-3: USB disconnect, address 25
> |usb 3-3: new full speed USB device using ohci_hcd and address 26
> |usb 3-3: configuration #1 chosen from 2 choices
> |ti_usb_3410_5052 3-3:1.0: TI USB 3410 1 port adapter converter detected
> |ti_usb_3410_5052: probe of 3-3:1.0 failed with error -5
> `-*-
>
>  I think it's worth having in .21, just for people, who is known to have
>  troubles with udev settings for this driver, and just to have more beauty
>  there. It's not clear when authors will start to merge their stuff, and
>  i'm still waiting to help with firmware loading (bigger patch-set i was
>  originally working on). Also, i think, Greg is wrong, doing nothing to
>  remove such ugliness.
>
>  Thus, i'm sending this to our big Wizard, Linus.
>
>  (two: small and fat white spaces were removed in the function i've
>   changed, still remembering my makefile patches with
> one-whitespace-to-much-noise
>   bug on the beginning of 2.6.21 ;)
>
>  Thanks anyways!
>
>  drivers/usb/serial/ti_usb_3410_5052.c |   68
> +++++-----------------------------
>  1 file changed, 11 insertions(+), 57 deletions(-)
>
> Index: linux-2.6.21-rc5/drivers/usb/serial/ti_usb_3410_5052.c
> ===================================================================
> --- linux-2.6.21-rc5.orig/drivers/usb/serial/ti_usb_3410_5052.c	2007-03-27
> 08:00:26.338095634 +0200
> +++ linux-2.6.21-rc5/drivers/usb/serial/ti_usb_3410_5052.c	2007-03-27
> 08:09:36.803596727 +0200
> @@ -1,4 +1,3 @@
> -/* vi: ts=8 sw=8
> - *
> +/*
>   * TI 3410/5052 USB Serial Driver
>   *
> @@ -17,54 +16,4 @@
>   * technical support, or Al Borchers <[email protected]>, or
>   * Peter Berger <[email protected]>.
> - *
> - * This driver needs this hotplug script in
> /etc/hotplug/usb/ti_usb_3410_5052
> - * or in /etc/hotplug.d/usb/ti_usb_3410_5052.hotplug to set the device
> - * configuration.
> - *
> - * #!/bin/bash
> - *
> - * BOOT_CONFIG=1
> - * ACTIVE_CONFIG=2
> - *
> - * if [[ "$ACTION" != "add" ]]
> - * then
> - * 	exit
> - * fi
> - *
> - * CONFIG_PATH=/sys${DEVPATH%/?*}/bConfigurationValue
> - *
> - * if [[ 0`cat $CONFIG_PATH` -ne $BOOT_CONFIG ]]
> - * then
> - * 	exit
> - * fi
> - *
> - * PRODUCT=${PRODUCT%/?*}		# delete version
> - * VENDOR_ID=`printf "%d" 0x${PRODUCT%/?*}`
> - * PRODUCT_ID=`printf "%d" 0x${PRODUCT#*?/}`
> - *
> - * PARAM_PATH=/sys/module/ti_usb_3410_5052/parameters
> - *
> - * function scan() {
> - * 	s=$1
> - * 	shift
> - * 	for i
> - * 	do
> - * 		if [[ $s -eq $i ]]
> - * 		then
> - * 			return 0
> - * 		fi
> - * 	done
> - * 	return 1
> - * }
> - *
> - * IFS=$IFS,
> - *
> - * if (scan $VENDOR_ID 1105 `cat $PARAM_PATH/vendor_3410` &&
> - * scan $PRODUCT_ID 13328 `cat $PARAM_PATH/product_3410`) ||
> - * (scan $VENDOR_ID 1105 `cat $PARAM_PATH/vendor_5052` &&
> - * scan $PRODUCT_ID 20562 20818 20570 20575 `cat $PARAM_PATH/product_5052`)
> - * then
> - * 	echo $ACTIVE_CONFIG > $CONFIG_PATH
> - * fi
>   */
>
> @@ -452,11 +401,16 @@ static int ti_startup(struct usb_serial
>  		}
>
> -		status = -ENODEV;
> +		status = 1; /* positive status -- device to be reconfigured */
>  		goto free_tdev;
> -	}
> +	}
>
> -	/* the second configuration must be set (in sysfs by hotplug script) */
> +	/* active configuration must be set */
>  	if (dev->actconfig->desc.bConfigurationValue == TI_BOOT_CONFIG) {
> -		status = -ENODEV;
> +		dev_info(&dev->dev, "setting configuration #%u (activating)\n",
> +			 TI_ACTIVE_CONFIG);
> +
> +		status = usb_driver_set_configuration(dev, TI_ACTIVE_CONFIG);
> +		status = status ? status: 1;
> +
>  		goto free_tdev;
>  	}
> @@ -488,5 +442,5 @@ static int ti_startup(struct usb_serial
>  		tport->tp_uart_mode = 0;	/* default is RS232 */
>  	}
> -
> +
>  	return 0;
>
> ____
>




-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel