Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 14 Jul 2020 18:08:03 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     sathyanarayanan.kuppuswamy@linux.intel.com
Cc:     bhelgaas@google.com, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org, ashok.raj@intel.com,
        Jay Vosburgh <jay.vosburgh@canonical.com>
Subject: Re: [PATCH v2 1/2] PCI/ERR: Fix fatal error recovery for non-hotplug
 capable devices
Message-ID: <20200714230803.GA92891@bjorn-Precision-5520>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ce417fbf81a8a46a89535f44b9224ee9fbb55a29.1591307288.git.sathyanarayanan.kuppuswamy@linux.intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jun 04, 2020 at 02:50:01PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> Fatal (DPC) error recovery is currently broken for non-hotplug
> capable devices. With current implementation, after successful
> fatal error recovery, non-hotplug capable device state won't be
> restored properly. You can find related issues in following links.
> 
> https://lkml.org/lkml/2020/5/27/290
> https://lore.kernel.org/linux-pci/12115.1588207324@famine/
> https://lkml.org/lkml/2020/3/28/328

Can you please convert these all to lore.kernel.org links?  lkml.org
is not quite as useful or reliable.

> Current fatal error recovery implementation relies on hotplug handler
> for detaching/re-enumerating the affected devices/drivers on DLLSC
> state changes. 

Can you remind us exactly how this relies on hotplug?  I know it
*does*, but I can't remember how.  It would sure be nice if we could
decouple this from pciehp somehow.

> So when dealing with non-hotplug capable devices,
> recovery code does not restore the state of the affected devices
> correctly. Correct implementation should call report_slot_reset()
> function after resetting the link to restore the state of the
> device/driver.

We don't restore the state correctly.  What does this look like to the
user?  Does the device not work?

> So use PCI_ERS_RESULT_NEED_RESET as error status for successful
> reset_link() operation and use PCI_ERS_RESULT_DISCONNECT for failure
> case. PCI_ERS_RESULT_NEED_RESET error state will ensure slot_reset()
> is called after reset link operation which will also fix the above
> mentioned issue.

I think PCI_ERS_RESULT_NEED_RESET results in calling driver
->slot_reset() callbacks, right?  Where does the state restoration
happen?

No, I guess it must be something in the hotplug driver that restores
the state, because you said devices below hotplug-capable ports work
correctly, but others don't.

> [original patch is from jay.vosburgh@canonical.com]
> [original patch link https://lore.kernel.org/linux-pci/12115.1588207324@famine/]
> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/pci/pcie/err.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index 14bb8f54723e..5fe8561c7185 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -165,8 +165,28 @@ 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) {
> +		/*
> +		 * After resetting the link using reset_link() call, the
> +		 * possible value of error status is either
> +		 * PCI_ERS_RESULT_DISCONNECT (failure case) or
> +		 * PCI_ERS_RESULT_NEED_RESET (success case).
> +		 * So ignore the return value of report_error_detected()
> +		 * call for fatal errors. Instead use
> +		 * PCI_ERS_RESULT_NEED_RESET as initial status value.
> +		 *
> +		 * Ignoring the status return value of report_error_detected()
> +		 * call will also help in case of EDR mode based error
> +		 * recovery. In EDR mode AER and DPC Capabilities are owned by
> +		 * firmware and hence report_error_detected() call will possibly
> +		 * return PCI_ERS_RESULT_NO_AER_DRIVER. So if we don't ignore
> +		 * the return value of report_error_detected() then
> +		 * pcie_do_recovery() would report incorrect status after
> +		 * successful recovery. Ignoring PCI_ERS_RESULT_NO_AER_DRIVER
> +		 * in non EDR case should not have any functional impact.

I can't make sense out of the comment.  We already ignore the "status"
from pci_walk_bus(bus, report_frozen_detected, &status).

No idea what to make of the second paragraph.  If we make the commit
log make sense, maybe some summary of that would be useful here.

I think this code is equivalent and makes the patch much clearer:

  status = reset_link(dev);
  if (status == PCI_ERS_RESULT_RECOVERED) {
    status = PCI_ERS_RESULT_NEED_RESET;
  } else {
    status = PCI_ERS_RESULT_DISCONNECT;
    goto failed;
  }

> +		 */
> +		status = PCI_ERS_RESULT_NEED_RESET;
> +		if (reset_link(dev) != PCI_ERS_RESULT_RECOVERED) {
> +			status = PCI_ERS_RESULT_DISCONNECT;
>  			pci_warn(dev, "link reset failed\n");
>  			goto failed;
>  		}
> -- 
> 2.17.1
>