From owner-freebsd-usb@FreeBSD.ORG  Tue Aug 19 22:05:30 2008
Return-Path: <owner-freebsd-usb@FreeBSD.ORG>
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 6671E1065672;
	Tue, 19 Aug 2008 22:05:30 +0000 (UTC) (envelope-from imp@bsdimp.com)
Received: from harmony.bsdimp.com (bsdimp.com [199.45.160.85])
	by mx1.freebsd.org (Postfix) with ESMTP id 087B68FC16;
	Tue, 19 Aug 2008 22:05:29 +0000 (UTC) (envelope-from imp@bsdimp.com)
Received: from localhost (localhost [127.0.0.1])
	by harmony.bsdimp.com (8.14.2/8.14.1) with ESMTP id m7JM2LCv090765;
	Tue, 19 Aug 2008 16:02:21 -0600 (MDT) (envelope-from imp@bsdimp.com)
Date: Tue, 19 Aug 2008 16:02:21 -0600 (MDT)
Message-Id: <20080819.160221.41717240.imp@bsdimp.com>
To: hselasky@c2i.net
From: Warner Losh <imp@bsdimp.com>
In-Reply-To: <200808192228.39015.hselasky@c2i.net>
References: <48AB233C.2010602@FreeBSD.org>
	<20080819200017.GC16977@elvis.mu.org>
	<200808192228.39015.hselasky@c2i.net>
X-Mailer: Mew version 3.3 on Emacs 21.3 / Mule 5.0 (SAKAKI)
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Cc: freebsd-usb@freebsd.org
Subject: Re: usb4bsd patch review
X-BeenThere: freebsd-usb@freebsd.org
X-Mailman-Version: 2.1.5
Precedence: list
List-Id: FreeBSD support for USB <freebsd-usb.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-usb>,
	<mailto:freebsd-usb-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/freebsd-usb>
List-Post: <mailto:freebsd-usb@freebsd.org>
List-Help: <mailto:freebsd-usb-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-usb>,
	<mailto:freebsd-usb-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Tue, 19 Aug 2008 22:05:30 -0000

From: Hans Petter Selasky <hselasky@c2i.net>
Subject: Re: usb4bsd patch review [was Re: ...]
Date: Tue, 19 Aug 2008 22:28:36 +0200

> > >
> > > 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.
> >
> > I need to think about this, Hans gave me a better argument on
> > AIM earlier for it, I need to reload this in my head.
> >
> > > >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);
> > > >}
> > > >
> 
> In the existing kernel code, "device_get_children()" is used many places 
> without checking the error code. I have patches, but they are not part of the 
> patch-set.
> 
> Also freeing a pointer to zero bytes is not logical. I'm not sure if this is 
> allowed in kernel space?
> 
> ptr = malloc(0, ... )
> free(0);
> 
> The device_get_children() could have returned an error if there are no 
> children, but again, the existing code does not check this return value.

The existing code is fine when there's no children.  The semantics of
the kernel allocation routines are such that there's no problem.  I
don't think there's a bug here at all.  The bug here is that the
interface is impossible to lock, but that's another issue....

Having said that, I rather like the idea of having a
device_delete_all_children.

Warner