Re: [PATCH 1/1] powerpc/eeh: Prevent EEH false positives on PMCSR reads in D3cold

[Vaibhav Jain <vaibhav@linux.ibm.com>]

Hi Narayana,

Thanks for the patch. My review comments inline below:
Narayana Murty N <nnmlinux@linux.ibm.com> writes:

> On pseries systems, RTAS-based PCI config space reads can trigger
> false EEH events when attempting to read the Power Management Control
> Status Register (PMCSR) while a device is in D3cold state. This occurs
> because the device is powered off and cannot respond to config space
> accesses, causing the platform to report an error condition.
Can you share more details of the test-case(s) in which PMCSR register
read is triggered while the PCI device is in D3Cold state.


>
> This patch addresses the issue by:
>
> 1. Caching PM capability information in struct eeh_dev:
>    - pm_cap: Offset of the PM capability structure
>    - pmcsr_offset: Absolute config space offset of PMCSR register
>
> 2. Synchronizing PM capability data between pci_dev and eeh_dev:
>    - During device probe (eeh_probe_device), copy pm_cap from pci_dev
>    - During early init (pseries_eeh_init_edev), discover pm_cap via
>      firmware before pci_dev exists
>
> 3. Intercepting PMCSR reads in D3cold state:
>    - In rtas_pci_dn_write_config, detect PMCSR read attempts
>    - Check if device is in D3cold via pdev->current_state
>    - Return synthetic success value instead of performing RTAS call
>    - Prevents hardware access that would trigger false EEH event
>
> The fix handles both early boot (before pci_dev exists) and runtime
> scenarios, ensuring PM capability information is always available when
> needed. By blocking RTAS reads to PMCSR when devices are in D3cold,
> we prevent spurious EEH events while maintaining proper error detection
> for genuine hardware failures.

The patch is adding extra branch in rtas_pci_dn_write_config() which is
the hot path for PCI device config space access. Can you share details
on the extra overhead the patch introduces in rtas_pci_dn_write_config()

That being said:
I prefer access to PMCSR being fixed at the call sites which may be
trigerring EEH you have mentioned above instead of adding a constant
overhead to rtas_pci_dn_write_config() 

>
> Signed-off-by: Narayana Murty N <nnmlinux@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/eeh.h               |  9 +++++++
>  arch/powerpc/kernel/eeh.c                    | 16 ++++++++++++
>  arch/powerpc/kernel/rtas_pci.c               | 26 ++++++++++++++++++++
>  arch/powerpc/platforms/pseries/eeh_pseries.c | 16 ++++++++++++
>  4 files changed, 67 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
> index b7ebb4ac2c71..224a3adcd34e 100644
> --- a/arch/powerpc/include/asm/eeh.h
> +++ b/arch/powerpc/include/asm/eeh.h
> @@ -139,6 +139,15 @@ struct eeh_dev {
>  	int pcie_cap;			/* Saved PCIe capability	*/
>  	int aer_cap;			/* Saved AER capability		*/
>  	int af_cap;			/* Saved AF capability		*/
> +	/*
> +	 * Cached PCI PM capability information.
> +	 * pm_cap == 0 means the device does not have PCI PM capability
> +	 * or it has not been discovered yet.
> +	 * pmcsr_offset is the absolute config-space offset of PMCSR:
> +	 *     pm_cap + PCI_PM_CTRL
> +	 */
> +	u8 pm_cap;
> +	u16 pmcsr_offset;
>  	struct eeh_pe *pe;		/* Associated PE		*/
>  	struct list_head entry;		/* Membership in eeh_pe.edevs	*/
>  	struct list_head rmv_entry;	/* Membership in rmv_list	*/
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index bb836f02101c..4402166df8c0 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -997,6 +997,21 @@ int eeh_init(struct eeh_ops *ops)
>  	return eeh_event_init();
>  }
>  
> +#ifdef CONFIG_EEH
> +static void eeh_sync_pm_cap(struct eeh_dev *edev, struct pci_dev *pdev)
> +{
> +	if (!edev || !pdev)
> +		return;
> +
> +	/*
> +	 * Prefer PCI core cached PM capability once pci_dev exists.
> +	 * If pm_cap is zero, clear pmcsr_offset as well.
> +	 */
> +	edev->pm_cap = pdev->pm_cap;
> +	edev->pmcsr_offset = pdev->pm_cap ? pdev->pm_cap + PCI_PM_CTRL : 0;
> +}
> +#endif
> +
>  /**
>   * eeh_probe_device() - Perform EEH initialization for the indicated pci device
>   * @dev: pci device for which to set up EEH
> @@ -1050,6 +1065,7 @@ void eeh_probe_device(struct pci_dev *dev)
>  	/* bind the pdev and the edev together */
>  	edev->pdev = dev;
>  	dev->dev.archdata.edev = edev;
> +	eeh_sync_pm_cap(edev, dev);
>  	eeh_addr_cache_insert_dev(dev);
>  	eeh_sysfs_add_device(dev);
>  }
> diff --git a/arch/powerpc/kernel/rtas_pci.c b/arch/powerpc/kernel/rtas_pci.c
> index fccf96e897f6..9668cd0411e1 100644
> --- a/arch/powerpc/kernel/rtas_pci.c
> +++ b/arch/powerpc/kernel/rtas_pci.c
> @@ -95,6 +95,24 @@ static int rtas_pci_read_config(struct pci_bus *bus,
>  	return ret;
>  }
>  
> +static bool eeh_handle_pmcsr_read(struct eeh_dev *edev, int size,
> +				  u32 *val, int *pcibios_ret)
> +{
> +	struct pci_dev *pdev;
> +
> +	if (!edev)
> +		return false;
> +
> +	pdev = edev->pdev;
> +	if (!pdev || pdev->current_state != PCI_D3cold)
> +		return false;
> +
> +	*val = EEH_IO_ERROR_VALUE(size);
> +	*pcibios_ret = PCIBIOS_SUCCESSFUL;
> +
> +	return true;
> +}
> +
>  int rtas_pci_dn_write_config(struct pci_dn *pdn, int where, int size, u32 val)
>  {
>  	unsigned long buid, addr;
> @@ -108,6 +126,14 @@ int rtas_pci_dn_write_config(struct pci_dn *pdn, int where, int size, u32 val)
>  	if (pdn->edev && pdn->edev->pe &&
>  	    (pdn->edev->pe->state & EEH_PE_CFG_BLOCKED))
>  		return PCIBIOS_SET_FAILED;
> +
> +	if (unlikely(pdn->edev && pdn->edev->pmcsr_offset &&
> +		     size == 2 && where == pdn->edev->pmcsr_offset)) {
> +		int pcibios_ret;
> +
> +		if (eeh_handle_pmcsr_read(pdn->edev, size, &val, &pcibios_ret))
> +			return pcibios_ret;
> +	}
>  #endif
>  
>  	addr = rtas_config_addr(pdn->busno, pdn->devfn, where);
> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
> index b12ef382fec7..0e7efb4bf2d4 100644
> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
> @@ -351,6 +351,21 @@ static struct eeh_pe *pseries_eeh_pe_get_parent(struct eeh_dev *edev)
>  	return NULL;
>  }
>  
> +static void pseries_eeh_init_pm_cap(struct pci_dn *pdn, struct eeh_dev *edev)
> +{
> +	edev->pm_cap = 0;
> +	edev->pmcsr_offset = 0;
> +
> +	if (!pdn || !edev)
> +		return;
> +
> +	edev->pm_cap = pseries_eeh_find_cap(pdn, PCI_CAP_ID_PM);
> +	if (!edev->pm_cap)
> +		return;
> +
> +	edev->pmcsr_offset = edev->pm_cap + PCI_PM_CTRL;
> +}
> +
>  /**
>   * pseries_eeh_init_edev - initialise the eeh_dev and eeh_pe for a pci_dn
>   *
> @@ -408,6 +423,7 @@ static void pseries_eeh_init_edev(struct pci_dn *pdn)
>  	edev->pcix_cap = pseries_eeh_find_cap(pdn, PCI_CAP_ID_PCIX);
>  	edev->pcie_cap = pseries_eeh_find_cap(pdn, PCI_CAP_ID_EXP);
>  	edev->aer_cap = pseries_eeh_find_ecap(pdn, PCI_EXT_CAP_ID_ERR);
> +	pseries_eeh_init_pm_cap(pdn, edev);
>  	edev->mode &= 0xFFFFFF00;
>  	if ((pdn->class_code >> 8) == PCI_CLASS_BRIDGE_PCI) {
>  		edev->mode |= EEH_DEV_BRIDGE;
> -- 
> 2.51.1
>
>

-- 
Cheers
~ Vaibhav