diff mbox series

[1/9] PCI/AER: Remove redundant Device Control Error Reporting Enable

Message ID 20230118234612.272916-2-helgaas@kernel.org (mailing list archive)
State Accepted
Headers show
Series PCI/AER: Remove redundant Device Control Error Reporting Enable | expand

Commit Message

Bjorn Helgaas Jan. 18, 2023, 11:46 p.m. UTC 
From: Bjorn Helgaas <bhelgaas@google.com>

The following bits in the PCIe Device Control register enable sending of
ERR_COR, ERR_NONFATAL, or ERR_FATAL Messages (or reporting internally in
the case of Root Ports):

  Correctable Error Reporting Enable
  Non-Fatal Error Reporting Enable
  Fatal Error Reporting Enable
  Unsupported Request Reporting Enable

These enable bits are set by pci_enable_pcie_error_reporting(), and since
f26e58bf6f54 ("PCI/AER: Enable error reporting when AER is native"), we
do that in this path during enumeration:

  pci_init_capabilities
    pci_aer_init
      pci_enable_pcie_error_reporting

Previously, the AER service driver also traversed the hierarchy when
claiming a Root Port, enabling error reporting for downstream devices, but
this is redundant.

Remove the code that enables this error reporting in the AER .probe() path.
Also remove similar code that disables error reporting in the AER .remove()
path.

Note that these Device Control Reporting Enable bits do not control
interrupt generation.  That's done by the similarly-named bits in the AER
Root Error Command register, which are still set by aer_probe() and cleared
by aer_remove(), since the AER service driver handles those interrupts.
See PCIe r6.0, sec 6.2.6.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: Stefan Roese <sr@denx.de>
Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Keith Busch <kbusch@kernel.org>
---
 drivers/pci/pcie/aer.c | 48 ------------------------------------------
 1 file changed, 48 deletions(-)

Comments

Kuppuswamy Sathyanarayanan Jan. 19, 2023, 3:49 a.m. UTC | #1 
On 1/18/23 3:46 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> The following bits in the PCIe Device Control register enable sending of
> ERR_COR, ERR_NONFATAL, or ERR_FATAL Messages (or reporting internally in
> the case of Root Ports):
> 
>   Correctable Error Reporting Enable
>   Non-Fatal Error Reporting Enable
>   Fatal Error Reporting Enable
>   Unsupported Request Reporting Enable
> 
> These enable bits are set by pci_enable_pcie_error_reporting(), and since
> f26e58bf6f54 ("PCI/AER: Enable error reporting when AER is native"), we
> do that in this path during enumeration:
> 
>   pci_init_capabilities
>     pci_aer_init
>       pci_enable_pcie_error_reporting
> 
> Previously, the AER service driver also traversed the hierarchy when
> claiming a Root Port, enabling error reporting for downstream devices, but
> this is redundant.
> 
> Remove the code that enables this error reporting in the AER .probe() path.
> Also remove similar code that disables error reporting in the AER .remove()
> path.
> 
> Note that these Device Control Reporting Enable bits do not control
> interrupt generation.  That's done by the similarly-named bits in the AER
> Root Error Command register, which are still set by aer_probe() and cleared
> by aer_remove(), since the AER service driver handles those interrupts.
> See PCIe r6.0, sec 6.2.6.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Cc: Ashok Raj <ashok.raj@intel.com>
> Cc: Keith Busch <kbusch@kernel.org>
> ---

Looks fine to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>  drivers/pci/pcie/aer.c | 48 ------------------------------------------
>  1 file changed, 48 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 625f7b2cafe4..b7b69e0c778c 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -1224,42 +1224,6 @@ static irqreturn_t aer_irq(int irq, void *context)
>  	return IRQ_WAKE_THREAD;
>  }
>  
> -static int set_device_error_reporting(struct pci_dev *dev, void *data)
> -{
> -	bool enable = *((bool *)data);
> -	int type = pci_pcie_type(dev);
> -
> -	if ((type == PCI_EXP_TYPE_ROOT_PORT) ||
> -	    (type == PCI_EXP_TYPE_RC_EC) ||
> -	    (type == PCI_EXP_TYPE_UPSTREAM) ||
> -	    (type == PCI_EXP_TYPE_DOWNSTREAM)) {
> -		if (enable)
> -			pci_enable_pcie_error_reporting(dev);
> -		else
> -			pci_disable_pcie_error_reporting(dev);
> -	}
> -
> -	return 0;
> -}
> -
> -/**
> - * set_downstream_devices_error_reporting - enable/disable the error reporting  bits on the root port and its downstream ports.
> - * @dev: pointer to root port's pci_dev data structure
> - * @enable: true = enable error reporting, false = disable error reporting.
> - */
> -static void set_downstream_devices_error_reporting(struct pci_dev *dev,
> -						   bool enable)
> -{
> -	set_device_error_reporting(dev, &enable);
> -
> -	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
> -		pcie_walk_rcec(dev, set_device_error_reporting, &enable);
> -	else if (dev->subordinate)
> -		pci_walk_bus(dev->subordinate, set_device_error_reporting,
> -			     &enable);
> -
> -}
> -
>  /**
>   * aer_enable_rootport - enable Root Port's interrupts when receiving messages
>   * @rpc: pointer to a Root Port data structure
> @@ -1289,12 +1253,6 @@ static void aer_enable_rootport(struct aer_rpc *rpc)
>  	pci_read_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, &reg32);
>  	pci_write_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, reg32);
>  
> -	/*
> -	 * Enable error reporting for the root port device and downstream port
> -	 * devices.
> -	 */
> -	set_downstream_devices_error_reporting(pdev, true);
> -
>  	/* Enable Root Port's interrupt in response to error messages */
>  	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>  	reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> @@ -1313,12 +1271,6 @@ static void aer_disable_rootport(struct aer_rpc *rpc)
>  	int aer = pdev->aer_cap;
>  	u32 reg32;
>  
> -	/*
> -	 * Disable error reporting for the root port device and downstream port
> -	 * devices.
> -	 */
> -	set_downstream_devices_error_reporting(pdev, false);
> -
>  	/* Disable Root's interrupt in response to error messages */
>  	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>  	reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
Stefan Roese Jan. 24, 2023, 7:22 a.m. UTC | #2 
On 1/19/23 00:46, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> The following bits in the PCIe Device Control register enable sending of
> ERR_COR, ERR_NONFATAL, or ERR_FATAL Messages (or reporting internally in
> the case of Root Ports):
> 
>    Correctable Error Reporting Enable
>    Non-Fatal Error Reporting Enable
>    Fatal Error Reporting Enable
>    Unsupported Request Reporting Enable
> 
> These enable bits are set by pci_enable_pcie_error_reporting(), and since
> f26e58bf6f54 ("PCI/AER: Enable error reporting when AER is native"), we
> do that in this path during enumeration:
> 
>    pci_init_capabilities
>      pci_aer_init
>        pci_enable_pcie_error_reporting
> 
> Previously, the AER service driver also traversed the hierarchy when
> claiming a Root Port, enabling error reporting for downstream devices, but
> this is redundant.
> 
> Remove the code that enables this error reporting in the AER .probe() path.
> Also remove similar code that disables error reporting in the AER .remove()
> path.
> 
> Note that these Device Control Reporting Enable bits do not control
> interrupt generation.  That's done by the similarly-named bits in the AER
> Root Error Command register, which are still set by aer_probe() and cleared
> by aer_remove(), since the AER service driver handles those interrupts.
> See PCIe r6.0, sec 6.2.6.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Cc: Ashok Raj <ashok.raj@intel.com>
> Cc: Keith Busch <kbusch@kernel.org>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   drivers/pci/pcie/aer.c | 48 ------------------------------------------
>   1 file changed, 48 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 625f7b2cafe4..b7b69e0c778c 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -1224,42 +1224,6 @@ static irqreturn_t aer_irq(int irq, void *context)
>   	return IRQ_WAKE_THREAD;
>   }
>   
> -static int set_device_error_reporting(struct pci_dev *dev, void *data)
> -{
> -	bool enable = *((bool *)data);
> -	int type = pci_pcie_type(dev);
> -
> -	if ((type == PCI_EXP_TYPE_ROOT_PORT) ||
> -	    (type == PCI_EXP_TYPE_RC_EC) ||
> -	    (type == PCI_EXP_TYPE_UPSTREAM) ||
> -	    (type == PCI_EXP_TYPE_DOWNSTREAM)) {
> -		if (enable)
> -			pci_enable_pcie_error_reporting(dev);
> -		else
> -			pci_disable_pcie_error_reporting(dev);
> -	}
> -
> -	return 0;
> -}
> -
> -/**
> - * set_downstream_devices_error_reporting - enable/disable the error reporting  bits on the root port and its downstream ports.
> - * @dev: pointer to root port's pci_dev data structure
> - * @enable: true = enable error reporting, false = disable error reporting.
> - */
> -static void set_downstream_devices_error_reporting(struct pci_dev *dev,
> -						   bool enable)
> -{
> -	set_device_error_reporting(dev, &enable);
> -
> -	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
> -		pcie_walk_rcec(dev, set_device_error_reporting, &enable);
> -	else if (dev->subordinate)
> -		pci_walk_bus(dev->subordinate, set_device_error_reporting,
> -			     &enable);
> -
> -}
> -
>   /**
>    * aer_enable_rootport - enable Root Port's interrupts when receiving messages
>    * @rpc: pointer to a Root Port data structure
> @@ -1289,12 +1253,6 @@ static void aer_enable_rootport(struct aer_rpc *rpc)
>   	pci_read_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, &reg32);
>   	pci_write_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, reg32);
>   
> -	/*
> -	 * Enable error reporting for the root port device and downstream port
> -	 * devices.
> -	 */
> -	set_downstream_devices_error_reporting(pdev, true);
> -
>   	/* Enable Root Port's interrupt in response to error messages */
>   	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>   	reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> @@ -1313,12 +1271,6 @@ static void aer_disable_rootport(struct aer_rpc *rpc)
>   	int aer = pdev->aer_cap;
>   	u32 reg32;
>   
> -	/*
> -	 * Disable error reporting for the root port device and downstream port
> -	 * devices.
> -	 */
> -	set_downstream_devices_error_reporting(pdev, false);
> -
>   	/* Disable Root's interrupt in response to error messages */
>   	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>   	reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;

Viele Grüße,
Stefan Roese
diff mbox series

Patch

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 625f7b2cafe4..b7b69e0c778c 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -1224,42 +1224,6 @@  static irqreturn_t aer_irq(int irq, void *context)
 	return IRQ_WAKE_THREAD;
 }
 
-static int set_device_error_reporting(struct pci_dev *dev, void *data)
-{
-	bool enable = *((bool *)data);
-	int type = pci_pcie_type(dev);
-
-	if ((type == PCI_EXP_TYPE_ROOT_PORT) ||
-	    (type == PCI_EXP_TYPE_RC_EC) ||
-	    (type == PCI_EXP_TYPE_UPSTREAM) ||
-	    (type == PCI_EXP_TYPE_DOWNSTREAM)) {
-		if (enable)
-			pci_enable_pcie_error_reporting(dev);
-		else
-			pci_disable_pcie_error_reporting(dev);
-	}
-
-	return 0;
-}
-
-/**
- * set_downstream_devices_error_reporting - enable/disable the error reporting  bits on the root port and its downstream ports.
- * @dev: pointer to root port's pci_dev data structure
- * @enable: true = enable error reporting, false = disable error reporting.
- */
-static void set_downstream_devices_error_reporting(struct pci_dev *dev,
-						   bool enable)
-{
-	set_device_error_reporting(dev, &enable);
-
-	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
-		pcie_walk_rcec(dev, set_device_error_reporting, &enable);
-	else if (dev->subordinate)
-		pci_walk_bus(dev->subordinate, set_device_error_reporting,
-			     &enable);
-
-}
-
 /**
  * aer_enable_rootport - enable Root Port's interrupts when receiving messages
  * @rpc: pointer to a Root Port data structure
@@ -1289,12 +1253,6 @@  static void aer_enable_rootport(struct aer_rpc *rpc)
 	pci_read_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, &reg32);
 	pci_write_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, reg32);
 
-	/*
-	 * Enable error reporting for the root port device and downstream port
-	 * devices.
-	 */
-	set_downstream_devices_error_reporting(pdev, true);
-
 	/* Enable Root Port's interrupt in response to error messages */
 	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
 	reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
@@ -1313,12 +1271,6 @@  static void aer_disable_rootport(struct aer_rpc *rpc)
 	int aer = pdev->aer_cap;
 	u32 reg32;
 
-	/*
-	 * Disable error reporting for the root port device and downstream port
-	 * devices.
-	 */
-	set_downstream_devices_error_reporting(pdev, false);
-
 	/* Disable Root's interrupt in response to error messages */
 	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
 	reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;