Date: Tue, 19 Aug 2008 22:19:14 +0200 From: Hans Petter Selasky <hselasky@c2i.net> To: Kris Kennaway <kris@freebsd.org> Cc: freebsd-usb@freebsd.org Subject: Re: usb4bsd patch review [was Re: ...] Message-ID: <200808192219.19246.hselasky@c2i.net> In-Reply-To: <48AB233C.2010602@FreeBSD.org> References: <20080818205914.GJ16977@elvis.mu.org> <200808191758.22981.hselasky@c2i.net> <48AB233C.2010602@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi,
On Tuesday 19 August 2008, Kris Kennaway 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.
>
> >> 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 mean I need to make another patchset. And that the current patchset will
break the kernel compilation if blindly committed after mpsafetty.
>
> >>> 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)?
I think that is correct.
>
> 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.
It is still possible to separate the USB drivers. In my experience grouping
the drivers gives a more user-friendly experience. For example you have a USB
serial port adapter and plug it in. Then all you need to do is to kldload
usb2_serial regardless of adapter. It is like a container module. I don't
opponent that for kernel compilation you can have a more fine-grained control
what drivers are actually included. I will see about fixing that.
>
> >> 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.
>
> >>> 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.
>
> > 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);
> + }
>
> >>> 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.
This is because the mtx_lock/unlock was unbalanced in the first place! The
original code used to call mtx_destroy with the mutex locked. Now I added an
unlock before the destroy.
<cite>
snd_mtxunlock(m->lock);
/* mixer uninit can sleep --hps */
MIXER_UNINIT(m);
snd_mtxfree(m->lock);
</cite>
> However it looks like it may have been a bug in the old
> code, I am not sure.
Right.
> Also is it safe to drop and reacquire the lock here?
You have to drop the lock, else I get witness warnings.
>
> 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.
That is just a safety measure, because the Sound code has some ifdef's to
enable it to run without mutexes, and in that case Giant must be used.
>
> > 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.
>
> >>> 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.
There are multiple issues:
1) If PAGE_SIZE is 16384 bytes, then the code simply fails.
2) Sometimes PAGE_SHIFT is already defined.
#if PAGE_SIZE == 4096
#define PAGE_SHIFT 12
#elif PAGE_SIZE == 8192
#define PAGE_SHIFT 13
#else
#error PAGE_SHIFT undefined!
#endif
>
> Anyway, if the module is not complete then it should probably not be
> imported until it is.
Yes, I agree about that.
--HPS
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200808192219.19246.hselasky>