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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.