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

[Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com>]

On 7/2/25 4:11 AM, Rafael J. Wysocki wrote:
> On Thu, May 29, 2025 at 11:41 PM Sathyanarayanan Kuppuswamy
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>
>> On 5/29/25 4:16 AM, Badal Nilawar 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);
>>> +
>>> +     /* 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 */
>>
>> Do you need a new lock ?
> Yes.
>
>> Is it possible to reuse existing mutex like device_lock()?
> No.
>
> Doing such things results in code where nobody knows what the given
> lock scope is.

Got it. Thanks for the clarification.


-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer