| Message ID | 20230315235449.1279209-1-sathyanarayanan.kuppuswamy@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] PCI/EDR: Clear PCIe Device Status errors after EDR error recovery | expand |
Hi Bjorn, Any comments on this patch? On 3/15/23 4:54 PM, Kuppuswamy Sathyanarayanan wrote: > Commit 068c29a248b6 ("PCI/ERR: Clear PCIe Device Status errors only if > OS owns AER") adds support to clear error status in the Device Status > Register(DEVSTA) only if OS owns the AER support. But this change > breaks the requirement of the EDR feature which requires OS to cleanup > the error registers even if firmware owns the control of AER support. > > More details about this requirement can be found in PCIe Firmware > specification v3.3, Table 4-6 Interpretation of the _OSC Control Field. > If the OS supports the Error Disconnect Recover (EDR) feature and > firmware sends the EDR event, then during the EDR recovery window, OS > is responsible for the device error recovery and holds the ownership of > the following error registers. > > • Device Status Register > • Uncorrectable Error Status Register > • Correctable Error Status Register > • Root Error Status Register > • RP PIO Status Register > > So call pcie_clear_device_status() in edr_handle_event() if the error > recovery is successful. > > Reported-by: Tsaur Erwin <erwin.tsaur@intel.com> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > --- > > Changes since v1: > * Rebased on top of v6.3-rc1. > * Fixed a typo in pcie_clear_device_status(). > > drivers/pci/pcie/edr.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c > index a6b9b479b97a..87734e4c3c20 100644 > --- a/drivers/pci/pcie/edr.c > +++ b/drivers/pci/pcie/edr.c > @@ -193,6 +193,7 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data) > */ > if (estate == PCI_ERS_RESULT_RECOVERED) { > pci_dbg(edev, "DPC port successfully recovered\n"); > + pcie_clear_device_status(edev); > acpi_send_edr_status(pdev, edev, EDR_OST_SUCCESS); > } else { > pci_dbg(edev, "DPC port recovery failed\n");
[+cc Jonathan, author of 068c29a248b6] On Wed, Mar 15, 2023 at 04:54:49PM -0700, Kuppuswamy Sathyanarayanan wrote: > Commit 068c29a248b6 ("PCI/ERR: Clear PCIe Device Status errors only if > OS owns AER") adds support to clear error status in the Device Status > Register(DEVSTA) only if OS owns the AER support. But this change > breaks the requirement of the EDR feature which requires OS to cleanup > the error registers even if firmware owns the control of AER support. > > More details about this requirement can be found in PCIe Firmware > specification v3.3, Table 4-6 Interpretation of the _OSC Control Field. > If the OS supports the Error Disconnect Recover (EDR) feature and > firmware sends the EDR event, then during the EDR recovery window, OS > is responsible for the device error recovery and holds the ownership of > the following error registers. > > • Device Status Register > • Uncorrectable Error Status Register > • Correctable Error Status Register > • Root Error Status Register > • RP PIO Status Register > > So call pcie_clear_device_status() in edr_handle_event() if the error > recovery is successful. IIUC, after ac1c8e35a326 ("PCI/DPC: Add Error Disconnect Recover (EDR) support") appeared in v5.7-rc1, DEVSTA was always cleared in this path: edr_handle_event pcie_do_recovery pcie_clear_device_status After 068c29a248b6 ("PCI/ERR: Clear PCIe Device Status errors only if OS owns AER") appeared in v5.9-rc1, we only clear DEVSTA if the OS owns the AER Capability: edr_handle_event pcie_do_recovery if (pcie_aer_is_native(dev)) # <-- new test pcie_clear_device_status So in the case where the OS does *not* own AER, and it receives an EDR notification, DEVSTA is not cleared when it should be. Right? I assume we should have a Fixes: tag here, since this patch should be backported to every kernel that contains 068c29a248b6. Possibly even a stable tag, although it's arguable whether it's "critical" per Documentation/process/stable-kernel-rules.rst. > Reported-by: Tsaur Erwin <erwin.tsaur@intel.com> I assume this report was internal, and there's no mailing list post or bugzilla issue URL we can include here? > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > --- > > Changes since v1: > * Rebased on top of v6.3-rc1. > * Fixed a typo in pcie_clear_device_status(). > > drivers/pci/pcie/edr.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c > index a6b9b479b97a..87734e4c3c20 100644 > --- a/drivers/pci/pcie/edr.c > +++ b/drivers/pci/pcie/edr.c > @@ -193,6 +193,7 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data) > */ > if (estate == PCI_ERS_RESULT_RECOVERED) { > pci_dbg(edev, "DPC port successfully recovered\n"); > + pcie_clear_device_status(edev); It's a little weird to work around a change inside pcie_do_recovery() by clearing it here, and that means we clear it twice in the AER native case, but I don't see any simpler way to do this, so this seems fine as the fix for the current issue. Question though: in the AER native case, pcie_do_recovery() calls both: pcie_clear_device_status() and pci_aer_clear_nonfatal_status() In this patch, you only call pcie_clear_device_status(). Do you care about pci_aer_clear_nonfatal_status(), too? The overall design for clearing status has gotten pretty complicated as we've added error handling methods (firmware-first, DPC, EDR), and there are so many different places and cases that it's hard to be sure we do them all correctly. I don't really know how to clean this up, so I'm just attaching my notes about the current state: - AER native handling: handle_error_source if (info->severity == AER_CORRECTABLE) clear PCI_ERR_COR_STATUS <-- if (pcie_aer_is_native(dev)) pdrv->err_handler->cor_error_detected() pcie_clear_device_status clear PCI_EXP_DEVSTA <-- else pcie_do_recovery pcie_clear_device_status clear PCI_EXP_DEVSTA <-- pci_aer_clear_nonfatal_status clear PCI_ERR_UNCOR_STATUS <-- - Firmware-first handling: status is cleared by firmware before event is reported to OS via HEST - DPC native handling: dpc_handler dpc_process_error if (rp_extensions) dpc_process_rp_pio_error clear PCI_EXP_DPC_RP_PIO_STATUS <-- else if (...) pci_aer_clear_nonfatal_status clear PCI_ERR_UNCOR_STATUS <-- pci_aer_clear_fatal_status clear PCI_ERR_UNCOR_STATUS <-- pcie_do_recovery if (AER native) pcie_clear_device_status clear PCI_EXP_DEVSTA <-- pci_aer_clear_nonfatal_status clear PCI_ERR_UNCOR_STATUS <-- - EDR event handling: edr_handle_event dpc_process_error if (rp_extensions) dpc_process_rp_pio_error clear PCI_EXP_DPC_RP_PIO_STATUS <-- else if (...) pci_aer_clear_nonfatal_status clear PCI_ERR_UNCOR_STATUS <-- pci_aer_clear_fatal_status clear PCI_ERR_UNCOR_STATUS <-- pci_aer_raw_clear_status clear PCI_ERR_ROOT_STATUS <-- clear PCI_ERR_COR_STATUS <-- clear PCI_ERR_UNCOR_STATUS <-- pcie_do_recovery if (AER native) pcie_clear_device_status clear PCI_EXP_DEVSTA <-- pci_aer_clear_nonfatal_status clear PCI_ERR_UNCOR_STATUS <-- if (PCI_ERS_RESULT_RECOVERED) pcie_clear_device_status clear PCI_EXP_DEVSTA <-- > acpi_send_edr_status(pdev, edev, EDR_OST_SUCCESS); > } else { > pci_dbg(edev, "DPC port recovery failed\n"); > -- > 2.34.1 >
Hi Bjorn, Thanks for the review. On 3/29/23 3:09 PM, Bjorn Helgaas wrote: > [+cc Jonathan, author of 068c29a248b6] > > On Wed, Mar 15, 2023 at 04:54:49PM -0700, Kuppuswamy Sathyanarayanan wrote: >> Commit 068c29a248b6 ("PCI/ERR: Clear PCIe Device Status errors only if >> OS owns AER") adds support to clear error status in the Device Status >> Register(DEVSTA) only if OS owns the AER support. But this change >> breaks the requirement of the EDR feature which requires OS to cleanup >> the error registers even if firmware owns the control of AER support. >> >> More details about this requirement can be found in PCIe Firmware >> specification v3.3, Table 4-6 Interpretation of the _OSC Control Field. >> If the OS supports the Error Disconnect Recover (EDR) feature and >> firmware sends the EDR event, then during the EDR recovery window, OS >> is responsible for the device error recovery and holds the ownership of >> the following error registers. >> >> • Device Status Register >> • Uncorrectable Error Status Register >> • Correctable Error Status Register >> • Root Error Status Register >> • RP PIO Status Register >> >> So call pcie_clear_device_status() in edr_handle_event() if the error >> recovery is successful. > > IIUC, after ac1c8e35a326 ("PCI/DPC: Add Error Disconnect Recover (EDR) > support") appeared in v5.7-rc1, DEVSTA was always cleared in this path: > > edr_handle_event > pcie_do_recovery > pcie_clear_device_status > > After 068c29a248b6 ("PCI/ERR: Clear PCIe Device Status errors only if > OS owns AER") appeared in v5.9-rc1, we only clear DEVSTA if the OS > owns the AER Capability: > > edr_handle_event > pcie_do_recovery > if (pcie_aer_is_native(dev)) # <-- new test > pcie_clear_device_status > > So in the case where the OS does *not* own AER, and it receives an EDR > notification, DEVSTA is not cleared when it should be. Right? Correct. > > I assume we should have a Fixes: tag here, since this patch should be > backported to every kernel that contains 068c29a248b6. Possibly even > a stable tag, although it's arguable whether it's "critical" per > Documentation/process/stable-kernel-rules.rst. Yes. But this error is only reproducible in the EDR use case. So I am not sure whether it can be considered a critical fix. Fixes: 068c29a248b6 ("PCI/ERR: Clear PCIe Device Status errors only if OS owns AER") > >> Reported-by: Tsaur Erwin <erwin.tsaur@intel.com> > > I assume this report was internal, and there's no mailing list post or > bugzilla issue URL we can include here? Yes. > >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >> --- >> >> Changes since v1: >> * Rebased on top of v6.3-rc1. >> * Fixed a typo in pcie_clear_device_status(). >> >> drivers/pci/pcie/edr.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c >> index a6b9b479b97a..87734e4c3c20 100644 >> --- a/drivers/pci/pcie/edr.c >> +++ b/drivers/pci/pcie/edr.c >> @@ -193,6 +193,7 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data) >> */ >> if (estate == PCI_ERS_RESULT_RECOVERED) { >> pci_dbg(edev, "DPC port successfully recovered\n"); >> + pcie_clear_device_status(edev); > > It's a little weird to work around a change inside pcie_do_recovery() > by clearing it here, and that means we clear it twice in the AER > native case, but I don't see any simpler way to do this, so this seems > fine as the fix for the current issue. In AER native case, edr_handle_event() will never be triggered. So it won't be cleared twice. Other way is to add a new parameter to pcie_do_recovery(..., edr) and use it to conditionally call pcie_clear_device_status(). But I think current way is less complex. edr_handle_event pcie_do_recovery(..., edr=1) if (pcie_aer_is_native(dev) || edr) pcie_clear_device_status > > Question though: in the AER native case, pcie_do_recovery() calls > both: > > pcie_clear_device_status() and > pci_aer_clear_nonfatal_status() > > In this patch, you only call pcie_clear_device_status(). Do you care > about pci_aer_clear_nonfatal_status(), too? Yes, we care about it. Since we call dpc_process_error() in EDR handler, it will eventually clear error status via pci_aer_clear_nonfatal_status() and pci_aer_clear_fatal_status() within dpc_process_error(). > > The overall design for clearing status has gotten pretty complicated > as we've added error handling methods (firmware-first, DPC, EDR), and > there are so many different places and cases that it's hard to be sure > we do them all correctly. > > I don't really know how to clean this up, so I'm just attaching my > notes about the current state: Good summary! I can see a lot of overlap in clearing PCI_ERR_UNCOR_STATUS and PCI_EXP_DEVSTA. > > - AER native handling: > > handle_error_source > if (info->severity == AER_CORRECTABLE) > clear PCI_ERR_COR_STATUS <-- > if (pcie_aer_is_native(dev)) > pdrv->err_handler->cor_error_detected() > pcie_clear_device_status > clear PCI_EXP_DEVSTA <-- > else > pcie_do_recovery > pcie_clear_device_status > clear PCI_EXP_DEVSTA <-- > pci_aer_clear_nonfatal_status > clear PCI_ERR_UNCOR_STATUS <-- > > - Firmware-first handling: status is cleared by firmware before > event is reported to OS via HEST > > - DPC native handling: > > dpc_handler > dpc_process_error > if (rp_extensions) > dpc_process_rp_pio_error > clear PCI_EXP_DPC_RP_PIO_STATUS <-- > else if (...) > pci_aer_clear_nonfatal_status > clear PCI_ERR_UNCOR_STATUS <-- > pci_aer_clear_fatal_status > clear PCI_ERR_UNCOR_STATUS <-- > pcie_do_recovery > if (AER native) > pcie_clear_device_status > clear PCI_EXP_DEVSTA <-- > pci_aer_clear_nonfatal_status > clear PCI_ERR_UNCOR_STATUS <-- > > - EDR event handling: > > edr_handle_event > dpc_process_error > if (rp_extensions) > dpc_process_rp_pio_error > clear PCI_EXP_DPC_RP_PIO_STATUS <-- > else if (...) > pci_aer_clear_nonfatal_status > clear PCI_ERR_UNCOR_STATUS <-- > pci_aer_clear_fatal_status > clear PCI_ERR_UNCOR_STATUS <-- > pci_aer_raw_clear_status > clear PCI_ERR_ROOT_STATUS <-- > clear PCI_ERR_COR_STATUS <-- > clear PCI_ERR_UNCOR_STATUS <-- > pcie_do_recovery > if (AER native) > pcie_clear_device_status > clear PCI_EXP_DEVSTA <-- > pci_aer_clear_nonfatal_status > clear PCI_ERR_UNCOR_STATUS <-- > if (PCI_ERS_RESULT_RECOVERED) > pcie_clear_device_status > clear PCI_EXP_DEVSTA <-- > >> acpi_send_edr_status(pdev, edev, EDR_OST_SUCCESS); >> } else { >> pci_dbg(edev, "DPC port recovery failed\n"); >> -- >> 2.34.1 >>
On Wed, Mar 29, 2023 at 03:38:04PM -0700, Sathyanarayanan Kuppuswamy wrote: > On 3/29/23 3:09 PM, Bjorn Helgaas wrote: > > On Wed, Mar 15, 2023 at 04:54:49PM -0700, Kuppuswamy Sathyanarayanan wrote: > >> Commit 068c29a248b6 ("PCI/ERR: Clear PCIe Device Status errors only if > >> OS owns AER") adds support to clear error status in the Device Status > >> Register(DEVSTA) only if OS owns the AER support. But this change > >> breaks the requirement of the EDR feature which requires OS to cleanup > >> the error registers even if firmware owns the control of AER support. > > I assume we should have a Fixes: tag here, since this patch should be > > backported to every kernel that contains 068c29a248b6. Possibly even > > a stable tag, although it's arguable whether it's "critical" per > > Documentation/process/stable-kernel-rules.rst. > > Yes. But this error is only reproducible in the EDR use case. So I > am not sure whether it can be considered a critical fix. I don't know how widespread EDR implementation is. What is the user-visible issue without this fix? "lspci" shows status bits set even after recovery? Subsequent EDR notifications cause us to report errors that were previously reported and recovered? Spurious EDR notifications because of status bits that should have been cleared? This kind of information would be useful in the commit log anyway. Since the risk is low (the change only affects EDR processing) and the the experience without this change might be poor (please clarify what that experience is), I think I would be inclined to mark it for stable. > > It's a little weird to work around a change inside pcie_do_recovery() > > by clearing it here, and that means we clear it twice in the AER > > native case, but I don't see any simpler way to do this, so this seems > > fine as the fix for the current issue. > > In AER native case, edr_handle_event() will never be triggered. So it > won't be cleared twice. This sounds like a plausible assumption. But is there actually spec language that says EDR notification is not allowed in the AER native case (when OS owns the AER Capability)? I looked but didn't find anything. > Other way is to add a new parameter to pcie_do_recovery(..., edr) and use > it to conditionally call pcie_clear_device_status(). But I think current > way is less complex. I agree. > > Question though: in the AER native case, pcie_do_recovery() calls > > both: > > > > pcie_clear_device_status() and > > pci_aer_clear_nonfatal_status() > > > > In this patch, you only call pcie_clear_device_status(). Do you care > > about pci_aer_clear_nonfatal_status(), too? > > Yes, we care about it. Since we call dpc_process_error() in EDR handler, > it will eventually clear error status via pci_aer_clear_nonfatal_status() > and pci_aer_clear_fatal_status() within dpc_process_error(). dpc_process_error() calls pci_aer_clear_nonfatal_status() in *some* (but not all) cases. I didn't try to work out whether those match the cases where pcie_do_recovery() called it before 068c29a248b6. I guess we can assume it's equivalent for now. > > The overall design for clearing status has gotten pretty complicated > > as we've added error handling methods (firmware-first, DPC, EDR), and > > there are so many different places and cases that it's hard to be sure > > we do them all correctly. > > > > I don't really know how to clean this up, so I'm just attaching my > > notes about the current state: > > Good summary! I can see a lot of overlap in clearing > PCI_ERR_UNCOR_STATUS and PCI_EXP_DEVSTA. Actually I do have one idea: in the firmware-first case, firmware collects all the status information, clears it, and then passes the status on to the OS. In this case we don't need to clear the status registers in handle_error_source(), pcie_do_recovery(), etc. So I think the OS *should* be able to do something similar by collecting the status information and clearing it first, before starting error handling. This might let us collect the status clearing together in one place and also converge the firmware-first and native error handling paths. Obviously that would be a major future project. Bjorn
Hi Bjorn, On 3/30/23 8:45 AM, Bjorn Helgaas wrote: > On Wed, Mar 29, 2023 at 03:38:04PM -0700, Sathyanarayanan Kuppuswamy wrote: >> On 3/29/23 3:09 PM, Bjorn Helgaas wrote: >>> On Wed, Mar 15, 2023 at 04:54:49PM -0700, Kuppuswamy Sathyanarayanan wrote: >>>> Commit 068c29a248b6 ("PCI/ERR: Clear PCIe Device Status errors only if >>>> OS owns AER") adds support to clear error status in the Device Status >>>> Register(DEVSTA) only if OS owns the AER support. But this change >>>> breaks the requirement of the EDR feature which requires OS to cleanup >>>> the error registers even if firmware owns the control of AER support. > >>> I assume we should have a Fixes: tag here, since this patch should be >>> backported to every kernel that contains 068c29a248b6. Possibly even >>> a stable tag, although it's arguable whether it's "critical" per >>> Documentation/process/stable-kernel-rules.rst. >> >> Yes. But this error is only reproducible in the EDR use case. So I >> am not sure whether it can be considered a critical fix. > > I don't know how widespread EDR implementation is. What is the I believe it is used by a few vendors (Nvidia, Intel, Dell, etc). I have seen EDR related messages in some of the posted DPC related dmesg logs. > user-visible issue without this fix? "lspci" shows status bits set > even after recovery? Subsequent EDR notifications cause us to report > errors that were previously reported and recovered? Spurious EDR > notifications because of status bits that should have been cleared? > This kind of information would be useful in the commit log anyway. I don't think we will get spurious EDR notifications. Since the only stale entry is in DEVSTA status register, the user will at most see errors bits set, even after successful recovery. However, if the user relies on some automation scripts to check the device status after error recovery, it may have an impact. > > Since the risk is low (the change only affects EDR processing) and the > the experience without this change might be poor (please clarify what > that experience is), I think I would be inclined to mark it for > stable. Ok. IMO, since it will be visible to the user (although not fatal), we can send it to the stable. But I will let you decide on it. > >>> It's a little weird to work around a change inside pcie_do_recovery() >>> by clearing it here, and that means we clear it twice in the AER >>> native case, but I don't see any simpler way to do this, so this seems >>> fine as the fix for the current issue. >> >> In AER native case, edr_handle_event() will never be triggered. So it >> won't be cleared twice. > > This sounds like a plausible assumption. But is there actually spec > language that says EDR notification is not allowed in the AER native > case (when OS owns the AER Capability)? I looked but didn't find > anything. > In the PCIe firmware specification v3.3, table "Table 4-6: Interpretation of the _OSC Control Field, Returned Value", field "PCI Express Downstream Port Containment configuration control", it explains that the firmware can use EDR notification only when OS DPC control is not requested or denied by firmware. >> Other way is to add a new parameter to pcie_do_recovery(..., edr) and use >> it to conditionally call pcie_clear_device_status(). But I think current >> way is less complex. > > I agree. > >>> Question though: in the AER native case, pcie_do_recovery() calls >>> both: >>> >>> pcie_clear_device_status() and >>> pci_aer_clear_nonfatal_status() >>> >>> In this patch, you only call pcie_clear_device_status(). Do you care >>> about pci_aer_clear_nonfatal_status(), too? >> >> Yes, we care about it. Since we call dpc_process_error() in EDR handler, >> it will eventually clear error status via pci_aer_clear_nonfatal_status() >> and pci_aer_clear_fatal_status() within dpc_process_error(). > > dpc_process_error() calls pci_aer_clear_nonfatal_status() in *some* > (but not all) cases. I didn't try to work out whether those match the > cases where pcie_do_recovery() called it before 068c29a248b6. I guess > we can assume it's equivalent for now. We also call pci_aer_raw_clear_status() in dpc_process_error() which also should clear the fatal and non-fatal errors. > >>> The overall design for clearing status has gotten pretty complicated >>> as we've added error handling methods (firmware-first, DPC, EDR), and >>> there are so many different places and cases that it's hard to be sure >>> we do them all correctly. >>> >>> I don't really know how to clean this up, so I'm just attaching my >>> notes about the current state: >> >> Good summary! I can see a lot of overlap in clearing >> PCI_ERR_UNCOR_STATUS and PCI_EXP_DEVSTA. > > Actually I do have one idea: in the firmware-first case, firmware > collects all the status information, clears it, and then passes the > status on to the OS. In this case we don't need to clear the status > registers in handle_error_source(), pcie_do_recovery(), etc. So the idea is to get the error info in a particular format using something like _DSM call? What about the legacy use case? For legacy firmware, we still need to support the old format, right? > > So I think the OS *should* be able to do something similar by > collecting the status information and clearing it first, before Something similar to updating struct aer_stats and struct aer_err_src? > starting error handling. This might let us collect the status > clearing together in one place and also converge the firmware-first > and native error handling paths. > > Obviously that would be a major future project. Agree. It would require changes to firmware spec, AER and DPC driver. Maybe we can start with cleaning up the native mode first. I would be happy to contribute to it. Let me think about the possible methods and get back to you. > > Bjorn
On Thu, Mar 30, 2023 at 11:46:45PM -0700, Sathyanarayanan Kuppuswamy wrote: > On 3/30/23 8:45 AM, Bjorn Helgaas wrote: > > This sounds like a plausible assumption. But is there actually spec > > language that says EDR notification is not allowed in the AER native > > case (when OS owns the AER Capability)? I looked but didn't find > > anything. > > In the PCIe firmware specification v3.3, table "Table 4-6: Interpretation of > the _OSC Control Field, Returned Value", field "PCI Express Downstream Port > Containment configuration control", it explains that the firmware can use > EDR notification only when OS DPC control is not requested or denied by > firmware. I'm sure that's the intent, but I don't see that restriction in the spec. Here's what I'm looking at, which doesn't directly restrict generation of EDR notifications: If control of this feature was requested and denied, or was not requested, firmware is responsible for initializing Downstream Port Containment Extended Capability Structures per firmware policy. Further, [the OS is permitted to write several registers while processing an EDR notification] > > Actually I do have one idea: in the firmware-first case, firmware > > collects all the status information, clears it, and then passes the > > status on to the OS. In this case we don't need to clear the status > > registers in handle_error_source(), pcie_do_recovery(), etc. > > So the idea is to get the error info in a particular format using > something like _DSM call? No, that's not what I'm thinking at all. I definitely would not want to add a new _DSM, which would add yet another case the OS has to handle. In the firmware-first case, the firmware collects the error status and clears it before handing the info off to the OS error handling path. In the native case, the OS should be able to collect the error status and clear it before starting the OS error handling path. Same register accesses, should be indistinguishable from the device point of view, it's just that the register accesses would be done by the OS instead of by firmware. Bjorn
On Wed, Mar 15, 2023 at 04:54:49PM -0700, Kuppuswamy Sathyanarayanan wrote: > Commit 068c29a248b6 ("PCI/ERR: Clear PCIe Device Status errors only if > OS owns AER") adds support to clear error status in the Device Status > Register(DEVSTA) only if OS owns the AER support. But this change > breaks the requirement of the EDR feature which requires OS to cleanup > the error registers even if firmware owns the control of AER support. > > More details about this requirement can be found in PCIe Firmware > specification v3.3, Table 4-6 Interpretation of the _OSC Control Field. > If the OS supports the Error Disconnect Recover (EDR) feature and > firmware sends the EDR event, then during the EDR recovery window, OS > is responsible for the device error recovery and holds the ownership of > the following error registers. > > • Device Status Register > • Uncorrectable Error Status Register > • Correctable Error Status Register > • Root Error Status Register > • RP PIO Status Register > > So call pcie_clear_device_status() in edr_handle_event() if the error > recovery is successful. > > Reported-by: Tsaur Erwin <erwin.tsaur@intel.com> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > --- > > Changes since v1: > * Rebased on top of v6.3-rc1. > * Fixed a typo in pcie_clear_device_status(). > > drivers/pci/pcie/edr.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c > index a6b9b479b97a..87734e4c3c20 100644 > --- a/drivers/pci/pcie/edr.c > +++ b/drivers/pci/pcie/edr.c > @@ -193,6 +193,7 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data) > */ > if (estate == PCI_ERS_RESULT_RECOVERED) { > pci_dbg(edev, "DPC port successfully recovered\n"); > + pcie_clear_device_status(edev); > acpi_send_edr_status(pdev, edev, EDR_OST_SUCCESS); The implementation note in PCI Firmware r3.3, sec 4.6.12, shows the OS clearing error status *after* _OST is evaluated. On the other hand, the _OSC DPC control bit in table 4-6 says that if the OS does not have DPC control, it can only write the Device Status error bits between the EDR Notify and invoking _OST. Is one of those wrong, or am I missing something? Bjorn
+ Mahesh On 4/6/23 2:07 PM, Bjorn Helgaas wrote: > On Wed, Mar 15, 2023 at 04:54:49PM -0700, Kuppuswamy Sathyanarayanan wrote: >> Commit 068c29a248b6 ("PCI/ERR: Clear PCIe Device Status errors only if >> OS owns AER") adds support to clear error status in the Device Status >> Register(DEVSTA) only if OS owns the AER support. But this change >> breaks the requirement of the EDR feature which requires OS to cleanup >> the error registers even if firmware owns the control of AER support. >> >> More details about this requirement can be found in PCIe Firmware >> specification v3.3, Table 4-6 Interpretation of the _OSC Control Field. >> If the OS supports the Error Disconnect Recover (EDR) feature and >> firmware sends the EDR event, then during the EDR recovery window, OS >> is responsible for the device error recovery and holds the ownership of >> the following error registers. >> >> • Device Status Register >> • Uncorrectable Error Status Register >> • Correctable Error Status Register >> • Root Error Status Register >> • RP PIO Status Register >> >> So call pcie_clear_device_status() in edr_handle_event() if the error >> recovery is successful. >> >> Reported-by: Tsaur Erwin <erwin.tsaur@intel.com> >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >> --- >> >> Changes since v1: >> * Rebased on top of v6.3-rc1. >> * Fixed a typo in pcie_clear_device_status(). >> >> drivers/pci/pcie/edr.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c >> index a6b9b479b97a..87734e4c3c20 100644 >> --- a/drivers/pci/pcie/edr.c >> +++ b/drivers/pci/pcie/edr.c >> @@ -193,6 +193,7 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data) >> */ >> if (estate == PCI_ERS_RESULT_RECOVERED) { >> pci_dbg(edev, "DPC port successfully recovered\n"); >> + pcie_clear_device_status(edev); >> acpi_send_edr_status(pdev, edev, EDR_OST_SUCCESS); > > The implementation note in PCI Firmware r3.3, sec 4.6.12, shows the OS > clearing error status *after* _OST is evaluated. > > On the other hand, the _OSC DPC control bit in table 4-6 says that if > the OS does not have DPC control, it can only write the Device Status > error bits between the EDR Notify and invoking _OST. > > Is one of those wrong, or am I missing something? Agree. It is conflicting info. IMO, the argument that the OS is allowed to clear the error registers during the EDR windows makes more sense. If OS is allowed to touch error registers owned by firmware after that window, it would lead to race conditions. Mahesh, let us know your comments. Maybe we need to fix this in the firmware specification. > > Bjorn
On Thu, Apr 06, 2023 at 02:52:02PM -0700, Sathyanarayanan Kuppuswamy wrote: > On 4/6/23 2:07 PM, Bjorn Helgaas wrote: > > On Wed, Mar 15, 2023 at 04:54:49PM -0700, Kuppuswamy Sathyanarayanan wrote: > >> Commit 068c29a248b6 ("PCI/ERR: Clear PCIe Device Status errors only if > >> OS owns AER") adds support to clear error status in the Device Status > >> Register(DEVSTA) only if OS owns the AER support. But this change > >> breaks the requirement of the EDR feature which requires OS to cleanup > >> the error registers even if firmware owns the control of AER support. > >> > >> More details about this requirement can be found in PCIe Firmware > >> specification v3.3, Table 4-6 Interpretation of the _OSC Control Field. > >> If the OS supports the Error Disconnect Recover (EDR) feature and > >> firmware sends the EDR event, then during the EDR recovery window, OS > >> is responsible for the device error recovery and holds the ownership of > >> the following error registers. > >> > >> • Device Status Register > >> • Uncorrectable Error Status Register > >> • Correctable Error Status Register > >> • Root Error Status Register > >> • RP PIO Status Register > >> > >> So call pcie_clear_device_status() in edr_handle_event() if the error > >> recovery is successful. > >> > >> Reported-by: Tsaur Erwin <erwin.tsaur@intel.com> > >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > >> --- > >> > >> Changes since v1: > >> * Rebased on top of v6.3-rc1. > >> * Fixed a typo in pcie_clear_device_status(). > >> > >> drivers/pci/pcie/edr.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c > >> index a6b9b479b97a..87734e4c3c20 100644 > >> --- a/drivers/pci/pcie/edr.c > >> +++ b/drivers/pci/pcie/edr.c > >> @@ -193,6 +193,7 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data) > >> */ > >> if (estate == PCI_ERS_RESULT_RECOVERED) { > >> pci_dbg(edev, "DPC port successfully recovered\n"); > >> + pcie_clear_device_status(edev); > >> acpi_send_edr_status(pdev, edev, EDR_OST_SUCCESS); > > > > The implementation note in PCI Firmware r3.3, sec 4.6.12, shows the OS > > clearing error status *after* _OST is evaluated. > > > > On the other hand, the _OSC DPC control bit in table 4-6 says that if > > the OS does not have DPC control, it can only write the Device Status > > error bits between the EDR Notify and invoking _OST. > > > > Is one of those wrong, or am I missing something? > > Agree. It is conflicting info. IMO, the argument that the OS is allowed to > clear the error registers during the EDR windows makes more sense. If OS > is allowed to touch error registers owned by firmware after that window, > it would lead to race conditions. > > Mahesh, let us know your comments. Maybe we need to fix this in the firmware > specification. My assumption was this sequence is something like this, where firmware *can't* collect error status from devices below the Downstream Port because DPC has been triggered and they are not accessible: - Hardware triggers DPC in a Downstream Port - Firmware fields error interrupt - Firmware captures Downstream Port error info (devices below are not accessible because of DPC) - Firmware sends EDR Notify to OS - OS brings Downstream Port out of DPC - OS collects error status from devices below Downstream Port - OS evaluates _OST - Firmware captures error status from devices below Downstream Port But that doesn't explain why *firmware* could not clear the error status of those devices after it captures it. I guess the flowchart *does* show firmware clearing the error status in the "do not continue recovery" path.
My assumption was this sequence is something like this, where firmware
*can't* collect error status from devices below the Downstream Port because DPC has been triggered and they are not accessible:
- Hardware triggers DPC in a Downstream Port
- Firmware fields error interrupt
- Firmware captures Downstream Port error info (devices below are
not accessible because of DPC)
- Firmware sends EDR Notify to OS
- OS brings Downstream Port out of DPC
- OS collects error status from devices below Downstream Port
- OS evaluates _OST
- Firmware captures error status from devices below Downstream Port
MN: The above flow is correct. The error registers on the device are sticky, so they should survive DPC (=hot reset).
But that doesn't explain why *firmware* could not clear the error status of those devices after it captures it.
MN: Again you are right. There is no reason why firmware could not clear error status of the device after the link has been brought out of DPC, but after a lot of back and forth, it was decided that OS will clear the error register on the device during DPC recovery=success path. This was more than 4 years ago and I honestly don't remember why we went this way.
I guess the flowchart *does* show firmware clearing the error status in the "do not continue recovery" path.
Thank you,
Mahesh
-----Original Message-----
From: Bjorn Helgaas <helgaas@kernel.org>
Sent: Thursday, April 06, 2023 3:22 PM
To: Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc: Natu, Mahesh <mahesh.natu@intel.com>; Bjorn Helgaas <bhelgaas@google.com>; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] PCI/EDR: Clear PCIe Device Status errors after EDR error recovery
On Thu, Apr 06, 2023 at 02:52:02PM -0700, Sathyanarayanan Kuppuswamy wrote:
> On 4/6/23 2:07 PM, Bjorn Helgaas wrote:
> > On Wed, Mar 15, 2023 at 04:54:49PM -0700, Kuppuswamy Sathyanarayanan wrote:
> >> Commit 068c29a248b6 ("PCI/ERR: Clear PCIe Device Status errors only
> >> if OS owns AER") adds support to clear error status in the Device
> >> Status
> >> Register(DEVSTA) only if OS owns the AER support. But this change
> >> breaks the requirement of the EDR feature which requires OS to
> >> cleanup the error registers even if firmware owns the control of AER support.
> >>
> >> More details about this requirement can be found in PCIe Firmware
> >> specification v3.3, Table 4-6 Interpretation of the _OSC Control Field.
> >> If the OS supports the Error Disconnect Recover (EDR) feature and
> >> firmware sends the EDR event, then during the EDR recovery window,
> >> OS is responsible for the device error recovery and holds the
> >> ownership of the following error registers.
> >>
> >> • Device Status Register
> >> • Uncorrectable Error Status Register • Correctable Error Status
> >> Register • Root Error Status Register • RP PIO Status Register
> >>
> >> So call pcie_clear_device_status() in edr_handle_event() if the
> >> error recovery is successful.
> >>
> >> Reported-by: Tsaur Erwin <erwin.tsaur@intel.com>
> >> Signed-off-by: Kuppuswamy Sathyanarayanan
> >> <sathyanarayanan.kuppuswamy@linux.intel.com>
> >> ---
> >>
> >> Changes since v1:
> >> * Rebased on top of v6.3-rc1.
> >> * Fixed a typo in pcie_clear_device_status().
> >>
> >> drivers/pci/pcie/edr.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c index
> >> a6b9b479b97a..87734e4c3c20 100644
> >> --- a/drivers/pci/pcie/edr.c
> >> +++ b/drivers/pci/pcie/edr.c
> >> @@ -193,6 +193,7 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data)
> >> */
> >> if (estate == PCI_ERS_RESULT_RECOVERED) {
> >> pci_dbg(edev, "DPC port successfully recovered\n");
> >> + pcie_clear_device_status(edev);
> >> acpi_send_edr_status(pdev, edev, EDR_OST_SUCCESS);
> >
> > The implementation note in PCI Firmware r3.3, sec 4.6.12, shows the
> > OS clearing error status *after* _OST is evaluated.
> >
> > On the other hand, the _OSC DPC control bit in table 4-6 says that
> > if the OS does not have DPC control, it can only write the Device
> > Status error bits between the EDR Notify and invoking _OST.
> >
> > Is one of those wrong, or am I missing something?
>
> Agree. It is conflicting info. IMO, the argument that the OS is
> allowed to clear the error registers during the EDR windows makes more
> sense. If OS is allowed to touch error registers owned by firmware
> after that window, it would lead to race conditions.
>
> Mahesh, let us know your comments. Maybe we need to fix this in the
> firmware specification.
My assumption was this sequence is something like this, where firmware
*can't* collect error status from devices below the Downstream Port because DPC has been triggered and they are not accessible:
- Hardware triggers DPC in a Downstream Port
- Firmware fields error interrupt
- Firmware captures Downstream Port error info (devices below are
not accessible because of DPC)
- Firmware sends EDR Notify to OS
- OS brings Downstream Port out of DPC
- OS collects error status from devices below Downstream Port
- OS evaluates _OST
- Firmware captures error status from devices below Downstream Port
But that doesn't explain why *firmware* could not clear the error status of those devices after it captures it.
I guess the flowchart *does* show firmware clearing the error status in the "do not continue recovery" path.
Hi Bjorn, On 4/6/23 3:21 PM, Bjorn Helgaas wrote: > On Thu, Apr 06, 2023 at 02:52:02PM -0700, Sathyanarayanan Kuppuswamy wrote: >> On 4/6/23 2:07 PM, Bjorn Helgaas wrote: >>> On Wed, Mar 15, 2023 at 04:54:49PM -0700, Kuppuswamy Sathyanarayanan wrote: >>>> Commit 068c29a248b6 ("PCI/ERR: Clear PCIe Device Status errors only if >>>> OS owns AER") adds support to clear error status in the Device Status >>>> Register(DEVSTA) only if OS owns the AER support. But this change >>>> breaks the requirement of the EDR feature which requires OS to cleanup >>>> the error registers even if firmware owns the control of AER support. >>>> >>>> More details about this requirement can be found in PCIe Firmware >>>> specification v3.3, Table 4-6 Interpretation of the _OSC Control Field. >>>> If the OS supports the Error Disconnect Recover (EDR) feature and >>>> firmware sends the EDR event, then during the EDR recovery window, OS >>>> is responsible for the device error recovery and holds the ownership of >>>> the following error registers. >>>> >>>> • Device Status Register >>>> • Uncorrectable Error Status Register >>>> • Correctable Error Status Register >>>> • Root Error Status Register >>>> • RP PIO Status Register >>>> >>>> So call pcie_clear_device_status() in edr_handle_event() if the error >>>> recovery is successful. >>>> >>>> Reported-by: Tsaur Erwin <erwin.tsaur@intel.com> >>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >>>> --- >>>> >>>> Changes since v1: >>>> * Rebased on top of v6.3-rc1. >>>> * Fixed a typo in pcie_clear_device_status(). >>>> >>>> drivers/pci/pcie/edr.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c >>>> index a6b9b479b97a..87734e4c3c20 100644 >>>> --- a/drivers/pci/pcie/edr.c >>>> +++ b/drivers/pci/pcie/edr.c >>>> @@ -193,6 +193,7 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data) >>>> */ >>>> if (estate == PCI_ERS_RESULT_RECOVERED) { >>>> pci_dbg(edev, "DPC port successfully recovered\n"); >>>> + pcie_clear_device_status(edev); >>>> acpi_send_edr_status(pdev, edev, EDR_OST_SUCCESS); >>> >>> The implementation note in PCI Firmware r3.3, sec 4.6.12, shows the OS >>> clearing error status *after* _OST is evaluated. >>> >>> On the other hand, the _OSC DPC control bit in table 4-6 says that if >>> the OS does not have DPC control, it can only write the Device Status >>> error bits between the EDR Notify and invoking _OST. >>> >>> Is one of those wrong, or am I missing something? >> >> Agree. It is conflicting info. IMO, the argument that the OS is allowed to >> clear the error registers during the EDR windows makes more sense. If OS >> is allowed to touch error registers owned by firmware after that window, >> it would lead to race conditions. >> >> Mahesh, let us know your comments. Maybe we need to fix this in the firmware >> specification. > > My assumption was this sequence is something like this, where firmware > *can't* collect error status from devices below the Downstream Port > because DPC has been triggered and they are not accessible: > > - Hardware triggers DPC in a Downstream Port > - Firmware fields error interrupt > - Firmware captures Downstream Port error info (devices below are > not accessible because of DPC) > - Firmware sends EDR Notify to OS > - OS brings Downstream Port out of DPC > - OS collects error status from devices below Downstream Port > - OS evaluates _OST > - Firmware captures error status from devices below Downstream Port > > But that doesn't explain why *firmware* could not clear the error > status of those devices after it captures it. > > I guess the flowchart *does* show firmware clearing the error status > in the "do not continue recovery" path. In this patch, we are clearing the port error status. So I think it is fine to do it in EDR window. Agree?
On Thu, Apr 06, 2023 at 10:31:20PM -0700, Sathyanarayanan Kuppuswamy wrote: > On 4/6/23 3:21 PM, Bjorn Helgaas wrote: > > On Thu, Apr 06, 2023 at 02:52:02PM -0700, Sathyanarayanan Kuppuswamy wrote: > >> On 4/6/23 2:07 PM, Bjorn Helgaas wrote: > >>> On Wed, Mar 15, 2023 at 04:54:49PM -0700, Kuppuswamy Sathyanarayanan wrote: > >>>> Commit 068c29a248b6 ("PCI/ERR: Clear PCIe Device Status errors only if > >>>> OS owns AER") adds support to clear error status in the Device Status > >>>> Register(DEVSTA) only if OS owns the AER support. But this change > >>>> breaks the requirement of the EDR feature which requires OS to cleanup > >>>> the error registers even if firmware owns the control of AER support. > >>>> > >>>> More details about this requirement can be found in PCIe Firmware > >>>> specification v3.3, Table 4-6 Interpretation of the _OSC Control Field. > >>>> If the OS supports the Error Disconnect Recover (EDR) feature and > >>>> firmware sends the EDR event, then during the EDR recovery window, OS > >>>> is responsible for the device error recovery and holds the ownership of > >>>> the following error registers. > >>>> > >>>> • Device Status Register > >>>> • Uncorrectable Error Status Register > >>>> • Correctable Error Status Register > >>>> • Root Error Status Register > >>>> • RP PIO Status Register > >>>> > >>>> So call pcie_clear_device_status() in edr_handle_event() if the error > >>>> recovery is successful. > >>>> > >>>> Reported-by: Tsaur Erwin <erwin.tsaur@intel.com> > >>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > >>>> --- > >>>> > >>>> Changes since v1: > >>>> * Rebased on top of v6.3-rc1. > >>>> * Fixed a typo in pcie_clear_device_status(). > >>>> > >>>> drivers/pci/pcie/edr.c | 1 + > >>>> 1 file changed, 1 insertion(+) > >>>> > >>>> diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c > >>>> index a6b9b479b97a..87734e4c3c20 100644 > >>>> --- a/drivers/pci/pcie/edr.c > >>>> +++ b/drivers/pci/pcie/edr.c > >>>> @@ -193,6 +193,7 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data) > >>>> */ > >>>> if (estate == PCI_ERS_RESULT_RECOVERED) { > >>>> pci_dbg(edev, "DPC port successfully recovered\n"); > >>>> + pcie_clear_device_status(edev); > >>>> acpi_send_edr_status(pdev, edev, EDR_OST_SUCCESS); > >>> > >>> The implementation note in PCI Firmware r3.3, sec 4.6.12, shows the OS > >>> clearing error status *after* _OST is evaluated. > >>> > >>> On the other hand, the _OSC DPC control bit in table 4-6 says that if > >>> the OS does not have DPC control, it can only write the Device Status > >>> error bits between the EDR Notify and invoking _OST. > >>> > >>> Is one of those wrong, or am I missing something? > >> > >> Agree. It is conflicting info. IMO, the argument that the OS is allowed to > >> clear the error registers during the EDR windows makes more sense. If OS > >> is allowed to touch error registers owned by firmware after that window, > >> it would lead to race conditions. > >> > >> Mahesh, let us know your comments. Maybe we need to fix this in the firmware > >> specification. > > > > My assumption was this sequence is something like this, where firmware > > *can't* collect error status from devices below the Downstream Port > > because DPC has been triggered and they are not accessible: > > > > - Hardware triggers DPC in a Downstream Port > > - Firmware fields error interrupt > > - Firmware captures Downstream Port error info (devices below are > > not accessible because of DPC) > > - Firmware sends EDR Notify to OS > > - OS brings Downstream Port out of DPC > > - OS collects error status from devices below Downstream Port > > - OS evaluates _OST > > - Firmware captures error status from devices below Downstream Port > > > > But that doesn't explain why *firmware* could not clear the error > > status of those devices after it captures it. > > > > I guess the flowchart *does* show firmware clearing the error status > > in the "do not continue recovery" path. > > In this patch, we are clearing the port error status. So I think it is > fine to do it in EDR window. Agree? Ah, right, of course, thanks for pulling me back out of the weeds! Yes, I do agree. An EDR notification is issued on a bus device that is still present, i.e., a DPC port or parent, but child devices have been disconnected (ACPI v6.3, sec 5.6.6). So in edr_handle_event(), pdev is the bus device that receives the notification, edev is a bus device (possibly the same as pdev, or possibly a child, but one that is still present), and child devices of edev have been disconnected. Prior to 068c29a248b6, pcie_do_recovery(edev) always cleared DEVSTA for edev. Afterwards it only clears DEVSTA if the OS owns AER. This patch makes edr_handle_event() clear DEVSTA for edev (the Port device). I had been looking at the bottom blue OS "Clear error status, ..." box in the PCI Firmware implementation note, but that's the wrong one. This is actually the "Clear port error status, bring Port out of DPC, ..." box higher up. That's clearly before _OST and thus inside the window. That box *does* suggest clearing the port error status before bringing the port out of DPC, and we're doing it in the opposite order: edr_handle_event(pdev) edev = acpi_dpc_port_get(pdev) # Both pdev and edev are present; pdev is same as edev or a # parent of edev; children of edev are disconnected dpc_process_error(edev) pcie_do_recovery(edev, dpc_reset_link) if (state == pci_channel_io_frozen) dpc_reset_link # (reset_subordinates) pci_write_config_word(PCI_EXP_DPC_STATUS_TRIGGER) # exit DPC if (AER native) pcie_clear_device_status(edev) clear PCI_EXP_DEVSTA # doesn't happen if (PCI_ERS_RESULT_RECOVERED) pcie_clear_device_status clear PCI_EXP_DEVSTA # added by this patch Does it matter? I dunno, but I don't *think* so. We really don't care about the value of PCI_EXP_DEVSTA anywhere except pci_wait_for_pending_transaction(), which isn't applicable here. And I don't think the fact that it probably has an Error Detected bit set when exiting DPC is a problem. Bjorn
[+cc Jonathan, Mahesh] On Wed, Mar 15, 2023 at 04:54:49PM -0700, Kuppuswamy Sathyanarayanan wrote: > Commit 068c29a248b6 ("PCI/ERR: Clear PCIe Device Status errors only if > OS owns AER") adds support to clear error status in the Device Status > Register(DEVSTA) only if OS owns the AER support. But this change > breaks the requirement of the EDR feature which requires OS to cleanup > the error registers even if firmware owns the control of AER support. > > More details about this requirement can be found in PCIe Firmware > specification v3.3, Table 4-6 Interpretation of the _OSC Control Field. > If the OS supports the Error Disconnect Recover (EDR) feature and > firmware sends the EDR event, then during the EDR recovery window, OS > is responsible for the device error recovery and holds the ownership of > the following error registers. > > • Device Status Register > • Uncorrectable Error Status Register > • Correctable Error Status Register > • Root Error Status Register > • RP PIO Status Register > > So call pcie_clear_device_status() in edr_handle_event() if the error > recovery is successful. > > Reported-by: Tsaur Erwin <erwin.tsaur@intel.com> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> Sorry I spent so much time on this, but I finally applied it to pci/aer for v6.4. I added the "Fixes: 068c29a248b6" line. It's not that 068c29a248b6 is a bug, just that EDR relied on behavior that 068c29a248b6 legitimately removed, so this patch should go where 068c29a248b6 goes. I wordsmithed the commit log to add hints about things I stumbled over. I'll post a follow-up patch to add a couple comments there, too. Thanks for your patience in educating me through all this. > --- > > Changes since v1: > * Rebased on top of v6.3-rc1. > * Fixed a typo in pcie_clear_device_status(). > > drivers/pci/pcie/edr.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c > index a6b9b479b97a..87734e4c3c20 100644 > --- a/drivers/pci/pcie/edr.c > +++ b/drivers/pci/pcie/edr.c > @@ -193,6 +193,7 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data) > */ > if (estate == PCI_ERS_RESULT_RECOVERED) { > pci_dbg(edev, "DPC port successfully recovered\n"); > + pcie_clear_device_status(edev); > acpi_send_edr_status(pdev, edev, EDR_OST_SUCCESS); > } else { > pci_dbg(edev, "DPC port recovery failed\n"); > -- > 2.34.1 >
On Fri, Apr 07, 2023 at 04:51:44PM -0500, Bjorn Helgaas wrote: > I'll post a follow-up patch to add a couple comments there, > too. Comments I propose: PCI/EDR: Add edr_handle_event() comments EDR documentation is a bit sketchy. Add a couple comments to edr_handle_event() about the devices involved. Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c index 87734e4c3c20..135ddb53661c 100644 --- a/drivers/pci/pcie/edr.c +++ b/drivers/pci/pcie/edr.c @@ -151,9 +151,17 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data) if (event != ACPI_NOTIFY_DISCONNECT_RECOVER) return; + /* + * pdev itself is still present, but one or more of its child + * devices have been disconnected (ACPI v6.3, sec 5.6.6). + */ pci_info(pdev, "EDR event received\n"); - /* Locate the port which issued EDR event */ + /* + * Locate the port that experienced the containment event. pdev + * may be that port or a parent of it (PCI Firmware r3.3, sec + * 4.6.13). + */ edev = acpi_dpc_port_get(pdev); if (!edev) { pci_err(pdev, "Firmware failed to locate DPC port\n");
Hi Bjorn, On 4/7/23 9:46 AM, Bjorn Helgaas wrote: > On Thu, Apr 06, 2023 at 10:31:20PM -0700, Sathyanarayanan Kuppuswamy wrote: >> On 4/6/23 3:21 PM, Bjorn Helgaas wrote: >>> On Thu, Apr 06, 2023 at 02:52:02PM -0700, Sathyanarayanan Kuppuswamy wrote: >>>> On 4/6/23 2:07 PM, Bjorn Helgaas wrote: >>>>> On Wed, Mar 15, 2023 at 04:54:49PM -0700, Kuppuswamy Sathyanarayanan wrote: >>>>>> Commit 068c29a248b6 ("PCI/ERR: Clear PCIe Device Status errors only if >>>>>> OS owns AER") adds support to clear error status in the Device Status >>>>>> Register(DEVSTA) only if OS owns the AER support. But this change >>>>>> breaks the requirement of the EDR feature which requires OS to cleanup >>>>>> the error registers even if firmware owns the control of AER support. >>>>>> >>>>>> More details about this requirement can be found in PCIe Firmware >>>>>> specification v3.3, Table 4-6 Interpretation of the _OSC Control Field. >>>>>> If the OS supports the Error Disconnect Recover (EDR) feature and >>>>>> firmware sends the EDR event, then during the EDR recovery window, OS >>>>>> is responsible for the device error recovery and holds the ownership of >>>>>> the following error registers. >>>>>> >>>>>> • Device Status Register >>>>>> • Uncorrectable Error Status Register >>>>>> • Correctable Error Status Register >>>>>> • Root Error Status Register >>>>>> • RP PIO Status Register >>>>>> >>>>>> So call pcie_clear_device_status() in edr_handle_event() if the error >>>>>> recovery is successful. >>>>>> >>>>>> Reported-by: Tsaur Erwin <erwin.tsaur@intel.com> >>>>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >>>>>> --- >>>>>> >>>>>> Changes since v1: >>>>>> * Rebased on top of v6.3-rc1. >>>>>> * Fixed a typo in pcie_clear_device_status(). >>>>>> >>>>>> drivers/pci/pcie/edr.c | 1 + >>>>>> 1 file changed, 1 insertion(+) >>>>>> >>>>>> diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c >>>>>> index a6b9b479b97a..87734e4c3c20 100644 >>>>>> --- a/drivers/pci/pcie/edr.c >>>>>> +++ b/drivers/pci/pcie/edr.c >>>>>> @@ -193,6 +193,7 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data) >>>>>> */ >>>>>> if (estate == PCI_ERS_RESULT_RECOVERED) { >>>>>> pci_dbg(edev, "DPC port successfully recovered\n"); >>>>>> + pcie_clear_device_status(edev); >>>>>> acpi_send_edr_status(pdev, edev, EDR_OST_SUCCESS); >>>>> >>>>> The implementation note in PCI Firmware r3.3, sec 4.6.12, shows the OS >>>>> clearing error status *after* _OST is evaluated. >>>>> >>>>> On the other hand, the _OSC DPC control bit in table 4-6 says that if >>>>> the OS does not have DPC control, it can only write the Device Status >>>>> error bits between the EDR Notify and invoking _OST. >>>>> >>>>> Is one of those wrong, or am I missing something? >>>> >>>> Agree. It is conflicting info. IMO, the argument that the OS is allowed to >>>> clear the error registers during the EDR windows makes more sense. If OS >>>> is allowed to touch error registers owned by firmware after that window, >>>> it would lead to race conditions. >>>> >>>> Mahesh, let us know your comments. Maybe we need to fix this in the firmware >>>> specification. >>> >>> My assumption was this sequence is something like this, where firmware >>> *can't* collect error status from devices below the Downstream Port >>> because DPC has been triggered and they are not accessible: >>> >>> - Hardware triggers DPC in a Downstream Port >>> - Firmware fields error interrupt >>> - Firmware captures Downstream Port error info (devices below are >>> not accessible because of DPC) >>> - Firmware sends EDR Notify to OS >>> - OS brings Downstream Port out of DPC >>> - OS collects error status from devices below Downstream Port >>> - OS evaluates _OST >>> - Firmware captures error status from devices below Downstream Port >>> >>> But that doesn't explain why *firmware* could not clear the error >>> status of those devices after it captures it. >>> >>> I guess the flowchart *does* show firmware clearing the error status >>> in the "do not continue recovery" path. >> >> In this patch, we are clearing the port error status. So I think it is >> fine to do it in EDR window. Agree? > > Ah, right, of course, thanks for pulling me back out of the weeds! > Yes, I do agree. Thanks. > > An EDR notification is issued on a bus device that is still present, > i.e., a DPC port or parent, but child devices have been disconnected > (ACPI v6.3, sec 5.6.6). IMO, instead of bus device, we can call it as root port or down stream port. Please check the PCI firmware specification, r3.3, section 4.3.13. Firmware may wish to issue Error Disconnect Recover notification on a port that is parent of the port that experienced the containment event. So it is either a downstream port or a root port. > > So in edr_handle_event(), pdev is the bus device that receives the > notification, edev is a bus device (possibly the same as pdev, or > possibly a child, but one that is still present), and child devices of > edev have been disconnected. > > Prior to 068c29a248b6, pcie_do_recovery(edev) always cleared DEVSTA > for edev. Afterwards it only clears DEVSTA if the OS owns AER. This > patch makes edr_handle_event() clear DEVSTA for edev (the Port > device). > > I had been looking at the bottom blue OS "Clear error status, ..." box > in the PCI Firmware implementation note, but that's the wrong one. > This is actually the "Clear port error status, bring Port out of DPC, > ..." box higher up. That's clearly before _OST and thus inside the > window. Correct. > > That box *does* suggest clearing the port error status before bringing > the port out of DPC, and we're doing it in the opposite order: > > edr_handle_event(pdev) > edev = acpi_dpc_port_get(pdev) > # Both pdev and edev are present; pdev is same as edev or a > # parent of edev; children of edev are disconnected > dpc_process_error(edev) > pcie_do_recovery(edev, dpc_reset_link) > if (state == pci_channel_io_frozen) > dpc_reset_link # (reset_subordinates) > pci_write_config_word(PCI_EXP_DPC_STATUS_TRIGGER) # exit DPC > if (AER native) > pcie_clear_device_status(edev) > clear PCI_EXP_DEVSTA # doesn't happen > if (PCI_ERS_RESULT_RECOVERED) > pcie_clear_device_status > clear PCI_EXP_DEVSTA # added by this patch > > Does it matter? I dunno, but I don't *think* so. We really don't > care about the value of PCI_EXP_DEVSTA anywhere except > pci_wait_for_pending_transaction(), which isn't applicable here. And > I don't think the fact that it probably has an Error Detected bit set > when exiting DPC is a problem. Agree that it is not a fatal issue. But leaving the stale error state is something that needs to be fixed. > > Bjorn
Hi Bjorn, On 4/7/23 2:51 PM, Bjorn Helgaas wrote: > [+cc Jonathan, Mahesh] > > On Wed, Mar 15, 2023 at 04:54:49PM -0700, Kuppuswamy Sathyanarayanan wrote: >> Commit 068c29a248b6 ("PCI/ERR: Clear PCIe Device Status errors only if >> OS owns AER") adds support to clear error status in the Device Status >> Register(DEVSTA) only if OS owns the AER support. But this change >> breaks the requirement of the EDR feature which requires OS to cleanup >> the error registers even if firmware owns the control of AER support. >> >> More details about this requirement can be found in PCIe Firmware >> specification v3.3, Table 4-6 Interpretation of the _OSC Control Field. >> If the OS supports the Error Disconnect Recover (EDR) feature and >> firmware sends the EDR event, then during the EDR recovery window, OS >> is responsible for the device error recovery and holds the ownership of >> the following error registers. >> >> • Device Status Register >> • Uncorrectable Error Status Register >> • Correctable Error Status Register >> • Root Error Status Register >> • RP PIO Status Register >> >> So call pcie_clear_device_status() in edr_handle_event() if the error >> recovery is successful. >> >> Reported-by: Tsaur Erwin <erwin.tsaur@intel.com> >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > Sorry I spent so much time on this, but I finally applied it to > pci/aer for v6.4. > > I added the "Fixes: 068c29a248b6" line. It's not that 068c29a248b6 is > a bug, just that EDR relied on behavior that 068c29a248b6 legitimately > removed, so this patch should go where 068c29a248b6 goes. > Agree > I wordsmithed the commit log to add hints about things I stumbled > over. I'll post a follow-up patch to add a couple comments there, > too. > > Thanks for your patience in educating me through all this. Thanks for working on it. I really appreciate your help. > >> --- >> >> Changes since v1: >> * Rebased on top of v6.3-rc1. >> * Fixed a typo in pcie_clear_device_status(). >> >> drivers/pci/pcie/edr.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c >> index a6b9b479b97a..87734e4c3c20 100644 >> --- a/drivers/pci/pcie/edr.c >> +++ b/drivers/pci/pcie/edr.c >> @@ -193,6 +193,7 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data) >> */ >> if (estate == PCI_ERS_RESULT_RECOVERED) { >> pci_dbg(edev, "DPC port successfully recovered\n"); >> + pcie_clear_device_status(edev); >> acpi_send_edr_status(pdev, edev, EDR_OST_SUCCESS); >> } else { >> pci_dbg(edev, "DPC port recovery failed\n"); >> -- >> 2.34.1 >>
On 4/7/23 2:52 PM, Bjorn Helgaas wrote: > On Fri, Apr 07, 2023 at 04:51:44PM -0500, Bjorn Helgaas wrote: >> I'll post a follow-up patch to add a couple comments there, >> too. > > Comments I propose: > > > PCI/EDR: Add edr_handle_event() comments > > EDR documentation is a bit sketchy. Add a couple comments to > edr_handle_event() about the devices involved. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c > index 87734e4c3c20..135ddb53661c 100644 > --- a/drivers/pci/pcie/edr.c > +++ b/drivers/pci/pcie/edr.c > @@ -151,9 +151,17 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data) > if (event != ACPI_NOTIFY_DISCONNECT_RECOVER) > return; > > + /* > + * pdev itself is still present, but one or more of its child > + * devices have been disconnected (ACPI v6.3, sec 5.6.6). > + */ Maybe add info that pdev can be a root port or a downstream port? > pci_info(pdev, "EDR event received\n"); > > - /* Locate the port which issued EDR event */ > + /* > + * Locate the port that experienced the containment event. pdev > + * may be that port or a parent of it (PCI Firmware r3.3, sec > + * 4.6.13). > + */ > edev = acpi_dpc_port_get(pdev); > if (!edev) { > pci_err(pdev, "Firmware failed to locate DPC port\n"); Otherwise, looks good to me. Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
On Fri, Apr 07, 2023 at 03:19:32PM -0700, Sathyanarayanan Kuppuswamy wrote: > On 4/7/23 9:46 AM, Bjorn Helgaas wrote: > > On Thu, Apr 06, 2023 at 10:31:20PM -0700, Sathyanarayanan Kuppuswamy wrote: > >> On 4/6/23 3:21 PM, Bjorn Helgaas wrote: > >>> On Thu, Apr 06, 2023 at 02:52:02PM -0700, Sathyanarayanan Kuppuswamy wrote: > >>>> On 4/6/23 2:07 PM, Bjorn Helgaas wrote: > >>>>> On Wed, Mar 15, 2023 at 04:54:49PM -0700, Kuppuswamy Sathyanarayanan wrote: > >>>>>> Commit 068c29a248b6 ("PCI/ERR: Clear PCIe Device Status errors only if > >>>>>> OS owns AER") adds support to clear error status in the Device Status > >>>>>> Register(DEVSTA) only if OS owns the AER support. But this change > >>>>>> breaks the requirement of the EDR feature which requires OS to cleanup > >>>>>> the error registers even if firmware owns the control of AER support. > ... > > An EDR notification is issued on a bus device that is still present, > > i.e., a DPC port or parent, but child devices have been disconnected > > (ACPI v6.3, sec 5.6.6). > > IMO, instead of bus device, we can call it as root port or down stream > port. Please check the PCI firmware specification, r3.3, section 4.3.13. Yeah, that makes sense. I just used the "bus device" language because that's what's in the ACPI spec. But I think the PCI terms would probably be more helpful here. And I'll cite the r6.5 spec instead of v6.3. > Firmware may wish to issue Error Disconnect Recover notification on a port > that is parent of the port that experienced the containment event. > > So it is either a downstream port or a root port. Right. > > That box *does* suggest clearing the port error status before bringing > > the port out of DPC, and we're doing it in the opposite order: > > > > edr_handle_event(pdev) > > edev = acpi_dpc_port_get(pdev) > > # Both pdev and edev are present; pdev is same as edev or a > > # parent of edev; children of edev are disconnected > > dpc_process_error(edev) > > pcie_do_recovery(edev, dpc_reset_link) > > if (state == pci_channel_io_frozen) > > dpc_reset_link # (reset_subordinates) > > pci_write_config_word(PCI_EXP_DPC_STATUS_TRIGGER) # exit DPC > > if (AER native) > > pcie_clear_device_status(edev) > > clear PCI_EXP_DEVSTA # doesn't happen > > if (PCI_ERS_RESULT_RECOVERED) > > pcie_clear_device_status > > clear PCI_EXP_DEVSTA # added by this patch > > > > Does it matter? I dunno, but I don't *think* so. We really don't > > care about the value of PCI_EXP_DEVSTA anywhere except > > pci_wait_for_pending_transaction(), which isn't applicable here. And > > I don't think the fact that it probably has an Error Detected bit set > > when exiting DPC is a problem. > > Agree that it is not a fatal issue. But leaving the stale error state > is something that needs to be fixed. Definitely agree we should clear the stale state. I just meant that I don't think it matters that we clear the status *after* exiting DPC, instead of clearing it before exiting DPC as shown in the flowchart. Bjorn
diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c index a6b9b479b97a..87734e4c3c20 100644 --- a/drivers/pci/pcie/edr.c +++ b/drivers/pci/pcie/edr.c @@ -193,6 +193,7 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data) */ if (estate == PCI_ERS_RESULT_RECOVERED) { pci_dbg(edev, "DPC port successfully recovered\n"); + pcie_clear_device_status(edev); acpi_send_edr_status(pdev, edev, EDR_OST_SUCCESS); } else { pci_dbg(edev, "DPC port recovery failed\n");
Commit 068c29a248b6 ("PCI/ERR: Clear PCIe Device Status errors only if OS owns AER") adds support to clear error status in the Device Status Register(DEVSTA) only if OS owns the AER support. But this change breaks the requirement of the EDR feature which requires OS to cleanup the error registers even if firmware owns the control of AER support. More details about this requirement can be found in PCIe Firmware specification v3.3, Table 4-6 Interpretation of the _OSC Control Field. If the OS supports the Error Disconnect Recover (EDR) feature and firmware sends the EDR event, then during the EDR recovery window, OS is responsible for the device error recovery and holds the ownership of the following error registers. • Device Status Register • Uncorrectable Error Status Register • Correctable Error Status Register • Root Error Status Register • RP PIO Status Register So call pcie_clear_device_status() in edr_handle_event() if the error recovery is successful. Reported-by: Tsaur Erwin <erwin.tsaur@intel.com> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> --- Changes since v1: * Rebased on top of v6.3-rc1. * Fixed a typo in pcie_clear_device_status(). drivers/pci/pcie/edr.c | 1 + 1 file changed, 1 insertion(+)