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>