On Wed, May 01, 2024 at 02:25:43AM +0000, Kuppuswamy Sathyanarayanan wrote:
> During the Error Disconnect Recover (EDR) spec transition from r3.2 ECN
> to PCI firmware spec r3.3, improvements were made to definitions of
> EDR_PORT_DPC_ENABLE_DSM (0x0C) and EDR_PORT_LOCATE_DSM(0x0D) _DSMs.
>
> Specifically,
>
> * EDR_PORT_DPC_ENABLE_DSM _DSM version changed from 5 to 6, and
> arg4 is now a package type instead of an integer in version 5.
> * EDR_PORT_LOCATE_DSM _DSM uses BIT(31) to return the status of the
> operation.
>
> Ensure _DSM implementation aligns with PCI firmware r3.3 spec
> recommendation. More details about the EDR_PORT_DPC_ENABLE_DSM and
> EDR_PORT_LOCATE_DSM _DSMs can be found in PCI firmware specification,
> r3.3, sec 4.6.12 and sec 4.6.13.
>
> While at it, fix a typo in EDR_PORT_LOCATE_DSM comments.
>
> Fixes: ac1c8e35a326 ("PCI/DPC: Add Error Disconnect Recover (EDR) support")
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> ---
> drivers/pci/pcie/edr.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c
> index 5f4914d313a1..fea098e22e3e 100644
> --- a/drivers/pci/pcie/edr.c
> +++ b/drivers/pci/pcie/edr.c
> @@ -35,7 +35,7 @@ static int acpi_enable_dpc(struct pci_dev *pdev)
> * Behavior when calling unsupported _DSM functions is undefined,
> * so check whether EDR_PORT_DPC_ENABLE_DSM is supported.
> */
> - if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
> + if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 6,
> 1ULL << EDR_PORT_DPC_ENABLE_DSM))
How confident are we that this won't break any existing platforms?
Any idea how many platforms implement EDR_PORT_DPC_ENABLE_DSM and what
Revision IDs they support?
> return 0;
>
> @@ -47,11 +47,11 @@ static int acpi_enable_dpc(struct pci_dev *pdev)
> 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.
> + * Per PCI Firmware Specification r3.3, 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,
> + obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 6,
> EDR_PORT_DPC_ENABLE_DSM, &argv4);
> if (!obj)
> return 0;
> @@ -86,7 +86,7 @@ static struct pci_dev *acpi_dpc_port_get(struct pci_dev *pdev)
>
> /*
> * Behavior when calling unsupported _DSM functions is undefined,
> - * so check whether EDR_PORT_DPC_ENABLE_DSM is supported.
> + * so check whether EDR_PORT_LOCATE_DSM is supported.
> */
> if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
> 1ULL << EDR_PORT_LOCATE_DSM))
> @@ -103,6 +103,17 @@ static struct pci_dev *acpi_dpc_port_get(struct pci_dev *pdev)
> return NULL;
> }
>
> + /*
> + * Per PCI Firmware Specification r3.3, sec 4.6.13, bit 31 represents
> + * the success/failure of the operation. If bit 31 is set, the operation
> + * is failed.
> + */
> + if (obj->integer.value & BIT(31)) {
> + ACPI_FREE(obj);
> + pci_err(pdev, "Locate Port _DSM failed\n");
> + return NULL;
> + }
This changes two _DSMs, and I think it should be two patches.
Same question here: we now depend on functionality we didn't depend on
before. How confident are we in this?
> /*
> * Firmware returns DPC port BDF details in following format:
> * 15:8 = bus
> --
> 2.25.1
>
