Hi,
On 5/1/24 2:50 PM, Bjorn Helgaas wrote:
> 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?
Since we are already using arg4 as package, it wont work with
platforms that implement version 5. So I think we won't be
breaking any existing users of version 5.
> Any idea how many platforms implement EDR_PORT_DPC_ENABLE_DSM and what
> Revision IDs they support?
I am not very sure about it. I think it is being used in some Dell
and Nvidia platforms.
@Satish from Dell, tested this fix in some Dell server platforms
that implements this support and found it working.
@Tushar Dave, since you previously submitted some error report
related to EDR, I assume you have some platforms that uses these
_DSMs. Can you please take a look at this patch and let us know
whether it works for you?
>
>> 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.
Ok. I can split it into two patches.
> Same question here: we now depend on functionality we didn't depend on
> before. How confident are we in this?
In some platforms I have tested so far, it seems to work. But I am
not sure whether there are other platforms that does not implement
this support.
IMO, since this change aligns with the spec, it is best to fix it.
>
>> /*
>> * Firmware returns DPC port BDF details in following format:
>> * 15:8 = bus
>> --
>> 2.25.1
>>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
