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

From: Sathyanarayanan Kuppuswamy  <sathyanarayanan.kuppuswamy@linux.intel.com>
To: Bjorn Helgaas <helgaas@kernel.org>, linux-pci@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>,
	Stefan Roese <sr@denx.de>, Ashok Raj <ashok.raj@intel.com>,
	Keith Busch <kbusch@kernel.org>
Subject: Re: [PATCH 1/9] PCI/AER: Remove redundant Device Control Error Reporting Enable
Date: Wed, 18 Jan 2023 19:49:06 -0800	[thread overview]
Message-ID: <001efe2e-0ae8-e3fe-f872-d5401ec802c9@linux.intel.com> (raw)
In-Reply-To: <20230118234612.272916-2-helgaas@kernel.org>

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;

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer