From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755219AbeDTOMf (ORCPT ); Fri, 20 Apr 2018 10:12:35 -0400 Received: from mga17.intel.com ([192.55.52.151]:55931 "EHLO mga17.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755009AbeDTOMc (ORCPT ); Fri, 20 Apr 2018 10:12:32 -0400 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,301,1520924400"; d="scan'208";a="222034246" Subject: Re: [PATCH 1/2] IB/hfi1: Try slot reset before secondary bus reset To: "Deucher, Alexander" , Bjorn Helgaas , Jason Gunthorpe Cc: Sinan Kaya , Bjorn Helgaas , "linux-pci@vger.kernel.org" , "sulrich@codeaurora.org" , "timur@codeaurora.org" , "linux-arm-msm@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , Mike Marciniszyn , Doug Ledford , "open list:HFI1 DRIVER" , open list References: <1524167784-5911-1-git-send-email-okaya@codeaurora.org> <20180419202632.GE14063@ziepe.ca> <20180419214722.GO28657@bhelgaas-glaptop.roam.corp.google.com> From: Dennis Dalessandro Message-ID: <9c893ac2-9482-2134-9ca9-04a306e9a416@intel.com> Date: Fri, 20 Apr 2018 10:12:28 -0400 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 4/19/2018 6:11 PM, Deucher, Alexander wrote: >> -----Original Message----- >> From: Bjorn Helgaas [mailto:helgaas@kernel.org] >> Sent: Thursday, April 19, 2018 5:47 PM >> To: Jason Gunthorpe >> Cc: Sinan Kaya ; Bjorn Helgaas >> ; linux-pci@vger.kernel.org; >> sulrich@codeaurora.org; timur@codeaurora.org; linux-arm- >> msm@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Mike >> Marciniszyn ; Dennis Dalessandro >> ; Doug Ledford ; >> open list:HFI1 DRIVER ; open list > kernel@vger.kernel.org>; Deucher, Alexander >> >> Subject: Re: [PATCH 1/2] IB/hfi1: Try slot reset before secondary bus reset >> >> [+cc Alex, who might know why DRM drivers have their own PCIe Gen3 >> code] >> >> On Thu, Apr 19, 2018 at 02:26:32PM -0600, Jason Gunthorpe wrote: >>> On Thu, Apr 19, 2018 at 03:56:23PM -0400, Sinan Kaya wrote: >>>> The infiniband adapter might be connected to a PCI hotplug slot. >>>> Performing secondary bus reset on a hotplug slot causes PCI link >> up/down interrupts. >>>> >>>> Hotplug driver removes the device from system when a link down >>>> interrupt is observed and performs re-enumeration when link up >> interrupt is observed. >>>> >>>> This conflicts with what this code is trying to do. Try secondary >>>> bus reset only if pci_reset_slot() fails/unsupported. >>>> >>>> Signed-off-by: Sinan Kaya >>>> drivers/infiniband/hw/hfi1/pcie.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/infiniband/hw/hfi1/pcie.c >>>> b/drivers/infiniband/hw/hfi1/pcie.c >>>> index 83d66e8..75f49e3 100644 >>>> +++ b/drivers/infiniband/hw/hfi1/pcie.c >>>> @@ -908,7 +908,8 @@ static int trigger_sbr(struct hfi1_devdata *dd) >>> >>> The code above this hunk is: >>> >>> /* >>> * Trigger a secondary bus reset (SBR) on ourselves using our parent. >>> * >>> * Based on pci_parent_bus_reset() which is not exported by the >>> * kernel core. >>> */ >>> static int trigger_sbr(struct hfi1_devdata *dd) { >>> >>> [..] >>> >>> This really seems like something the PCI core should be helping with, >>> drivers shouldn't be doing stuff like this. I get the feeling this >>> should be a common need if drivers support various error recovery >>> schemes? >>> >>> Bjorn, would be appropriate to export pci_parent_bus_reset() or some >>> variation therin?? >> >> I agree it would be really nice if the PCI core could help out somehow so we >> could get some of this code out of individual drivers. >> >> If fact, stepping back a few paces, this HFI reset path is part of a transition to >> PCIe gen3 signaling, and I'm not sure why *that* is in the driver either. >> >> There's an ongoing discussion [1] about why this gen3 code is in the driver. >> Several DRM drivers include similar code (cik_pcie_gen3_enable(), >> si_pcie_gen3_enable()). >> >> I *thought* the hardware was supposed to automatically negotiate to the >> highest rate supported by both sides without any help at all from software. >> But since several drivers have code to do it themselves, I wonder if I'm >> missing something, or maybe there's something the PCI core should be doing >> that it isn't, and the driver code is basically working around that PCI core >> deficiency. > > My understanding was that some platfoms only bring up the link in gen 1 mode for compatibility reasons. TBH, I'm not that familiar with how the links come up on different platforms. > > Alex > I'm checking with our HW folks and the guys that wrote the PCI code in our driver. I quite frankly just don't recall what all is going on here. Let me find out for sure before I give out wrong information. -Denny