| Message ID | 20240617200411.1426554-4-terry.bowman@amd.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add RAS support for CXL root ports, CXL downstream switch ports, and CXL upstream switch ports | expand |
On Mon, 17 Jun 2024 15:04:05 -0500 Terry Bowman <terry.bowman@amd.com> wrote: > PCIe port devices are bound to portdrv, the PCIe port bus driver. portdrv > does not implement an AER correctable handler (CE) but does implement the > AER uncorrectable error (UCE). The UCE handler is fairly straightforward > in that it only checks for frozen error state and returns the next step > for recovery accordingly. > > As a result, port devices relying on AER correctable internal errors (CIE) > and AER uncorrectable internal errors (UIE) will not be handled. Note, > the PCIe spec indicates AER CIE/UIE can be used to report implementation > specific errors.[1] > > CXL root ports, CXL downstream switch ports, and CXL upstream switch ports > are examples of devices using the AER CIE/UIE for implementation specific > purposes. These CXL ports use the AER interrupt and AER CIE/UIE status to > report CXL RAS errors.[2] > > Add an atomic notifier to portdrv's CE/UCE handlers. Use the atomic > notifier to report CIE/UIE errors to the registered functions. This will > require adding a CE handler and updating the existing UCE handler. > > For the UCE handler, the CXL spec states UIE errors should return need > reset: "The only method of recovering from an Uncorrectable Internal Error > is reset or hardware replacement."[1] > > [1] PCI6.0 - 6.2.10 Internal Errors > [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/portdrv.c | 32 ++++++++++++++++++++++++++++++++ > drivers/pci/pcie/portdrv.h | 2 ++ > 2 files changed, 34 insertions(+) > > diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c > index 14a4b89a3b83..86d80e0e9606 100644 > --- a/drivers/pci/pcie/portdrv.c > +++ b/drivers/pci/pcie/portdrv.c > @@ -37,6 +37,9 @@ struct portdrv_service_data { > u32 service; > }; > > +ATOMIC_NOTIFIER_HEAD(portdrv_aer_internal_err_chain); > +EXPORT_SYMBOL_GPL(portdrv_aer_internal_err_chain); Perhaps these should be per instance of the portdrv? I'd imagine we only want to register CXL ones on CXL ports etc and it's annoying to have to check at runtime for relevance of a particular notifier. > + > /** > * release_pcie_device - free PCI Express port service device structure > * @dev: Port service device to release > @@ -745,11 +748,39 @@ static void pcie_portdrv_shutdown(struct pci_dev *dev) > static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev, > pci_channel_state_t error) > { > + if (dev->aer_cap) { > + u32 status; > + > + pci_read_config_dword(dev, dev->aer_cap + PCI_ERR_UNCOR_STATUS, > + &status); > + > + if (status & PCI_ERR_UNC_INTN) { > + atomic_notifier_call_chain(&portdrv_aer_internal_err_chain, > + AER_FATAL, (void *)dev); Don't think the cast is needed as always fine to implicitly cast to and from void * in C. > + return PCI_ERS_RESULT_NEED_RESET; > + } > + } > + > if (error == pci_channel_io_frozen) > return PCI_ERS_RESULT_NEED_RESET; > return PCI_ERS_RESULT_CAN_RECOVER; > } > > +static void pcie_portdrv_cor_error_detected(struct pci_dev *dev) > +{ > + u32 status; > + > + if (!dev->aer_cap) > + return; > + > + pci_read_config_dword(dev, dev->aer_cap + PCI_ERR_COR_STATUS, > + &status); > + > + if (status & PCI_ERR_COR_INTERNAL) > + atomic_notifier_call_chain(&portdrv_aer_internal_err_chain, > + AER_CORRECTABLE, (void *)dev); No need for the cast. > +} > + > static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev) > { > size_t off = offsetof(struct pcie_port_service_driver, slot_reset); > @@ -780,6 +811,7 @@ static const struct pci_device_id port_pci_ids[] = { > > static const struct pci_error_handlers pcie_portdrv_err_handler = { > .error_detected = pcie_portdrv_error_detected, > + .cor_error_detected = pcie_portdrv_cor_error_detected, > .slot_reset = pcie_portdrv_slot_reset, > .mmio_enabled = pcie_portdrv_mmio_enabled, > }; > diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h > index 12c89ea0313b..8a39197f0203 100644 > --- a/drivers/pci/pcie/portdrv.h > +++ b/drivers/pci/pcie/portdrv.h > @@ -121,4 +121,6 @@ static inline void pcie_pme_interrupt_enable(struct pci_dev *dev, bool en) {} > #endif /* !CONFIG_PCIE_PME */ > > struct device *pcie_port_find_device(struct pci_dev *dev, u32 service); > + > +extern struct atomic_notifier_head portdrv_aer_internal_err_chain; > #endif /* _PORTDRV_H_ */
Terry Bowman wrote: > PCIe port devices are bound to portdrv, the PCIe port bus driver. portdrv > does not implement an AER correctable handler (CE) but does implement the > AER uncorrectable error (UCE). The UCE handler is fairly straightforward > in that it only checks for frozen error state and returns the next step > for recovery accordingly. > > As a result, port devices relying on AER correctable internal errors (CIE) > and AER uncorrectable internal errors (UIE) will not be handled. Note, > the PCIe spec indicates AER CIE/UIE can be used to report implementation > specific errors.[1] > > CXL root ports, CXL downstream switch ports, and CXL upstream switch ports > are examples of devices using the AER CIE/UIE for implementation specific > purposes. These CXL ports use the AER interrupt and AER CIE/UIE status to > report CXL RAS errors.[2] > > Add an atomic notifier to portdrv's CE/UCE handlers. Use the atomic > notifier to report CIE/UIE errors to the registered functions. This will > require adding a CE handler and updating the existing UCE handler. > > For the UCE handler, the CXL spec states UIE errors should return need > reset: "The only method of recovering from an Uncorrectable Internal Error > is reset or hardware replacement."[1] > > [1] PCI6.0 - 6.2.10 Internal Errors > [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/portdrv.c | 32 ++++++++++++++++++++++++++++++++ > drivers/pci/pcie/portdrv.h | 2 ++ > 2 files changed, 34 insertions(+) > > diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c > index 14a4b89a3b83..86d80e0e9606 100644 > --- a/drivers/pci/pcie/portdrv.c > +++ b/drivers/pci/pcie/portdrv.c > @@ -37,6 +37,9 @@ struct portdrv_service_data { > u32 service; > }; > > +ATOMIC_NOTIFIER_HEAD(portdrv_aer_internal_err_chain); > +EXPORT_SYMBOL_GPL(portdrv_aer_internal_err_chain); > + > /** > * release_pcie_device - free PCI Express port service device structure > * @dev: Port service device to release > @@ -745,11 +748,39 @@ static void pcie_portdrv_shutdown(struct pci_dev *dev) > static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev, > pci_channel_state_t error) > { > + if (dev->aer_cap) { > + u32 status; > + > + pci_read_config_dword(dev, dev->aer_cap + PCI_ERR_UNCOR_STATUS, > + &status); > + > + if (status & PCI_ERR_UNC_INTN) { > + atomic_notifier_call_chain(&portdrv_aer_internal_err_chain, > + AER_FATAL, (void *)dev); > + return PCI_ERS_RESULT_NEED_RESET; > + } > + } > + Oh, this is a finer grained / lower-level location than I was expecting. I was expecting that the notifier was just conveying the port interrupt notification to a driver that knew how to take the next step. This pcie_portdrv_error_detected() is a notification that is already "downstream" of the AER notification. If PCIe does not care about CIE and UIE then don't make it care, but redirect the notifications to the CXL side that may care. Leave the portdrv handlers PCIe native as much as possible. Now, I have not thought through the full implications of that suggestion, but for now am reacting to this AER -> PCIe err_handler -> CXL notfier as potentially more awkward than AER -> CXL notifier. It's a separate error handling domain that the PCIe side likely does not want to worry about. PCIe side is only responsible for allowing CXL to register for the notifications beacuse the AER interrupt is shared.
Hi Jonathan, I added responses inline below. On 6/20/24 07:30, Jonathan Cameron wrote: > On Mon, 17 Jun 2024 15:04:05 -0500 > Terry Bowman <terry.bowman@amd.com> wrote: > >> PCIe port devices are bound to portdrv, the PCIe port bus driver. portdrv >> does not implement an AER correctable handler (CE) but does implement the >> AER uncorrectable error (UCE). The UCE handler is fairly straightforward >> in that it only checks for frozen error state and returns the next step >> for recovery accordingly. >> >> As a result, port devices relying on AER correctable internal errors (CIE) >> and AER uncorrectable internal errors (UIE) will not be handled. Note, >> the PCIe spec indicates AER CIE/UIE can be used to report implementation >> specific errors.[1] >> >> CXL root ports, CXL downstream switch ports, and CXL upstream switch ports >> are examples of devices using the AER CIE/UIE for implementation specific >> purposes. These CXL ports use the AER interrupt and AER CIE/UIE status to >> report CXL RAS errors.[2] >> >> Add an atomic notifier to portdrv's CE/UCE handlers. Use the atomic >> notifier to report CIE/UIE errors to the registered functions. This will >> require adding a CE handler and updating the existing UCE handler. >> >> For the UCE handler, the CXL spec states UIE errors should return need >> reset: "The only method of recovering from an Uncorrectable Internal Error >> is reset or hardware replacement."[1] >> >> [1] PCI6.0 - 6.2.10 Internal Errors >> [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/portdrv.c | 32 ++++++++++++++++++++++++++++++++ >> drivers/pci/pcie/portdrv.h | 2 ++ >> 2 files changed, 34 insertions(+) >> >> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c >> index 14a4b89a3b83..86d80e0e9606 100644 >> --- a/drivers/pci/pcie/portdrv.c >> +++ b/drivers/pci/pcie/portdrv.c >> @@ -37,6 +37,9 @@ struct portdrv_service_data { >> u32 service; >> }; >> >> +ATOMIC_NOTIFIER_HEAD(portdrv_aer_internal_err_chain); >> +EXPORT_SYMBOL_GPL(portdrv_aer_internal_err_chain); > > Perhaps these should be per instance of the portdrv? > I'd imagine we only want to register CXL ones on CXL ports etc > and it's annoying to have to check at runtime for relevance > of a particular notifier. > This could be made per-instance by moving to the PCI/device drvdata. This would likely need a portdrv setup-init helper function to enable for a particular PCI device. >> + >> /** >> * release_pcie_device - free PCI Express port service device structure >> * @dev: Port service device to release >> @@ -745,11 +748,39 @@ static void pcie_portdrv_shutdown(struct pci_dev *dev) >> static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev, >> pci_channel_state_t error) >> { >> + if (dev->aer_cap) { >> + u32 status; >> + >> + pci_read_config_dword(dev, dev->aer_cap + PCI_ERR_UNCOR_STATUS, >> + &status); >> + >> + if (status & PCI_ERR_UNC_INTN) { >> + atomic_notifier_call_chain(&portdrv_aer_internal_err_chain, >> + AER_FATAL, (void *)dev); > > Don't think the cast is needed as always fine to implicitly cast to and from > void * in C. > Ok. >> + return PCI_ERS_RESULT_NEED_RESET; >> + } >> + } >> + >> if (error == pci_channel_io_frozen) >> return PCI_ERS_RESULT_NEED_RESET; >> return PCI_ERS_RESULT_CAN_RECOVER; >> } >> >> +static void pcie_portdrv_cor_error_detected(struct pci_dev *dev) >> +{ >> + u32 status; >> + >> + if (!dev->aer_cap) >> + return; >> + >> + pci_read_config_dword(dev, dev->aer_cap + PCI_ERR_COR_STATUS, >> + &status); >> + >> + if (status & PCI_ERR_COR_INTERNAL) >> + atomic_notifier_call_chain(&portdrv_aer_internal_err_chain, >> + AER_CORRECTABLE, (void *)dev); > > No need for the cast. > Ok Regards, Terry >> +} >> + >> static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev) >> { >> size_t off = offsetof(struct pcie_port_service_driver, slot_reset); >> @@ -780,6 +811,7 @@ static const struct pci_device_id port_pci_ids[] = { >> >> static const struct pci_error_handlers pcie_portdrv_err_handler = { >> .error_detected = pcie_portdrv_error_detected, >> + .cor_error_detected = pcie_portdrv_cor_error_detected, >> .slot_reset = pcie_portdrv_slot_reset, >> .mmio_enabled = pcie_portdrv_mmio_enabled, >> }; >> diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h >> index 12c89ea0313b..8a39197f0203 100644 >> --- a/drivers/pci/pcie/portdrv.h >> +++ b/drivers/pci/pcie/portdrv.h >> @@ -121,4 +121,6 @@ static inline void pcie_pme_interrupt_enable(struct pci_dev *dev, bool en) {} >> #endif /* !CONFIG_PCIE_PME */ >> >> struct device *pcie_port_find_device(struct pci_dev *dev, u32 service); >> + >> +extern struct atomic_notifier_head portdrv_aer_internal_err_chain; >> #endif /* _PORTDRV_H_ */ >
Hi Dan, I added responses inline below. On 6/21/24 14:36, Dan Williams wrote: > Terry Bowman wrote: >> PCIe port devices are bound to portdrv, the PCIe port bus driver. portdrv >> does not implement an AER correctable handler (CE) but does implement the >> AER uncorrectable error (UCE). The UCE handler is fairly straightforward >> in that it only checks for frozen error state and returns the next step >> for recovery accordingly. >> >> As a result, port devices relying on AER correctable internal errors (CIE) >> and AER uncorrectable internal errors (UIE) will not be handled. Note, >> the PCIe spec indicates AER CIE/UIE can be used to report implementation >> specific errors.[1] >> >> CXL root ports, CXL downstream switch ports, and CXL upstream switch ports >> are examples of devices using the AER CIE/UIE for implementation specific >> purposes. These CXL ports use the AER interrupt and AER CIE/UIE status to >> report CXL RAS errors.[2] >> >> Add an atomic notifier to portdrv's CE/UCE handlers. Use the atomic >> notifier to report CIE/UIE errors to the registered functions. This will >> require adding a CE handler and updating the existing UCE handler. >> >> For the UCE handler, the CXL spec states UIE errors should return need >> reset: "The only method of recovering from an Uncorrectable Internal Error >> is reset or hardware replacement."[1] >> >> [1] PCI6.0 - 6.2.10 Internal Errors >> [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/portdrv.c | 32 ++++++++++++++++++++++++++++++++ >> drivers/pci/pcie/portdrv.h | 2 ++ >> 2 files changed, 34 insertions(+) >> >> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c >> index 14a4b89a3b83..86d80e0e9606 100644 >> --- a/drivers/pci/pcie/portdrv.c >> +++ b/drivers/pci/pcie/portdrv.c >> @@ -37,6 +37,9 @@ struct portdrv_service_data { >> u32 service; >> }; >> >> +ATOMIC_NOTIFIER_HEAD(portdrv_aer_internal_err_chain); >> +EXPORT_SYMBOL_GPL(portdrv_aer_internal_err_chain); >> + >> /** >> * release_pcie_device - free PCI Express port service device structure >> * @dev: Port service device to release >> @@ -745,11 +748,39 @@ static void pcie_portdrv_shutdown(struct pci_dev *dev) >> static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev, >> pci_channel_state_t error) >> { >> + if (dev->aer_cap) { >> + u32 status; >> + >> + pci_read_config_dword(dev, dev->aer_cap + PCI_ERR_UNCOR_STATUS, >> + &status); >> + >> + if (status & PCI_ERR_UNC_INTN) { >> + atomic_notifier_call_chain(&portdrv_aer_internal_err_chain, >> + AER_FATAL, (void *)dev); >> + return PCI_ERS_RESULT_NEED_RESET; >> + } >> + } >> + > > Oh, this is a finer grained / lower-level location than I was > expecting. I was expecting that the notifier was just conveying the port > interrupt notification to a driver that knew how to take the next step. > This pcie_portdrv_error_detected() is a notification that is already > "downstream" of the AER notification. > My intent was to implement the UIE/CIE "implementation specific" behavior as mentioned in the PCI spec. This included allowing port devices to be notified if needed. This plan is not ideal but works within the PCI portdrv situation and before we can introduce a CXL specific portdriver. > If PCIe does not care about CIE and UIE then don't make it care, but > redirect the notifications to the CXL side that may care. > > Leave the portdrv handlers PCIe native as much as possible. > > Now, I have not thought through the full implications of that > suggestion, but for now am reacting to this AER -> PCIe err_handler -> > CXL notfier as potentially more awkward than AER -> CXL notifier. It's a > separate error handling domain that the PCIe side likely does not want > to worry about. PCIe side is only responsible for allowing CXL to > register for the notifications beacuse the AER interrupt is shared. Hmmm, this sounds like either option#2 or introducing a CXL portdrv service driver. Thanks for the reviews and please let me know which option you would like me to purse. Regards, Terry
Terry Bowman wrote: > Hi Dan, > > I added responses inline below. > > On 6/21/24 14:36, Dan Williams wrote: > > Terry Bowman wrote: > >> PCIe port devices are bound to portdrv, the PCIe port bus driver. portdrv > >> does not implement an AER correctable handler (CE) but does implement the > >> AER uncorrectable error (UCE). The UCE handler is fairly straightforward > >> in that it only checks for frozen error state and returns the next step > >> for recovery accordingly. > >> > >> As a result, port devices relying on AER correctable internal errors (CIE) > >> and AER uncorrectable internal errors (UIE) will not be handled. Note, > >> the PCIe spec indicates AER CIE/UIE can be used to report implementation > >> specific errors.[1] > >> > >> CXL root ports, CXL downstream switch ports, and CXL upstream switch ports > >> are examples of devices using the AER CIE/UIE for implementation specific > >> purposes. These CXL ports use the AER interrupt and AER CIE/UIE status to > >> report CXL RAS errors.[2] > >> > >> Add an atomic notifier to portdrv's CE/UCE handlers. Use the atomic > >> notifier to report CIE/UIE errors to the registered functions. This will > >> require adding a CE handler and updating the existing UCE handler. > >> > >> For the UCE handler, the CXL spec states UIE errors should return need > >> reset: "The only method of recovering from an Uncorrectable Internal Error > >> is reset or hardware replacement."[1] > >> > >> [1] PCI6.0 - 6.2.10 Internal Errors > >> [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/portdrv.c | 32 ++++++++++++++++++++++++++++++++ > >> drivers/pci/pcie/portdrv.h | 2 ++ > >> 2 files changed, 34 insertions(+) > >> > >> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c > >> index 14a4b89a3b83..86d80e0e9606 100644 > >> --- a/drivers/pci/pcie/portdrv.c > >> +++ b/drivers/pci/pcie/portdrv.c > >> @@ -37,6 +37,9 @@ struct portdrv_service_data { > >> u32 service; > >> }; > >> > >> +ATOMIC_NOTIFIER_HEAD(portdrv_aer_internal_err_chain); > >> +EXPORT_SYMBOL_GPL(portdrv_aer_internal_err_chain); > >> + > >> /** > >> * release_pcie_device - free PCI Express port service device structure > >> * @dev: Port service device to release > >> @@ -745,11 +748,39 @@ static void pcie_portdrv_shutdown(struct pci_dev *dev) > >> static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev, > >> pci_channel_state_t error) > >> { > >> + if (dev->aer_cap) { > >> + u32 status; > >> + > >> + pci_read_config_dword(dev, dev->aer_cap + PCI_ERR_UNCOR_STATUS, > >> + &status); > >> + > >> + if (status & PCI_ERR_UNC_INTN) { > >> + atomic_notifier_call_chain(&portdrv_aer_internal_err_chain, > >> + AER_FATAL, (void *)dev); > >> + return PCI_ERS_RESULT_NEED_RESET; > >> + } > >> + } > >> + > > > > Oh, this is a finer grained / lower-level location than I was > > expecting. I was expecting that the notifier was just conveying the port > > interrupt notification to a driver that knew how to take the next step. > > This pcie_portdrv_error_detected() is a notification that is already > > "downstream" of the AER notification. > > > > My intent was to implement the UIE/CIE "implementation specific" behavior as > mentioned in the PCI spec. This included allowing port devices to be notified if > needed. This plan is not ideal but works within the PCI portdrv situation > and before we can introduce a CXL specific portdriver. ...but it really isn't implementation specific behavior like all the other anonymous internal error cases. This is an open standard definition that just happens to alias with the PCIe "internal" notification mechanism. > > > If PCIe does not care about CIE and UIE then don't make it care, but > > redirect the notifications to the CXL side that may care. > > > > Leave the portdrv handlers PCIe native as much as possible. > > > > Now, I have not thought through the full implications of that > > suggestion, but for now am reacting to this AER -> PCIe err_handler -> > > CXL notfier as potentially more awkward than AER -> CXL notifier. It's a > > separate error handling domain that the PCIe side likely does not want > > to worry about. PCIe side is only responsible for allowing CXL to > > register for the notifications beacuse the AER interrupt is shared. > > Hmmm, this sounds like either option#2 or introducing a CXL portdrv service > driver. > > Thanks for the reviews and please let me know which option you > would like me to purse. So after looking at this patchset I think calling the PCIe portdrv error handler set for anything other than PCIe errors is likely a mistake. The CXL protocol side of the house can experience errors that have no relation to errors that PCIe needs to handle or care about. I am thinking something like cxl_rch_handle_error() becomes cxl_handle_error() and when that successfully handles the error then no need to trigger pcie_do_recovery(). pcie_do_recovery() is too tightly scoped to error recovery that is reasonable for PCIe links. That may not be reasonable to CXL devices where protocol errors potentially implicate that a system memory transaction failed. The blast radius of CXL protocol errors are not constrained to single devices like the PCIe case. With that change something like a new cxl_do_recovery() can operate on the cxl_port topology and know that it has exclusive control of the error handling registers.
On 6/24/24 16:46, Dan Williams wrote: > Terry Bowman wrote: >> Hi Dan, >> >> I added responses inline below. >> >> On 6/21/24 14:36, Dan Williams wrote: >>> Terry Bowman wrote: >>>> PCIe port devices are bound to portdrv, the PCIe port bus driver. portdrv >>>> does not implement an AER correctable handler (CE) but does implement the >>>> AER uncorrectable error (UCE). The UCE handler is fairly straightforward >>>> in that it only checks for frozen error state and returns the next step >>>> for recovery accordingly. >>>> >>>> As a result, port devices relying on AER correctable internal errors (CIE) >>>> and AER uncorrectable internal errors (UIE) will not be handled. Note, >>>> the PCIe spec indicates AER CIE/UIE can be used to report implementation >>>> specific errors.[1] >>>> >>>> CXL root ports, CXL downstream switch ports, and CXL upstream switch ports >>>> are examples of devices using the AER CIE/UIE for implementation specific >>>> purposes. These CXL ports use the AER interrupt and AER CIE/UIE status to >>>> report CXL RAS errors.[2] >>>> >>>> Add an atomic notifier to portdrv's CE/UCE handlers. Use the atomic >>>> notifier to report CIE/UIE errors to the registered functions. This will >>>> require adding a CE handler and updating the existing UCE handler. >>>> >>>> For the UCE handler, the CXL spec states UIE errors should return need >>>> reset: "The only method of recovering from an Uncorrectable Internal Error >>>> is reset or hardware replacement."[1] >>>> >>>> [1] PCI6.0 - 6.2.10 Internal Errors >>>> [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/portdrv.c | 32 ++++++++++++++++++++++++++++++++ >>>> drivers/pci/pcie/portdrv.h | 2 ++ >>>> 2 files changed, 34 insertions(+) >>>> >>>> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c >>>> index 14a4b89a3b83..86d80e0e9606 100644 >>>> --- a/drivers/pci/pcie/portdrv.c >>>> +++ b/drivers/pci/pcie/portdrv.c >>>> @@ -37,6 +37,9 @@ struct portdrv_service_data { >>>> u32 service; >>>> }; >>>> >>>> +ATOMIC_NOTIFIER_HEAD(portdrv_aer_internal_err_chain); >>>> +EXPORT_SYMBOL_GPL(portdrv_aer_internal_err_chain); >>>> + >>>> /** >>>> * release_pcie_device - free PCI Express port service device structure >>>> * @dev: Port service device to release >>>> @@ -745,11 +748,39 @@ static void pcie_portdrv_shutdown(struct pci_dev *dev) >>>> static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev, >>>> pci_channel_state_t error) >>>> { >>>> + if (dev->aer_cap) { >>>> + u32 status; >>>> + >>>> + pci_read_config_dword(dev, dev->aer_cap + PCI_ERR_UNCOR_STATUS, >>>> + &status); >>>> + >>>> + if (status & PCI_ERR_UNC_INTN) { >>>> + atomic_notifier_call_chain(&portdrv_aer_internal_err_chain, >>>> + AER_FATAL, (void *)dev); >>>> + return PCI_ERS_RESULT_NEED_RESET; >>>> + } >>>> + } >>>> + >>> >>> Oh, this is a finer grained / lower-level location than I was >>> expecting. I was expecting that the notifier was just conveying the port >>> interrupt notification to a driver that knew how to take the next step. >>> This pcie_portdrv_error_detected() is a notification that is already >>> "downstream" of the AER notification. >>> >> >> My intent was to implement the UIE/CIE "implementation specific" behavior as >> mentioned in the PCI spec. This included allowing port devices to be notified if >> needed. This plan is not ideal but works within the PCI portdrv situation >> and before we can introduce a CXL specific portdriver. > > ...but it really isn't implementation specific behavior like all the > other anonymous internal error cases. This is an open standard > definition that just happens to alias with the PCIe "internal" > notification mechanism. > >> >>> If PCIe does not care about CIE and UIE then don't make it care, but >>> redirect the notifications to the CXL side that may care. >>> >>> Leave the portdrv handlers PCIe native as much as possible. >>> >>> Now, I have not thought through the full implications of that >>> suggestion, but for now am reacting to this AER -> PCIe err_handler -> >>> CXL notfier as potentially more awkward than AER -> CXL notifier. It's a >>> separate error handling domain that the PCIe side likely does not want >>> to worry about. PCIe side is only responsible for allowing CXL to >>> register for the notifications beacuse the AER interrupt is shared. >> >> Hmmm, this sounds like either option#2 or introducing a CXL portdrv service >> driver. >> >> Thanks for the reviews and please let me know which option you >> would like me to purse. > > So after looking at this patchset I think calling the PCIe portdrv error > handler set for anything other than PCIe errors is likely a mistake. The > CXL protocol side of the house can experience errors that have no > relation to errors that PCIe needs to handle or care about. > > I am thinking something like cxl_rch_handle_error() becomes > cxl_handle_error() and when that successfully handles the error then no > need to trigger pcie_do_recovery(). > > pcie_do_recovery() is too tightly scoped to error recovery that is > reasonable for PCIe links. That may not be reasonable to CXL devices > where protocol errors potentially implicate that a system memory > transaction failed. The blast radius of CXL protocol errors are not > constrained to single devices like the PCIe case. > > With that change something like a new cxl_do_recovery() can operate on > the cxl_port topology and know that it has exclusive control of the > error handling registers. Ok, I'll refactor the existing AER RCH downstream port handling to support CXL USP, DSP, and RP as well. I can incorporate much of the feedback from this RFC into the new patchset. Thanks Dan. Regards, Terry
On 6/18/2024 4:04 AM, Terry Bowman wrote: > PCIe port devices are bound to portdrv, the PCIe port bus driver. portdrv > does not implement an AER correctable handler (CE) but does implement the > AER uncorrectable error (UCE). The UCE handler is fairly straightforward > in that it only checks for frozen error state and returns the next step > for recovery accordingly. > > As a result, port devices relying on AER correctable internal errors (CIE) > and AER uncorrectable internal errors (UIE) will not be handled. Note, > the PCIe spec indicates AER CIE/UIE can be used to report implementation > specific errors.[1] > > CXL root ports, CXL downstream switch ports, and CXL upstream switch ports > are examples of devices using the AER CIE/UIE for implementation specific > purposes. These CXL ports use the AER interrupt and AER CIE/UIE status to > report CXL RAS errors.[2] > > Add an atomic notifier to portdrv's CE/UCE handlers. Use the atomic > notifier to report CIE/UIE errors to the registered functions. This will > require adding a CE handler and updating the existing UCE handler. > > For the UCE handler, the CXL spec states UIE errors should return need > reset: "The only method of recovering from an Uncorrectable Internal Error > is reset or hardware replacement."[1] > > [1] PCI6.0 - 6.2.10 Internal Errors > [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/portdrv.c | 32 ++++++++++++++++++++++++++++++++ > drivers/pci/pcie/portdrv.h | 2 ++ > 2 files changed, 34 insertions(+) > > diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c > index 14a4b89a3b83..86d80e0e9606 100644 > --- a/drivers/pci/pcie/portdrv.c > +++ b/drivers/pci/pcie/portdrv.c > @@ -37,6 +37,9 @@ struct portdrv_service_data { > u32 service; > }; > > +ATOMIC_NOTIFIER_HEAD(portdrv_aer_internal_err_chain); > +EXPORT_SYMBOL_GPL(portdrv_aer_internal_err_chain); > + > /** > * release_pcie_device - free PCI Express port service device structure > * @dev: Port service device to release > @@ -745,11 +748,39 @@ static void pcie_portdrv_shutdown(struct pci_dev *dev) > static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev, > pci_channel_state_t error) > { > + if (dev->aer_cap) { > + u32 status; > + > + pci_read_config_dword(dev, dev->aer_cap + PCI_ERR_UNCOR_STATUS, > + &status); > + > + if (status & PCI_ERR_UNC_INTN) { > + atomic_notifier_call_chain(&portdrv_aer_internal_err_chain, > + AER_FATAL, (void *)dev); > + return PCI_ERS_RESULT_NEED_RESET; > + } > + } > + > if (error == pci_channel_io_frozen) > return PCI_ERS_RESULT_NEED_RESET; > return PCI_ERS_RESULT_CAN_RECOVER; > } > > +static void pcie_portdrv_cor_error_detected(struct pci_dev *dev) > +{ > + u32 status; > + > + if (!dev->aer_cap) > + return; Seems like that dev->aer_cap checking is not needed for cor_error_detected, aer_get_device_error_info() already checked it and won't call handle_error_source() if device has not AER capability. But I am curious why pci_aer_handle_error() checks dev->aer_cap again after aer_get_device_error_info(). > + > + pci_read_config_dword(dev, dev->aer_cap + PCI_ERR_COR_STATUS, > + &status); > + > + if (status & PCI_ERR_COR_INTERNAL) > + atomic_notifier_call_chain(&portdrv_aer_internal_err_chain, > + AER_CORRECTABLE, (void *)dev); > +} > + > static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev) > { > size_t off = offsetof(struct pcie_port_service_driver, slot_reset); > @@ -780,6 +811,7 @@ static const struct pci_device_id port_pci_ids[] = { > > static const struct pci_error_handlers pcie_portdrv_err_handler = { > .error_detected = pcie_portdrv_error_detected, > + .cor_error_detected = pcie_portdrv_cor_error_detected, > .slot_reset = pcie_portdrv_slot_reset, > .mmio_enabled = pcie_portdrv_mmio_enabled, > }; > diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h > index 12c89ea0313b..8a39197f0203 100644 > --- a/drivers/pci/pcie/portdrv.h > +++ b/drivers/pci/pcie/portdrv.h > @@ -121,4 +121,6 @@ static inline void pcie_pme_interrupt_enable(struct pci_dev *dev, bool en) {} > #endif /* !CONFIG_PCIE_PME */ > > struct device *pcie_port_find_device(struct pci_dev *dev, u32 service); > + > +extern struct atomic_notifier_head portdrv_aer_internal_err_chain; > #endif /* _PORTDRV_H_ */
On 6/25/24 21:54, Li, Ming4 wrote: > On 6/18/2024 4:04 AM, Terry Bowman wrote: >> PCIe port devices are bound to portdrv, the PCIe port bus driver. portdrv >> does not implement an AER correctable handler (CE) but does implement the >> AER uncorrectable error (UCE). The UCE handler is fairly straightforward >> in that it only checks for frozen error state and returns the next step >> for recovery accordingly. >> >> As a result, port devices relying on AER correctable internal errors (CIE) >> and AER uncorrectable internal errors (UIE) will not be handled. Note, >> the PCIe spec indicates AER CIE/UIE can be used to report implementation >> specific errors.[1] >> >> CXL root ports, CXL downstream switch ports, and CXL upstream switch ports >> are examples of devices using the AER CIE/UIE for implementation specific >> purposes. These CXL ports use the AER interrupt and AER CIE/UIE status to >> report CXL RAS errors.[2] >> >> Add an atomic notifier to portdrv's CE/UCE handlers. Use the atomic >> notifier to report CIE/UIE errors to the registered functions. This will >> require adding a CE handler and updating the existing UCE handler. >> >> For the UCE handler, the CXL spec states UIE errors should return need >> reset: "The only method of recovering from an Uncorrectable Internal Error >> is reset or hardware replacement."[1] >> >> [1] PCI6.0 - 6.2.10 Internal Errors >> [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/portdrv.c | 32 ++++++++++++++++++++++++++++++++ >> drivers/pci/pcie/portdrv.h | 2 ++ >> 2 files changed, 34 insertions(+) >> >> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c >> index 14a4b89a3b83..86d80e0e9606 100644 >> --- a/drivers/pci/pcie/portdrv.c >> +++ b/drivers/pci/pcie/portdrv.c >> @@ -37,6 +37,9 @@ struct portdrv_service_data { >> u32 service; >> }; >> >> +ATOMIC_NOTIFIER_HEAD(portdrv_aer_internal_err_chain); >> +EXPORT_SYMBOL_GPL(portdrv_aer_internal_err_chain); >> + >> /** >> * release_pcie_device - free PCI Express port service device structure >> * @dev: Port service device to release >> @@ -745,11 +748,39 @@ static void pcie_portdrv_shutdown(struct pci_dev *dev) >> static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev, >> pci_channel_state_t error) >> { >> + if (dev->aer_cap) { >> + u32 status; >> + >> + pci_read_config_dword(dev, dev->aer_cap + PCI_ERR_UNCOR_STATUS, >> + &status); >> + >> + if (status & PCI_ERR_UNC_INTN) { >> + atomic_notifier_call_chain(&portdrv_aer_internal_err_chain, >> + AER_FATAL, (void *)dev); >> + return PCI_ERS_RESULT_NEED_RESET; >> + } >> + } >> + >> if (error == pci_channel_io_frozen) >> return PCI_ERS_RESULT_NEED_RESET; >> return PCI_ERS_RESULT_CAN_RECOVER; >> } >> >> +static void pcie_portdrv_cor_error_detected(struct pci_dev *dev) >> +{ >> + u32 status; >> + >> + if (!dev->aer_cap) >> + return; > > Seems like that dev->aer_cap checking is not needed for cor_error_detected, aer_get_device_error_info() already checked it and won't call handle_error_source() if device has not AER capability. But I am curious why pci_aer_handle_error() checks dev->aer_cap again after aer_get_device_error_info(). > Hi Ming, I agree this check should be removed. Regards, Terry
diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c index 14a4b89a3b83..86d80e0e9606 100644 --- a/drivers/pci/pcie/portdrv.c +++ b/drivers/pci/pcie/portdrv.c @@ -37,6 +37,9 @@ struct portdrv_service_data { u32 service; }; +ATOMIC_NOTIFIER_HEAD(portdrv_aer_internal_err_chain); +EXPORT_SYMBOL_GPL(portdrv_aer_internal_err_chain); + /** * release_pcie_device - free PCI Express port service device structure * @dev: Port service device to release @@ -745,11 +748,39 @@ static void pcie_portdrv_shutdown(struct pci_dev *dev) static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev, pci_channel_state_t error) { + if (dev->aer_cap) { + u32 status; + + pci_read_config_dword(dev, dev->aer_cap + PCI_ERR_UNCOR_STATUS, + &status); + + if (status & PCI_ERR_UNC_INTN) { + atomic_notifier_call_chain(&portdrv_aer_internal_err_chain, + AER_FATAL, (void *)dev); + return PCI_ERS_RESULT_NEED_RESET; + } + } + if (error == pci_channel_io_frozen) return PCI_ERS_RESULT_NEED_RESET; return PCI_ERS_RESULT_CAN_RECOVER; } +static void pcie_portdrv_cor_error_detected(struct pci_dev *dev) +{ + u32 status; + + if (!dev->aer_cap) + return; + + pci_read_config_dword(dev, dev->aer_cap + PCI_ERR_COR_STATUS, + &status); + + if (status & PCI_ERR_COR_INTERNAL) + atomic_notifier_call_chain(&portdrv_aer_internal_err_chain, + AER_CORRECTABLE, (void *)dev); +} + static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev) { size_t off = offsetof(struct pcie_port_service_driver, slot_reset); @@ -780,6 +811,7 @@ static const struct pci_device_id port_pci_ids[] = { static const struct pci_error_handlers pcie_portdrv_err_handler = { .error_detected = pcie_portdrv_error_detected, + .cor_error_detected = pcie_portdrv_cor_error_detected, .slot_reset = pcie_portdrv_slot_reset, .mmio_enabled = pcie_portdrv_mmio_enabled, }; diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h index 12c89ea0313b..8a39197f0203 100644 --- a/drivers/pci/pcie/portdrv.h +++ b/drivers/pci/pcie/portdrv.h @@ -121,4 +121,6 @@ static inline void pcie_pme_interrupt_enable(struct pci_dev *dev, bool en) {} #endif /* !CONFIG_PCIE_PME */ struct device *pcie_port_find_device(struct pci_dev *dev, u32 service); + +extern struct atomic_notifier_head portdrv_aer_internal_err_chain; #endif /* _PORTDRV_H_ */
PCIe port devices are bound to portdrv, the PCIe port bus driver. portdrv does not implement an AER correctable handler (CE) but does implement the AER uncorrectable error (UCE). The UCE handler is fairly straightforward in that it only checks for frozen error state and returns the next step for recovery accordingly. As a result, port devices relying on AER correctable internal errors (CIE) and AER uncorrectable internal errors (UIE) will not be handled. Note, the PCIe spec indicates AER CIE/UIE can be used to report implementation specific errors.[1] CXL root ports, CXL downstream switch ports, and CXL upstream switch ports are examples of devices using the AER CIE/UIE for implementation specific purposes. These CXL ports use the AER interrupt and AER CIE/UIE status to report CXL RAS errors.[2] Add an atomic notifier to portdrv's CE/UCE handlers. Use the atomic notifier to report CIE/UIE errors to the registered functions. This will require adding a CE handler and updating the existing UCE handler. For the UCE handler, the CXL spec states UIE errors should return need reset: "The only method of recovering from an Uncorrectable Internal Error is reset or hardware replacement."[1] [1] PCI6.0 - 6.2.10 Internal Errors [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/portdrv.c | 32 ++++++++++++++++++++++++++++++++ drivers/pci/pcie/portdrv.h | 2 ++ 2 files changed, 34 insertions(+)