From patchwork Wed Nov 6 18:15:58 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Bjorn Helgaas X-Patchwork-Id: 3148671 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 F348EBEEB2 for ; Wed, 6 Nov 2013 18:16:09 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 9F0D320503 for ; Wed, 6 Nov 2013 18:16:08 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 46EB52050E for ; Wed, 6 Nov 2013 18:16:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754443Ab3KFSQF (ORCPT ); Wed, 6 Nov 2013 13:16:05 -0500 Received: from mail-ie0-f173.google.com ([209.85.223.173]:48398 "EHLO mail-ie0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751716Ab3KFSQC (ORCPT ); Wed, 6 Nov 2013 13:16:02 -0500 Received: by mail-ie0-f173.google.com with SMTP id u16so17702259iet.4 for ; Wed, 06 Nov 2013 10:16:01 -0800 (PST) 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:content-transfer-encoding :in-reply-to:user-agent; bh=blqrCDgTMpxC0014OYLDS2szhYWqKLfQV3JINnbDiqU=; b=OlIFayrk0EjO/8jxiK3NqJrYLG8CVJG6SlCTaHYXs2sKpAqWEpN2it4EAa9RciYJP9 s7E9yKAS3e8MeW931xSDSDixRxaHvwiUjgRIR4yer6DFMUgVxPvs1IBS4h+fTu8JjzKD oqfuvwP19dSu8LPn1Ohsagon3BfJSLOqT7ySg3v0jxRd28L6DUB35GaMpw6PD46bzBsI xDdN0Jr1h7ELcrICWoZhJDVEfnLoVi8XbLcHBIBaDYb4ueCghyQe2D58ledg6Wrz51ZZ HZ1CT8GWhORVS1TAmVYRyqUS8Nb/KiUNRziQ8ddGjGHSDTYzBfIcGVI3u4C3FfGpe4Ar r6zg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition :content-transfer-encoding:in-reply-to:user-agent; bh=blqrCDgTMpxC0014OYLDS2szhYWqKLfQV3JINnbDiqU=; b=mH4Cf9PI1LM1jqE8VktFruiCppJBumE4jg20DFKGAvxcKo2AAcE34OZb1r3jNgK/Ie pD19GnpPt4HVPfDCg2hto1pZi+wbVIyLSvbRfUegLstHS9c6OAui/g3qyizoktu8rgJM b66evD5gSjaMKY9Zwhz7e7W9XJh0l9ysNnQVjcuayENGKgN6JWHDVTOFSDsjFwK4MAZa a3EE5tJ5jNclg153K3FNLZSV/iBbPVygjS8Bbxo9TDLfqPGuoWbxCSjdFdO9kG9ml73m h+7SB9Yuo/Ssm4alkB7dlhcBO82hG9AcLlPPEaIPr9WxeBDnjPVGh6foIz5k9QGeUwVK XMdA== X-Gm-Message-State: ALoCoQmZTvSz/cizQDlaxGyG2Z3GTOrIdQ8iSfM4wxLNx7g1Oq5JLr6Isd3ywe5u3eL8xpSkr+pl+NHIi2i+iSEJ6yM/+T+lR54jwwUsJCeQu9TfkUFwSbYbwv4i+5t/Z5ya2uBtBM3ShxdC0zcs5/OLMzgsC0kKdaaL9jDaYU48UvwjOBkFlhAMUvXBtriNpIPdxh6XV3/DTeZdQlQoVM7J6vqGwa08qw== X-Received: by 10.50.45.73 with SMTP id k9mr21314966igm.38.1383761761451; Wed, 06 Nov 2013 10:16:01 -0800 (PST) Received: from google.com ([172.29.124.25]) by mx.google.com with ESMTPSA id ft2sm15269004igb.5.2013.11.06.10.16.00 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Wed, 06 Nov 2013 10:16:00 -0800 (PST) Date: Wed, 6 Nov 2013 11:15:58 -0700 From: Bjorn Helgaas To: Yinghai Lu Cc: "linux-pci@vger.kernel.org" , Wei Yang , Nishank Trivedi Subject: Re: [PATCH] PCI: Use pci_is_root_bus() to check for root bus Message-ID: <20131106181558.GA14444@google.com> References: <20131105232903.3790.8738.stgit@bhelgaas-glaptop.roam.corp.google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org X-Spam-Status: No, score=-3.6 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FSL_HELO_FAKE, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_DKIM_INVALID, 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 [+cc Nishank] On Tue, Nov 05, 2013 at 07:39:10PM -0800, Yinghai Lu wrote: > On Tue, Nov 5, 2013 at 3:29 PM, Bjorn Helgaas wrote: > > pci_enable_device_flags() and pci_enable_bridge() assume that > > "bus->self == NULL" means that "bus" is a root bus. That assumption is > > false; see 2ba29e270e97 ("PCI: Use pci_is_root_bus() to check for root > > bus") for details. > > > > This patch changes them to use pci_is_root_bus() instead. > > > > Signed-off-by: Bjorn Helgaas > > --- > > drivers/pci/pci.c | 9 ++++----- > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > index ac40f90..de65bf7 100644 > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -1150,10 +1150,8 @@ static void pci_enable_bridge(struct pci_dev *dev) > > { > > int retval; > > > > - if (!dev) > > - return; > > - > > May need to keep this checking. > > virtual bus (for sriov virtual function) could have bus->self == NULL. Ah, you're right, thanks!  When "dev" is a VF, "dev->bus->self" may be NULL.  If we call pci_enable_device() on a VF, "dev->bus" is not a root bus, so we'll call pci_enable_bridge(dev->bus->self) when "dev->bus->self" is NULL, so we'll dereference a NULL pointer. But currently we just return when "dev == NULL", and I think that masks a deeper problem: what if we enable IOV but never call pci_enable_device(PF)? In that case, the upstream bridge may not be enabled, and when we call pci_enable_device(VF), we'll do nothing, so the upstream bridge remains disabled. I didn't see anywhere the core requires the PF to be enabled before IOV is enabled.  I checked all the current callers of pci_enable_sriov(), and they all call pci_enable_device(PF) first.  But I don't think anything *prevents* a driver from calling pci_enable_sriov(PF) without doing pci_enable_device(PF). Or the PCI core could enable VFs even with no driver attached, e.g., if we called pci_enable_sriov() from sriov_numvfs_store() (for the sysfs "sriov_numvfs" file).  We talked about that a bit at the PCI miniconf in New Orleans.  IIRC, there are some Cisco boxes where the firmware enables IOV, so the VFs are enabled before Linux even sees the PF, and a driver could bind to a VF and do pci_enable_device(VF) even if there's no PF driver at all.  If that VF is on a virtual bus, we won't enable the upstream bridge, and the VF may not work. What do you think of the patches below? > > - pci_enable_bridge(dev->bus->self); > > + if (!pci_is_root_bus(dev->bus)) > > + pci_enable_bridge(dev->bus->self); > > > > if (pci_is_enabled(dev)) { > > if (!dev->is_busmaster) > > @@ -1188,7 +1186,8 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags) > > if (atomic_inc_return(&dev->enable_cnt) > 1) > > return 0; /* already enabled */ > > > > - pci_enable_bridge(dev->bus->self); > > + if (!pci_is_root_bus(dev->bus)) > > + pci_enable_bridge(dev->bus->self); > > > > /* only skip sriov related */ > > for (i = 0; i <= PCI_ROM_RESOURCE; i++) > > commit dfb66fee4715c747a94abd45c20fbe302b10e49c Author: Bjorn Helgaas Date: Wed Nov 6 10:11:48 2013 -0700 PCI: Add pci_upstream_bridge() This adds a pci_upstream_bridge() interface to find the PCI-to-PCI bridge upstream from a device. If the device is on a root bus, i.e., the upstream bridge is a host bridge instead of a PCI bridge, this returns NULL. If the device is a VF, this returns the bridge upstream from the PF corresponding to the VF. This is important for VFs on virtual buses, where "dev->bus->self == NULL". 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/include/linux/pci.h b/include/linux/pci.h index d3a888a..e09d19a 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -480,6 +480,18 @@ static inline bool pci_is_root_bus(struct pci_bus *pbus) return !(pbus->parent); } +static inline struct pci_dev *pci_upstream_bridge(struct pci_dev *dev) +{ + if (pci_is_root_bus(dev->bus)) + return NULL; + + dev = pci_physfn(dev); + if (pci_is_root_bus(dev->bus)) + return NULL; + + return dev->bus->self; +} + #ifdef CONFIG_PCI_MSI static inline bool pci_dev_msi_enabled(struct pci_dev *pci_dev) { commit 4e5415f02e32c85e902cd9692eab18200e14b347 Author: Bjorn Helgaas Date: Wed Nov 6 10:00:51 2013 -0700 PCI: Enable upstream bridges even for VFs on virtual buses Previously we enabled the upstream PCI-to-PCI bridge only when "dev->bus->self != NULL". In the case of a VF on a virtual bus, where "bus->self == NULL", we didn't enable the upstream bridge. This fixes that by enabling the upstream bridge of the PF corresponding to the VF. Signed-off-by: Bjorn Helgaas diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index ac40f90..744dc26 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1150,10 +1150,8 @@ static void pci_enable_bridge(struct pci_dev *dev) { int retval; - if (!dev) - return; - - pci_enable_bridge(dev->bus->self); + if (!pci_is_root_bus(dev->bus)) + pci_enable_bridge(pci_upstream_bridge(dev)); if (pci_is_enabled(dev)) { if (!dev->is_busmaster) @@ -1188,7 +1186,8 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags) if (atomic_inc_return(&dev->enable_cnt) > 1) return 0; /* already enabled */ - pci_enable_bridge(dev->bus->self); + if (!pci_is_root_bus(dev->bus)) + pci_enable_bridge(pci_upstream_bridge(dev)); /* only skip sriov related */ for (i = 0; i <= PCI_ROM_RESOURCE; i++)