Re: [PATCH v8 11/14] PCI/RCEC: Add RCiEP's linked RCEC to AER/ERR

From: Bjorn Helgaas <helgaas@kernel.org>
To: "Kelley, Sean V" <sean.v.kelley@intel.com>
Cc: "Zhuo, Qiuxu" <qiuxu.zhuo@intel.com>,
	"Jonathan.Cameron@huawei.com" <Jonathan.Cameron@huawei.com>,
	"Kuppuswamy,
	Sathyanarayanan" <sathyanarayanan.kuppuswamy@intel.com>,
	"Raj, Ashok" <ashok.raj@intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Luck, Tony" <tony.luck@intel.com>,
	"Wysocki, Rafael J" <rafael.j.wysocki@intel.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"seanvk.dev@oregontracks.org" <seanvk.dev@oregontracks.org>,
	"bhelgaas@google.com" <bhelgaas@google.com>
Subject: Re: [PATCH v8 11/14] PCI/RCEC: Add RCiEP's linked RCEC to AER/ERR
Date: Mon, 12 Oct 2020 17:58:48 -0500	[thread overview]
Message-ID: <20201012225848.GA3754175@bjorn-Precision-5520> (raw)
In-Reply-To: <96d2cf35723265ce189ba2053fe4c347a816e9a4.camel@intel.com>

On Fri, Oct 09, 2020 at 11:51:39PM +0000, Kelley, Sean V wrote:
> On Fri, 2020-10-09 at 15:07 -0700, Sean V Kelley wrote:

> So I tested the following out, including your moving flr to aer.c:
> 
> - Renamed flr_on_rciep() to flr_on_rc() for RC devices (RC_END and
> RC_EC)
> 
> - Moved check on dev->rcec into aer_root_reset() including the FLR.
> 
> - Reworked pci_walk_bridge() to drop extra dev argument and check
> locally for the bridge->rcec. Maybe should also check on type when
> checking on bridge->rcec.
> 
> Note I didn't use the check on aer_cap existence because I think you
> had added that for simply being able to skip over for the non-native
> case and I handle that with the single goto at the beginning which
> takes you to the FLR.

Right.  Well, my thinking was that "root" would be a device with the
AER Root Error Command and Root Error Status registers, i.e., a Root
Port or RCEC.  IIUC that basically means the APEI case where firmware
gives us an error record.

Isn't the existing v5.9 code buggy in that it unconditionally pokes
these registers?  I think the APEI path can end up here, and firmware
probably has not granted us control over AER.

Somewhat related question: I'm a little skeptical about the fact that
aer_root_reset() currently does:

  - clear ROOT_PORT_INTR_ON_MESG_MASK
  - do reset
  - clear PCI_ERR_ROOT_STATUS
  - enable ROOT_PORT_INTR_ON_MESG_MASK

In the APEI path all this AER register manipulation must be done by
firmware before passing the error record to the OS.  So in the native
case where the OS does own the AER registers, why can't the OS do that
manipulation in the same order, i.e., all before doing the reset?

> So this is rough, compiled, tested with AER injections but that's it...

I couldn't actually apply the patch below because it seems to be
whitespace-damaged, but I think I like it.

  - It would be nice to be able to just call pcie_flr() and not have
    to add flr_on_rc().  I can't remember why we need the
    pcie_has_flr() / pcie_flr() dance.  It seems racy and ugly, but I
    have a vague recollection that there actually is some reason for
    it.

  - I would *rather* consolidate the AER register updates and test for
    the non-native case once instead of treating it like a completely
    separate path with a "goto".  But maybe not possible.  Not a big
    deal either way.

  - Getting rid of the extra "dev" argument to pci_walk_bridge() is a
    great side-effect.  I didn't even notice that.

  - If we can simplify that "state == pci_channel_io_frozen" case as
    this does, that is a *big* deal because there are other patches
    just waiting to touch that reset and it will be much simpler if
    there's only one reset_subordinate_devices() call there.

If you do work this up, I'd really appreciate it if you can start with
my pci/err branch so I don't have to re-do all the tweaks I've already
done:

  https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=pci/err

Bjorn