From owner-freebsd-usb@FreeBSD.ORG Tue Aug 19 20:17:45 2008 Return-Path: Delivered-To: freebsd-usb@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id A796A1065688; Tue, 19 Aug 2008 20:17:45 +0000 (UTC) (envelope-from hselasky@c2i.net) Received: from swip.net (mailfe13.swipnet.se [212.247.155.129]) by mx1.freebsd.org (Postfix) with ESMTP id A1FC68FC20; Tue, 19 Aug 2008 20:17:44 +0000 (UTC) (envelope-from hselasky@c2i.net) X-Cloudmark-Score: 0.000000 [] X-Cloudmark-Analysis: v=1.0 c=1 a=H3poPDSvFasA:10 a=fj6dw-CIB2gA:10 a=6MIg2jpqvhTpo/gR8GzG7Q==:17 a=r3QWF4mezmgulWj3MVkA:9 a=nWsatMDgZTJ8sheTY0oA:7 a=zML3vKV0_yQ_J_QS1C5eEBnLJFkA:4 a=50e4U0PicR4A:10 Received: from [62.113.133.243] (account mc467741@c2i.net [62.113.133.243] verified) by mailfe13.swip.net (CommuniGate Pro SMTP 5.2.6) with ESMTPA id 656052876; Tue, 19 Aug 2008 22:17:42 +0200 From: Hans Petter Selasky To: Kris Kennaway Date: Tue, 19 Aug 2008 22:19:14 +0200 User-Agent: KMail/1.9.7 References: <20080818205914.GJ16977@elvis.mu.org> <200808191758.22981.hselasky@c2i.net> <48AB233C.2010602@FreeBSD.org> In-Reply-To: <48AB233C.2010602@FreeBSD.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200808192219.19246.hselasky@c2i.net> Cc: freebsd-usb@freebsd.org Subject: Re: usb4bsd patch review [was Re: ...] X-BeenThere: freebsd-usb@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: FreeBSD support for USB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 19 Aug 2008 20:17:45 -0000 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. snd_mtxunlock(m->lock); /* mixer uninit can sleep --hps */ MIXER_UNINIT(m); snd_mtxfree(m->lock); > 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