[PATCH for-rc 0/2] IB/hfi1: PCI bug due to pci core changes

[PATCH for-rc 0/2] IB/hfi1: PCI bug due to pci core changes

[Dennis Dalessandro]

Hi Bjorn, Doug and Jason,

As discussed on the pci list [1] our driver was broken with the following three
changes in 4.19 merge window:

c6a44ba950d1 ("PCI: Rename pci_try_reset_bus() to pci_reset_bus()")
409888e0966e ("IB/hfi1: Use pci_try_reset_bus() for initiating PCI Secondary Bus Reset")
811c5cb37df4 ("PCI: Unify try slot and bus reset API")

Ideally I'd like to see those patches reverted but we could also go with
something like the following series. I know there is a desire to have a more
clean API but since this is already the rc phase I think that is best delayed.
Clearly the first attempt at an API clean up did not work out so well.

I'm not sure how you maintainers want to handle this since the fix straddles
both subsystems so I'm sending it to both linux-rdma and linux-pci.

At the request of discussion on the linux-pci list I have also filed a bug [2].

[1] https://marc.info/?l=linux-pci&m=153539692917785&w=2
[2] https://bugzilla.kernel.org/show_bug.cgi?id=200985

Thanks


---

Dennis Dalessandro (2):
      PCI: Fix faulty logic in pci_reset_bus()
      IB/hfi1,PCI: Allow bus reset while probing


 drivers/infiniband/hw/hfi1/pcie.c |   11 ++++-------
 drivers/pci/pci.c                 |    3 ++-
 include/linux/pci.h               |    3 +++
 3 files changed, 9 insertions(+), 8 deletions(-)

--
-Denny

[PATCH for-rc 1/2] PCI: Fix faulty logic in pci_reset_bus()

[Dennis Dalessandro]

The pci_rest_bus() function calls into pci_probe_reset_slot() to determine
whether to call the slot or bus reset. The check has faulty logic in that
it does not account for pci_probe_reset_slot() being able to return an
errno. Fix by only calling the slot reset when the function returns 0.
Treat < 1 and > 1 the same.

Cc: Sinan Kaya <okaya@codeaurora.org>
Fixes: 811c5cb37df4 ("PCI: Unify try slot and bus reset API")
Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
---
 drivers/pci/pci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 29ff961..30b2603 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5200,7 +5200,7 @@ static int __pci_reset_bus(struct pci_bus *bus)
  */
 int pci_reset_bus(struct pci_dev *pdev)
 {
-	return pci_probe_reset_slot(pdev->slot) ?
+	return (!pci_probe_reset_slot(pdev->slot)) ?
 	    __pci_reset_slot(pdev->slot) : __pci_reset_bus(pdev->bus);
 }
 EXPORT_SYMBOL_GPL(pci_reset_bus);

Re: [PATCH for-rc 1/2] PCI: Fix faulty logic in pci_reset_bus()

[Sinan Kaya]

On 8/31/2018 10:34 AM, Dennis Dalessandro wrote:
> The pci_rest_bus() function calls into pci_probe_reset_slot() to determine
> whether to call the slot or bus reset. The check has faulty logic in that
> it does not account for pci_probe_reset_slot() being able to return an
> errno. Fix by only calling the slot reset when the function returns 0.
> Treat < 1 and > 1 the same.
> 
> Cc: Sinan Kaya<okaya@codeaurora.org>
> Fixes: 811c5cb37df4 ("PCI: Unify try slot and bus reset API")
> Reviewed-by: Michael J. Ruhl<michael.j.ruhl@intel.com>
> Signed-off-by: Dennis Dalessandro<dennis.dalessandro@intel.com>

Nit. Small typo on the first sentence (pci_rest_bus()).

Reviewed-by: Sinan Kaya <okaya@kernel.org>

Re: [PATCH for-rc 1/2] PCI: Fix faulty logic in pci_reset_bus()

[Jason Gunthorpe]

On Fri, Aug 31, 2018 at 10:58:32AM -0700, Sinan Kaya wrote:
> On 8/31/2018 10:34 AM, Dennis Dalessandro wrote:
> > The pci_rest_bus() function calls into pci_probe_reset_slot() to determine
> > whether to call the slot or bus reset. The check has faulty logic in that
> > it does not account for pci_probe_reset_slot() being able to return an
> > errno. Fix by only calling the slot reset when the function returns 0.
> > Treat < 1 and > 1 the same.
> > 
> > Cc: Sinan Kaya<okaya@codeaurora.org>
> > Fixes: 811c5cb37df4 ("PCI: Unify try slot and bus reset API")
> > Reviewed-by: Michael J. Ruhl<michael.j.ruhl@intel.com>
> > Signed-off-by: Dennis Dalessandro<dennis.dalessandro@intel.com>
> 
> Nit. Small typo on the first sentence (pci_rest_bus()).
> 
> Reviewed-by: Sinan Kaya <okaya@kernel.org>

Bjorn,

Are you OK to apply this series through the RDMA tree (for rc3), or do
you want to take it through PCI?

https://patchwork.kernel.org/patch/10584277/

Thanks,
Jason

Re: [PATCH for-rc 1/2] PCI: Fix faulty logic in pci_reset_bus()

[Sinan Kaya]

On 9/4/2018 1:59 PM, Jason Gunthorpe wrote:
> On Fri, Aug 31, 2018 at 10:58:32AM -0700, Sinan Kaya wrote:
>> On 8/31/2018 10:34 AM, Dennis Dalessandro wrote:
>>> The pci_rest_bus() function calls into pci_probe_reset_slot() to determine
>>> whether to call the slot or bus reset. The check has faulty logic in that
>>> it does not account for pci_probe_reset_slot() being able to return an
>>> errno. Fix by only calling the slot reset when the function returns 0.
>>> Treat < 1 and > 1 the same.
>>>
>>> Cc: Sinan Kaya<okaya@codeaurora.org>
>>> Fixes: 811c5cb37df4 ("PCI: Unify try slot and bus reset API")
>>> Reviewed-by: Michael J. Ruhl<michael.j.ruhl@intel.com>
>>> Signed-off-by: Dennis Dalessandro<dennis.dalessandro@intel.com>
>>
>> Nit. Small typo on the first sentence (pci_rest_bus()).
>>
>> Reviewed-by: Sinan Kaya <okaya@kernel.org>
> 
> Bjorn,
> 
> Are you OK to apply this series through the RDMA tree (for rc3), or do
> you want to take it through PCI?
> 
> https://patchwork.kernel.org/patch/10584277/

Please don't apply the entire series yet. First patch is good to go.

Second one is a hack. We are trying to find a better solution for the second
patch.

https://bugzilla.kernel.org/show_bug.cgi?id=200985


> 
> Thanks,
> Jason
> 

Re: [PATCH for-rc 1/2] PCI: Fix faulty logic in pci_reset_bus()

[Jason Gunthorpe]

On Tue, Sep 04, 2018 at 02:16:13PM -0700, Sinan Kaya wrote:
> On 9/4/2018 1:59 PM, Jason Gunthorpe wrote:
> > On Fri, Aug 31, 2018 at 10:58:32AM -0700, Sinan Kaya wrote:
> > > On 8/31/2018 10:34 AM, Dennis Dalessandro wrote:
> > > > The pci_rest_bus() function calls into pci_probe_reset_slot() to determine
> > > > whether to call the slot or bus reset. The check has faulty logic in that
> > > > it does not account for pci_probe_reset_slot() being able to return an
> > > > errno. Fix by only calling the slot reset when the function returns 0.
> > > > Treat < 1 and > 1 the same.
> > > > 
> > > > Cc: Sinan Kaya<okaya@codeaurora.org>
> > > > Fixes: 811c5cb37df4 ("PCI: Unify try slot and bus reset API")
> > > > Reviewed-by: Michael J. Ruhl<michael.j.ruhl@intel.com>
> > > > Signed-off-by: Dennis Dalessandro<dennis.dalessandro@intel.com>
> > > 
> > > Nit. Small typo on the first sentence (pci_rest_bus()).
> > > 
> > > Reviewed-by: Sinan Kaya <okaya@kernel.org>
> > 
> > Bjorn,
> > 
> > Are you OK to apply this series through the RDMA tree (for rc3), or do
> > you want to take it through PCI?
> > 
> > https://patchwork.kernel.org/patch/10584277/
> 
> Please don't apply the entire series yet. First patch is good to go.
> 
> Second one is a hack. We are trying to find a better solution for the second
> patch.

Don't expect me to follow bugzilla too :|

I'll drop this series off patchworks then, resend when you have something..

Jason

Re: [PATCH for-rc 1/2] PCI: Fix faulty logic in pci_reset_bus()

[Sinan Kaya]

On 9/4/2018 2:30 PM, Jason Gunthorpe wrote:
> On Tue, Sep 04, 2018 at 02:16:13PM -0700, Sinan Kaya wrote:
>> On 9/4/2018 1:59 PM, Jason Gunthorpe wrote:
>>> On Fri, Aug 31, 2018 at 10:58:32AM -0700, Sinan Kaya wrote:
>>>> On 8/31/2018 10:34 AM, Dennis Dalessandro wrote:
>>>>> The pci_rest_bus() function calls into pci_probe_reset_slot() to determine
>>>>> whether to call the slot or bus reset. The check has faulty logic in that
>>>>> it does not account for pci_probe_reset_slot() being able to return an
>>>>> errno. Fix by only calling the slot reset when the function returns 0.
>>>>> Treat < 1 and > 1 the same.
>>>>>
>>>>> Cc: Sinan Kaya<okaya@codeaurora.org>
>>>>> Fixes: 811c5cb37df4 ("PCI: Unify try slot and bus reset API")
>>>>> Reviewed-by: Michael J. Ruhl<michael.j.ruhl@intel.com>
>>>>> Signed-off-by: Dennis Dalessandro<dennis.dalessandro@intel.com>
>>>>
>>>> Nit. Small typo on the first sentence (pci_rest_bus()).
>>>>
>>>> Reviewed-by: Sinan Kaya <okaya@kernel.org>
>>>
>>> Bjorn,
>>>
>>> Are you OK to apply this series through the RDMA tree (for rc3), or do
>>> you want to take it through PCI?
>>>
>>> https://patchwork.kernel.org/patch/10584277/
>>
>> Please don't apply the entire series yet. First patch is good to go.
>>
>> Second one is a hack. We are trying to find a better solution for the second
>> patch.
> 
> Don't expect me to follow bugzilla too :|
> 
> I'll drop this series off patchworks then, resend when you have something..
> 

Sure, I'm trying to contain the change to PCI tree only. Hopefully, it can go
  through Bjorn's tree. If we fail, we are back to this change. I'll ping you in
  that case.

> Jason
> 

[PATCH for-rc 2/2] IB/hfi1,PCI: Allow bus reset while probing

[Dennis Dalessandro]

Calling into the new API to reset the secondary bus results in a
deadlock. This occurs because the device/bus is already locked at
probe time. Reverting back to the old behavior while the API is
improved.

Fixes: c6a44ba950d1 ("PCI: Rename pci_try_reset_bus() to pci_reset_bus()")
Fixes: 409888e0966e ("IB/hfi1: Use pci_try_reset_bus() for initiating PCI Secondary Bus Reset")
Cc: Sinan Kaya <okaya@codeaurora.org>
Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
---
 drivers/infiniband/hw/hfi1/pcie.c |   11 ++++-------
 drivers/pci/pci.c                 |    1 +
 include/linux/pci.h               |    3 +++
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/pcie.c b/drivers/infiniband/hw/hfi1/pcie.c
index eec8375..6c967dd 100644
--- a/drivers/infiniband/hw/hfi1/pcie.c
+++ b/drivers/infiniband/hw/hfi1/pcie.c
@@ -893,14 +893,11 @@ static int trigger_sbr(struct hfi1_devdata *dd)
 		}
 
 	/*
-	 * A secondary bus reset (SBR) issues a hot reset to our device.
-	 * The following routine does a 1s wait after the reset is dropped
-	 * per PCI Trhfa (recovery time).  PCIe 3.0 section 6.6.1 -
-	 * Conventional Reset, paragraph 3, line 35 also says that a 1s
-	 * delay after a reset is required.  Per spec requirements,
-	 * the link is either working or not after that point.
+	 * This is an end around to do an SBR during probe time. A new API needs
+	 * to be implemented to have cleaner interface but this fixes the
+	 * current brokenness
 	 */
-	return pci_reset_bus(dev);
+	return pci_bridge_secondary_bus_reset(dev->bus->self);
 }
 
 /*
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 30b2603..1835f3a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4547,6 +4547,7 @@ int pci_bridge_secondary_bus_reset(struct pci_dev *dev)
 
 	return pci_dev_wait(dev, "bus reset", PCIE_RESET_READY_POLL_MS);
 }
+EXPORT_SYMBOL_GPL(pci_bridge_secondary_bus_reset);
 
 static int pci_parent_bus_reset(struct pci_dev *dev, int probe)
 {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index e72ca8d..6925828 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1235,6 +1235,9 @@ void pci_bus_add_resource(struct pci_bus *bus, struct resource *res,
 int devm_request_pci_bus_resources(struct device *dev,
 				   struct list_head *resources);
 
+/* Temporary until new and working PCI SBR API in place */
+int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
+
 #define pci_bus_for_each_resource(bus, res, i)				\
 	for (i = 0;							\
 	    (res = pci_bus_resource_n(bus, i)) || i < PCI_BRIDGE_RESOURCE_NUM; \

Re: [PATCH for-rc 0/2] IB/hfi1: PCI bug due to pci core changes

[Bjorn Helgaas]

On Fri, Aug 31, 2018 at 10:33:54AM -0700, Dennis Dalessandro wrote:
> Hi Bjorn, Doug and Jason,
> 
> As discussed on the pci list [1] our driver was broken with the following three
> changes in 4.19 merge window:
> 
> c6a44ba950d1 ("PCI: Rename pci_try_reset_bus() to pci_reset_bus()")
> 409888e0966e ("IB/hfi1: Use pci_try_reset_bus() for initiating PCI Secondary Bus Reset")
> 811c5cb37df4 ("PCI: Unify try slot and bus reset API")
> 
> Ideally I'd like to see those patches reverted but we could also go with
> something like the following series. I know there is a desire to have a more
> clean API but since this is already the rc phase I think that is best delayed.
> Clearly the first attempt at an API clean up did not work out so well.
> 
> I'm not sure how you maintainers want to handle this since the fix straddles
> both subsystems so I'm sending it to both linux-rdma and linux-pci.
> 
> At the request of discussion on the linux-pci list I have also filed a bug [2].
> 
> [1] https://marc.info/?l=linux-pci&m=153539692917785&w=2
> [2] https://bugzilla.kernel.org/show_bug.cgi?id=200985
> 
> Thanks
> 
> 
> ---
> 
> Dennis Dalessandro (2):
>       PCI: Fix faulty logic in pci_reset_bus()
>       IB/hfi1,PCI: Allow bus reset while probing
> 
> 
>  drivers/infiniband/hw/hfi1/pcie.c |   11 ++++-------
>  drivers/pci/pci.c                 |    3 ++-
>  include/linux/pci.h               |    3 +++
>  3 files changed, 9 insertions(+), 8 deletions(-)

I applied both of these to for-linus for v4.19, thanks!