Date: Tue, 19 Aug 2008 22:38:02 +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: <48AB3D3A.4040303@FreeBSD.org> In-Reply-To: <200808192219.19246.hselasky@c2i.net> References: <20080818205914.GJ16977@elvis.mu.org> <200808191758.22981.hselasky@c2i.net> <48AB233C.2010602@FreeBSD.org> <200808192219.19246.hselasky@c2i.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Hans Petter Selasky wrote: >> 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. OK, thanks for clarifying. >>>>> 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. OK, so does this mean we should expect to see a revised commit candidate patch from you and/or alfred later once that is finished? >> 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. You could look at what the sound code does, (I think it is specifically the snd_driver module). This implements auto-loading of the "right" drivers by loading them and then unloading the ones that don't attach. This still has overhead though, so users often will just compile in the ones they know they have. >> Also is it safe to drop and reacquire the lock here? > > You have to drop the lock, else I get witness warnings. Yes, and presumably rightly so. It doesn't mean that dropping the lock and later reacquiring it is safe though; it could introduce races. >> 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. OK, this seems unrelated to USB then and is something you should discuss with the sound maintainers. It sounds to me like the ability to "run without mutexes" is the real bug here, and "support" for that should be removed completely instead of patching it up downstream. >>>> 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 OK, but again, why? If some architecture is not defining the same symbols as the others then maybe that architecture should be fixed, instead of working around the effect downstream. Kris P.S. There were a number of questions you didn't answer, can I assume those will follow later?
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?48AB3D3A.4040303>