Re: [PATCH v4 02/11] PCI/ACPI: Per root port allow one Aux power limit request

["Nilawar, Badal" <badal.nilawar@intel.com>]

On 02-07-2025 16:51, Rafael J. Wysocki wrote:
> On Thu, May 29, 2025 at 1:14 PM Badal Nilawar <badal.nilawar@intel.com> wrote:
>> For given root port allow one Aux power limit request.
>>
>> 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);
> Use a mutex locking guard for this new lock, please.
Sure.
>
>> +
>> +       /* 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;
> Maybe -EALREADY?

 From definition it is for operation in progress. Not sure if it 
appropriate for "allow once" scenario.

#define EALREADY        114     /* Operation already in progress */

Thanks,
Badal

>
>> +       }
>> +
>>          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
>>