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

[Warner Losh]

On Jun 25, 2013, at 9:51 AM, John Baldwin wrote:

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

I'd rather have that done at the interrupt controller level, which sadly we don't model and put most, but not quite all, of the functionality in the nexus driver. My experience is a bit colored from the PC Card and CardBus stuff, since there we intercepted the setup_intr calls and did the interrupt pass through directly for the child to cope with the sudden removal cases where the bridge driver know the card was gone, so the interrupt was dispatched to it, and only further dispatched to the child if the bridge thought it was still there. I'd assumed we'd need to that for proper support of hot-plug PCIe (including surprise removal support), but since we don't have that, I guess the PCI code just passes everything to the nexus.

I think that's a long way of saying that this is a good path, and we may have code on x86 that could benefit from it now that's doing ad-hoc things... I'd love to see this next step, and the current patch you have is sufficient for the  more constrained problem it is trying to solve.

Warner