On 05-09-2025 00:06, Bjorn Helgaas wrote:
> On Thu, May 29, 2025 at 04:46:45PM +0530, Badal Nilawar wrote:
>> For given root port allow one Aux power limit request.
> Please include the reason why we do this.  I don't think we need it.

  This is added as aggregation is not supported. I will add the reason.

>
> PCI Firmware r3.3, sec 4.6.10 says this _DSM function can be invoked
> multiple times with different requests.
>
> We might need a mutex just to avoid concurrent evaluations of this
> _DSM function by different drivers; I'm not sure whether there's a
> requirement to avoid that or whether the ACPI core already enforces
> something like that.  But that would be a separate thing from
> aux_power_limit.

As discussed in patch1 as driver's pci_dev  will be passed to 
pci_acpi_request_d3cold_aux_power(), will save that |pci_dev| in |struct 
acpi_device_power| to support multiple |_DSM| invocations by the same 
driver.

Thanks,
Badal

>
>> Cc: Rafael J. Wysocki<rafael.j.wysocki@intel.com>
>> Cc: Anshuman Gupta<anshuman.gupta@intel.com>
>> Signed-off-by: Badal Nilawar<badal.nilawar@intel.com>
>> ---
>>   drivers/acpi/scan.c     |  1 +
>>   drivers/pci/pci-acpi.c  | 25 ++++++++++++++++++++++++-
>>   include/acpi/acpi_bus.h |  2 ++
>>   3 files changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index fb1fe9f3b1a3..9ae7be9db01a 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -745,6 +745,7 @@ int acpi_device_add(struct acpi_device *device)
>>   	INIT_LIST_HEAD(&device->physical_node_list);
>>   	INIT_LIST_HEAD(&device->del_list);
>>   	mutex_init(&device->physical_node_lock);
>> +	mutex_init(&device->power.aux_pwr_lock);
>>   
>>   	mutex_lock(&acpi_device_lock);
>>   
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index 87f30910a5f1..d33efba4ca94 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -1451,6 +1451,7 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>   	union acpi_object *out_obj;
>>   	acpi_handle handle;
>>   	int result, ret = -EINVAL;
>> +	struct acpi_device *adev;
>>   
>>   	if (!dev || !retry_interval)
>>   		return -EINVAL;
>> @@ -1464,11 +1465,27 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>   		return -ENODEV;
>>   	}
>>   
>> +	adev = ACPI_COMPANION(&dev->dev);
>> +	if (!adev)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&adev->power.aux_pwr_lock);
>> +
>> +	/* Check if aux power already granted */
>> +	if (adev->power.aux_power_limit) {
>> +		pci_info(dev, "D3cold Aux Power request already granted: %u mW\n",
>> +			 adev->power.aux_power_limit);
>> +		mutex_unlock(&adev->power.aux_pwr_lock);
>> +		return -EPERM;
>> +	}
>> +
>>   	out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
>>   					  DSM_PCI_D3COLD_AUX_POWER_LIMIT,
>>   					  &in_obj, ACPI_TYPE_INTEGER);
>> -	if (!out_obj)
>> +	if (!out_obj) {
>> +		mutex_unlock(&adev->power.aux_pwr_lock);
>>   		return -EINVAL;
>> +	}
>>   
>>   	result = out_obj->integer.value;
>>   	if (retry_interval)
>> @@ -1478,14 +1495,17 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>   	case 0x0:
>>   		pci_dbg(dev, "D3cold Aux Power %u mW request denied\n",
>>   			requested_power);
>> +		adev->power.aux_power_limit = 0;
>>   		break;
>>   	case 0x1:
>>   		pci_info(dev, "D3cold Aux Power request granted: %u mW\n",
>>   			 requested_power);
>> +		adev->power.aux_power_limit = requested_power;
>>   		ret = 0;
>>   		break;
>>   	case 0x2:
>>   		pci_info(dev, "D3cold Aux Power: Main power won't be removed\n");
>> +		adev->power.aux_power_limit = 0;
>>   		ret = -EBUSY;
>>   		break;
>>   	default:
>> @@ -1500,9 +1520,12 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>   			pci_err(dev, "D3cold Aux Power: Reserved or unsupported response: 0x%x\n",
>>   				result);
>>   		}
>> +		adev->power.aux_power_limit = 0;
>>   		break;
>>   	}
>>   
>> +	mutex_unlock(&adev->power.aux_pwr_lock);
>> +
>>   	ACPI_FREE(out_obj);
>>   	return ret;
>>   }
>> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
>> index aad1a95e6863..c4ce3d84be00 100644
>> --- a/include/acpi/acpi_bus.h
>> +++ b/include/acpi/acpi_bus.h
>> @@ -294,6 +294,8 @@ struct acpi_device_power {
>>   	struct acpi_device_power_flags flags;
>>   	struct acpi_device_power_state states[ACPI_D_STATE_COUNT];	/* Power states (D0-D3Cold) */
>>   	u8 state_for_enumeration; /* Deepest power state for enumeration */
>> +	u32 aux_power_limit;		/* aux power limit granted by bios */
>> +	struct mutex aux_pwr_lock;	/* prevent concurrent aux power limit requests */
>>   };
>>   
>>   struct acpi_dep_data {
>> -- 
>> 2.34.1
>>