From patchwork Tue Jul 23 15:56:45 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bjorn Helgaas X-Patchwork-Id: 2832060 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: X-Original-To: patchwork-linux-pci@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 82A9AC0319 for ; Tue, 23 Jul 2013 15:56:53 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 291AF2021F for ; Tue, 23 Jul 2013 15:56:52 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 47E9520217 for ; Tue, 23 Jul 2013 15:56:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933256Ab3GWP4t (ORCPT ); Tue, 23 Jul 2013 11:56:49 -0400 Received: from mail-ob0-f175.google.com ([209.85.214.175]:33672 "EHLO mail-ob0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932180Ab3GWP4s (ORCPT ); Tue, 23 Jul 2013 11:56:48 -0400 Received: by mail-ob0-f175.google.com with SMTP id tb18so1307552obb.6 for ; Tue, 23 Jul 2013 08:56:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=8AAsqSis84KYqb+IVmvfB6lVgqVjpwO0kVbW01JBiZg=; b=Z1q/eqDm35HsQfpcy3mdDynlXbOh9ZdAnCnamEGqnPdxUfZ3xv5lalmpq3nZOzU68M MTah8TBxZhmr5a9yjIvokbQyXNobC29nkUpzEJcqmdohWEI2xGR7Ko7UWk7TrBb1ryZ8 CqZGqpnF5CiPzz10153cGsbLc7syB1WdO4DbCZCz1fbfIXuuKLTbR9quuHHMOx3MSPZF DyNSb6n62jI2wXL0hJ5wGnDjp9h42AvEHsqjiXnn2wx4M1lNeB55EVpRsF/E6pWaQ8g7 imTauIcxYP4Hf9ksOj1VIYBxfD0XNnVeDlqIEBkxs97vpT+RrbaNlrfs/y3oW8/JZF7j R2QQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent :x-gm-message-state; bh=8AAsqSis84KYqb+IVmvfB6lVgqVjpwO0kVbW01JBiZg=; b=lOrkeY+G3LYVqnC5v28MROq79pfTFY9vqMG3gpiFpmo3YiYii89Pi7dUg98AU3KukK HIRgIxKRfcv851MF/qd/y5QrjSheUPsgdbjCVlzpWpdj/Wse/4gz3gjzw050vKm3DQrG 7vkC+t0Kuv2/HfimiiFDNm2UmBgp1V4IDDwJx8YfhpvEt4Zs3h5r9NKk9WTlfu5Ylr5+ WEVSVn3685BadqawC7o5BOJexVSqtvh1hXj4N1xB/3N8afyaCSAWBKROuHMbXDDWSwNg AJkVcgjw7uVaVrJrmLRSNRJ9i6XekGdpyDMzJRKnS5VlP2YY+jOycPZsKMIZ78rtmCT6 Todw== X-Received: by 10.60.52.78 with SMTP id r14mr32182681oeo.9.1374595007377; Tue, 23 Jul 2013 08:56:47 -0700 (PDT) Received: from google.com ([172.29.125.35]) by mx.google.com with ESMTPSA id rs1sm40938197obb.12.2013.07.23.08.56.46 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Tue, 23 Jul 2013 08:56:46 -0700 (PDT) Date: Tue, 23 Jul 2013 09:56:45 -0600 From: Bjorn Helgaas To: Yinghai Lu Cc: "linux-pci@vger.kernel.org" , Yijing Wang , Jiang Liu Subject: Re: [PATCH] PCI: Separate stop and remove devices in pciehp Message-ID: <20130723155645.GA18422@google.com> References: <1374261258-23036-1-git-send-email-yinghai@kernel.org> <1374261258-23036-2-git-send-email-yinghai@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Gm-Message-State: ALoCoQmasfKqtpZnP4K1PDtl5e25Ip92PZLuUc3lwdK5myNKv52Epfz9p7zeu5/xq0AD6H0jsI0qYeBTZcK5/9t3JU3CjT77tbOIaUSDZ6JhcCZPTOAOGmET77WfES48QbMih8bhEEghLSq10jNZPWOsHYuIj+xuZbnvvu4vYJnkKWG5q841RTFO1YSYlY+533TsIik4OajDUOS8SE+PKnLUppHVSZUwsw== Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_DKIM_INVALID, T_FRT_BEFORE, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, Jul 22, 2013 at 07:32:06PM -0700, Yinghai Lu wrote: > On Mon, Jul 22, 2013 at 2:23 PM, Bjorn Helgaas wrote: > > Evidently this is really part of a series, but you didn't label it > > that way. It looks like this applies on top of your "PCI: Fix hotplug > > remove with sriov again" patch? > > Yes. > > that one need be back ported to 3.9 and later. > > this one need to be for 3.11 final, as Jiang's patch is in 3.11-rc0. > > > > > On Fri, Jul 19, 2013 at 1:14 PM, Yinghai Lu wrote: > >> After commit dc087f2f6a2925e81831f3016b9cbb6e470e7423 > >> (PCI: Simplify IOV implementation and fix reference count races) > >> VF need to be removed via virtfn_remove to make sure ref to PF > >> is put back. > > > > I'm sure this makes sense, but it needs background. I haven't figured > > out exactly what the problem is. You're describing the lowest-level > > details, but not the overall picture that would make it understandable > > to the rest of us. > > Overall, after we reversely loop the bus_devices, VF get stop and removed, > but fail to release ref to PF, and prevent PF to be removed and freed. > > > > >> So we can not call stop_and_remove for VF before PF, that will > >> make VF get removed directly before PF's driver try to use > >> virtfn_remove to do it. > >> > >> Solution is separating stop and remove in two iterations. > >> > >> In first iteration, we stop VF driver at first with iterating devices > >> reversely, and later during stop PF driver, disable_sriov will use > >> virtfn_remove to remove VFs. > >> > >> Also some driver (like mlx4_core) need VF's driver get stopped before PF. > > > > If this is relevant, please give a pointer to the mlx4_core code that > > requires VFs to be stopped before the PF. > > that is add-on benefits. > > drivers/net/ethernet/mellanox/mlx4/main.c:: > static void mlx4_remove_one(struct pci_dev *pdev) > { > struct mlx4_dev *dev = pci_get_drvdata(pdev); > struct mlx4_priv *priv = mlx4_priv(dev); > int p; > > if (dev) { > /* in SRIOV it is not allowed to unload the pf's > * driver while there are alive vf's */ > if (mlx4_is_master(dev)) { > if (mlx4_how_many_lives_vf(dev)) > printk(KERN_ERR "Removing PF when > there are assigned VF's !!!\n"); > } > > mlx4_how_many_lives_vf() is checking how VFs have driver loaded. > > > > >> To make it simple, separate VGA checking out and do that at first, > >> if there is VGA in the chain, do even try to stop or remove any device > >> under that bus. > > > > This part seems like it could be a separate patch. > > ok, will separate it out in next rev. > > > > >> Need this one for v3.11. > > > > OK, but why? We need a better explanation of what problem this fixes. > > It sounds like it fixes a reference counting problem in v3.11-rc1, > > but I don't know what the effect of that problem is. Maybe it means a > > device isn't freed when it should be? Maybe it means we can't add a > > new device after hot-removing an SR-IOV device? > > Yes. Can you include an example that shows the failure, like you did for the "Fix hotplug remove" patch? > ... > >> Index: linux-2.6/drivers/pci/remove.c > >> =================================================================== > >> --- linux-2.6.orig/drivers/pci/remove.c > >> +++ linux-2.6/drivers/pci/remove.c > >> @@ -56,7 +56,7 @@ void pci_remove_bus(struct pci_bus *bus) > >> } > >> EXPORT_SYMBOL(pci_remove_bus); > >> > >> -static void pci_stop_bus_device(struct pci_dev *dev) > >> +void pci_stop_bus_device(struct pci_dev *dev) > >> { > >> struct pci_bus *bus = dev->subordinate; > >> struct pci_dev *child, *tmp; > >> @@ -75,8 +75,9 @@ static void pci_stop_bus_device(struct p > >> > >> pci_stop_dev(dev); > >> } > >> +EXPORT_SYMBOL(pci_stop_bus_device); > >> > >> -static void pci_remove_bus_device(struct pci_dev *dev) > >> +void pci_remove_bus_device(struct pci_dev *dev) > >> { > >> struct pci_bus *bus = dev->subordinate; > >> struct pci_dev *child, *tmp; > >> @@ -92,6 +93,7 @@ static void pci_remove_bus_device(struct > >> > >> pci_destroy_dev(dev); > >> } > >> +EXPORT_SYMBOL(pci_remove_bus_device); > > > > I really don't want to export these two symbols unless we have to. > > Obviously this is the heart of your patch, so we probably *will* have > > to. > > Agree. > > > > > But maybe they can at least be confined to drivers/pci code, and not > > exported to modules? I don't think there's any reason pciehp needs to > > be a module. I was planning to merge a patch restricting it to be > > built-in this cycle anyway. > > sure, you can apply that patch (make pciehp to be built-in) at first. Below is the patch I intend to apply. We can easily do this for v3.12. But your patch needs to be in v3.11, so we'll have to figure out how to handle that. Maybe we can put the Kconfig change in v3.11, too. It should be low-risk because it doesn't add any new code paths that weren't in v3.10. Bjorn commit 04216ce0f2381090572142ebab28f63bae157d8d Author: Bjorn Helgaas Date: Thu Jun 27 10:16:19 2013 -0600 PCI: pciehp: Convert pciehp to be builtin only, not modular Convert pciehp to be builtin only, with no module option. Signed-off-by: Bjorn Helgaas --- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig index a82e70a..7958e59 100644 --- a/drivers/pci/pcie/Kconfig +++ b/drivers/pci/pcie/Kconfig @@ -14,15 +14,12 @@ config PCIEPORTBUS # Include service Kconfig here # config HOTPLUG_PCI_PCIE - tristate "PCI Express Hotplug driver" + bool "PCI Express Hotplug driver" depends on HOTPLUG_PCI && PCIEPORTBUS help Say Y here if you have a motherboard that supports PCI Express Native Hotplug - To compile this driver as a module, choose M here: the - module will be called pciehp. - When in doubt, say N. source "drivers/pci/pcie/aer/Kconfig"