LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
To: Sinan Kaya <okaya@codeaurora.org>
Cc: Jason Gunthorpe <jgg@ziepe.ca>,
	Bjorn Helgaas <bhelgaas@google.com>,
	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 <mike.marciniszyn@intel.com>,
	Dennis Dalessandro <dennis.dalessandro@intel.com>,
	Doug Ledford <dledford@redhat.com>,
	"open list:HFI1 DRIVER" <linux-rdma@vger.kernel.org>,
	open list <linux-kernel@vger.kernel.org>,
	Alex Deucher <alexander.deucher@amd.com>,
	Rajat Jain <rajatja@google.com>,
	Alex Williamson <alex.williamson@redhat.com>
Subject: Re: [PATCH 1/2] IB/hfi1: Try slot reset before secondary bus reset
Date: Fri, 20 Apr 2018 09:00:49 -0500	[thread overview]
Message-ID: <20180420140049.GP28657@bhelgaas-glaptop.roam.corp.google.com> (raw)
In-Reply-To: <290e9530-dcde-9c10-7ae0-59ac4c509db4@codeaurora.org>

[+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.

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.

Bjorn

  reply	other threads:[~2018-04-20 14:00 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 [this message]
2018-04-20 14:23         ` Sinan Kaya
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

Reply instructions:

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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180420140049.GP28657@bhelgaas-glaptop.roam.corp.google.com \
    --to=helgaas@kernel.org \
    --cc=alex.williamson@redhat.com \
    --cc=alexander.deucher@amd.com \
    --cc=bhelgaas@google.com \
    --cc=dennis.dalessandro@intel.com \
    --cc=dledford@redhat.com \
    --cc=jgg@ziepe.ca \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-rdma@vger.kernel.org \
    --cc=mike.marciniszyn@intel.com \
    --cc=okaya@codeaurora.org \
    --cc=rajatja@google.com \
    --cc=sulrich@codeaurora.org \
    --cc=timur@codeaurora.org \
    --subject='Re: [PATCH 1/2] IB/hfi1: Try slot reset before secondary bus reset' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* 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).