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=