Date: Tue, 25 Jun 2013 08:37:23 -0400 From: John Baldwin <jhb@freebsd.org> To: Warner Losh <imp@bsdimp.com> 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: <201306250837.24093.jhb@freebsd.org> In-Reply-To: <9886E26E-767F-4136-9A42-B7E38DBFE170@bsdimp.com> References: <201306241659.15119.jhb@freebsd.org> <9886E26E-767F-4136-9A42-B7E38DBFE170@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201306250837.24093.jhb>