LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Alex Williamson <alex.williamson@redhat.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Sinan Kaya <okaya@codeaurora.org>, 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>
Subject: Re: [PATCH 1/2] IB/hfi1: Try slot reset before secondary bus reset
Date: Fri, 20 Apr 2018 09:04:20 -0600	[thread overview]
Message-ID: <20180420090420.03fb1e6c@w520.home> (raw)
In-Reply-To: <20180420140049.GP28657@bhelgaas-glaptop.roam.corp.google.com>

On Fri, 20 Apr 2018 09:00:49 -0500
Bjorn Helgaas <helgaas@kernel.org> 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.
> 
> 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.

Is there a concern here about whether the endpoint device driver or the
PCI core really knows better about link retraining?  This makes me
remember my unfinished (and need to revisit) Pericom quirk[1] where
errata in the PCIe switch requires that upstream and downstream links
are balanced (ie. same link rate) or else enabling ACS results in
packets not properly flowing through the switch.  If an endpoint driver
starts deciding to retrain links, overriding quirks in the PCI core,
then such topology manipulation isn't possible.  Why does the
driver .probe() function think it can retrain at a higher link rate
than PCI core?  Thanks,

Alex

[1]https://lists.linuxfoundation.org/pipermail/iommu/2016-October/018890.html

  parent reply	other threads:[~2018-04-20 15:04 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
2018-04-20 15:04         ` Alex Williamson [this message]
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=20180420090420.03fb1e6c@w520.home \
    --to=alex.williamson@redhat.com \
    --cc=alexander.deucher@amd.com \
    --cc=bhelgaas@google.com \
    --cc=dennis.dalessandro@intel.com \
    --cc=dledford@redhat.com \
    --cc=helgaas@kernel.org \
    --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).