Re: [PATCH v9 12/15] PCI/RCEC: Add RCiEP's linked RCEC to AER/ERR

From: Sean V Kelley
Date: Mon Oct 19 2020 - 15:07:42 EST

Next message: Palmer Dabbelt: "[GIT PULL] RISC-V Patches for the 5.10 Merge Window, Part 1"
Previous message: Joao Martins: "Re: [PATCH 00/35] Enhance memory utilization with DMEMFS"
In reply to: Sean V Kelley: "Re: [PATCH v9 12/15] PCI/RCEC: Add RCiEP's linked RCEC to AER/ERR"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sat, 2020-10-17 at 09:14 -0700, Sean V Kelley wrote:
> On 16 Oct 2020, at 13:30, Bjorn Helgaas wrote:
>
> > [+to Jonathan]
> >
> > On Thu, Oct 15, 2020 at 05:11:10PM -0700, Sean V Kelley wrote:
> > > From: Qiuxu Zhuo <qiuxu.zhuo@xxxxxxxxx>
> > >
> > > When attempting error recovery for an RCiEP associated with an
> > > RCEC
> > > device,
> > > there needs to be a way to update the Root Error Status, the
> > > Uncorrectable
> > > Error Status and the Uncorrectable Error Severity of the parent
> > > RCEC.
> > > In
> > > some non-native cases in which there is no OS-visible device
> > > associated
> > > with the RCiEP, there is nothing to act upon as the firmware is
> > > acting
> > > before the OS.
> > >
> > > Add handling for the linked RCEC in AER/ERR while taking into
> > > account
> > > non-native cases.
> > >
> > > Co-developed-by: Sean V Kelley <sean.v.kelley@xxxxxxxxx>
> > > Link:
> > > https://lore.kernel.org/r/20201002184735.1229220-12-seanvk.dev@xxxxxxxxxxxxxxxx
> > > Signed-off-by: Sean V Kelley <sean.v.kelley@xxxxxxxxx>
> > > Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@xxxxxxxxx>
> > > Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > > ---
> > > drivers/pci/pcie/aer.c | 53
> > > ++++++++++++++++++++++++++++++------------
> > > drivers/pci/pcie/err.c | 20 ++++++++--------
> > > 2 files changed, 48 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> > > index 65dff5f3457a..083f69b67bfd 100644
> > > --- a/drivers/pci/pcie/aer.c
> > > +++ b/drivers/pci/pcie/aer.c
> > > @@ -1357,27 +1357,50 @@ static int aer_probe(struct pcie_device
> > > *dev)
> > > */
> > > static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
> > > {
> > > -       int aer = dev->aer_cap;
> > > +       int type = pci_pcie_type(dev);
> > > +       struct pci_dev *root;
> > > +       int aer = 0;
> > > +       int rc = 0;
> > >         u32 reg32;
> > > -       int rc;
> > >
> > > +       if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END)
> >
> > "type == PCI_EXP_TYPE_RC_END"
>
> Right, I merged your suggested changes which added the type. Will
> correct.
>
> >
> > > +               /*
> > > +                * The reset should only clear the Root Error
> > > Status
> > > +                * of the RCEC. Only perform this for the
> > > +                * native case, i.e., an RCEC is present.
> > > +                */
> > > +               root = dev->rcec;
> > > +       else
> > > +               root = dev;
> > >
> > > -       /* Disable Root's interrupt in response to error messages
> > > */
> > > -       pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND,
> > > &reg32);
> > > -       reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> > > -       pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND,
> > > reg32);
> > > +       if (root)
> > > +               aer = dev->aer_cap;
> > >
> > > -       rc = pci_bus_error_reset(dev);
> > > -       pci_info(dev, "Root Port link has been reset\n");
> > > +       if (aer) {
> > > +               /* Disable Root's interrupt in response to error
> > > messages */
> > > +               pci_read_config_dword(root, aer +
> > > PCI_ERR_ROOT_COMMAND, &reg32);
> > > +               reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> > > +               pci_write_config_dword(root, aer +
> > > PCI_ERR_ROOT_COMMAND, reg32);
> >
> > Not directly related to *this* patch, but my assumption was that in
> > the APEI case, the firmware should retain ownership of the AER
> > Capability, so the OS should not touch PCI_ERR_ROOT_COMMAND and
> > PCI_ERR_ROOT_STATUS.
> >
> > But this code appears to ignore that ownership. Jonathan, you must
> > have looked at this recently for 068c29a248b6 ("PCI/ERR: Clear PCIe
> > Device Status errors only if OS owns AER"). Do you have any
> > insight
> > about this?
> >
> > > -       /* Clear Root Error Status */
> > > -       pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS,
> > > &reg32);
> > > -       pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS,
> > > reg32);
> > > +               /* Clear Root Error Status */
> > > +               pci_read_config_dword(root, aer +
> > > PCI_ERR_ROOT_STATUS, &reg32);
> > > +               pci_write_config_dword(root, aer +
> > > PCI_ERR_ROOT_STATUS, reg32);
> > >
> > > -       /* Enable Root Port's interrupt in response to error
> > > messages */
> > > -       pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND,
> > > &reg32);
> > > -       reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> > > -       pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND,
> > > reg32);
> > > +               /* Enable Root Port's interrupt in response to
> > > error messages */
> > > +               pci_read_config_dword(root, aer +
> > > PCI_ERR_ROOT_COMMAND, &reg32);
> > > +               reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> > > +               pci_write_config_dword(root, aer +
> > > PCI_ERR_ROOT_COMMAND, reg32);
> > > +       }
> > > +
> > > +       if ((type == PCI_EXP_TYPE_RC_EC) || (type ==
> > > PCI_EXP_TYPE_RC_END))
> > > {
> > > +               if (pcie_has_flr(root)) {
> > > +                       rc = pcie_flr(root);
> > > +                       pci_info(dev, "has been reset (%d)\n",
> > > rc);
> > > +               }
> > > +       } else {
> > > +               rc = pci_bus_error_reset(root);
> >
> > Don't we want "dev" for both the FLR and pci_bus_error_reset()? I
> > think "root == dev" except when dev is an RCiEP. When dev is an
> > RCiEP, "root" is the RCEC (if present), and we want to reset the
> > RCiEP, not the RCEC.
>
> Right, when I did the goto in the earlier incarnation, I always set
> root
> to dev at the start and in the merge it needs to be dev always except
> for the RC_END where RCEC exists. Will change without bringing back
> the
> goto…
>
> +       struct pci_dev *root = dev;
>
> …
>
> +non_native:
> +       if ((type == PCI_EXP_TYPE_RC_EC) || (type ==
> PCI_EXP_TYPE_RC_END)) {
> +               rc = flr_on_rc(root);
> +               pci_info(dev, "has been reset (%d)\n", rc);
> +       } else {
> +               rc = pci_bus_error_reset(root);
> +               pci_info(dev, "Root Port link has been reset (%d)\n",
> rc);
> +       }
>
>
> >
> > > +               pci_info(dev, "Root Port link has been reset
> > > (%d)\n", rc);
> > > +       }
> >
> > There are a couple changes here that I think should be split out.
> >
> > Based on my theory that when firmware retains control of AER, the
> > OS
> > should not touch PCI_ERR_ROOT_COMMAND and PCI_ERR_ROOT_STATUS, and
> > any
> > updates to them would have to be done by firmware before we get
> > here,
> > I suggested reordering this:
> >
> > - clear PCI_ERR_ROOT_COMMAND ROOT_PORT_INTR_ON_MESG_MASK
> > - do reset
> > - clear PCI_ERR_ROOT_STATUS (for APEI, presumably done by
> > firmware?)
> > - enable PCI_ERR_ROOT_COMMAND ROOT_PORT_INTR_ON_MESG_MASK
> >
> > to this:
> >
> > - clear PCI_ERR_ROOT_COMMAND ROOT_PORT_INTR_ON_MESG_MASK
> > - clear PCI_ERR_ROOT_STATUS
> > - enable PCI_ERR_ROOT_COMMAND ROOT_PORT_INTR_ON_MESG_MASK
> > - do reset
> >
> > If my theory is correct, I think we should still reorder this, but:
> >
> > - It's a significant behavior change that deserves its own patch
> > so
> >     we can document/bisect/revert.
> >
> > - I'm not sure why we clear the PCI_ERR_ROOT_COMMAND error
> > reporting
> >     bits. In the new "clear COMMAND, clear STATUS, enable COMMAND"
> >     order, it looks superfluous. There's no reason to disable
> > error
> >     reporting while clearing the status bits.
> >
> >     The current "clear, reset, enable" order suggests that the
> > reset
> >     might cause errors that we should ignore. I don't know whether
> >     that's the case or not. It dates from 6c2b374d7485 ("PCI-
> > Express
> >     AER implemetation: AER core and aerdriver"), which doesn't
> >     elaborate.
> >
> > - Should we also test for OS ownership of AER before touching
> >     PCI_ERR_ROOT_STATUS?
> >
> > - If we remove the PCI_ERR_ROOT_COMMAND fiddling (and I
> > tentatively
> >     think we *should* unless we can justify it), that would also
> >     deserve its own patch. Possibly (1) remove
> > PCI_ERR_ROOT_COMMAND
> >     fiddling, (2) reorder PCI_ERR_ROOT_STATUS clearing and reset,
> > (3)
> >     test for OS ownership of AER (?), (4) the rest of this patch.
>
> You’ve highlighted some good questions.

Reading Ethan's reply and also thinking about separation from an _OSC
perspective perhaps something like this could be done with a check in
aer_root_reset().

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 65dff5f3457a..70bf637042ff 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -1357,27 +1357,46 @@ static int aer_probe(struct pcie_device *dev)
*/
static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
{
+ int type = pci_pcie_type(dev);
+ struct pci_dev *root = dev;
int aer = dev->aer_cap;
+ int rc = 0;
u32 reg32;
- int rc;

+ if (!pcie_aer_is_native(dev))
+ return PCI_ERS_RESULT_RECOVERD;
+
+ if (type == PCI_EXP_TYPE_RC_END)
+ /*
+ * The reset should only clear the Root Error Status
+ * of the RCEC. Only perform this for the
+ * native case, i.e., an RCEC is present.
+ */
+ root = dev->rcec;

/* Disable Root's interrupt in response to error messages */
- pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
+ pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND,
&reg32);
reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
- pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
-
- rc = pci_bus_error_reset(dev);
- pci_info(dev, "Root Port link has been reset\n");
+ pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND,
reg32);

/* Clear Root Error Status */
- pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &reg32);
- pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, reg32);
+ pci_read_config_dword(root, aer + PCI_ERR_ROOT_STATUS, &reg32);
+ pci_write_config_dword(root, aer + PCI_ERR_ROOT_STATUS, reg32);

/* Enable Root Port's interrupt in response to error messages
*/
- pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
+ pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND,
&reg32);
reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
- pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
+ pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND,
reg32);
+
+ if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END)
{
+ if (pcie_has_flr(dev)) {
+ rc = pcie_flr(dev);
+ pci_info(dev, "has been reset (%d)\n", rc);
+ }
+ } else {
+ rc = pci_bus_error_reset(dev);
+ pci_info(dev, "Root Port link has been reset (%d)\n",
rc);
+ }

>
> I think we should remove the fiddling until we have a clearer picture
> and put that into its own patch.
>
> Sean
> >
> > >         return rc ? PCI_ERS_RESULT_DISCONNECT :
> > > PCI_ERS_RESULT_RECOVERED;
> > > }
> > > diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> > > index 7883c9791562..cbc5abfe767b 100644
> > > --- a/drivers/pci/pcie/err.c
> > > +++ b/drivers/pci/pcie/err.c
> > > @@ -148,10 +148,10 @@ static int report_resume(struct pci_dev
> > > *dev,
> > > void *data)
> > >
> > > /**
> > > * pci_walk_bridge - walk bridges potentially AER affected
> > > - * @bridge:    bridge which may be a Port, an RCEC with
> > > associated
> > > RCiEPs,
> > > - *             or an RCiEP associated with an RCEC
> > > - * @cb:                callback to be called for each device
> > > found
> > > - * @userdata:  arbitrary pointer to be passed to callback
> > > + * @bridge   bridge which may be an RCEC with associated RCiEPs,
> > > + *           or a Port.
> > > + * @cb       callback to be called for each device found
> > > + * @userdata arbitrary pointer to be passed to callback.
> > > *
> > > * If the device provided is a bridge, walk the subordinate bus,
> > > including
> > > * any bridged devices on buses under this bus. Call the
> > > provided
> > > callback
> > > @@ -164,8 +164,14 @@ static void pci_walk_bridge(struct pci_dev
> > > *bridge,
> > >                             int (*cb)(struct pci_dev *, void *),
> > >                             void *userdata)
> > > {
> > > +       /*
> > > +        * In a non-native case where there is no OS-visible
> > > reporting
> > > +        * device the bridge will be NULL, i.e., no RCEC, no
> > > Downstream
> > > Port.
> > > +        */
> > >         if (bridge->subordinate)
> > >                 pci_walk_bus(bridge->subordinate, cb, userdata);
> > > +       else if (bridge->rcec)
> > > +               cb(bridge->rcec, userdata);
> > >         else
> > >                 cb(bridge, userdata);
> > > }
> > > @@ -194,12 +200,6 @@ pci_ers_result_t pcie_do_recovery(struct
> > > pci_dev
> > > *dev,
> > >         pci_dbg(bridge, "broadcast error_detected message\n");
> > >         if (state == pci_channel_io_frozen) {
> > >                 pci_walk_bridge(bridge, report_frozen_detected,
> > > &status);
> > > -               if (type == PCI_EXP_TYPE_RC_END) {
> > > -                       pci_warn(dev, "subordinate device reset
> > > not possible for
> > > RCiEP\n");
> > > -                       status = PCI_ERS_RESULT_NONE;
> > > -                       goto failed;
> > > -               }
> > > -
> > >                 status = reset_subordinates(bridge);
> > >                 if (status != PCI_ERS_RESULT_RECOVERED) {
> > >                         pci_warn(bridge, "subordinate device
> > > reset failed\n");
> > > --
> > > 2.28.0
> > >

Next message: Palmer Dabbelt: "[GIT PULL] RISC-V Patches for the 5.10 Merge Window, Part 1"
Previous message: Joao Martins: "Re: [PATCH 00/35] Enhance memory utilization with DMEMFS"
In reply to: Sean V Kelley: "Re: [PATCH v9 12/15] PCI/RCEC: Add RCiEP's linked RCEC to AER/ERR"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]