Date: Tue, 19 Aug 2008 13:00:17 -0700 From: Alfred Perlstein <alfred@freebsd.org> To: Kris Kennaway <kris@FreeBSD.org> Cc: freebsd-usb@freebsd.org Subject: Re: usb4bsd patch review [was Re: ...] Message-ID: <20080819200017.GC16977@elvis.mu.org> In-Reply-To: <48AB233C.2010602@FreeBSD.org> References: <20080818205914.GJ16977@elvis.mu.org> <20080818214711.J88849@maildrop.int.zabbadoz.net> <20080819055821.GO16977@elvis.mu.org> <200808191758.22981.hselasky@c2i.net> <48AB233C.2010602@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
* Kris Kennaway <kris@FreeBSD.org> [080819 12:47] wrote:
> Hans Petter Selasky wrote:
> >Hi,
> >
> >On Tuesday 19 August 2008, Alfred Perlstein wrote:
> >>Hans, here's some final review questions, I've added responses
> >>where I can recall off the top of my head answers, but I need
> >>you to fill in the rest.
>
> Just so we are clear, this is actually an "initial review" since it's
> the first time the commit candidate has been posted for public review,
> as far as I can tell.
Kris, Hans usb code has been in a public repository with patches
posted to the lists for a long time.
> >>need to wait for smp tty code.
> >
> >If this requires changes in the USB serial port drivers there will be
> >trouble.
>
> I am not sure what you mean by this statement, since it can be
> interpreted in several ways, some not so friendly.
I think hans means that there might be trouble in the usb2 system,
but that doesn't spread out to anything else.
If there's a problem in usb2, then don't use it, we'll fix it shortly.
> >>>The patch only includes kernel parts, is there any impact on userland?
> >
> >No. Userland will build like before. The current USB userland utilities
> >and libusb from ports will only work with the old USB stack. I'm working
> >on a replacement for "usbdevs" called "usbconfig" which has some more
> >functionality and a FreeBSD specific libusb, which is compatible with
> >libusb from sourceforge. I have most of it finished in my private SVN
> >repository, and will add it to my P4 repository soon.
>
> This raises the question of why the kernel changes need to be committed
> now, and what benefit they bring in the absence of a compatible
> userland. Shouldn't the commit happen after both kernel and userland
> are ready (and reviewed)?
This is a huge amount of code to haul around... again, we are not
getting rid of "oldusb", if you don't want to be bleeding edge
usb2, then you don't need it.
> Another comment: the new code seems to bundle all new drivers into
> "subsystems" but not allow them to be loaded individually. This is
> quite contrary to how the rest of the kernel is structured, and seems to
> be of questionable benefit. For example, users will rarely have more
> than one USB ethernet driver in use on a given system but "device
> usb2_ethernet" will compile in all 7 drivers.
Interem issue, this is just to reduce the complexity for the time being,
we will "remodularize it" before it replaces "oldusb" or as needed.
> >>I think the userland tools need to be switched to the new headers,
> >>however what's more likely to happen is that after a soak period of
> >>a few weeks, we will move the new to replace the old and userland should
> >>be just fine.
> >>
> >>>There are various style bugs introduced. That could be fixed easily I
> >>>guess?
> >>Yes, this is a LOT of new code, let's leave a few whitespace nits for
> >>others to play with. :)
> >
> >All code has been and is continuously styled with a modified "indent"
> >utility:
> >
> >(cat $F | indent -Toss_mixerinfo -TFILE -Tu_char -Tu_int -Tu_long \
> > -TTAILQ_HEAD -TLIST_HEAD -TTAILQ_ENTRY -TLIST_ENTRY \
> > -TSTAILQ_HEAD -TSTAILQ_ENTRY \
> > -Tu_short -Tfd_set -ta -st -bad -bap -nbbb -nbc -br -nbs \
> > -c41 -cd41 -cdb -ce -ci4 -cli0 -d0 -di8 -ndj -ei -nfc1 \
> > -nfcb -i8 -ip8 -l79 -lc77 -ldi0 -nlp -npcs -psl -sc \
> > -nsob -nv |
> > sed -e "s/_HEAD [(]/_HEAD(/g" |
> > sed -e "s/_ENTRY [(]/_ENTRY(/g" |
> > sed -e "s/ __packed/ __packed/g" |
> > sed -e "s/ __aligned/ __aligned/g" |
> > sed -e "s/^#define /#define /g") > temp
> >
> >If there are any more options I can add, then please let me know.
>
> Mechanical indent tools can be a useful starting point for style(9)
> compliance but are not the end point of that process. Usually there
> will need to be manual tweaks.
That is fine.
>
> >>>In kern/subr_bus.c you are taking steps to avoid zero size
> >>>allocations, which haven't been there up to now. Why do we need that?
> >
> >It is a workaround. USB2 defines the following function, which can be
> >called when there are no children on a device, which must be handled
> >correctly cross-platform. If this function could be directly implemented
> >in subr_bus.c we would not require the modifications to
> >"device_get_children" at all or any memory allocation.
>
> What do the newbus guys say about this? Adding a workaround in
> underlying code for a problem caused by your own code is often a signal
> that you're not going about it the right way. At the very least the
> reason for the special case should be documented here.
I need to think about this, Hans gave me a better argument on
AIM earlier for it, I need to reload this in my head.
> >int
> >device_delete_all_children(device_t dev)
> >{
> > device_t *devlist;
> > int devcount;
> > int error;
> >
> > error = device_get_children(dev, &devlist, &devcount);
> > if (error == 0) {
> > while (devcount-- > 0) {
> > error = device_delete_child(dev,
> > devlist[devcount]);
> > if (error) {
> > break;
> > }
> > }
> > free(devlist, M_TEMP);
> > }
> > return (error);
> >}
> >
> >>??Hans??
> >>
> >>>There is a question about whether to avoid the conditional locking in
> >>>dev/sound/pcm/channel.c in favour of recursive locking?
> >
> >That is also possible. The type of the mutex in question can be changed.
> >The feedback I've gotten so far has been against the use of recursive
> >mutexes. Alternativly, expose two variants of the function in question:
> >
> >xxx_unlocked()
> >xxx_locked()
>
> It is correct that in general recursive locking is considered
> undesirable, but you should also not play games to work around the fact
> that your locking is in fact being called recursively, as in:
>
> - CHN_LOCK(c);
> + uint8_t do_unlock;
> + if (CHN_LOCK_OWNED(c)) {
> + /*
> + * Allow sound drivers to call this function with
> + * "CHN_LOCK()" locked:
> + */
> + do_unlock = 0;
> + } else {
> + do_unlock = 1;
> + CHN_LOCK(c);
> + }
Yes, but sometimes it's hard to do so.
I think such things can be addressed. Do consider the amount of
progress here, and realize it's a giant step forward with a few
nits we can address with time.
> >>>The locking in dev/sound/pcm/mixer.c appears to be strange; the
> >>>locking appears to have substantially changed without corresponding
> >>>changes to the non-USB drivers.
> >
> >All the mixer changes are in the detach code for the mixer and should not
> >require any changes to non-USB drivers.
>
> The change here:
>
> - snd_mtxlock(m->lock);
> + /* mixer uninit can sleep --hps */
>
> MIXER_UNINIT(m);
>
> @@ -704,14 +704,24 @@
> return EBUSY;
> }
>
> + /* destroy dev can sleep --hps */
> +
> + snd_mtxunlock(m->lock);
> +
> pdev->si_drv1 = NULL;
> destroy_dev(pdev);
>
> + snd_mtxlock(m->lock);
> +
> for (i = 0; i < SOUND_MIXER_NRDEVICES; i++)
> mixer_set(m, i, 0);
>
> mixer_setrecsrc(m, SOUND_MASK_MIC);
>
> + snd_mtxunlock(m->lock);
>
> seems to change locking since you remove a mtx_lock and don't add it
> back anywhere. However it looks like it may have been a bug in the old
> code, I am not sure. Also is it safe to drop and reacquire the lock here?
>
> What do the sound maintainers think about these changes?
>
> >>>Some of the new sound drivers are Giant-locked? I am not sure we
> >>>should be adding new drivers that are not mpsafe.
> >
> >I assume that you mean USB drivers? s/sound/USB/
>
> I am referring to this:
>
> +struct mtx *
> +mixer_get_lock(struct snd_mixer *m)
> +{
> + if (m->lock == NULL) {
> + return (&Giant);
> + }
> + return (m->lock);
> +}
>
> This seems to say that m->lock now may be NULL in situations where it
> was not before (since there is no handling for m->lock == NULL in
> existing parts of the code), so the Giant locking is new.
>
> >Regarding Giant use:
> >
> >All USB drivers that can work without Giant has been converted to work
> >without Giant. Some USB drivers like USB serial port drivers cannot work
> >without Giant because they depend on a Giant locked TTY layer. Same with
> >keyboard. Another example is UHUB which use Giant simply because the
> >device_xxx() functions require Giant.
> >
> >The Giant lock is not used in any critical paths.
> >
> >>I think this will be addressed shortly... but..
> >>
> >
> >>(please comment)
> >>
> >>>There seems to be a README.TXT but no manpages for the new API or
> >>>drivers. Are these pending?
> >
> >The manpages are not ready. This is something I need to do. I would
> >appreciate some help here like where I find manpage templates and where I
> >should put these files.
>
> The doc@ folks can probably answer this question if you ask them. It's
> also worth recalling that people asked for this 3 months ago as part of
> the process of getting this code commit-ready.
Can we consider leveraging the doc people and doing the docs post
commit? I think we can get something nice in short time.
>
> >>>Why is the diff to compat/ndis/ntoskrnl_var.h necessary?
> >>It's needed for compiling usb2_ndis, but...
> >
> >Without this patch the usb2_ndis module will not compile on certain
> >architectures. If you remove this patch you will need to decouple the
> >usb2_ndis module, which is not complete anyway, from the default build.
>
> Why will it not compile? This looks like a workaround for some other
> issue that should be solved directly.
>
> Anyway, if the module is not complete then it should probably not be
> imported until it is.
Yes, but it breaks the module without it, so it's a chicken and egg
issue, having to haul around a patch that will eventually have to go
in anyhow doesn't make much sense.
At the end of this all, there are some small requests in here along
with some large requests, if you take the time to add up the time
needed to get all of this according to your request, it pushes back
the delivery of the essential kernel functionality by several weeks
if not months.
--
- Alfred Perlstein
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080819200017.GC16977>