From: Bjorn Helgaas <helgaas@kernel.org>
To: sathyanarayanan.kuppuswamy@linux.intel.com
Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
ashok.raj@intel.com, Keith Busch <keith.busch@intel.com>
Subject: Re: [PATCH v12 5/8] PCI/AER: Allow clearing Error Status Register in FF mode
Date: Fri, 17 Jan 2020 16:56:04 -0600 [thread overview]
Message-ID: <20200117220902.GA139245@google.com> (raw)
In-Reply-To: <e4fa9c6253fd2bfc49746f98e4024fb4980c8936.1578682741.git.sathyanarayanan.kuppuswamy@linux.intel.com>
On Sun, Jan 12, 2020 at 02:43:59PM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> As per PCI firmware specification r3.2 System Firmware Intermediary
> (SFI) _OSC and DPC Updates ECR
> (https://members.pcisig.com/wg/PCI-SIG/document/13563), sec titled
> "DPC Event Handling Implementation Note", page 10, Error Disconnect
> Recover (EDR) support allows OS to handle error recovery and clearing
> Error Registers even in FF mode. So create exception for FF mode checks
> in pci_cleanup_aer_uncorrect_error_status(), pci_aer_clear_fatal_status()
> and pci_cleanup_aer_error_status_regs() functions when its being called
> from DPC code path.
>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Acked-by: Keith Busch <keith.busch@intel.com>
> ---
> Documentation/PCI/pcieaer-howto.rst | 2 +-
> drivers/net/ethernet/intel/ice/ice_main.c | 2 +-
> drivers/ntb/hw/idt/ntb_hw_idt.c | 2 +-
> drivers/pci/pci.c | 2 +-
> drivers/pci/pci.h | 2 +-
> drivers/pci/pcie/aer.c | 16 ++++++++--------
> drivers/pci/pcie/dpc.c | 4 ++--
> drivers/pci/pcie/err.c | 2 +-
> drivers/scsi/lpfc/lpfc_attr.c | 2 +-
> include/linux/aer.h | 12 ++++++++----
> 10 files changed, 25 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/PCI/pcieaer-howto.rst b/Documentation/PCI/pcieaer-howto.rst
> index 18bdefaafd1a..184c966b61cb 100644
> --- a/Documentation/PCI/pcieaer-howto.rst
> +++ b/Documentation/PCI/pcieaer-howto.rst
> @@ -243,7 +243,7 @@ messages to root port when an error is detected.
>
> ::
>
> - int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev);`
> + int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev, bool ff_check);
>
> pci_cleanup_aer_uncorrect_error_status cleanups the uncorrectable
> error status register.
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index 69bff085acf7..8378dbf3500b 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -3491,7 +3491,7 @@ static pci_ers_result_t ice_pci_err_slot_reset(struct pci_dev *pdev)
> result = PCI_ERS_RESULT_DISCONNECT;
> }
>
> - err = pci_cleanup_aer_uncorrect_error_status(pdev);
> + err = pci_cleanup_aer_uncorrect_error_status(pdev, 1);
> if (err)
> dev_dbg(&pdev->dev,
> "pci_cleanup_aer_uncorrect_error_status failed, error %d\n",
> diff --git a/drivers/ntb/hw/idt/ntb_hw_idt.c b/drivers/ntb/hw/idt/ntb_hw_idt.c
> index dcf234680535..9023308be2de 100644
> --- a/drivers/ntb/hw/idt/ntb_hw_idt.c
> +++ b/drivers/ntb/hw/idt/ntb_hw_idt.c
> @@ -2675,7 +2675,7 @@ static int idt_init_pci(struct idt_ntb_dev *ndev)
> if (ret != 0)
> dev_warn(&pdev->dev, "PCIe AER capability disabled\n");
> else /* Cleanup uncorrectable error status before getting to init */
> - pci_cleanup_aer_uncorrect_error_status(pdev);
> + pci_cleanup_aer_uncorrect_error_status(pdev, 1);
>
> /* First enable the PCI device */
> ret = pcim_enable_device(pdev);
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 63366344acff..5bd72d9343f6 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -280,8 +280,8 @@ static void dpc_process_error(struct dpc_dev *dpc)
> dpc_get_aer_uncorrect_severity(pdev, &info) &&
> aer_get_device_error_info(pdev, &info)) {
> aer_print_error(pdev, &info);
> - pci_cleanup_aer_uncorrect_error_status(pdev);
> - pci_aer_clear_fatal_status(pdev);
> + pci_cleanup_aer_uncorrect_error_status(pdev, 0);
> + pci_aer_clear_fatal_status(pdev, 0);
> }
>
> /* We configure DPC so it only triggers on ERR_FATAL */
> diff --git a/drivers/scsi/lpfc/lpfc_attr.c b/drivers/scsi/lpfc/lpfc_attr.c
> index 4ff82b36a37a..5b37efd29e20 100644
> --- a/drivers/scsi/lpfc/lpfc_attr.c
> +++ b/drivers/scsi/lpfc/lpfc_attr.c
> @@ -4810,7 +4810,7 @@ lpfc_aer_cleanup_state(struct device *dev, struct device_attribute *attr,
> return -EINVAL;
>
> if (phba->hba_flag & HBA_AER_ENABLED)
> - rc = pci_cleanup_aer_uncorrect_error_status(phba->pcidev);
> + rc = pci_cleanup_aer_uncorrect_error_status(phba->pcidev, 1);
>
> if (rc == 0)
> return strlen(buf);
> diff --git a/include/linux/aer.h b/include/linux/aer.h
> index fa19e01f418a..f4f49df500ad 100644
> --- a/include/linux/aer.h
> +++ b/include/linux/aer.h
> @@ -44,8 +44,10 @@ struct aer_capability_regs {
> /* PCIe port driver needs this function to enable AER */
> int pci_enable_pcie_error_reporting(struct pci_dev *dev);
> int pci_disable_pcie_error_reporting(struct pci_dev *dev);
> -int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev);
> -int pci_cleanup_aer_error_status_regs(struct pci_dev *dev);
> +int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev,
> + bool ff_check);
IIUC your intent is that drivers should only call this with
ff_check=1, and only the single case in dpc.c should call it with
ff_check=0. But the existence of that parameter creates an
opportunity for drivers to do it wrong.
The "ff_check==0" case needs to be internal to drivers/pci and not
even visible to drivers. E.g., you can leave the interface of
pci_cleanup_aer_uncorrect_error_status() alone and make it call
internal-to-PCI function.
There should be no changes to drivers or the documentation.
> +int pci_cleanup_aer_error_status_regs(struct pci_dev *dev,
> + bool ff_check);
Same for this.
pci_cleanup_aer_error_status_regs() is only used inside drivers/pci
anyway, so should probably be moved out of linux/aer.h.
> void pci_save_aer_state(struct pci_dev *dev);
> void pci_restore_aer_state(struct pci_dev *dev);
> #else
> @@ -57,11 +59,13 @@ static inline int pci_disable_pcie_error_reporting(struct pci_dev *dev)
> {
> return -EINVAL;
> }
> -static inline int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
> +static inline int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev,
> + bool ff_check)
> {
> return -EINVAL;
> }
> -static inline int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
> +static inline int pci_cleanup_aer_error_status_regs(struct pci_dev *dev,
> + bool ff_check)
> {
> return -EINVAL;
> }
> --
> 2.21.0
>
next prev parent reply other threads:[~2020-01-17 22:56 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-12 22:43 [PATCH v12 0/8] Add Error Disconnect Recover (EDR) support sathyanarayanan.kuppuswamy
2020-01-12 22:43 ` [PATCH v12 1/8] PCI/ERR: Update error status after reset_link() sathyanarayanan.kuppuswamy
2020-01-12 22:43 ` [PATCH v12 2/8] PCI/DPC: Allow dpc_probe() even if firmware first mode is enabled sathyanarayanan.kuppuswamy
2020-01-12 22:43 ` [PATCH v12 3/8] PCI/DPC: Add dpc_process_error() wrapper function sathyanarayanan.kuppuswamy
2020-01-12 22:43 ` [PATCH v12 4/8] PCI/DPC: Add Error Disconnect Recover (EDR) support sathyanarayanan.kuppuswamy
2020-01-17 22:56 ` Bjorn Helgaas
2020-01-12 22:43 ` [PATCH v12 5/8] PCI/AER: Allow clearing Error Status Register in FF mode sathyanarayanan.kuppuswamy
2020-01-17 22:56 ` Bjorn Helgaas [this message]
2020-01-12 22:44 ` [PATCH v12 6/8] PCI/DPC: Update comments related to DPC recovery on NON_FATAL errors sathyanarayanan.kuppuswamy
2020-01-12 22:44 ` [PATCH v12 7/8] PCI/DPC: Clear AER registers in EDR mode sathyanarayanan.kuppuswamy
2020-01-12 22:44 ` [PATCH v12 8/8] PCI/ACPI: Enable EDR support sathyanarayanan.kuppuswamy
2020-01-16 22:10 ` Bjorn Helgaas
2020-01-16 22:24 ` Kuppuswamy Sathyanarayanan
2020-01-16 22:58 ` Bjorn Helgaas
2020-01-16 23:01 ` Kuppuswamy Sathyanarayanan
2020-01-17 20:41 ` Bjorn Helgaas
2020-01-17 23:04 ` Kuppuswamy Sathyanarayanan
2020-01-14 19:48 ` [PATCH v12 0/8] Add Error Disconnect Recover (EDR) support Bjorn Helgaas
2020-01-18 0:18 ` Bjorn Helgaas
2020-01-18 1:10 ` Kuppuswamy Sathyanarayanan
2020-01-18 15:15 ` Bjorn Helgaas
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=20200117220902.GA139245@google.com \
--to=helgaas@kernel.org \
--cc=ashok.raj@intel.com \
--cc=keith.busch@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=sathyanarayanan.kuppuswamy@linux.intel.com \
/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).