[PATCH v1] PCI/EDR: Align EDR implementation with PCI firmware r3.3 spec

[PATCH v1] PCI/EDR: Align EDR implementation with PCI firmware r3.3 spec

[Kuppuswamy Sathyanarayanan]

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@linux.intel.com>
---
 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))
 		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;
+	}
+
 	/*
 	 * Firmware returns DPC port BDF details in following format:
 	 *	15:8 = bus
-- 
2.25.1

Re: [PATCH v1] PCI/EDR: Align EDR implementation with PCI firmware r3.3 spec

[Bjorn Helgaas]

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@linux.intel.com>
> ---
>  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
> 

Re: [PATCH v1] PCI/EDR: Align EDR implementation with PCI firmware r3.3 spec

[Kuppuswamy Sathyanarayanan]

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@linux.intel.com>
>> ---
>>  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

RE: [PATCH v1] PCI/EDR: Align EDR implementation with PCI firmware r3.3 spec

[Thatchanamurthy, Satish]

Hi Sathya/Bjorn,

Right, from Dell we did perform limited testing on one selected platform with this EDR patch and it did work.
But if need to be tested with multiple platforms then we need to test few other configs. Please advise

Thanks,
Satish


Internal Use - Confidential
-----Original Message-----
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Sent: Tuesday, April 30, 2024 9:26 PM
To: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; Thatchanamurthy, Satish <Satish.Thatchanamurt@Dell.com>
Subject: [PATCH v1] PCI/EDR: Align EDR implementation with PCI firmware r3.3 spec


[EXTERNAL EMAIL]

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@linux.intel.com>
---
 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))
                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;
+       }
+
        /*
         * Firmware returns DPC port BDF details in following format:
         *      15:8 = bus
--
2.25.1

Re: [PATCH v1] PCI/EDR: Align EDR implementation with PCI firmware r3.3 spec

[Bjorn Helgaas]

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@linux.intel.com>

I split this into two patches and applied them to pci/edr for v6.10,
thanks!

Take a look here:
https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=edr
and make sure I didn't mess it up (only differences are comments and
commit logs).

> ---
>  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))
>  		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;
> +	}
> +
>  	/*
>  	 * Firmware returns DPC port BDF details in following format:
>  	 *	15:8 = bus
> -- 
> 2.25.1
> 

Re: [PATCH v1] PCI/EDR: Align EDR implementation with PCI firmware r3.3 spec

[Kuppuswamy Sathyanarayanan]

On 5/8/24 1:14 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@linux.intel.com>
> I split this into two patches and applied them to pci/edr for v6.10,
> thanks!
>
> Take a look here:
> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=edr
> and make sure I didn't mess it up (only differences are comments and
> commit logs).
>

Thanks for doing it. It looks good to me.

>> ---
>>  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))
>>  		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;
>> +	}
>> +
>>  	/*
>>  	 * Firmware returns DPC port BDF details in following format:
>>  	 *	15:8 = bus
>> -- 
>> 2.25.1
>>
-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer