Date: Tue, 19 Aug 2008 20:47:08 +0100 From: Kris Kennaway <kris@FreeBSD.org> To: Hans Petter Selasky <hselasky@c2i.net> Cc: freebsd-usb@freebsd.org Subject: Re: usb4bsd patch review [was Re: ...] Message-ID: <48AB233C.2010602@FreeBSD.org> In-Reply-To: <200808191758.22981.hselasky@c2i.net> References: <20080818205914.GJ16977@elvis.mu.org> <20080818214711.J88849@maildrop.int.zabbadoz.net> <20080819055821.GO16977@elvis.mu.org> <200808191758.22981.hselasky@c2i.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
>>> 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)?
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.
>> 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. 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.
>>> 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.
Kris
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?48AB233C.2010602>