| Message ID | 3849919.JfvvSOo2yN@kreacher |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI: scan: Add PNP0D80 to the _DEP exceptions list | expand |
Hi, On 12/5/20 4:29 PM, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > The PNP0D80 ("Windows-compatible System Power Management Controller") > device ID is used for identifying the special device object providing > the LPI (Low-power S0 Idle) _DSM interface [1]. That device object > does not supply any operation regions, but it appears in _DEP lists > for other devices in the ACPI tables on some systems to enforce > specific enumeration ordering that does not matter in Linux. > > For this reason, _DEP list entries pointing to the device object whose > _CID returns PNP0D80 need not be taken into account as real operation > region dependencies, so add that device ID to the list of device IDs > for which the matching _DEP list entries should be ignored. > > Accordingly, update the function used for matching device IDs in that > list to allow it to check _CID as well as _HID and rename it to > acpi_info_matches_ids(). > > Link: https://www.uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf # [1] > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Thank you for doing this, I contemplated doing the exact same thing but never got around to it. One small review remark inline: > --- > drivers/acpi/scan.c | 27 +++++++++++++++++++++------ > 1 file changed, 21 insertions(+), 6 deletions(-) > > Index: linux-pm/drivers/acpi/scan.c > =================================================================== > --- linux-pm.orig/drivers/acpi/scan.c > +++ linux-pm/drivers/acpi/scan.c > @@ -719,25 +719,40 @@ int acpi_device_add(struct acpi_device * > /* -------------------------------------------------------------------------- > Device Enumeration > -------------------------------------------------------------------------- */ > -static bool acpi_info_matches_hids(struct acpi_device_info *info, > - const char * const hids[]) > +static bool acpi_info_matches_ids(struct acpi_device_info *info, > + const char * const ids[]) > { > + struct acpi_pnp_device_id_list *cid_list = NULL; > int i; > > if (!(info->valid & ACPI_VALID_HID)) > return false; > > - for (i = 0; hids[i]; i++) { > - if (!strcmp(info->hardware_id.string, hids[i])) > + if (info->valid & ACPI_VALID_CID) > + cid_list = &info->compatible_id_list; > + > + for (i = 0; ids[i]; i++) { > + int j; > + > + if (!strcmp(info->hardware_id.string, ids[i])) > return true; > + > + if (!cid_list) > + continue; > + > + for (j = 0; j < cid_list->count; j++) { > + if (!strcmp(cid_list->ids[j].string, ids[i])) > + return true; > + } > } > > return false; > } > > /* List of HIDs for which we ignore matching ACPI devices, when checking _DEP lists. */ > -static const char * const acpi_ignore_dep_hids[] = { > +static const char * const acpi_ignore_dep_ids[] = { > "INT3396", /* Windows System Power Management Controller */ I think this one can be dropped now, I checked my acpidump / dsdt.dsl collection and 45/45 DSDTs declaring a _HID of INT3396 also added a _CID of PNP0D80 to this. Regards, Hans > + "PNP0D80", /* Windows-compatible System Power Management Controller */ > NULL > }; > > @@ -1857,7 +1872,7 @@ static void acpi_device_dep_initialize(s > continue; > } > > - skip = acpi_info_matches_hids(info, acpi_ignore_dep_hids); > + skip = acpi_info_matches_ids(info, acpi_ignore_dep_ids); > kfree(info); > > if (skip) > > >
On Sat, Dec 5, 2020 at 7:41 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 12/5/20 4:29 PM, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > The PNP0D80 ("Windows-compatible System Power Management Controller") > > device ID is used for identifying the special device object providing > > the LPI (Low-power S0 Idle) _DSM interface [1]. That device object > > does not supply any operation regions, but it appears in _DEP lists > > for other devices in the ACPI tables on some systems to enforce > > specific enumeration ordering that does not matter in Linux. > > > > For this reason, _DEP list entries pointing to the device object whose > > _CID returns PNP0D80 need not be taken into account as real operation > > region dependencies, so add that device ID to the list of device IDs > > for which the matching _DEP list entries should be ignored. > > > > Accordingly, update the function used for matching device IDs in that > > list to allow it to check _CID as well as _HID and rename it to > > acpi_info_matches_ids(). > > > > Link: https://www.uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf # [1] > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Thank you for doing this, I contemplated doing the exact same > thing but never got around to it. > > One small review remark inline: > > > --- > > drivers/acpi/scan.c | 27 +++++++++++++++++++++------ > > 1 file changed, 21 insertions(+), 6 deletions(-) > > > > Index: linux-pm/drivers/acpi/scan.c > > =================================================================== > > --- linux-pm.orig/drivers/acpi/scan.c > > +++ linux-pm/drivers/acpi/scan.c > > @@ -719,25 +719,40 @@ int acpi_device_add(struct acpi_device * > > /* -------------------------------------------------------------------------- > > Device Enumeration > > -------------------------------------------------------------------------- */ > > -static bool acpi_info_matches_hids(struct acpi_device_info *info, > > - const char * const hids[]) > > +static bool acpi_info_matches_ids(struct acpi_device_info *info, > > + const char * const ids[]) > > { > > + struct acpi_pnp_device_id_list *cid_list = NULL; > > int i; > > > > if (!(info->valid & ACPI_VALID_HID)) > > return false; > > > > - for (i = 0; hids[i]; i++) { > > - if (!strcmp(info->hardware_id.string, hids[i])) > > + if (info->valid & ACPI_VALID_CID) > > + cid_list = &info->compatible_id_list; > > + > > + for (i = 0; ids[i]; i++) { > > + int j; > > + > > + if (!strcmp(info->hardware_id.string, ids[i])) > > return true; > > + > > + if (!cid_list) > > + continue; > > + > > + for (j = 0; j < cid_list->count; j++) { > > + if (!strcmp(cid_list->ids[j].string, ids[i])) > > + return true; > > + } > > } > > > > return false; > > } > > > > /* List of HIDs for which we ignore matching ACPI devices, when checking _DEP lists. */ > > -static const char * const acpi_ignore_dep_hids[] = { > > +static const char * const acpi_ignore_dep_ids[] = { > > "INT3396", /* Windows System Power Management Controller */ > > I think this one can be dropped now, I checked my acpidump / dsdt.dsl > collection and 45/45 DSDTs declaring a _HID of INT3396 also added a _CID of > PNP0D80 to this. Sure, in a separate patch. Thanks!
Index: linux-pm/drivers/acpi/scan.c =================================================================== --- linux-pm.orig/drivers/acpi/scan.c +++ linux-pm/drivers/acpi/scan.c @@ -719,25 +719,40 @@ int acpi_device_add(struct acpi_device * /* -------------------------------------------------------------------------- Device Enumeration -------------------------------------------------------------------------- */ -static bool acpi_info_matches_hids(struct acpi_device_info *info, - const char * const hids[]) +static bool acpi_info_matches_ids(struct acpi_device_info *info, + const char * const ids[]) { + struct acpi_pnp_device_id_list *cid_list = NULL; int i; if (!(info->valid & ACPI_VALID_HID)) return false; - for (i = 0; hids[i]; i++) { - if (!strcmp(info->hardware_id.string, hids[i])) + if (info->valid & ACPI_VALID_CID) + cid_list = &info->compatible_id_list; + + for (i = 0; ids[i]; i++) { + int j; + + if (!strcmp(info->hardware_id.string, ids[i])) return true; + + if (!cid_list) + continue; + + for (j = 0; j < cid_list->count; j++) { + if (!strcmp(cid_list->ids[j].string, ids[i])) + return true; + } } return false; } /* List of HIDs for which we ignore matching ACPI devices, when checking _DEP lists. */ -static const char * const acpi_ignore_dep_hids[] = { +static const char * const acpi_ignore_dep_ids[] = { "INT3396", /* Windows System Power Management Controller */ + "PNP0D80", /* Windows-compatible System Power Management Controller */ NULL }; @@ -1857,7 +1872,7 @@ static void acpi_device_dep_initialize(s continue; } - skip = acpi_info_matches_hids(info, acpi_ignore_dep_hids); + skip = acpi_info_matches_ids(info, acpi_ignore_dep_ids); kfree(info); if (skip)