diff mbox series

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

Message ID 20230315235449.1279209-1-sathyanarayanan.kuppuswamy@linux.intel.com (mailing list archive)
State Accepted
Delegated to: Bjorn Helgaas
Headers show
Series [v2] PCI/EDR: Clear PCIe Device Status errors after EDR error recovery | expand

Commit Message

Kuppuswamy Sathyanarayanan March 15, 2023, 11:54 p.m. UTC
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(+)

Comments

Kuppuswamy Sathyanarayanan March 24, 2023, 5:53 p.m. UTC | #1
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");
Bjorn Helgaas March 29, 2023, 10:09 p.m. UTC | #2
[+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
>
Kuppuswamy Sathyanarayanan March 29, 2023, 10:38 p.m. UTC | #3
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
>>
Bjorn Helgaas March 30, 2023, 3:45 p.m. UTC | #4
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
Kuppuswamy Sathyanarayanan March 31, 2023, 6:46 a.m. UTC | #5
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
Bjorn Helgaas March 31, 2023, 3:10 p.m. UTC | #6
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
Bjorn Helgaas April 6, 2023, 9:07 p.m. UTC | #7
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
Kuppuswamy Sathyanarayanan April 6, 2023, 9:52 p.m. UTC | #8
+ 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
Bjorn Helgaas April 6, 2023, 10:21 p.m. UTC | #9
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.
Natu, Mahesh April 6, 2023, 10:46 p.m. UTC | #10
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.
Kuppuswamy Sathyanarayanan April 7, 2023, 5:31 a.m. UTC | #11
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?
Bjorn Helgaas April 7, 2023, 4:46 p.m. UTC | #12
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
Bjorn Helgaas April 7, 2023, 9:51 p.m. UTC | #13
[+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
>
Bjorn Helgaas April 7, 2023, 9:52 p.m. UTC | #14
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");
Kuppuswamy Sathyanarayanan April 7, 2023, 10:19 p.m. UTC | #15
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
Kuppuswamy Sathyanarayanan April 7, 2023, 10:21 p.m. UTC | #16
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
>>
Kuppuswamy Sathyanarayanan April 7, 2023, 10:25 p.m. UTC | #17
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>
Bjorn Helgaas April 7, 2023, 10:41 p.m. UTC | #18
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 mbox series

Patch

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");