On Mon, Mar 23, 2020 at 05:26:07PM -0700, sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx wrote: > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> > > Error Disconnect Recover (EDR) is a feature that allows ACPI firmware to > notify OSPM that a device has been disconnected due to an error condition > (ACPI v6.3, sec 5.6.6). OSPM advertises its support for EDR on PCI devices > via _OSC (see [1], sec 4.5.1, table 4-4). The OSPM EDR notify handler > should invalidate software state associated with disconnected devices and > may attempt to recover them. OSPM communicates the status of recovery to > the firmware via _OST (sec 6.3.5.2). > > For PCIe, firmware may use Downstream Port Containment (DPC) to support > EDR. Per [1], sec 4.5.1, table 4-6, even if firmware has retained control > of DPC, OSPM may read/write DPC control and status registers during the EDR > notification processing window, i.e., from the time it receives an EDR > notification until it clears the DPC Trigger Status. > > Note that per [1], sec 4.5.1 and 4.5.2.4, > > 1. If the OS supports EDR, it should advertise that to firmware by > setting OSC_PCI_EDR_SUPPORT in _OSC Support. > > 2. If the OS sets OSC_PCI_EXPRESS_DPC_CONTROL in _OSC Control to request > control of the DPC capability, it must also set OSC_PCI_EDR_SUPPORT in > _OSC Support. > > Add an EDR notify handler to attempt recovery. > > [1] Downstream Port Containment Related Enhancements ECN, Jan 28, 2019, > affecting PCI Firmware Specification, Rev. 3.2 > https://members.pcisig.com/wg/PCI-SIG/document/12888 > +static int acpi_enable_dpc(struct pci_dev *pdev) > +{ > + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev); > + union acpi_object *obj, argv4, req; > + int status; > + > + /* > + * Some firmware implementations will return default values for > + * unsupported _DSM calls. So checking acpi_evaluate_dsm() return > + * value for NULL condition is not a complete method for finding > + * whether given _DSM function is supported or not. So use > + * explicit func 0 call to find whether given _DSM function is > + * supported or not. > + */ > + status = acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5, > + 1ULL << EDR_PORT_DPC_ENABLE_DSM); > + if (!status) > + return 0; > + > + status = 0; > + req.type = ACPI_TYPE_INTEGER; > + req.integer.value = 1; > + > + argv4.type = ACPI_TYPE_PACKAGE; > + argv4.package.count = 1; > + argv4.package.elements = &req; > + > + /* > + * Per Downstream Port Containment Related Enhancements ECN to PCI > + * Firmware Specification r3.2, sec 4.6.12, EDR_PORT_DPC_ENABLE_DSM is > + * optional. Return success if it's not implemented. > + */ > + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5, > + EDR_PORT_DPC_ENABLE_DSM, &argv4); This has been upstream for a while, just a follow-up question: this _DSM function was defined by the ECN with Rev 5. The ECN was incorporated into the PCI Firmware spec r3.3 with slightly different behavior as Rev 6. The main differences are: ECN - Rev 5 - Arg3 is an Integer - Return is 0 (DPC disabled) or 1 (DPC enabled) r3.3 spec - Rev 6 - Arg3 is a Package of one Integer - Return is 0 (DPC disabled, Hot-Plug Surprise may be set), 1 (DPC enabled, Hot-Plug Surprise may be cleared), or 2 (failure) So the question is whether this actually implements Rev 5 or Rev 6? It looks like this builds a *package* for Arg3 (which would correspond to Rev 6), but we're evaluating Rev 5, which specified an Integer. The meaning of the Arg3 values is basically the same, so I don't see an issue there, but it looks like if a platform implemented Rev 5 according to the ECN to take a bare Integer, this might not work correctly. > + if (!obj) > + return 0; > + > + if (obj->type != ACPI_TYPE_INTEGER) { > + pci_err(pdev, FW_BUG "Enable DPC _DSM returned non integer\n"); > + status = -EIO; > + } > + > + if (obj->integer.value != 1) { > + pci_err(pdev, "Enable DPC _DSM failed to enable DPC\n"); > + status = -EIO; > + } > + > + ACPI_FREE(obj); > + > + return status; > +}
