Re: [PATCH v2] PCI/EDR: Clear PCIe Device Status errors after EDR error recovery

[Sathyanarayanan Kuppuswamy]

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