Re: [PATCHv4 08/12] PCI: ERR: Always use the first downstream port

[Keith Busch <keith.busch@intel.com>]

On Thu, Sep 27, 2018 at 05:56:25PM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 26, 2018 at 04:19:25PM -0600, Keith Busch wrote:
> > On Wed, Sep 26, 2018 at 05:01:16PM -0500, Bjorn Helgaas wrote:
> > > On Thu, Sep 20, 2018 at 10:27:13AM -0600, Keith Busch wrote:
> > > > The link reset always used the first bridge device, but AER broadcast
> > > > error handling may have reported an end device. This means the reset may
> > > > hit devices that were never notified of the impending error recovery.
> > > > 
> > > > This patch uses the first downstream port in the hierarchy considered
> > > > reliable. An error detected by a switch upstream port should mean it
> > > > occurred on its upstream link, so the patch selects the parent device
> > > > if the error is not a root or downstream port.
> > > 
> > > I'm not really clear on what "Always use the first downstream port"
> > > means.  Always use it for *what*?
> 
> And I forgot to ask what "first downstream port" means.

The "first downstream port" was supposed to mean the first DSP we
find when walking toward the root from the pci_dev that reported
ERR_[NON]FATAL.

By "use", I mean "walking down the sub-tree for error handling".

After thinking more on this, that doesn't really capture the intent. A
better way might be:

  Run error handling starting from the downstream port of the bus
  reporting an error

I'm struggling to make that short enough for a changelog subject.

> > I'll see if I can better rephrase.
> > 
> > Error handling should notify all affected pci functions. If an end device
> > detects and emits ERR_FATAL, the old way would have only notified that
> > end-device driver, but other functions may be on or below the same bus.
> > 
> > Using the downstream port that connects to that bus where the error was
> > detectedas the anchor point to broadcast error handling progression,
> > we can notify all functions so they have a chance to prepare for the
> > link reset.
> 
> So do I understand correctly that if the ERR_FATAL source is:
> 
>   - a Switch Upstream Port, you assume the problem is with the Link
>     upstream from the Port, and that Link may need to be reset, so you
>     notify everything below that Link, including the Upstream Port,
>     everything below it (the Downstream Ports and anything below
>     them), and potentially even any peers of the Upstream Port (is it
>     even possible for a Upstream Port to have peer multi-function
>     devices?)

Yep, the Microsemi Switchtec is one such real life example of an end
device function on the same bus as a USP.


>   - a Switch Downstream Port, you assume the Port (and the Link going
>     downstream) may need to be reset, so you notify the Port and
>     anything below it
> 
>   - an Endpoint, you assume the Link leading to the Endpoint may need
>     to be reset, so you notify the Endpoint, any peer multi-function
>     devices, any related SR-IOV devices, and any devices below a peer
>     that happens to be a bridge
> 
> And this is different from before because it notifies more devices in
> some cases?  There was a pci_walk_bus() in broadcast_error_message(),
> so we should have notified several devices in *some* cases, at least.

broadcast_error_message() had been using the pci_dev that detected the
error, and it's pci_walk_bus() used dev->subordinate. If the pci_dev
that detected an error was an end device, we didn't walk the bus.