LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Sinan Kaya <firstname.lastname@example.org>
To: Bjorn Helgaas <email@example.com>
Cc: Jason Gunthorpe <firstname.lastname@example.org>,
Bjorn Helgaas <email@example.com>,
Mike Marciniszyn <firstname.lastname@example.org>,
Dennis Dalessandro <email@example.com>,
Doug Ledford <firstname.lastname@example.org>,
"open list:HFI1 DRIVER" <email@example.com>,
open list <firstname.lastname@example.org>,
Alex Deucher <email@example.com>,
Rajat Jain <firstname.lastname@example.org>,
Alex Williamson <email@example.com>
Subject: Re: [PATCH 1/2] IB/hfi1: Try slot reset before secondary bus reset
Date: Fri, 20 Apr 2018 10:23:18 -0400 [thread overview]
Message-ID: <firstname.lastname@example.org> (raw)
On 4/20/2018 10:00 AM, Bjorn Helgaas wrote:
> [+cc Rajat, Alex because of their interest in the reset/hotplug issue]
> For context, Sinan's patch is this:
>> diff --git a/drivers/infiniband/hw/hfi1/pcie.c b/drivers/infiniband/hw/hfi1/pcie.c
>> index 83d66e8..75f49e3 100644
>> --- a/drivers/infiniband/hw/hfi1/pcie.c
>> +++ b/drivers/infiniband/hw/hfi1/pcie.c
>> @@ -908,7 +908,8 @@ static int trigger_sbr(struct hfi1_devdata *dd)
>> * delay after a reset is required. Per spec requirements,
>> * the link is either working or not after that point.
>> - pci_reset_bridge_secondary_bus(dev->bus->self);
>> + if (pci_reset_slot(dev->slot))
>> + pci_reset_bridge_secondary_bus(dev->bus->self);
> On Thu, Apr 19, 2018 at 06:19:32PM -0400, Sinan Kaya wrote:
>> On 4/19/2018 5:47 PM, Bjorn Helgaas wrote:
>>>> 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.
> What I was really thinking here was about the whole Gen3 transition
> thing, not the reset thing by itself.
>> I can create a function called pci_reset_link() and move both slot and
>> secondary bus reset inside.
> What exactly is your patch fixing? Is it the following?
> If the HFI link is not operating at 8GT/s, the driver's .probe()
> method tries to transition it to 8GT/s, which involves resetting the
> HFI device with pci_reset_bridge_secondary_bus(). If the HFI device
> is in a hotplug slot, the reset causes a "Link Down" event, which
> causes the pciehp driver to remove the HFI device and re-enumerate
> it when the link comes back up.
> When pciehp removes the device, it calls the HFI .remove() method,
> which is a problem because the .probe() method is still active.
> It looks like this should deadlock because __device_attach() holds
> the device_lock while calling .probe() and the
> device_release_driver() path tries to acquire it.
> Your patch uses pci_reset_slot(), which connects with Rajat's work
> (06a8d89af551 ("PCI: pciehp: Disable link notification across slot
> reset")) to avoid hotplug events for intentional resets.
> So I think I just reverse-engineered the whole rationale for your
> patch :) Sorry about the long detour.
Yes, you are on track. Basically; for all callers in drivers directory,
I was trying to call hotplug reset as in (1/2) of this patch before
secondary bus reset.
I learnt about this during our DPC+hotplug interaction thread here:
> I'm having a hard time articulating my thoughts here. I think my
> concern is that knowledge about this reset/link down/hotplug issue is
> leaking out and we'll end up with different reset interfaces that may
> or may not result in hotplug events. This seems like a confusing API
> because it's hard to explain which interface a driver should use.
I think we should go ahead and combine slot reset and secondary bus
reset into a single API and hide the other ones (pci_reset_slot() and
pci_reset_bridge_secondary_bus()) from external users. This way,
people don't have to query if system supports hotplug or not like VFIO
Other drivers (AER/IB) not doing this are broken in hotplug systems today.
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
next prev parent reply other threads:[~2018-04-20 14:23 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-19 19:56 Sinan Kaya
2018-04-19 19:56 ` [PATCH 2/2] PCI/AER: " Sinan Kaya
2018-04-27 18:51 ` Sinan Kaya
2018-04-19 20:26 ` [PATCH 1/2] IB/hfi1: " Jason Gunthorpe
2018-04-19 20:35 ` Sinan Kaya
2018-04-19 21:47 ` Bjorn Helgaas
2018-04-19 22:11 ` Deucher, Alexander
2018-04-20 14:12 ` Dennis Dalessandro
2018-04-20 14:55 ` Jason Gunthorpe
2018-04-19 22:19 ` Sinan Kaya
2018-04-20 14:00 ` Bjorn Helgaas
2018-04-20 14:23 ` Sinan Kaya [this message]
2018-04-20 15:04 ` Alex Williamson
2018-04-23 17:28 ` Sinan Kaya
2018-04-23 19:10 ` Alex Williamson
2018-04-23 20:17 ` Sinan Kaya
2018-04-23 20:23 ` Bjorn Helgaas
2018-04-23 20:41 ` Alex Williamson
2018-04-25 14:13 ` Sinan Kaya
2018-04-20 14:54 ` Jason Gunthorpe
2018-06-19 21:43 ` Bjorn Helgaas
2018-06-19 22:21 ` Sinan Kaya
2018-06-22 14:01 ` Bjorn Helgaas
2018-06-22 16:04 ` Sinan Kaya
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--subject='Re: [PATCH 1/2] IB/hfi1: Try slot reset before secondary bus reset' \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).