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

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

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

True.

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

Yes. that's true. However, until we have something better...

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

I'm OK doing this as a separate step, I guess...

Warner
_______________________________________________
[email protected] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus
To unsubscribe, send any mail to "[email protected]"