Hi Narayana, Thanks for the patch. My review comments inline below: Narayana Murty N <[email protected]> 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 <[email protected]> > --- > 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
