Message-ID: <aVGA9Mgwht_4nlPD@smile.fi.intel.com>
Date: Sun, 28 Dec 2025 21:11:48 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: 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 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.

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

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

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

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

> +			/*
> +			 * Follow the PNP system driver and on x86 skip I/O
> +			 * resources that start below 0x100 (the "standard PC
> +			 * hardware" boundary).
> +			 */
> +			if (IS_ENABLED(CONFIG_X86) && res->start < 0x100) {
> +				dev_info(&adev->dev, "Skipped %pR\n", res);
> +				continue;
> +			}
> +			r = request_region(res->start, resource_size(res), regionid);
> +		} else if (res->flags & IORESOURCE_MEM) {
> +			r = request_mem_region(res->start, resource_size(res), regionid);
> +		} else {
> +			continue;
> +		}
> +
> +		if (r) {
> +			r->flags &= ~IORESOURCE_BUSY;
> +			dev_info(&adev->dev, "Reserved %pR\n", r);
> +			count++;
> +		} else {
> +			dev_info(&adev->dev, "Could not reserve %pR\n", res);
> +		}
> +	}
> +
> +	if (!count)
> +		kfree(regionid);
> +
> +exit:
> +	acpi_dev_free_resource_list(&resource_list);
> +}

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists