Date: Tue, 25 Jun 2013 08:15:00 -0600 From: Warner Losh <imp@bsdimp.com> To: John Baldwin <jhb@freebsd.org> Cc: Warner Losh <imp@freebsd.org>, new-bus@freebsd.org Subject: Re: [PATCH] Change the PCI bus driver to free resources leaked by drivers Message-ID: <BC40848E-5C75-451E-9B06-70F6B34E7950@bsdimp.com> In-Reply-To: <201306250837.24093.jhb@freebsd.org> References: <201306241659.15119.jhb@freebsd.org> <9886E26E-767F-4136-9A42-B7E38DBFE170@bsdimp.com> <201306250837.24093.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Jun 25, 2013, at 6:37 AM, John Baldwin wrote: > On Tuesday, June 25, 2013 12:43:35 am Warner Losh wrote: >>=20 >> On Jun 24, 2013, at 2:59 PM, John Baldwin wrote: >>=20 >>> Currently our driver model trusts drivers to DTRT and properly = release any=20 >>> 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. >>>=20 >>> http://www.FreeBSD.org/~jhb/patches/pci_clean_detach.patch >>=20 >> 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. >=20 > 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... >=20 > 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. >=20 > 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... >=20 > 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: >=20 > bus_revoke_intr(device_t child, struct resource *r) >=20 > Which did a teardown of all handlers on resource 'r' associated with = device > 'child'. >=20 > 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=
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?BC40848E-5C75-451E-9B06-70F6B34E7950>