Date: Thu, 31 May 2012 14:59:42 -0400 From: John Baldwin <jhb@freebsd.org> To: Warner Losh <imp@bsdimp.com> Cc: Norbert Koch <nkoch@demig.de>, freebsd-hackers@freebsd.org, Warner Losh <imp@freebsd.org> Subject: Re: bus device/ivars Message-ID: <201205311459.42538.jhb@freebsd.org> In-Reply-To: <2A30119B-0C5E-43A9-9B26-11FCF6685999@bsdimp.com> References: <4FC729A2.30205@demig.de> <201205311154.15062.jhb@freebsd.org> <2A30119B-0C5E-43A9-9B26-11FCF6685999@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201205311459.42538.jhb>