Date: Sun, 1 Feb 2009 12:20:17 +0100 From: Hans Petter Selasky <hselasky@c2i.net> To: Alfred Perlstein <alfred@freebsd.org> Cc: freebsd-usb@freebsd.org, Andrew Thompson <thompsa@freebsd.org> Subject: Re: USB2 patches Message-ID: <200902011220.18745.hselasky@c2i.net> In-Reply-To: <20090201030628.GE65558@elvis.mu.org> References: <20090131231957.GB31825@citylink.fud.org.nz> <20090201030628.GE65558@elvis.mu.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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()" >
+ 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?
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.
With regard to the ethernet/wlan drivers, it is Ok to create the "if" instance
in attach, but not to start programming the chip. This needs to be
synchronised otherwise. I suggets you make a task called:
zyd_first_time_configure() and kick this task before allocating ifnet
instances. Then when everything is ready you queue another task to set the
LINK flag, so that we don't start transmitting data during early chip
configuration! In case of failure you just drain the taskqueue.
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!
I'm not saying USB cannot use the taskqueue, but we have special requirements
which I cannot see that the taskqueue is a full drop in replacement for!
5) Many of your other changes are good!
--HPS
On Sunday 01 February 2009, Alfred Perlstein wrote:
> I'll defer to Hans if he feels confident or not about this.
>
> For what it's worth, we're about to switch GENERIC to use
> usb4bsd.
>
> -Alfred
>
> * Andrew Thompson <thompsa@FreeBSD.org> [090131 15:20] wrote:
> > Hi,
> >
> >
> > I have several patches in my svn user branch that I would like to see
> > committed to HEAD. Some of these change the usb2 core code so I am
> > interested in feedback or shootdowns. I am right behind the change to
> > USB2 and this is an effort to help.
> >
> > The patch can be found here,
> > http://people.freebsd.org/~thompsa/usb_head1.diff
> > 73 files changed, 9229 insertions(+), 13261 deletions(-)
> >
> > but its rather large so it may be easier to look at the various changes
> > via the svn web interface.
> >
> > http://svn.freebsd.org/viewvc/base/user/thompsa/usb/
> >
> >
> > r187750
> > http://svn.freebsd.org/viewvc/base?view=revision&revision=187750
> >
> > Change over to using taskqueue(9) instead of hand rolled threads and
> > the config_td system. This removes the config_td code and makes the
> > API much simpler to use.
> >
> > r187751, r187752, r187753, r187754, r187755, r187756
> > http://svn.freebsd.org/viewvc/base?view=revision&revision=187751
> >
> > Change over to usb2_proc w/ taskqueues for the usb2/ethernet,
> > usb2/serial and usb2/wlan code.
> >
> > r187965
> > http://svn.freebsd.org/viewvc/base?view=revision&revision=187965
> >
> > Move most of the ifnet logic into the usb2_ethernet module, this
> > includes, - make all usb ethernet interfaces named ue%d
> > - handle all threading in usb2_ethernet
> > - provide default ioctl handler
> > - handle mbuf rx
> > - provide locked callbacks for init,start,stop,etc
> >
> > The drivers are not much more than data pushers now.
> >
> >
> > regards,
> > Andrew
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200902011220.18745.hselasky>