From owner-freebsd-hackers@FreeBSD.ORG Thu May 31 18:59:44 2012 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 7DA191065670; Thu, 31 May 2012 18:59:44 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigknife-pt.tunnel.tserv9.chi1.ipv6.he.net [IPv6:2001:470:1f10:75::2]) by mx1.freebsd.org (Postfix) with ESMTP id 39ED28FC1F; Thu, 31 May 2012 18:59:44 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 80C44B999; Thu, 31 May 2012 14:59:43 -0400 (EDT) From: John Baldwin To: Warner Losh Date: Thu, 31 May 2012 14:59:42 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p13; KDE/4.5.5; amd64; ; ) References: <4FC729A2.30205@demig.de> <201205311154.15062.jhb@freebsd.org> <2A30119B-0C5E-43A9-9B26-11FCF6685999@bsdimp.com> In-Reply-To: <2A30119B-0C5E-43A9-9B26-11FCF6685999@bsdimp.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201205311459.42538.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Thu, 31 May 2012 14:59:43 -0400 (EDT) Cc: Norbert Koch , freebsd-hackers@freebsd.org, Warner Losh Subject: Re: bus device/ivars X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 31 May 2012 18:59:44 -0000 On Thursday, May 31, 2012 12:15:48 pm Warner Losh wrote: > > On May 31, 2012, at 9:54 AM, John Baldwin wrote: > > > On Thursday, May 31, 2012 4:19:46 am Norbert Koch wrote: > >> Hello, > >> > >> I have written a bus device driver > >> which itself is a pci driver. Child devices > >> may allocate resources from my bus device. > >> > >> My bus device does the usual > >> management of resources through > >> the children's ivars. > >> > >> My question is this: > >> > >> The bus device mallocs the > >> children's ivars in bus_add_child > >> and frees the ivars in either > >> bus_detach or bus_child_detached. > >> > >> The children are added in identify > >> methods through BUS_ADD_CHILD. > >> > >> As I understand the code the bus device's > >> bus_child_detached method is called > >> in device_delete_child only if > >> the child device is already attached. > >> > >> So, there seems to be a memory leak if > >> I delete the child device in either > >> identify or probe. > >> > >> My current solution (not tested yet) is to > >> explicitly call BUS_CHILD_DETACHED > >> in the child device's code before > >> calling device_delete_child. > >> > >> Is this the correct way or is > >> there a more elegant/cleaner solution? > >> > >> I expected to find something like a > >> BUS_DELETE_CHILD method. > > > > We should perhaps have a BUS_CHILD_DELETED? I think that would do what you > > want. We could maybe add a BUS_DELETE_CHILD(), but it would be assymmetric to > > have device_delete_child() call BUS_DELETE_CHILD() when device_add_child() > > does not call BUS_ADD_CHILD(). (Instead, BUS_ADD_CHILD() calls > > device_add_child, which is perhaps wrong.) > > > > For now I would change your child code to call a wrapper foo_delete_child() > > function from your child drivers directly rather than calling > > device_delete_child(). foo_delete_child() can do its cleanup and then call > > device_delete_child(). > > We likely should have a BUS_CHILD_DELETED function that can get called for each class in the stack when a child is deleted so you can remove the ivars. The ivars should likely stay around when the device is merely detached. Either that or we redo BUS_ADD_CHILD() such that device_add_child_ordered() invokes it (and it has a default method that does what device_add_child_ordered() does now). We could then mirror that with device_delete_child() and a BUS_DELETE_CHILD(). Here is the simpler fix (I think): Index: kern/subr_bus.c =================================================================== --- kern/subr_bus.c (revision 236313) +++ kern/subr_bus.c (working copy) @@ -1873,6 +1873,8 @@ return (error); if (child->devclass) devclass_delete_device(child->devclass, child); + if (child->parent) + BUS_CHILD_DELETED(dev, child); TAILQ_REMOVE(&dev->children, child, link); TAILQ_REMOVE(&bus_data_devices, child, devlink); kobj_delete((kobj_t) child, M_BUS); Index: kern/bus_if.m =================================================================== --- kern/bus_if.m (revision 236313) +++ kern/bus_if.m (working copy) @@ -160,6 +160,20 @@ }; /** + * @brief Notify a bus that a child was deleted + * + * Called at the beginning of device_delete_child() to allow the parent + * to teardown any bus-specific state for the child. + * + * @param _dev the device whose child is being deleted + * @param _child the child device which is being deleted + */ +METHOD void child_deleted { + device_t _dev; + device_t _child; +}; + +/** * @brief Notify a bus that a child was detached * * Called after the child's DEVICE_DETACH() method to allow the parent -- John Baldwin