From owner-freebsd-new-bus@FreeBSD.ORG Mon Jun 24 21:18:21 2013 Return-Path: Delivered-To: new-bus@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 7CC05A3B; Mon, 24 Jun 2013 21:18:21 +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 5C6CC107E; Mon, 24 Jun 2013 21:18:21 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 95905B918; Mon, 24 Jun 2013 17:18:20 -0400 (EDT) From: John Baldwin To: new-bus@freebsd.org Subject: [PATCH] Change the PCI bus driver to free resources leaked by drivers Date: Mon, 24 Jun 2013 16:59:14 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p25; KDE/4.5.5; amd64; ; ) MIME-Version: 1.0 Content-Type: Text/Plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Message-Id: <201306241659.15119.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Mon, 24 Jun 2013 17:18:20 -0400 (EDT) Cc: Warner Losh 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: Mon, 24 Jun 2013 21:18:21 -0000 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 -- John Baldwin From owner-freebsd-new-bus@FreeBSD.ORG Tue Jun 25 02:01:47 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 971DFCCA; Tue, 25 Jun 2013 02:01:47 +0000 (UTC) (envelope-from brooks@lor.one-eyed-alien.net) Received: from lor.one-eyed-alien.net (lor.one-eyed-alien.net [69.66.77.232]) by mx1.freebsd.org (Postfix) with ESMTP id DE00C1B3D; Tue, 25 Jun 2013 02:01:46 +0000 (UTC) Received: from lor.one-eyed-alien.net (localhost [127.0.0.1]) by lor.one-eyed-alien.net (8.14.5/8.14.5) with ESMTP id r5P21kw3072364; Mon, 24 Jun 2013 21:01:46 -0500 (CDT) (envelope-from brooks@lor.one-eyed-alien.net) Received: (from brooks@localhost) by lor.one-eyed-alien.net (8.14.7/8.14.7/Submit) id r5P21knQ072363; Mon, 24 Jun 2013 21:01:46 -0500 (CDT) (envelope-from brooks) Date: Mon, 24 Jun 2013 21:01:46 -0500 From: Brooks Davis To: John Baldwin Subject: Re: [PATCH] Change the PCI bus driver to free resources leaked by drivers Message-ID: <20130625020146.GD70873@lor.one-eyed-alien.net> References: <201306241659.15119.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="4zI0WCX1RcnW9Hbu" Content-Disposition: inline In-Reply-To: <201306241659.15119.jhb@freebsd.org> User-Agent: Mutt/1.5.21 (2010-09-15) 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 02:01:47 -0000 --4zI0WCX1RcnW9Hbu Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jun 24, 2013 at 04:59:14PM -0400, John Baldwin wrote: > Currently our driver model trusts drivers to DTRT and properly release an= y=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 unattach= ed > device and leaked a memory BAR and verified that the bus warned about the > leak and cleaned it up. Looks good in a quick review. -- Brooks --4zI0WCX1RcnW9Hbu Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (FreeBSD) iD4DBQFRyPoJXY6L6fI4GtQRApuZAKCGpXOYhJ0ikxGJyphcaD0GX7e7qgCYioGO IbYycfp8cu+VzEz7Cwt06Q== =20CY -----END PGP SIGNATURE----- --4zI0WCX1RcnW9Hbu-- From owner-freebsd-new-bus@FreeBSD.ORG Tue Jun 25 04:43:38 2013 Return-Path: Delivered-To: new-bus@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id C5EFFE9C for ; Tue, 25 Jun 2013 04:43:38 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from mail-ie0-x22f.google.com (mail-ie0-x22f.google.com [IPv6:2607:f8b0:4001:c03::22f]) by mx1.freebsd.org (Postfix) with ESMTP id 99B9511F0 for ; Tue, 25 Jun 2013 04:43:38 +0000 (UTC) Received: by mail-ie0-f175.google.com with SMTP id a13so27063732iee.34 for ; Mon, 24 Jun 2013 21:43:38 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=sender:subject:mime-version:content-type:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to:x-mailer :x-gm-message-state; bh=8ms06ediVvbfuXJ9EqZERaeGKonO90E7VKAIRVVkCE8=; b=aDmu8OS0Pi9eQxuXbwTJthvqY2gfInBA1gpEo2pIWXmh6clmAYyo4WmW/n70YNVD1r 01qoEZRjVJ4kvLK5G929pRwQkoWl5p32uBQDByyf/BtgvOjwq86M+RprhmAbQ7chj6Md GAYDTPMFJ++brGv7wNbt3eNboMWBiLrQqgqHJRkssCnStSeYj5R0DGctcxpBSgw8Ny7P UmHv3RKQXpEIjDMQtf8hmZ1KWOJMEsqcLdoHgOrFad/Qe6redogFdszC8vxxrL7qKaEZ 4lsjCKG4kuS7HSJhozke2PbUoA6Ke7SiTiEcZJc4PhJkJkRZrQvsk5vjtMfMThGSpo1X tM4w== X-Received: by 10.50.47.105 with SMTP id c9mr7383288ign.50.1372135418248; Mon, 24 Jun 2013 21:43:38 -0700 (PDT) Received: from 53.imp.bsdimp.com (50-78-194-198-static.hfc.comcastbusiness.net. [50.78.194.198]) by mx.google.com with ESMTPSA id fu2sm1879808igb.3.2013.06.24.21.43.36 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Mon, 24 Jun 2013 21:43:37 -0700 (PDT) Sender: Warner Losh Subject: Re: [PATCH] Change the PCI bus driver to free resources leaked by drivers Mime-Version: 1.0 (Apple Message framework v1085) Content-Type: text/plain; charset=us-ascii From: Warner Losh In-Reply-To: <201306241659.15119.jhb@freebsd.org> Date: Mon, 24 Jun 2013 22:43:35 -0600 Content-Transfer-Encoding: quoted-printable Message-Id: <9886E26E-767F-4136-9A42-B7E38DBFE170@bsdimp.com> References: <201306241659.15119.jhb@freebsd.org> To: John Baldwin X-Mailer: Apple Mail (2.1085) X-Gm-Message-State: ALoCoQkaT2ojwsCMmWpdbaaBJodvwlGz7ZgsPi8rNl56h3nPUplJsAsoY8n1i8pS8Jp0Yhv7A8hC 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 04:43:38 -0000 On Jun 24, 2013, at 2:59 PM, John Baldwin wrote: > 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 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. 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... We don't tear down any timers the device may have setup. This likely = isn't fixable. 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... Warner From owner-freebsd-new-bus@FreeBSD.ORG Tue Jun 25 13:52:47 2013 Return-Path: Delivered-To: new-bus@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 65F116E9; Tue, 25 Jun 2013 13:52:47 +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 4350C1C50; Tue, 25 Jun 2013 13:52:47 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 863E0B964; Tue, 25 Jun 2013 09:52:46 -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 08:37:23 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p25; KDE/4.5.5; amd64; ; ) References: <201306241659.15119.jhb@freebsd.org> <9886E26E-767F-4136-9A42-B7E38DBFE170@bsdimp.com> In-Reply-To: <9886E26E-767F-4136-9A42-B7E38DBFE170@bsdimp.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201306250837.24093.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Tue, 25 Jun 2013 09:52:46 -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 13:52:47 -0000 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 From owner-freebsd-new-bus@FreeBSD.ORG Tue Jun 25 14:15:05 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 403C0F43 for ; Tue, 25 Jun 2013 14:15:05 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from mail-ie0-x22c.google.com (mail-ie0-x22c.google.com [IPv6:2607:f8b0:4001:c03::22c]) by mx1.freebsd.org (Postfix) with ESMTP id 128611D58 for ; Tue, 25 Jun 2013 14:15:05 +0000 (UTC) Received: by mail-ie0-f172.google.com with SMTP id 16so28009498iea.17 for ; Tue, 25 Jun 2013 07:15:04 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=sender:subject:mime-version:content-type:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to:x-mailer :x-gm-message-state; bh=Ys/4NTdsJodB+8tG1qBOklvHsO1hrOGoLPcRcCooKQ4=; b=NbKhsmE1zUnxsC14jaUhUhLCVGxh0IlGV2YpMckrQBpatXtNFuVMzMuZiyI0CywTAG cUNyM5/WOAGc+0KUZTyKS7Ztb+/gTm6wyiPIUSDsfh2qT0C+/wGE2S67/6zVTwXWyGrg v93pDD4egFzsv9y1JYKv3wA8yFaAg6ZTWYYUC/MjS0Oj5QDDxfj8qMuxBHSO51VsiQ7e wxJ1kIDhNBq6pto9WCLbq2Dzm1qx+Kni54GMxC0cP1W3xl/BZGvrRPlhUqn4xJZtVOPj trSHY8wxDOtY9Ww4CH2wSaLUxu67vx0mOU4+8GY+FDAs8JSwCoXadu6NP8v70Mzcyjtt mwRA== X-Received: by 10.43.138.69 with SMTP id ir5mr10924237icc.90.1372169704784; Tue, 25 Jun 2013 07:15:04 -0700 (PDT) Received: from [10.0.0.53] (50-78-194-198-static.hfc.comcastbusiness.net. [50.78.194.198]) by mx.google.com with ESMTPSA id ir8sm3420141igb.6.2013.06.25.07.15.03 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 25 Jun 2013 07:15:03 -0700 (PDT) Sender: Warner Losh Subject: Re: [PATCH] Change the PCI bus driver to free resources leaked by drivers Mime-Version: 1.0 (Apple Message framework v1085) Content-Type: text/plain; charset=us-ascii From: Warner Losh In-Reply-To: <201306250837.24093.jhb@freebsd.org> Date: Tue, 25 Jun 2013 08:15:00 -0600 Content-Transfer-Encoding: quoted-printable Message-Id: References: <201306241659.15119.jhb@freebsd.org> <9886E26E-767F-4136-9A42-B7E38DBFE170@bsdimp.com> <201306250837.24093.jhb@freebsd.org> To: John Baldwin X-Mailer: Apple Mail (2.1085) X-Gm-Message-State: ALoCoQnrxunhf0XqsvZHEt5jTmxyK3S9Z0FbZ+/zusz99/yO0qAFndtMBqg8dnpd3KChMHw3xPdN 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 14:15:05 -0000 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= 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 From owner-freebsd-new-bus@FreeBSD.ORG Tue Jun 25 17:32:52 2013 Return-Path: Delivered-To: new-bus@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 3A9ABE4 for ; Tue, 25 Jun 2013 17:32:52 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from mail-ie0-x235.google.com (mail-ie0-x235.google.com [IPv6:2607:f8b0:4001:c03::235]) by mx1.freebsd.org (Postfix) with ESMTP id 0CFC819C2 for ; Tue, 25 Jun 2013 17:32:52 +0000 (UTC) Received: by mail-ie0-f181.google.com with SMTP id x12so28866734ief.12 for ; Tue, 25 Jun 2013 10:32:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=sender:subject:mime-version:content-type:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to:x-mailer :x-gm-message-state; bh=Jphf5E6p47/6BFu5c4Ja4SOjVvkd+CEtOSYu5Yw7ezM=; b=AIUn3z+UYx9NPg4IkkJRFKyJRJK9mqeDTIYmNQcup2I6ZSE3SENW1/rUerpUAmGLX0 wyGlE8gnrH3Rx9o/Q9IMkIeb8079LFcvmMlTrIsFJeBSOLl9nmt3LJPhxnWjhfWIBUiY UWPrbZAtyVPAs2d6mINr6QqkQH/KpuCzclc9Uzgyflv8K+Kd05VX6UM8zbEMpR3wiTiI S4BlMVmQSB0TBdl8KKDSkSAJVEHpyzK7RS7ziKMdGlyHM7FDHMpeulAN0pRoeYfs1UMF mKlxPiGpTe2lLeW5YD1oG5PuBSjay4M7xfd6/uiiYfHSIts0tjFdb5Sdpp8sEp/+hqWA zPYw== X-Received: by 10.50.225.67 with SMTP id ri3mr9231111igc.35.1372181571746; Tue, 25 Jun 2013 10:32:51 -0700 (PDT) Received: from monkey-bot.int.fusionio.com ([209.117.142.2]) by mx.google.com with ESMTPSA id kj5sm4128340igb.7.2013.06.25.10.32.49 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 25 Jun 2013 10:32:50 -0700 (PDT) Sender: Warner Losh Subject: Re: [PATCH] Change the PCI bus driver to free resources leaked by drivers Mime-Version: 1.0 (Apple Message framework v1085) Content-Type: text/plain; charset=us-ascii From: Warner Losh In-Reply-To: <201306251151.01717.jhb@freebsd.org> Date: Tue, 25 Jun 2013 11:32:47 -0600 Content-Transfer-Encoding: quoted-printable Message-Id: References: <201306241659.15119.jhb@freebsd.org> <201306250837.24093.jhb@freebsd.org> <201306251151.01717.jhb@freebsd.org> To: John Baldwin X-Mailer: Apple Mail (2.1085) X-Gm-Message-State: ALoCoQmJyx0yy5ulHt1ysOeuYW6Te7aIKtoPjFWB6bkJWsS5ePdUYeB8s8TVF00ja4J8gt6XcK07 Cc: 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:32:52 -0000 On Jun 25, 2013, at 9:51 AM, John Baldwin wrote: > On Tuesday, June 25, 2013 10:15:00 am Warner Losh wrote: >>=20 >> On Jun 25, 2013, at 6:37 AM, John Baldwin wrote: >>=20 >>> 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. >>=20 >> Yea, if ordering didn't matter, freeing them all would be a snap... >>=20 >>>> 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. >>=20 >> 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... >=20 > 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=