From owner-freebsd-new-bus@FreeBSD.ORG Tue Jun 25 17:05:51 2013 Return-Path: Delivered-To: new-bus@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 3694AAA7; Tue, 25 Jun 2013 17:05:51 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) by mx1.freebsd.org (Postfix) with ESMTP id 133D61895; Tue, 25 Jun 2013 17:05:51 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id E32C2B964; Tue, 25 Jun 2013 13:05:47 -0400 (EDT) From: John Baldwin To: Warner Losh Subject: Re: [PATCH] Change the PCI bus driver to free resources leaked by drivers Date: Tue, 25 Jun 2013 11:51:01 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p25; KDE/4.5.5; amd64; ; ) References: <201306241659.15119.jhb@freebsd.org> <201306250837.24093.jhb@freebsd.org> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201306251151.01717.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Tue, 25 Jun 2013 13:05:48 -0400 (EDT) Cc: Warner Losh , new-bus@freebsd.org X-BeenThere: freebsd-new-bus@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: FreeBSD's new-bus architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 Jun 2013 17:05:51 -0000 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. -- John Baldwin