| Message ID | 20240617200411.1426554-9-terry.bowman@amd.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [RFC,1/9] PCI/AER: Update AER driver to call root port and downstream port UCE handlers | expand |
On Mon, Jun 17, 2024 at 03:04:10PM -0500, Terry Bowman wrote: > AER correctable internal errors (CIE) and AER uncorrectable internal > errors (UIE) are disabled through the AER mask register by default.[1] > > CXL PCIe ports use the CIE/UIE to report RAS errors and as a result > need CIE/UIE enabled.[2] > > Change pci_aer_unmask_internal_errors() function to be exported for > the CXL driver and other drivers to use. I can't actually find a user for this. Maybe that's because you did weird partial CCs for your series, or maybe it's because you don't want to tell us. Either way it's a no-go.
On 6/19/24 02:09, Christoph Hellwig wrote: > On Mon, Jun 17, 2024 at 03:04:10PM -0500, Terry Bowman wrote: >> AER correctable internal errors (CIE) and AER uncorrectable internal >> errors (UIE) are disabled through the AER mask register by default.[1] >> >> CXL PCIe ports use the CIE/UIE to report RAS errors and as a result >> need CIE/UIE enabled.[2] >> >> Change pci_aer_unmask_internal_errors() function to be exported for >> the CXL driver and other drivers to use. > > I can't actually find a user for this. Maybe that's because you did > weird partial CCs for your series, or maybe it's because you don't > want to tell us. Either way it's a no-go. The use is in the following patchset (9/9) that missed being shared with PCI list. If there is rework I'll fix so both are sent to PCI list. https://lore.kernel.org/all/20240617200411.1426554-10-terry.bowman@amd.com/ Regards, Terry
On Mon, 17 Jun 2024 15:04:10 -0500 Terry Bowman <terry.bowman@amd.com> wrote: > AER correctable internal errors (CIE) and AER uncorrectable internal > errors (UIE) are disabled through the AER mask register by default.[1] > > CXL PCIe ports use the CIE/UIE to report RAS errors and as a result > need CIE/UIE enabled.[2] > > Change pci_aer_unmask_internal_errors() function to be exported for > the CXL driver and other drivers to use. I've perhaps forgotten the end conclusion, but I thought there was a request to just try enabling this in general and mask it out only for known broken devices? Admittedly that's a more daring path, so maybe I hallucinated it! > > [1] PCI6.0 - 7.8.4.3 Uncorrectable > [2] CXL3.1 - 12.2.2 CXL Root Ports, Downstream Switch Ports, and Upstream > Switch Ports > > Signed-off-by: Terry Bowman <terry.bowman@amd.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: linux-pci@vger.kernel.org > --- > drivers/pci/pcie/aer.c | 3 ++- > include/linux/aer.h | 6 ++++++ > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > index 4dc03cb9aff0..d7a1982f0c50 100644 > --- a/drivers/pci/pcie/aer.c > +++ b/drivers/pci/pcie/aer.c > @@ -951,7 +951,7 @@ static bool find_source_device(struct pci_dev *parent, > * Note: AER must be enabled and supported by the device which must be > * checked in advance, e.g. with pcie_aer_is_native(). > */ > -static void pci_aer_unmask_internal_errors(struct pci_dev *dev) > +void pci_aer_unmask_internal_errors(struct pci_dev *dev) > { > int aer = dev->aer_cap; > u32 mask; > @@ -964,6 +964,7 @@ static void pci_aer_unmask_internal_errors(struct pci_dev *dev) > mask &= ~PCI_ERR_COR_INTERNAL; > pci_write_config_dword(dev, aer + PCI_ERR_COR_MASK, mask); > } > +EXPORT_SYMBOL_GPL(pci_aer_unmask_internal_errors); > > static bool is_cxl_mem_dev(struct pci_dev *dev) > { > diff --git a/include/linux/aer.h b/include/linux/aer.h > index 4b97f38f3fcf..a4fd25ea0280 100644 > --- a/include/linux/aer.h > +++ b/include/linux/aer.h > @@ -50,6 +50,12 @@ static inline int pci_aer_clear_nonfatal_status(struct pci_dev *dev) > static inline int pcie_aer_is_native(struct pci_dev *dev) { return 0; } > #endif > > +#ifdef CONFIG_PCIEAER_CXL > +void pci_aer_unmask_internal_errors(struct pci_dev *dev); > +#else > +static inline void pci_aer_unmask_internal_errors(struct pci_dev *dev) { } > +#endif > + > void pci_print_aer(struct pci_dev *dev, int aer_severity, > struct aer_capability_regs *aer); > int cper_severity_to_aer(int cper_severity);
Hi Jonathan, I added a response inline below. On 6/20/24 08:11, Jonathan Cameron wrote: > On Mon, 17 Jun 2024 15:04:10 -0500 > Terry Bowman <terry.bowman@amd.com> wrote: > >> AER correctable internal errors (CIE) and AER uncorrectable internal >> errors (UIE) are disabled through the AER mask register by default.[1] >> >> CXL PCIe ports use the CIE/UIE to report RAS errors and as a result >> need CIE/UIE enabled.[2] >> >> Change pci_aer_unmask_internal_errors() function to be exported for >> the CXL driver and other drivers to use. > > I've perhaps forgotten the end conclusion, but I thought there was > a request to just try enabling this in general and mask it out only > for known broken devices? > > Admittedly that's a more daring path, so maybe I hallucinated it! > I remember there was discussion. A quick search for PCI_ERR_COR_INTERNAL and PCI_ERR_UNC_INTERNAL doesn't find any default enablement. Regards, Terry >> >> [1] PCI6.0 - 7.8.4.3 Uncorrectable >> [2] CXL3.1 - 12.2.2 CXL Root Ports, Downstream Switch Ports, and Upstream >> Switch Ports >> >> Signed-off-by: Terry Bowman <terry.bowman@amd.com> >> Cc: Bjorn Helgaas <bhelgaas@google.com> >> Cc: linux-pci@vger.kernel.org >> --- >> drivers/pci/pcie/aer.c | 3 ++- >> include/linux/aer.h | 6 ++++++ >> 2 files changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c >> index 4dc03cb9aff0..d7a1982f0c50 100644 >> --- a/drivers/pci/pcie/aer.c >> +++ b/drivers/pci/pcie/aer.c >> @@ -951,7 +951,7 @@ static bool find_source_device(struct pci_dev *parent, >> * Note: AER must be enabled and supported by the device which must be >> * checked in advance, e.g. with pcie_aer_is_native(). >> */ >> -static void pci_aer_unmask_internal_errors(struct pci_dev *dev) >> +void pci_aer_unmask_internal_errors(struct pci_dev *dev) >> { >> int aer = dev->aer_cap; >> u32 mask; >> @@ -964,6 +964,7 @@ static void pci_aer_unmask_internal_errors(struct pci_dev *dev) >> mask &= ~PCI_ERR_COR_INTERNAL; >> pci_write_config_dword(dev, aer + PCI_ERR_COR_MASK, mask); >> } >> +EXPORT_SYMBOL_GPL(pci_aer_unmask_internal_errors); >> >> static bool is_cxl_mem_dev(struct pci_dev *dev) >> { >> diff --git a/include/linux/aer.h b/include/linux/aer.h >> index 4b97f38f3fcf..a4fd25ea0280 100644 >> --- a/include/linux/aer.h >> +++ b/include/linux/aer.h >> @@ -50,6 +50,12 @@ static inline int pci_aer_clear_nonfatal_status(struct pci_dev *dev) >> static inline int pcie_aer_is_native(struct pci_dev *dev) { return 0; } >> #endif >> >> +#ifdef CONFIG_PCIEAER_CXL >> +void pci_aer_unmask_internal_errors(struct pci_dev *dev); >> +#else >> +static inline void pci_aer_unmask_internal_errors(struct pci_dev *dev) { } >> +#endif >> + >> void pci_print_aer(struct pci_dev *dev, int aer_severity, >> struct aer_capability_regs *aer); >> int cper_severity_to_aer(int cper_severity); >
On Mon, Jun 17, 2024 at 03:04:10PM -0500, Terry Bowman wrote: > AER correctable internal errors (CIE) and AER uncorrectable internal > errors (UIE) are disabled through the AER mask register by default.[1] Nit: "Correctable Errors" and "Uncorrectable Errors" are generic PCIe concepts that exist independent of AER, so I wouldn't prefix them with AER. The AER mask registers control *reporting* of errors, but of course they don't disable the errors themselves. > CXL PCIe ports use the CIE/UIE to report RAS errors and as a result > need CIE/UIE enabled.[2] > > Change pci_aer_unmask_internal_errors() function to be exported for > the CXL driver and other drivers to use. > > [1] PCI6.0 - 7.8.4.3 Uncorrectable s/PCI6.0 .../PCIe r6.0, sec 7.8.4.3/ since there is a conventional PCI spec as well. > [2] CXL3.1 - 12.2.2 CXL Root Ports, Downstream Switch Ports, and Upstream > Switch Ports > > Signed-off-by: Terry Bowman <terry.bowman@amd.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: linux-pci@vger.kernel.org > --- > drivers/pci/pcie/aer.c | 3 ++- > include/linux/aer.h | 6 ++++++ > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > index 4dc03cb9aff0..d7a1982f0c50 100644 > --- a/drivers/pci/pcie/aer.c > +++ b/drivers/pci/pcie/aer.c > @@ -951,7 +951,7 @@ static bool find_source_device(struct pci_dev *parent, > * Note: AER must be enabled and supported by the device which must be > * checked in advance, e.g. with pcie_aer_is_native(). > */ > -static void pci_aer_unmask_internal_errors(struct pci_dev *dev) > +void pci_aer_unmask_internal_errors(struct pci_dev *dev) > { > int aer = dev->aer_cap; > u32 mask; > @@ -964,6 +964,7 @@ static void pci_aer_unmask_internal_errors(struct pci_dev *dev) > mask &= ~PCI_ERR_COR_INTERNAL; > pci_write_config_dword(dev, aer + PCI_ERR_COR_MASK, mask); > } > +EXPORT_SYMBOL_GPL(pci_aer_unmask_internal_errors); > > static bool is_cxl_mem_dev(struct pci_dev *dev) > { > diff --git a/include/linux/aer.h b/include/linux/aer.h > index 4b97f38f3fcf..a4fd25ea0280 100644 > --- a/include/linux/aer.h > +++ b/include/linux/aer.h > @@ -50,6 +50,12 @@ static inline int pci_aer_clear_nonfatal_status(struct pci_dev *dev) > static inline int pcie_aer_is_native(struct pci_dev *dev) { return 0; } > #endif > > +#ifdef CONFIG_PCIEAER_CXL > +void pci_aer_unmask_internal_errors(struct pci_dev *dev); > +#else > +static inline void pci_aer_unmask_internal_errors(struct pci_dev *dev) { } > +#endif I don't like the idea of exporting a generic PCI interface that only does something when CONFIG_PCIEAER_CXL is enabled. If there's ever a non-CXL caller, it will be confused. Bjorn
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index 4dc03cb9aff0..d7a1982f0c50 100644 --- a/drivers/pci/pcie/aer.c +++ b/drivers/pci/pcie/aer.c @@ -951,7 +951,7 @@ static bool find_source_device(struct pci_dev *parent, * Note: AER must be enabled and supported by the device which must be * checked in advance, e.g. with pcie_aer_is_native(). */ -static void pci_aer_unmask_internal_errors(struct pci_dev *dev) +void pci_aer_unmask_internal_errors(struct pci_dev *dev) { int aer = dev->aer_cap; u32 mask; @@ -964,6 +964,7 @@ static void pci_aer_unmask_internal_errors(struct pci_dev *dev) mask &= ~PCI_ERR_COR_INTERNAL; pci_write_config_dword(dev, aer + PCI_ERR_COR_MASK, mask); } +EXPORT_SYMBOL_GPL(pci_aer_unmask_internal_errors); static bool is_cxl_mem_dev(struct pci_dev *dev) { diff --git a/include/linux/aer.h b/include/linux/aer.h index 4b97f38f3fcf..a4fd25ea0280 100644 --- a/include/linux/aer.h +++ b/include/linux/aer.h @@ -50,6 +50,12 @@ static inline int pci_aer_clear_nonfatal_status(struct pci_dev *dev) static inline int pcie_aer_is_native(struct pci_dev *dev) { return 0; } #endif +#ifdef CONFIG_PCIEAER_CXL +void pci_aer_unmask_internal_errors(struct pci_dev *dev); +#else +static inline void pci_aer_unmask_internal_errors(struct pci_dev *dev) { } +#endif + void pci_print_aer(struct pci_dev *dev, int aer_severity, struct aer_capability_regs *aer); int cper_severity_to_aer(int cper_severity);
AER correctable internal errors (CIE) and AER uncorrectable internal errors (UIE) are disabled through the AER mask register by default.[1] CXL PCIe ports use the CIE/UIE to report RAS errors and as a result need CIE/UIE enabled.[2] Change pci_aer_unmask_internal_errors() function to be exported for the CXL driver and other drivers to use. [1] PCI6.0 - 7.8.4.3 Uncorrectable [2] CXL3.1 - 12.2.2 CXL Root Ports, Downstream Switch Ports, and Upstream Switch Ports Signed-off-by: Terry Bowman <terry.bowman@amd.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: linux-pci@vger.kernel.org --- drivers/pci/pcie/aer.c | 3 ++- include/linux/aer.h | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-)