From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 23115C43461 for ; Fri, 11 Sep 2020 23:16:40 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D2C7C221F0 for ; Fri, 11 Sep 2020 23:16:39 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=intel-com.20150623.gappssmtp.com header.i=@intel-com.20150623.gappssmtp.com header.b="HZ89aGDm" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725894AbgIKXQ1 (ORCPT ); Fri, 11 Sep 2020 19:16:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50414 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725849AbgIKXQH (ORCPT ); Fri, 11 Sep 2020 19:16:07 -0400 Received: from mail-pf1-x443.google.com (mail-pf1-x443.google.com [IPv6:2607:f8b0:4864:20::443]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0E9C8C061573 for ; Fri, 11 Sep 2020 16:16:07 -0700 (PDT) Received: by mail-pf1-x443.google.com with SMTP id o20so8399674pfp.11 for ; Fri, 11 Sep 2020 16:16:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel-com.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=zJRunWZTz5mT2mX6pknfWLOXDlKwI3diMMXxkFcYofQ=; b=HZ89aGDm9VVQd5dkS9mj4yohPfMr+KO6evJg76I9VceCPQrD3OR6CNGFt0Cn524ZAb kRBFr8fdeHcT3EayNmF2c1ywStN88PmjJmUvx9ZkHHn1/Zv/2xVJwlKHBZF/VLXevuIF 5FPHq0h5nY4ybFnMHDoQTUjcOCbN5EiGb1oOf2dahZ7citdegCnJ1w2ig3tKx3s4FRMk tz+Mj/USUQ0k/EmID1W3lkeODsJQU1H5x6bb/yDMQx/zFp2p1wW0wa6NU9cLKhStsIo6 L7ZpMlCO7Hjwf1lsGxDQl0Ezim2iulbGJNJO5MMRzZpwvAlsxAF7K8VKFlDlxu/tJGWe QSWA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=zJRunWZTz5mT2mX6pknfWLOXDlKwI3diMMXxkFcYofQ=; b=NKCKCStI8JXM9NG62LsLLJgkBqdy83HXa3qud1jVTPP8VqNPdeTu9U3fNmBl3Fuv9X LtvIEo8LUfzsjAVOm81vLKUCahgem0wM07lfAn1sIlyQuKF7G1woqtPRktHV0esmfFyJ HdXdmty0yJXpyJe0qGUqPH5U1Y606XnXo9Motrs1ce3Bqr4dVfOP/9DYPY0rh/FX5uwN 601NsNXMWO2zJe8dS6lRCif6Cn4JyVeAfHU7SACPtwoTT/tDPpIOMZT18pBsRr17a1zx FFzeLUJ/c6u3Zs4FLfEoxGybTypUVkQ+nMXcYhVq3BSJ/6glFrDad9Q9nNpb8WKz+CH0 VCjA== X-Gm-Message-State: AOAM532nXKXjfGD54lU3puiRleBXH/Amhx68XF4+P3/c3M91rM+LhS5o 1DMpR4Qim+22qpavFw53RiRjqA== X-Google-Smtp-Source: ABdhPJw/eSEQcc0h4iHQQ5YgmO2y2XkwCrCTMwknxKhR5oLn/4DxYLpIAH8xcig3bYfQyg4k4KspXg== X-Received: by 2002:aa7:800c:: with SMTP id j12mr3997297pfi.130.1599866166465; Fri, 11 Sep 2020 16:16:06 -0700 (PDT) Received: from [192.168.1.102] (c-24-20-148-49.hsd1.or.comcast.net. [24.20.148.49]) by smtp.gmail.com with ESMTPSA id f4sm3022078pfj.147.2020.09.11.16.16.04 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Fri, 11 Sep 2020 16:16:05 -0700 (PDT) From: "Sean V Kelley" To: "Bjorn Helgaas" Cc: "Zhuo, Qiuxu" , Jonathan.Cameron@huawei.com, rjw@rjwysocki.net, sathyanarayanan.kuppuswamy@linux.intel.com, "Raj, Ashok" , "Luck, Tony" , linux-pci@vger.kernel.org, bhelgaas@google.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 04/10] PCI/RCEC: Add pcie_walk_rcec() to walk associated RCiEPs Date: Fri, 11 Sep 2020 16:16:03 -0700 X-Mailer: MailMate (1.13.2r5673) Message-ID: <44BE3112-64C2-4A64-B9A6-6DD466DC594D@intel.com> In-Reply-To: <20200905022308.GA379055@bjorn-Precision-5520> References: <20200905022308.GA379055@bjorn-Precision-5520> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 4 Sep 2020, at 19:23, Bjorn Helgaas wrote: > On Fri, Sep 04, 2020 at 10:18:30PM +0000, Kelley, Sean V wrote: >> Hi Bjorn, >> >> Quick question below... >> >> On Wed, 2020-09-02 at 14:55 -0700, Sean V Kelley wrote: >>> Hi Bjorn, >>> >>> On Wed, 2020-09-02 at 14:00 -0500, Bjorn Helgaas wrote: >>>> On Wed, Aug 12, 2020 at 09:46:53AM -0700, Sean V Kelley wrote: >>>>> From: Qiuxu Zhuo >>>>> >>>>> When an RCEC device signals error(s) to a CPU core, the CPU core >>>>> needs to walk all the RCiEPs associated with that RCEC to check >>>>> errors. So add the function pcie_walk_rcec() to walk all RCiEPs >>>>> associated with the RCEC device. >>>>> >>>>> Co-developed-by: Sean V Kelley >>>>> Signed-off-by: Sean V Kelley >>>>> Signed-off-by: Qiuxu Zhuo >>>>> Reviewed-by: Jonathan Cameron >>>>> --- >>>>> drivers/pci/pci.h | 4 +++ >>>>> drivers/pci/pcie/rcec.c | 76 >>>>> +++++++++++++++++++++++++++++++++++++++++ >>>>> 2 files changed, 80 insertions(+) >>>>> >>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h >>>>> index bd25e6047b54..8bd7528d6977 100644 >>>>> --- a/drivers/pci/pci.h >>>>> +++ b/drivers/pci/pci.h >>>>> @@ -473,9 +473,13 @@ static inline void pci_dpc_init(struct >>>>> pci_dev >>>>> *pdev) {} >>>>> #ifdef CONFIG_PCIEPORTBUS >>>>> void pci_rcec_init(struct pci_dev *dev); >>>>> void pci_rcec_exit(struct pci_dev *dev); >>>>> +void pcie_walk_rcec(struct pci_dev *rcec, int (*cb)(struct >>>>> pci_dev >>>>> *, void *), >>>>> + void *userdata); >>>>> #else >>>>> static inline void pci_rcec_init(struct pci_dev *dev) {} >>>>> static inline void pci_rcec_exit(struct pci_dev *dev) {} >>>>> +static inline void pcie_walk_rcec(struct pci_dev *rcec, int >>>>> (*cb)(struct pci_dev *, void *), >>>>> + void *userdata) {} >>>>> #endif >>>>> >>>>> #ifdef CONFIG_PCI_ATS >>>>> diff --git a/drivers/pci/pcie/rcec.c b/drivers/pci/pcie/rcec.c >>>>> index 519ae086ff41..405f92fcdf7f 100644 >>>>> --- a/drivers/pci/pcie/rcec.c >>>>> +++ b/drivers/pci/pcie/rcec.c >>>>> @@ -17,6 +17,82 @@ >>>>> >>>>> #include "../pci.h" >>>>> >>>>> +static int pcie_walk_rciep_devfn(struct pci_bus *bus, int >>>>> (*cb)(struct pci_dev *, void *), >>>>> + void *userdata, const unsigned long >>>>> bitmap) >>>>> +{ >>>>> + unsigned int devn, fn; >>>>> + struct pci_dev *dev; >>>>> + int retval; >>>>> + >>>>> + for_each_set_bit(devn, &bitmap, 32) { >>>>> + for (fn = 0; fn < 8; fn++) { >>>>> + dev = pci_get_slot(bus, PCI_DEVFN(devn, fn)); >>>> >>>> Wow, this is a lot of churning to call pci_get_slot() 256 times per >>>> bus for the "associated bus numbers" case where we pass a bitmap of >>>> 0xffffffff. They didn't really make it easy for software when they >>>> added the next/last bus number thing. >>>> >>>> Just thinking out loud here. What if we could set dev->rcec during >>>> enumeration, and then use that to build pcie_walk_rcec()? >>> >>> I think follow what you are doing. >>> >>> As we enumerate an RCEC, use the time to discover RCiEPs and >>> associate >>> each RCiEP's dev->rcec. Although BIOS already set the bitmap for >>> this >>> specific RCEC, it's more efficient to simply discover the devices >>> through the bus walk and verify each one found against the bitmap. >>> >>> Further, while we can be certain that an RCiEP found with a matching >>> device no. in a bitmap for an associated RCEC is correct, we cannot >>> be >>> certain that any RCiEP found on another bus range is correct unless >>> we >>> verify the bus is within that next/last bus range. >>> >>> Finally, that's where find_rcec() callback for rcec_assoc_rciep() >>> does >>> double duty by also checking on the "on-a-separate-bus" case >>> captured >>> potentially by find_rcec() during an RCiEP's bus walk. >>> >>> >>>> bool rcec_assoc_rciep(rcec, rciep) >>>> { >>>> if (rcec->bus == rciep->bus) >>>> return (rcec->bitmap contains rciep->devfn); >>>> >>>> return (rcec->next/last contains rciep->bus); >>>> } >>>> >>>> link_rcec(dev, data) >>>> { >>>> struct pci_dev *rcec = data; >>>> >>>> if ((dev is RCiEP) && rcec_assoc_rciep(rcec, dev)) >>>> dev->rcec = rcec; >>>> } >>>> >>>> find_rcec(dev, data) >>>> { >>>> struct pci_dev *rciep = data; >>>> >>>> if ((dev is RCEC) && rcec_assoc_rciep(dev, rciep)) >>>> rciep->rcec = dev; >>>> } >>>> >>>> pci_setup_device >>>> ... >> >> I just noticed your use of pci_setup_device(). Are you suggesting >> moving the call to pci_rcec_init() out of pci_init_capabilities() and >> move it into pci_setup_device()? If so, would pci_rcec_exit() still >> remain in pci_release_capabilities()? >> >> I'm just wondering if it could just remain in >> pci_init_capabilities(). > > Yeah, I didn't mean in pci_setup_device() specifically, just somewhere > in the callchain of pci_setup_device(). But you're right, it probably > would make more sense in pci_init_capabilities(), so I *should* have > said pci_scan_single_device() to be a little less specific. > > Bjorn I’ve done some experimenting with this approach, and I think there may be a problem of just walking the busses during enumeration pci_init_capabilities(). One problem is where one has an RCEC on a root bus: 6a(00.4) and an RCiEP on another root bus: 6b(00.0). They will never find each other in this approach through a normal pci_bus_walk() call using their respective root_bus. > +-[0000:6b]-+-00.0 > | +-00.1 > | +-00.2 > | \-00.3 > +-[0000:6a]-+-00.0 > | +-00.1 > | +-00.2 > | \-00.4 While having a lot of slot calls per bus is unfortunate, unless I’m mistaken you would have to walk every peer root_bus with your RCiEP in this example until you hit on the right RCEC, unless of course you have a bitmap associated RCEC on dev->bus. Conversely, if you are enumerating the above RCEC at 6a(00.4) and you attempt to link_rcec() through calls to pci_walk_bus(), the walk will still be limited to 6a and below; never encountering 6b(00.0). So you would then need an additional walk for each of the associated bus ranges, excluding the same bus as the RCEC. pci_init_capabilities() … pci_init_rcec() // Cached if (RCEC) Walk the dev->bus for bitmap associated RCiEP Walk all associated bus ranges for RCiEP else if (RCiEP) Walk the dev->bus for bitmap associated RCEC Walk all peer root_bus for RCEC, confirm if own dev->bus falls within discovered RCEC associated ranges The other problem here is temporal. I’m wondering if we may be trying to find associated devices at the pci_init_capabilities() stage prior to them being fully enumerated, i.e., RCEC has not been cached but we are searching with a future associated RCiEP. So one could encounter a race condition where one is checking on an RCiEP whose associated RCEC has not been enumerated yet. So let’s say one throws out RCiEP entirely and just relies upon RCEC to find the associations because one knows that an encountered RCEC (in pci_init_capabilities()) has already been cached. In that case you end up with the original implementation being done with this patch series… if (RCEC) Walk the dev->bus for bitmap associated RCiEP Walk all associated bus ranges for RCiEP Perhaps I’ve muddled some things here but it doesn’t look like the twain will meet unless I cover multiple peer root_bus and even then you may have an issue because the devices don’t yet fully exist from the perspective of the OS. Thanks, Sean