Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 19 Aug 2008 17:58:21 +0200
From:      Hans Petter Selasky <hselasky@c2i.net>
To:        Alfred Perlstein <alfred@freebsd.org>
Cc:        freebsd-usb@freebsd.org
Subject:   Re: usb4bsd patch review [was Re: ...]
Message-ID:  <200808191758.22981.hselasky@c2i.net>
In-Reply-To: <20080819055821.GO16977@elvis.mu.org>
References:  <20080818205914.GJ16977@elvis.mu.org> <20080818214711.J88849@maildrop.int.zabbadoz.net> <20080819055821.GO16977@elvis.mu.org>

next in thread | previous in thread | raw e-mail | index | archive | help
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.
>

> need to wait for smp tty code.

If this requires changes in the USB serial port drivers there will be trouble.

> >
> > 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.

>
> 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.

>
> > 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.

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()

> > 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.

>
> > 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/

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.

>
> > 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.

--HPS



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200808191758.22981.hselasky>