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@xxxxxxxxx>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
>
> 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
>>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
