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: Mon, 23 Apr 2018 14:41:04 -0600	[thread overview]
Message-ID: <20180423144104.2dad7495@w520.home> (raw)
In-Reply-To: <20180423202311.GA164898@bhelgaas-glaptop.roam.corp.google.com>

On Mon, 23 Apr 2018 15:23:11 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> On Mon, Apr 23, 2018 at 01:10:44PM -0600, Alex Williamson wrote:
> > On Mon, 23 Apr 2018 13:28:22 -0400
> > Sinan Kaya <okaya@codeaurora.org> wrote:
> >   
> > > On 4/20/2018 11:04 AM, Alex Williamson wrote:  
> > > > 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,    
> > > 
> > > The example given is for some serdes firmware load to happen in probe and
> > > then performing a retrain to reach to a better speed.
> > > 
> > > It becomes a chicken and egg problem. 
> > > 
> > > 1. Endpoint HW trains to gen1 by default pre-boot.
> > > 2. PCI core enumerates the device.
> > > 3. Endpoint driver gets loaded
> > > 4. Driver does the firmware programming followed by a link retrain.
> > > 
> > > I think it is the responsibility of the PCI core to provide reset APIs.
> > > However, expecting endpoint drivers to be knowledgeable about hotplug is
> > > too much.
> > > 
> > > We can certainly contain AER change into pci directory by moving the slot
> > > reset function to drivers/pci.h file.
> > > 
> > > But, we need to think about what to do about VFIO and other endpoint
> > > initiated reset cases. My suggestion was to move this into a single API and
> > > remove all other APIs from include/linux/pci.h.  
> > 
> > I'm a little confused about the relation between reset and retrain.
> > AIUI we can retrain the link without any sort of endpoint reset and if
> > some sort of driver/firmware setup is required on the endpoint to
> > achieve the target link speed, then I'd think we only want to retrain.  
> 
> In hfi1, do_pcie_gen3_transition() resets the device.  I don't know if
> retraining the link would be sufficient; maybe the reset is required
> to make the device use the new firmware.  I guess we already export
> reset interfaces, so if we add a retrain interface, drivers could
> choose what they need.
> 
> > How this is going to work with vfio is an interesting question.  We're
> > only providing access to the device, not the link to the device.  
> 
> > Multifunction endpoints become a big problem if one function starts
> > requesting link retraining while another is in use elsewhere.  
> 
> Can we just make it the driver's problem by returning -EPERM if one
> function requests a retrain while another function is in use?

Yes, this is basically how we handle bus resets through vfio-pci, if
there are multiple devices affected by a bus reset, slots or functions,
the caller needs to prove ownership of all of them before we do a bus
reset.  That's great for video cards where we have the GPU and audio
functions typically assigned together anyway, but I don't know that
it's a great solution for IB where you're more likely to want to assign
separate functions to separate users and nobody can get the device to
run at full speed unless all the functions are assigned to one user.
Thanks,

Alex

  reply	other threads:[~2018-04-23 20:41 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
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 [this message]
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=20180423144104.2dad7495@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).