Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6010286e-c7b4-34d1-94d1-46b7f152c942@linux.intel.com>
Date:   Fri, 7 Apr 2023 15:21:41 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.9.0
Subject: Re: [PATCH v2] PCI/EDR: Clear PCIe Device Status errors after EDR
 error recovery
Content-Language: en-US
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Bjorn Helgaas <bhelgaas@google.com>, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        "Natu, Mahesh" <mahesh.natu@intel.com>
References: <20230407215142.GA3825302@bhelgaas>
From:   Sathyanarayanan Kuppuswamy 
        <sathyanarayanan.kuppuswamy@linux.intel.com>
In-Reply-To: <20230407215142.GA3825302@bhelgaas>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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
>>

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer