From: Bjorn Helgaas <helgaas@kernel.org>
To: Hedi Berriche <hedi.berriche@hpe.com>
Cc: Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com>,
Ashok Raj <ashok.raj@intel.com>,
Bjorn Helgaas <bhelgaas@google.com>,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
Sinan Kaya <okaya@kernel.org>, Russ Anderson <rja@hpe.com>,
Joerg Roedel <jroedel@suse.com>,
stable@kernel.org, Keith Busch <kbusch@kernel.org>
Subject: Re: [PATCH v4 1/1] PCI/ERR: don't clobber status after reset_link()
Date: Fri, 8 Jan 2021 16:30:43 -0600 [thread overview]
Message-ID: <20210108223043.GA1477254@bjorn-Precision-5520> (raw)
In-Reply-To: <20201210224142.GA58424@bjorn-Precision-5520>
[+cc Keith]
On Thu, Dec 10, 2020 at 04:41:42PM -0600, Bjorn Helgaas wrote:
> On Mon, Nov 02, 2020 at 03:09:51PM +0000, Hedi Berriche wrote:
> > Commit 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
> > broke pcie_do_recovery(): updating status after reset_link() has the ill
> > side effect of causing recovery to fail if the error status is
> > PCI_ERS_RESULT_CAN_RECOVER or PCI_ERS_RESULT_NEED_RESET as the following
> > code will *never* run in the case of a successful reset_link()
> >
> > 177 if (status == PCI_ERS_RESULT_CAN_RECOVER) {
> > ...
> > 181 }
> >
> > 183 if (status == PCI_ERS_RESULT_NEED_RESET) {
> > ...
> > 192 }
>
> The line numbers are basically useless because they depend on some
> particular version of the file.
>
> > For instance in the case of PCI_ERS_RESULT_NEED_RESET we end up not
> > calling ->slot_reset() (because we skip report_slot_reset()) thus
> > breaking driver (re)initialisation.
> >
> > Don't clobber status with the return value of reset_link(); set status
> > to PCI_ERS_RESULT_RECOVERED, in case of successful link reset, if and
> > only if the initial value of error status is PCI_ERS_RESULT_DISCONNECT
> > or PCI_ERS_RESULT_NO_AER_DRIVER.
> >
> > Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
> > Signed-off-by: Hedi Berriche <hedi.berriche@hpe.com>
> >
> > Reviewed-by: Sinan Kaya <okaya@kernel.org>
> > Cc: Russ Anderson <rja@hpe.com>
> > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Ashok Raj <ashok.raj@intel.com>
> > Cc: Joerg Roedel <jroedel@suse.com>
> >
> > Cc: stable@kernel.org # v5.7+
> > ---
> > drivers/pci/pcie/err.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> > index c543f419d8f9..2730826cfd8a 100644
> > --- a/drivers/pci/pcie/err.c
> > +++ b/drivers/pci/pcie/err.c
> > @@ -165,10 +165,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> > pci_dbg(dev, "broadcast error_detected message\n");
> > if (state == pci_channel_io_frozen) {
> > pci_walk_bus(bus, report_frozen_detected, &status);
> > - status = reset_link(dev);
> > - if (status != PCI_ERS_RESULT_RECOVERED) {
> > + if (reset_link(dev) != PCI_ERS_RESULT_RECOVERED) {
> > pci_warn(dev, "link reset failed\n");
> > goto failed;
> > + } else {
> > + if (status == PCI_ERS_RESULT_DISCONNECT ||
> > + status == PCI_ERS_RESULT_NO_AER_DRIVER)
> > + status = PCI_ERS_RESULT_RECOVERED;
>
> This code (even before your patch) doesn't match
> Documentation/PCI/pci-error-recovery.rst very well. The code handles
> pci_channel_io_frozen specially, but I don't think this is mentioned
> in the doc.
>
> The doc says we call ->error_detected() for all affected drivers.
> Then we're supposed to do a slot reset if any driver returned
> NEED_RESET. But in fact, we always do a reset for the
> pci_channel_io_frozen case and never do one otherwise, regardless of
> what ->error_detected() returned.
>
> The doc says DISCONNECT means "Driver ... doesn't want to recover at
> all." Many drivers can return either NEED_RESET or DISCONNECT, and I
> assume they expect them to be handled differently. But I'm not sure
> what DISCONNECT really means. Do we reset the device? Do we not
> attempt recovery at all?
>
> After your patch, if the reset_link() succeeded, we convert DISCONNECT
> and NO_AER_DRIVER to RECOVERED. IIUC, that means we do exactly the
> same thing if the consensus of the ->error_detected() functions was
> RECOVERED, DISCONNECT, or NO_AER_DRIVER: we call reset_link() and
> continue with "status = PCI_ERS_RESULT_RECOVERED".
>
> (I'd reverse the sense of the "if (reset_link())" to make this easier
> to read)
Can we push this forward now? There are several pending patches in
this area from Keith and Sathyanarayanan; I haven't gotten to them
yet, so not sure whether they help address any of this.
> > }
> > } else {
> > pci_walk_bus(bus, report_normal_detected, &status);
> > --
> > 2.28.0
> >
next prev parent reply other threads:[~2021-01-08 22:31 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-02 15:09 [PATCH v4 0/1] PCI/ERR: fix regression introduced by 6d2c89441571 ("PCI/ERR: Update error status after reset_link()") Hedi Berriche
2020-11-02 15:09 ` [PATCH v4 1/1] PCI/ERR: don't clobber status after reset_link() Hedi Berriche
2020-11-12 15:59 ` Hedi Berriche
2020-12-10 22:41 ` Bjorn Helgaas
2021-01-08 22:30 ` Bjorn Helgaas [this message]
2021-01-09 4:04 ` Kuppuswamy, Sathyanarayanan
2021-02-08 12:33 ` Hedi Berriche
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=20210108223043.GA1477254@bjorn-Precision-5520 \
--to=helgaas@kernel.org \
--cc=ashok.raj@intel.com \
--cc=bhelgaas@google.com \
--cc=hedi.berriche@hpe.com \
--cc=jroedel@suse.com \
--cc=kbusch@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=okaya@kernel.org \
--cc=rja@hpe.com \
--cc=sathyanarayanan.kuppuswamy@linux.intel.com \
--cc=stable@kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).