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
