Date: Tue, 19 Aug 2008 21:42:11 +0100 From: Kris Kennaway <kris@FreeBSD.org> To: Alfred Perlstein <alfred@freebsd.org> Cc: freebsd-usb@freebsd.org Subject: Re: usb4bsd patch review [was Re: ...] Message-ID: <48AB3023.2030307@FreeBSD.org> In-Reply-To: <20080819200017.GC16977@elvis.mu.org> References: <20080818205914.GJ16977@elvis.mu.org> <20080818214711.J88849@maildrop.int.zabbadoz.net> <20080819055821.GO16977@elvis.mu.org> <200808191758.22981.hselasky@c2i.net> <48AB233C.2010602@FreeBSD.org> <20080819200017.GC16977@elvis.mu.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Alfred Perlstein wrote:
> * Kris Kennaway <kris@FreeBSD.org> [080819 12:47] 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.
>
> Kris, Hans usb code has been in a public repository with patches
> posted to the lists for a long time.
Having a public p4 branch is fine, but it's his active development
branch including lots of stuff that may or may not be stable, finalized,
intended for commit at all, etc. If you recall when you proposed the
integration back in May, 3-4 people asked for the commit candidate patch
then and didn't get one, and I haven't seen it posted on a mailing list
since then either.
If I missed a "commit candidate" email from you or Hans that was prior
to last night's "post attempted commit", then I apologise, but as far as
several of us can determine none was posted except possibly to some
people in private email. The general intention of a USB integration in
the future was well known for the past 3 months, but I think the fact
that it you had intended to do it last night came as a complete surprise
to many people who were expecting prior scheduling.
>>>>> 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)?
>
> This is a huge amount of code to haul around... again, we are not
> getting rid of "oldusb", if you don't want to be bleeding edge
> usb2, then you don't need it.
It just strikes me as odd that one half of the code needs to be
committed urgently when the other is not finished. I was of the belief
that you and Hans considered the USB code to be "complete" and hence
ready for integration, but it sounds like there are large parts still
incomplete or in flux. Some explanation of why this is not the case
would help to clarify things.
>>>>> 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);
>> + }
>
> Yes, but sometimes it's hard to do so.
>
> I think such things can be addressed. Do consider the amount of
> progress here, and realize it's a giant step forward with a few
> nits we can address with time.
I am taking no position on the bits of code I didn't review, but I hope
we can agree that technical questions about subsets of the patch that
were reviewed are worth resolving. Usually such concerns are resolved
as part of the process of preparation for a commit.
>>>>> 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.
>
> Can we consider leveraging the doc people and doing the docs post
> commit? I think we can get something nice in short time.
It sounds reasonable to me if you and Hans are going to work together on
that with the doc guys. Presumably this process would not just involve
someone from doc@ writing all the manpages though.
>>>>> 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.
>
> Yes, but it breaks the module without it, so it's a chicken and egg
> issue, having to haul around a patch that will eventually have to go
> in anyhow doesn't make much sense.
It is the "have to go in anyhow" that I am querying. If there is a real
solution that avoids the need for this patch then that is obviously
preferable.
> At the end of this all, there are some small requests in here along
> with some large requests, if you take the time to add up the time
> needed to get all of this according to your request, it pushes back
> the delivery of the essential kernel functionality by several weeks
> if not months.
Keep in mind that any delay at this point to address review concerns
could have been handled 3 months ago when the commit candidate diff was
first requested, and are usually handled as part of the standard
development practises we use in FreeBSD (posting diffs for public
comment with a pre-announced timetable, dealing with technical review,
etc). If following those steps after not previously having done so
means you now have to push back the timetable, that is unfortunate, but
necessary.
Kris
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?48AB3023.2030307>