[PATCH] Change the PCI bus driver to free resources leaked by drivers

[John Baldwin]

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.

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

> We don't tear down any timers the device may have setup. This likely isn't
> fixable.

Nor taskqueues, dma tags, static DMA allocations, etc.  There are all sorts
of things that new-bus is not aware of.

> We likely don't want to tear down interrupts in bus_deactivate_resource,
> since I think it is theoretically legal to deactivate an interrupt resource
> without tearing down the interrupt. But maybe it shouldn't be...

I have long thought that bus_setup_intr/bus_teardown_intr was a gross hack
in terms of the API.  It's a necessary hack (interrupts have more metadata
when they are "active"), but still gross.

If we want to solve the interrupt problem I do think it belongs in the nexus
layer (at least on x86.. whoever provides IRQ resources should be the
maintain the state).  You could easily have something that associated a
device_t with each interrupt handler and then add a new method along the
lines of:

bus_revoke_intr(device_t child, struct resource *r)

Which did a teardown of all handlers on resource 'r' associated with device
'child'.

However, I would probably prefer to do that sort of thing as a next step.

-- 
John Baldwin