linux-kernel - Re: [PATCH v12 5/8] PCI/AER: Allow clearing Error Status Register in FF mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200117220902.GA139245@google.com>
Date:   Fri, 17 Jan 2020 16:56:04 -0600
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     sathyanarayanan.kuppuswamy@...ux.intel.com
Cc:     linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        ashok.raj@...el.com, Keith Busch <keith.busch@...el.com>
Subject: Re: [PATCH v12 5/8] PCI/AER: Allow clearing Error Status Register in
 FF mode

On Sun, Jan 12, 2020 at 02:43:59PM -0800, sathyanarayanan.kuppuswamy@...ux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.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@...ux.intel.com>
> Acked-by: Keith Busch <keith.busch@...el.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
>