[PATCH] Change the PCI bus driver to free resources leaked by drivers
John Baldwin
jhb at freebsd.org
Tue Jun 25 17:05:51 UTC 2013
On Tuesday, June 25, 2013 10:15:00 am Warner Losh wrote:
>
> On Jun 25, 2013, at 6:37 AM, John Baldwin wrote:
>
> > On Tuesday, June 25, 2013 12:43:35 am Warner Losh wrote:
> >>
> >> On Jun 24, 2013, at 2:59 PM, John Baldwin wrote:
> >>
> >>> Currently our driver model trusts drivers to DTRT and properly release any
> >>> resources they allocated during probe() and attach(). I've added a new
> >>> resource_list() helper method to release active resources on a resource
> >>> list and used this to write a pci_child_detached() which cleans up any
> >>> active resources when a device fails to probe or a driver finishes
> >>> detach. It also fixes an issue where we did not power down devices when
> >>> the driver was detached (e.g. via kldunload). I've tested the resource
> >>> bits by writing a dummy driver that intentionally attached to an unattached
> >>> device and leaked a memory BAR and verified that the bus warned about the
> >>> leak and cleaned it up.
> >>>
> >>> http://www.FreeBSD.org/~jhb/patches/pci_clean_detach.patch
> >>
> >> I think most of pci_child_detached() could be a generic thing (except for the
> >> weird interaction with the msi-wart). This is likely fixable.
> >
> > The existing design we've gone with for this sort of thing is to provide
> > resource_list helpers, but let each bus driver decide which types of
> > resources it manages (see bus_print_child). Also, I think the order
> > matters (interrupts before memory & I/O). One thing the patch doesn't do
> > currently is explicitly list the resource ranges being freed.
>
> Yea, if ordering didn't matter, freeing them all would be a snap...
>
> >> We don't tear down any interrupt handlers that the device established. This
> >> is fixable, but the PCI bus would need to start tracking interrupts that are
> >> established...
> >
> > Eh, that is the part I don't like. That would be a lot of non-PCI specific
> > crap in the PCI bus driver.
>
> I'm not sure I understand this objection. We do (or did at one time) this sort
> of thing for the cardbus code and it found a few bugs in a couple of drivers...
I would rather solve this problem more generically so that if we want to add
a child_detached method for other buses like ACPI then they can just use a
library call (e.g. a bus_revoke_intr()) instead of having to do all the same
tracking themselves. The "right" place for this seems to be in whatever is
providing the IRQ resources and handles the actual bus_setup_intr calls to
create cookies, etc. On x86 this is the nexus. On other platforms it may
be in a nexus-like driver. I can work at prototyping something for review as
a next step.
--
John Baldwin
More information about the freebsd-new-bus
mailing list