Message-ID: <CAJZ5v0h-uOs1Q3RXxsbw4AOxTvY5siGrnxSLfXfk+Ea-ZUz7bQ@mail.gmail.com>
Date: Mon, 29 Dec 2025 14:12:10 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Linux ACPI <linux-acpi@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, Bjorn Helgaas <helgaas@...nel.org>, 
	Zhang Rui <rui.zhang@...el.com>, Chen Yu <yu.c.chen@...el.com>, 
	Mika Westerberg <mika.westerberg@...ux.intel.com>, Linux PCI <linux-pci@...r.kernel.org>, 
	Alex Hung <alexhung@...il.com>, Hans de Goede <hansg@...nel.org>, 
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, 
	platform-driver-x86@...r.kernel.org, AceLan Kao <acelan.kao@...onical.com>
Subject: Re: [PATCH v1 1/4] ACPI: PNP: Drop PNP0C01 and PNP0C02 from acpi_pnp_device_ids[]

On Sun, Dec 28, 2025 at 8:11 PM Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
>
> On Mon, Dec 15, 2025 at 02:34:06PM +0100, Rafael J. Wysocki wrote:
>
> > There is a long-standing problem with ACPI device enumeration that
> > if the given device has a compatible ID which is one of the generic
> > system resource device IDs (PNP0C01 and PNP0C02), it will be claimed
> > by the PNP scan handler and it will not be represented as a platform
> > device, so it cannot be handled by a platform driver.
> >
> > Drivers have been working around this issue by "manually" creating
> > platform devices that they can bind to (see the Intel HID driver for
> > one example) or adding their device IDs to acpi_nonpnp_device_ids[].
> > None of the above is particularly clean though and the only reason why
> > the PNP0C01 and PNP0C02 device IDs are present in acpi_pnp_device_ids[]
> > is to allow the legacy PNP system driver to bind to those devices and
> > reserve their resources so they are not used going forward.
> >
> > Obviously, to address this problem PNP0C01 and PNP0C02 need to be
> > dropped from acpi_pnp_device_ids[], but doing so without making any
> > other changes would be problematic because the ACPI core would then
> > create platform devices for the generic system resource device objects
> > and that would not work on all systems for two reasons.  First, the
> > PNP system driver explicitly avoids reserving I/O resources below the
> > "standard PC hardware" boundary, 0x100, to avoid conflicts in that range
> > (one possible case when this may happen is when the CMOS RTC driver is
> > involved), but the platform device creation code does not do that.
> > Second, there may be resource conflicts between the "system" devices and
> > the other devices in the system, possibly including conflicts with PCI
> > BARs.  Registering the PNP system driver via fs_initcall() helps to
> > manage those conflicts, even though it does not make them go away.
> > Resource conflicts during the registration of "motherboard resources"
> > that occur after PCI has claimed BARs are harmless as a rule and do
> > not need to be addressed in any specific way.
> >
> > To overcome the issues mentioned above, use the observation that it
> > is not actually necessary to create any device objects in addition
> > to struct acpi_device ones in order to reserve the "system" device
> > resources because that can be done directly in the ACPI device
> > enumeration code.
> >
> > Namely, modify acpi_default_enumeration() to add the given ACPI device
> > object to a special "system devices" list if its _HID is either PNP0C01
> > or PNP0C02 without creating a platform device for it.  Next, add a new
> > special acpi_scan_claim_resources() function that will be run via
> > fs_initcall() and will walk that list and reserve resources for each
> > device in it along the lines of what the PNP system driver does.
> >
> > Having made the above changes, drop PNP0C01 and PNP0C02 from
> > acpi_pnp_device_ids[] which will allow platform devices to be created
> > for ACPI device objects whose _CID lists contain PNP0C01 or PNP0C02,
> > but the _HID is not in acpi_pnp_device_ids[].
>
> ...
>
> > +static const char * const acpi_system_dev_ids[] = {
> > +     "PNP0C01", /* Memory controller */
> > +     "PNP0C02", /* Motherboard resource */
> > +     NULL
> > +};
>
> ...
>
> > +     if (match_string(acpi_system_dev_ids, -1, acpi_device_hid(device)) >= 0) {
>
> Using -1 makes sense when we have no direct visibility of the mentioned array.
> Here we have it visible and statically defined, hence the ARRAY_SIZE() is more
> appropriate.

But as it stands it is more consistent with what happens elsewhere in this file.

> > +             struct acpi_scan_system_dev *sd;
> > +
> > +             /*
> > +              * This is a generic system device, so there is no need to
> > +              * create a platform device for it, but its resources need to be
> > +              * reserved.  However, that needs to be done after all of the
> > +              * other device objects have been processed and PCI has claimed
> > +              * BARs in case there are resource conflicts.
> > +              */
> > +             sd = kmalloc(sizeof(*sd), GFP_KERNEL);
> > +             if (sd) {
> > +                     sd->adev = device;
> > +                     list_add_tail(&sd->node, &acpi_scan_system_dev_list);
> > +             }
> > +     } else {
> > +             /* For a regular device object, create a platform device. */
> > +             acpi_create_platform_device(device, NULL);
> > +     }
> > +     acpi_device_set_enumerated(device);
> >  }
>
> ...
>
> > +static void acpi_scan_claim_resources(struct acpi_device *adev)
> > +{
> > +     struct list_head resource_list = LIST_HEAD_INIT(resource_list);
> > +     struct resource_entry *rentry;
> > +     unsigned int count = 0;
> > +     const char *regionid;
>
> > +     if (acpi_dev_get_resources(adev, &resource_list, NULL, NULL) <= 0)
> > +             return;
>
> Strictly speaking the acpi_dev_free_resource_list() still needs to be called
> on 0 return as it's called only for the error cases.

The return value of 0 means that the list is empty, so there's nothing to free.

> I think this is the first and the only time I see a combined comparison <= 0
> for the acpi_dev_get_resources().

I don't see a problem here though.

> > +     regionid = kstrdup(dev_name(&adev->dev), GFP_KERNEL);
> > +     if (!regionid)
> > +             goto exit;
> > +
> > +     list_for_each_entry(rentry, &resource_list, node) {
> > +             struct resource *res = rentry->res;
> > +             struct resource *r;
> > +
> > +             /* Skip disabled and invalid resources. */
> > +             if ((res->flags & IORESOURCE_DISABLED) || res->end < res->start)
> > +                     continue;
>
> > +             if (res->flags & IORESOURCE_IO) {
>
> We have resource_type() helper.

I had considered using it, but then I couldn't convince myself that it
would matter, so I went for simpler code.

> And I believe the direct comparison in this case is better.

You seem to be arguing that it matters after all, so why does it matter?

Powered by blists - more mailing lists