From patchwork Thu Nov 7 03:00:54 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Wei Yang X-Patchwork-Id: 289142 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 60DB72C014B for ; Thu, 7 Nov 2013 14:01:06 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753585Ab3KGDBE (ORCPT ); Wed, 6 Nov 2013 22:01:04 -0500 Received: from e28smtp07.in.ibm.com ([122.248.162.7]:49126 "EHLO e28smtp07.in.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753571Ab3KGDBD (ORCPT ); Wed, 6 Nov 2013 22:01:03 -0500 Received: from /spool/local by e28smtp07.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 7 Nov 2013 08:31:00 +0530 Received: from d28dlp02.in.ibm.com (9.184.220.127) by e28smtp07.in.ibm.com (192.168.1.137) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Thu, 7 Nov 2013 08:30:57 +0530 Received: from d28relay02.in.ibm.com (d28relay02.in.ibm.com [9.184.220.59]) by d28dlp02.in.ibm.com (Postfix) with ESMTP id C047F394004D for ; Thu, 7 Nov 2013 08:30:33 +0530 (IST) Received: from d28av05.in.ibm.com (d28av05.in.ibm.com [9.184.220.67]) by d28relay02.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id rA730o5d30212188 for ; Thu, 7 Nov 2013 08:30:50 +0530 Received: from d28av05.in.ibm.com (localhost [127.0.0.1]) by d28av05.in.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id rA730uHe005020 for ; Thu, 7 Nov 2013 08:30:56 +0530 Received: from localhost (weiyang.cn.ibm.com [9.111.19.200]) by d28av05.in.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id rA730tZh004992; Thu, 7 Nov 2013 08:30:55 +0530 Date: Thu, 7 Nov 2013 11:00:54 +0800 From: Wei Yang To: Bjorn Helgaas Cc: Yinghai Lu , "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: <20131107030054.GA11245@weiyang.vnet.ibm.com> Reply-To: Wei Yang References: <20131105232903.3790.8738.stgit@bhelgaas-glaptop.roam.corp.google.com> <20131106181558.GA14444@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20131106181558.GA14444@google.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-MML: No X-Content-Scanned: Fidelis XPS MAILER x-cbid: 13110703-8878-0000-0000-000009961275 Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org On Wed, Nov 06, 2013 at 11:15:58AM -0700, Bjorn Helgaas wrote: >[+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? > Thanks Bjorn, this is really a potential problme. And your patches fix this problem. While I did a small change on the seconde one like this. Hope you like it :-) BTW, pci_enable_bridge() is only called in pci_enable_device_flags(). After change in these two patches, we pass a 'upstream bridge' to pci_enable_bridge(). I am not sure whether this 'upstream bridge' could be a VF? I took a look at the SPEC again, but not find clear clause. In case the 'upstream bridge' is always a PF, maybe we could simplize the logic in pci_enable_bridge(). While current logic is reasonable and clear. --- 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/pci.c b/drivers/pci/pci.c index bdd64b1..8d0ce48 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1153,7 +1153,7 @@ static void pci_enable_bridge(struct pci_dev *dev) if (!dev) return; - pci_enable_bridge(dev->bus->self); + pci_enable_bridge(pci_upstream_bridge(dev)); if (pci_is_enabled(dev)) { if (!dev->is_busmaster) { @@ -1190,7 +1190,7 @@ 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); + pci_enable_bridge(pci_upstream_bridge(dev)); /* only skip sriov related */ for (i = 0; i <= PCI_ROM_RESOURCE; i++)