Date: Sun, 1 Feb 2009 19:22:15 +0100 From: Hans Petter Selasky <hselasky@c2i.net> To: Andrew Thompson <thompsa@freebsd.org> Cc: freebsd-usb@freebsd.org Subject: Re: USB2 patches Message-ID: <200902011922.16810.hselasky@c2i.net> In-Reply-To: <20090201175021.GA32503@citylink.fud.org.nz> References: <20090131231957.GB31825@citylink.fud.org.nz> <200902011220.18745.hselasky@c2i.net> <20090201175021.GA32503@citylink.fud.org.nz>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Andrew,
On Sunday 01 February 2009, Andrew Thompson wrote:
> > 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?
Yes, some of the wireless USB drivers can program hundreds of registers during
startup.
> - 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.
I think you misunderstand. The attach code that loads firmware, reads eeprom
and performs other configuration must be handed off to another thread, or in
your case a taskqueue, so that the USB attach thread can go on and do other
things.
In the [was configure-] thread you can attach things synchronously. In the
case of network adapters you should not rely on the device_attach() event
from devd to do something, but rather the event from if_attach(), if such
exits.
> 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.
If you compare the old USB code with the new USB code for if_zyd for example
you see that there are dozens of error checks everywhere:
if (error != 0)
goto fail;
In the new USB code I've factored out all those checks into 3 lines of code in
every driver:
if (usb2_config_td_is_gone(&sc->sc_config_td)) {
goto error;
}
In your patch you remove all error checking! If the taskqueue system does not
have an API function that can tell if the taskqueue is being drained from
inside the taskqueue callback, the taskqueue system has to be modified! It
cannot replace the existing system like it is now!
> > 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.
Being a little off-sync is not the big problem. The big problem is if the
channel value is changing during execution of the code. For example, in the
beginning of the code the channel value is 5, then suddenly it becomes 6, and
the programming just ends up crazy!
xxx_set_channel()
{
hw_reg = array1[wlan->curchan];
write USB register;
hw_reg = array2[wlan->curchan];
write USB register;
}
> 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.
Good! But until then I strongly suggest you keep things like is.
>
> > 5) Many of your other changes are good!
>
> Thanks!
Can you explain one thing: How many threads are created to handle taskqueue
events? As many as required?
If three different taskqueues have three different events, then three threads
are used to execute these events?
--HPS
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200902011922.16810.hselasky>