On 4/11/24 11:07 AM, Bjorn Helgaas wrote: > 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. I think it implements rev 6. The version number needs to be updated. If you would like, I can submit a patch to fix it. > >> + 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; >> +} -- Sathyanarayanan Kuppuswamy Linux Kernel Developer
