Re: [PATCH v6 01/12] PCI/ACPI: Add D3cold Aux Power Limit_DSM method

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Jan 20, 2026 at 07:33:17PM +0530, Nilawar, Badal wrote:
> On 15-01-2026 01:54, Bjorn Helgaas wrote:
> > On Tue, Jan 13, 2026 at 10:12:02PM +0530, Badal Nilawar wrote:
> > > From: Anshuman Gupta <anshuman.gupta@intel.com>
> > > 
> > > Implement _DSM method 0Ah, as per PCI Firmware r3.3, sec 4.6.10,
> > > to request auxiliary power required by the device when in D3cold state.
> ...

> > > +int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_mw,
> > > +				      u32 *retry_interval)
> > > +{
> > > +	union acpi_object in_obj = {
> > > +		.integer.type = ACPI_TYPE_INTEGER,
> > > +		.integer.value = requested_mw,
> > > +	};
> > > +
> > > +	union acpi_object *out_obj;
> > > +	int result;
> > > +	struct pci_dev *bdev;
> > > +	struct acpi_device *adev;
> > > +	acpi_handle handle;
> > > +	struct aux_pwr *apwr, *next;
> > > +
> > > +	if (!dev)
> > > +		return -EINVAL;
> >
> > We talked about only allowing this for function 0:
> > https://lore.kernel.org/all/20250904183046.GA1267851@bhelgaas/
> 
> In rev5 there was suggestion from Rafael to remove function 0 check as
> synchronization among drivers will be
> tricky. https://patchwork.freedesktop.org/patch/681119/?series=145342&rev=5#comment_1255325

> > > +	case AUX_PWR_REQ_GRANTED:
> > > +		pci_info(bdev, "D3cold Aux Power request granted: %u mW\n",
> > > +			 requested_mw);
> > > +		apwr = kzalloc(sizeof(*apwr), GFP_KERNEL);
> > > +		if (apwr) {
> > > +			apwr->aux_pwr_limit = requested_mw;
> > > +			apwr->dev = &dev->dev;
> > > +			apwr->adev = adev;
> > > +			INIT_LIST_HEAD(&apwr->list);
> > > +			list_add(&acpi_aux_pwr_list,
> > > +				 &apwr->list);
> > > +		}
> >
> > I think we leak this allocation if the device is removed.  I think the
> > list idea is more complicated than aggregating would be.
> > 
> > I think we could:
> > 
> >    - add "aux_power_mw" in struct pci_dev
> > 
> >    - walk the tree below bdev, accumulating aux_power_mw
> >      (total_aux_power_mw += dev->aux_power_mw)
> > 
> >    - pass "total_aux_power_mw + requested_mw" to the _DSM
> > 
> >    - if successful, set dev->aux_power_mw = requested_mw
> 
> Introduced list based handling as in rev5 there was suggestion to avoid
> adding variables in acpi/acpi_power structures.
> To handle allocation leak in function acpi_device_release() I will add code
> to scan list and delete the entry.

I think it's a mistake to add list handling for this sort of
non-dynamic situation because it adds complexity and lifetime issues.

I'm OK with adding this data to struct pci_dev.  That means we don't
have to deal with any extra list management or lifetime issues, and
it's also a way to handle the aggregation and avoid the problem of
synchronizing between drivers.

My proposal is to aggregate the power requirements of all the devices
below the bridge that implements this _DSM (bdev), and store the total
requirement in bdev->aux_power_mw.

We can do the aggregation piecemeal.  As each downstream driver calls
this function, its requirement can be added to aux_power_mw.

> > > +		if (retry_interval) {
> > > +			*retry_interval = result & 0xF;
> > > +			return -EAGAIN;
> > > +		}
> > > +		return -EINVAL;
> >
> > I think we should do:
> > 
> >    case AUX_PWR_REQ_RETRY_INTERVAL_MIN ...  AUX_PWR_REQ_RETRY_INTERVAL_MAX:
> >      result &= 0xf;
> >      pci_info(bdev, "... needs retry", result);
> >      if (retry_interval)
> >        *retry_interval = result;
> >      return -EAGAIN;
> > 
> > I don't think it's useful to return different errors based on whether
> > the caller supplied a "retry_interval" pointer.
> 
> This is to indicate caller to retry after retry_interval returned by ACPI
> method.
>
> It's up to caller whether to retry or just proceed.

I understand what the retry_interval is for.  Obviously the caller can
always decide whether to retry.

How does it help the caller if we return -EAGAIN if it supplied a
retry_interval pointer, but -EINVAL if it didn't?

When the _DSM returns an AUX_PWR_REQ_RETRY_INTERVAL status, I think we
should always return -EAGAIN.

Bjorn