USB2 patches

[Andrew Thompson]

On Sun, Feb 01, 2009 at 12:20:17PM +0100, Hans Petter Selasky wrote:
> Hi Andrew,
> 
> We need to go through your patches together, to work out some issues.
> 
> 1) A nit.
> 
> -	if (usb2_proc_setup(&ssc->sc_config_td, p_mtx, USB_PRI_MED)) {
> -		usb2_com_units_free(root_unit, sub_units);
> -		return (ENOMEM);
> -	}
> +
> +	error = usb2_proc_create(&ssc->sc_tq, USB_PRI_MED, "ucom");
> +	if (error)
> 
> < missing "usb2_com_units_free()" >

thanks.

> +		return (error);
> +
> 
> 2) Please explain why you think that "usb2_proc_is_gone()" can be removed.
> 
> -	if (usb2_proc_is_gone(&ssc->sc_config_td)) {
> -		DPRINTF("proc is gone\n");
> -		return;		/* nothing to do */
> -	}
> 
> If the taskqueue system does not provide a teardown mechanism so that we 
> inside the callback can know if the taskqueue is being drained, then the 
> taskqueue cannot replace the original "usb2_proc_xxx()" ! Maybe this means we 
> need to extend the taskqueue system?

Possibly. Taskqueues will always complete the function callbacks before
freeing so you dont need to worry about resources disappearing under you.
The one reason would be to bail out of a long running task but,
 - Are there any tasks that actually run a long time?
 - If not, its easier just to wait on the drain

> 3)
> 
> In general avoid more than a few synchronous USB transfer inside 
> attach/detach. Always defer! Else there are corner cases where the device can 
> hang waiting for the transfer timeout 1-5 seconds for every USB transfer, and 
> that is unacceptable.

I disagree. It hugely simplifies drivers to know all the resources are
allocated after attach, you dont need to check for null pointers
throughout the code. If programming commands fail/timeout in the attach routine
then the driver needs to check this and abort the attach. You will wait
one or two timeouts max.

> 
> 4)
> 
> int onoff = USB_TASK_ARG(task);
> 
> This won't work! You need to be able to handle N-phase here, where N goes to 
> +infinity! If the DTR pin was toggled we should also see a toggle in the 
> programming of the DTR pin, not just re-program the last state! If there is a 
> USB_TASK_ARG() it must be constant and that is where I start seeing problems, 
> or features missing in the taskqueue system.
> 
> It is also very important that you somehow freeze the configuration at the 
> moment a command is issued. For example in the WLAN drivers. If the command 
> is queued when the channel is 5 then we should program channel 5 and not 
> fetch the latest channel value from the softc! IMPORTANT!

This isnt correct. Take your example of WLAN drivers, the net80211 layer
will use ic_curchan for its logic so the driver needs to program the
chip to this value at all times. You can _not_ queue channel changes as
these will be out of sync. Tasks are very short lived and program the
chipset to the current state where it otherwise could not have been done
due to thread sleeping.

Also, net80211 will soon have its own thread so you can do things like
state/channel changes directly but in a sleepable context.

> 5) Many of your other changes are good!

Thanks!


Andrew