[PATCH v1 0/2] ACPI: scan: Honor certain device identification rules

[PATCH v1 0/2] ACPI: scan: Honor certain device identification rules

[Rafael J. Wysocki]

Hi All,

There are some rules in the ACPI spec regarding which device identification
objects can be used together etc., but they are not followed by the kernel
code.

This series modifies the code to follow the spec more closely (see patch
changelogs for details).

Thanks!

[PATCH v1 1/2] ACPI: scan: Do not add device IDs from _CID if _HID is not valid

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Section 6.1.2 of ACPI 6.4 explicitly requires _HID to be present for
_CID to be defined, so don't add device IDs from _CID to the device
IDs list of a device if _HID is not valid.

Link: https://uefi.org/specs/ACPI/6.4/06_Device_Configuration/Device_Configuration.html#cid-compatible-id
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1338,11 +1338,11 @@ static void acpi_set_pnp_ids(acpi_handle
 		if (info->valid & ACPI_VALID_HID) {
 			acpi_add_id(pnp, info->hardware_id.string);
 			pnp->type.platform_id = 1;
-		}
-		if (info->valid & ACPI_VALID_CID) {
-			cid_list = &info->compatible_id_list;
-			for (i = 0; i < cid_list->count; i++)
-				acpi_add_id(pnp, cid_list->ids[i].string);
+			if (info->valid & ACPI_VALID_CID) {
+				cid_list = &info->compatible_id_list;
+				for (i = 0; i < cid_list->count; i++)
+					acpi_add_id(pnp, cid_list->ids[i].string);
+			}
 		}
 		if (info->valid & ACPI_VALID_ADR) {
 			pnp->bus_address = info->address;

[PATCH v1 2/2] ACPI: scan: Do not set type.bus_address if _HID is valid

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

According to Section 6.1 of ACPI 6.4, it is invalid to provide both
_HID and _ADR for one device at the same time, so modify the code to
set pnp.type.bus_address and pnp.bus_address for a device only if it
has _ADR, but it doesn't have a valid _HID.

Link: https://uefi.org/specs/ACPI/6.4/06_Device_Configuration/Device_Configuration.html#device-identification-objects
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1343,11 +1343,11 @@ static void acpi_set_pnp_ids(acpi_handle
 				for (i = 0; i < cid_list->count; i++)
 					acpi_add_id(pnp, cid_list->ids[i].string);
 			}
-		}
-		if (info->valid & ACPI_VALID_ADR) {
+		} else if (info->valid & ACPI_VALID_ADR) {
 			pnp->bus_address = info->address;
 			pnp->type.bus_address = 1;
 		}
+
 		if (info->valid & ACPI_VALID_UID)
 			pnp->unique_id = kstrdup(info->unique_id.string,
 							GFP_KERNEL);

Re: [PATCH v1 0/2] ACPI: scan: Honor certain device identification rules

[Andy Shevchenko]

On Tue, Oct 26, 2021 at 08:51:49PM +0200, Rafael J. Wysocki wrote:
> Hi All,
> 
> There are some rules in the ACPI spec regarding which device identification
> objects can be used together etc., but they are not followed by the kernel
> code.
> 
> This series modifies the code to follow the spec more closely (see patch
> changelogs for details).

I understand the motivation, but afraid about consequences on the OEM cheap
devices that are not always follow letter of the specification.

As per Intel platforms I would look into Baytrail / Cherrytrail devices for
the past (I think Hans may help here a lot) and into Elkhart Lake in the
present (for the letter I mostly refer to CSRT + DSDT cooperation to get
GP DMA devices enumerated, so I _hope_ DSDT shouldn't have _ADR and _HID
together).

Hence, from the code perspective
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

From the practice I would wait for some tests. I will try to find any new
information about latest firmware tables on Elkhart Lake machines.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/2] ACPI: scan: Honor certain device identification rules

[Andy Shevchenko]

On Tue, Oct 26, 2021 at 10:33:17PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 26, 2021 at 08:51:49PM +0200, Rafael J. Wysocki wrote:
> > Hi All,
> > 
> > There are some rules in the ACPI spec regarding which device identification
> > objects can be used together etc., but they are not followed by the kernel
> > code.
> > 
> > This series modifies the code to follow the spec more closely (see patch
> > changelogs for details).
> 
> I understand the motivation, but afraid about consequences on the OEM cheap
> devices that are not always follow letter of the specification.
> 
> As per Intel platforms I would look into Baytrail / Cherrytrail devices for
> the past (I think Hans may help here a lot) and into Elkhart Lake in the
> present (for the letter I mostly refer to CSRT + DSDT cooperation to get
> GP DMA devices enumerated, so I _hope_ DSDT shouldn't have _ADR and _HID
> together).
> 
> Hence, from the code perspective
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> From the practice I would wait for some tests. I will try to find any new
> information about latest firmware tables on Elkhart Lake machines.

So, what I see in Elkhart Lake

Case 1 - Sound Wire devices (2 times):

    Name (_ADR, 0x40000000)  // _ADR: Address
    Name (_CID, Package (0x02)  // _CID: Compatible ID
    {
        "PRP00001",
        "PNP0A05" /* Generic Container Device */
    })

Case 2 - GP DMA devices (3 times):

    Name (_ADR, 0x001D0003)  // _ADR: Address
    Name (_HID, "80864BB4")  // _HID: Hardware ID

Case 3 - Camera PMIC devices (5 x 2 (CLPn/DSCn) + 1 (PMIC) times = 11x):

    Name (_ADR, Zero)  // _ADR: Address
    Name (_HID, "INT3472")  // _HID: Hardware ID
    Name (_CID, "INT3472")  // _CID: Compatible ID

Case 4 - LNK devices (6 times):

    Name (_ADR, Zero)  // _ADR: Address
    ...

    Name (_UID, One)  // _UID: Unique ID
    Method (_HID, 0, NotSerialized)  // _HID: Hardware ID
    {
        Return (HCID (One))
    }

Case 5 - Camera sensors (2 times):

    Name (_ADR, Zero)  // _ADR: Address
    Name (_HID, "INT34xx")  // _HID: Hardware ID
    Name (_CID, "INT34xx")  // _CID: Compatible ID


I have no idea about cameras or audio devices, but what I'm worrying about
is GP DMA. This kind of devices are PCI, but due to Microsoft hack, called
CSRT, we have to have a possibility to match DSDT with CSRT ot retrieve
the crucial information from the latter while being enumerated by the former.

While it may be against the specification, there is no other way to achieve
that as far as I understand (without either breaking things in Linux or
getting yellow bang in Windows).

Can you confirm that your change won't modify behaviour for these devices?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/2] ACPI: scan: Honor certain device identification rules

[Andy Shevchenko]

On Wed, Oct 27, 2021 at 08:34:49PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 26, 2021 at 10:33:17PM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 26, 2021 at 08:51:49PM +0200, Rafael J. Wysocki wrote:
> > > Hi All,
> > > 
> > > There are some rules in the ACPI spec regarding which device identification
> > > objects can be used together etc., but they are not followed by the kernel
> > > code.
> > > 
> > > This series modifies the code to follow the spec more closely (see patch
> > > changelogs for details).
> > 
> > I understand the motivation, but afraid about consequences on the OEM cheap
> > devices that are not always follow letter of the specification.
> > 
> > As per Intel platforms I would look into Baytrail / Cherrytrail devices for
> > the past (I think Hans may help here a lot) and into Elkhart Lake in the
> > present (for the letter I mostly refer to CSRT + DSDT cooperation to get
> > GP DMA devices enumerated, so I _hope_ DSDT shouldn't have _ADR and _HID
> > together).
> > 
> > Hence, from the code perspective
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > From the practice I would wait for some tests. I will try to find any new
> > information about latest firmware tables on Elkhart Lake machines.
> 
> So, what I see in Elkhart Lake
> 
> Case 1 - Sound Wire devices (2 times):
> 
>     Name (_ADR, 0x40000000)  // _ADR: Address
>     Name (_CID, Package (0x02)  // _CID: Compatible ID
>     {
>         "PRP00001",
>         "PNP0A05" /* Generic Container Device */
>     })
> 
> Case 2 - GP DMA devices (3 times):
> 
>     Name (_ADR, 0x001D0003)  // _ADR: Address
>     Name (_HID, "80864BB4")  // _HID: Hardware ID
> 
> Case 3 - Camera PMIC devices (5 x 2 (CLPn/DSCn) + 1 (PMIC) times = 11x):
> 
>     Name (_ADR, Zero)  // _ADR: Address
>     Name (_HID, "INT3472")  // _HID: Hardware ID
>     Name (_CID, "INT3472")  // _CID: Compatible ID
> 
> Case 4 - LNK devices (6 times):
> 
>     Name (_ADR, Zero)  // _ADR: Address
>     ...
> 
>     Name (_UID, One)  // _UID: Unique ID
>     Method (_HID, 0, NotSerialized)  // _HID: Hardware ID
>     {
>         Return (HCID (One))
>     }
> 
> Case 5 - Camera sensors (2 times):
> 
>     Name (_ADR, Zero)  // _ADR: Address
>     Name (_HID, "INT34xx")  // _HID: Hardware ID
>     Name (_CID, "INT34xx")  // _CID: Compatible ID
> 
> 
> I have no idea about cameras or audio devices, but what I'm worrying about
> is GP DMA. This kind of devices are PCI, but due to Microsoft hack, called
> CSRT, we have to have a possibility to match DSDT with CSRT ot retrieve
> the crucial information from the latter while being enumerated by the former.
> 
> While it may be against the specification, there is no other way to achieve
> that as far as I understand (without either breaking things in Linux or
> getting yellow bang in Windows).
> 
> Can you confirm that your change won't modify behaviour for these devices?

Okay, I have looked into acpi_dma_parse_resource_group() and I don't see that
we actually use _HID there. We definitely use _CRS. However, _HID is used in
case when device is ACPI-enumerated (drivers/dma/dw/platform.c). Seems like
firmware should provide this part runtime (either _HID or _ADR, but not both).


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/2] ACPI: scan: Honor certain device identification rules

[Rafael J. Wysocki]

On Wed, Oct 27, 2021 at 8:11 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Oct 27, 2021 at 08:34:49PM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 26, 2021 at 10:33:17PM +0300, Andy Shevchenko wrote:
> > > On Tue, Oct 26, 2021 at 08:51:49PM +0200, Rafael J. Wysocki wrote:
> > > > Hi All,
> > > >
> > > > There are some rules in the ACPI spec regarding which device identification
> > > > objects can be used together etc., but they are not followed by the kernel
> > > > code.
> > > >
> > > > This series modifies the code to follow the spec more closely (see patch
> > > > changelogs for details).
> > >
> > > I understand the motivation, but afraid about consequences on the OEM cheap
> > > devices that are not always follow letter of the specification.
> > >
> > > As per Intel platforms I would look into Baytrail / Cherrytrail devices for
> > > the past (I think Hans may help here a lot) and into Elkhart Lake in the
> > > present (for the letter I mostly refer to CSRT + DSDT cooperation to get
> > > GP DMA devices enumerated, so I _hope_ DSDT shouldn't have _ADR and _HID
> > > together).
> > >
> > > Hence, from the code perspective
> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > >
> > > From the practice I would wait for some tests. I will try to find any new
> > > information about latest firmware tables on Elkhart Lake machines.
> >
> > So, what I see in Elkhart Lake
> >
> > Case 1 - Sound Wire devices (2 times):
> >
> >     Name (_ADR, 0x40000000)  // _ADR: Address
> >     Name (_CID, Package (0x02)  // _CID: Compatible ID
> >     {
> >         "PRP00001",
> >         "PNP0A05" /* Generic Container Device */
> >     })
> >
> > Case 2 - GP DMA devices (3 times):
> >
> >     Name (_ADR, 0x001D0003)  // _ADR: Address
> >     Name (_HID, "80864BB4")  // _HID: Hardware ID
> >
> > Case 3 - Camera PMIC devices (5 x 2 (CLPn/DSCn) + 1 (PMIC) times = 11x):
> >
> >     Name (_ADR, Zero)  // _ADR: Address
> >     Name (_HID, "INT3472")  // _HID: Hardware ID
> >     Name (_CID, "INT3472")  // _CID: Compatible ID
> >
> > Case 4 - LNK devices (6 times):
> >
> >     Name (_ADR, Zero)  // _ADR: Address
> >     ...
> >
> >     Name (_UID, One)  // _UID: Unique ID
> >     Method (_HID, 0, NotSerialized)  // _HID: Hardware ID
> >     {
> >         Return (HCID (One))
> >     }
> >
> > Case 5 - Camera sensors (2 times):
> >
> >     Name (_ADR, Zero)  // _ADR: Address
> >     Name (_HID, "INT34xx")  // _HID: Hardware ID
> >     Name (_CID, "INT34xx")  // _CID: Compatible ID
> >
> >
> > I have no idea about cameras or audio devices, but what I'm worrying about
> > is GP DMA. This kind of devices are PCI, but due to Microsoft hack, called
> > CSRT, we have to have a possibility to match DSDT with CSRT ot retrieve
> > the crucial information from the latter while being enumerated by the former.
> >
> > While it may be against the specification, there is no other way to achieve
> > that as far as I understand (without either breaking things in Linux or
> > getting yellow bang in Windows).
> >
> > Can you confirm that your change won't modify behaviour for these devices?
>
> Okay, I have looked into acpi_dma_parse_resource_group() and I don't see that
> we actually use _HID there. We definitely use _CRS. However, _HID is used in
> case when device is ACPI-enumerated (drivers/dma/dw/platform.c). Seems like
> firmware should provide this part runtime (either _HID or _ADR, but not both).

Right.

So after patch [2/2] _HID will always be used for the enumeration in
such cases which may not be what happens now - and what happens now
depends on the ordering in which the objects in question are seen
during the namespace walk.  If the DMA device is seen before the PCI
host bridge, it will be enumerated using the _HID.

Re: [PATCH v1 0/2] ACPI: scan: Honor certain device identification rules

[Rafael J. Wysocki]

On Wed, Oct 27, 2021 at 7:35 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Oct 26, 2021 at 10:33:17PM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 26, 2021 at 08:51:49PM +0200, Rafael J. Wysocki wrote:
> > > Hi All,
> > >
> > > There are some rules in the ACPI spec regarding which device identification
> > > objects can be used together etc., but they are not followed by the kernel
> > > code.
> > >
> > > This series modifies the code to follow the spec more closely (see patch
> > > changelogs for details).
> >
> > I understand the motivation, but afraid about consequences on the OEM cheap
> > devices that are not always follow letter of the specification.
> >
> > As per Intel platforms I would look into Baytrail / Cherrytrail devices for
> > the past (I think Hans may help here a lot) and into Elkhart Lake in the
> > present (for the letter I mostly refer to CSRT + DSDT cooperation to get
> > GP DMA devices enumerated, so I _hope_ DSDT shouldn't have _ADR and _HID
> > together).
> >
> > Hence, from the code perspective
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > From the practice I would wait for some tests. I will try to find any new
> > information about latest firmware tables on Elkhart Lake machines.
>
> So, what I see in Elkhart Lake
>
> Case 1 - Sound Wire devices (2 times):
>
>     Name (_ADR, 0x40000000)  // _ADR: Address

No _HID, so the IDs returned by the _CID below won't be used.

>     Name (_CID, Package (0x02)  // _CID: Compatible ID
>     {
>         "PRP00001",

The above device ID is invalid (one 0 too many).

>         "PNP0A05" /* Generic Container Device */

Without the change this causes a container device to be created, but
the only purpose of it may be offline/online (if the child devices
support offline/online).

This change should not be functionally relevant.

>     })
>
> Case 2 - GP DMA devices (3 times):
>
>     Name (_ADR, 0x001D0003)  // _ADR: Address

_ADR will be ignored which may not be expected.  Is this a PCI device?

>     Name (_HID, "80864BB4")  // _HID: Hardware ID
>
> Case 3 - Camera PMIC devices (5 x 2 (CLPn/DSCn) + 1 (PMIC) times = 11x):
>
>     Name (_ADR, Zero)  // _ADR: Address

_ADR will be ignored, which shouldn't matter.

>     Name (_HID, "INT3472")  // _HID: Hardware ID
>     Name (_CID, "INT3472")  // _CID: Compatible ID
>
> Case 4 - LNK devices (6 times):
>
>     Name (_ADR, Zero)  // _ADR: Address

Same here.

>     ...
>
>     Name (_UID, One)  // _UID: Unique ID
>     Method (_HID, 0, NotSerialized)  // _HID: Hardware ID
>     {
>         Return (HCID (One))
>     }
>
> Case 5 - Camera sensors (2 times):
>
>     Name (_ADR, Zero)  // _ADR: Address

And same here.

>     Name (_HID, "INT34xx")  // _HID: Hardware ID
>     Name (_CID, "INT34xx")  // _CID: Compatible ID
>
>
> I have no idea about cameras or audio devices, but what I'm worrying about
> is GP DMA. This kind of devices are PCI, but due to Microsoft hack, called
> CSRT, we have to have a possibility to match DSDT with CSRT ot retrieve
> the crucial information from the latter while being enumerated by the former.
>
> While it may be against the specification, there is no other way to achieve
> that as far as I understand (without either breaking things in Linux or
> getting yellow bang in Windows).

I'm not really sure why _HID is needed for this.  The PCI device ID
could be used for CRST matching just fine.

> Can you confirm that your change won't modify behaviour for these devices?

Well, the GP DMA thing may be broken by patch [2/2], but does Windows
actually use _ADR if _HID is provided?

Re: [PATCH v1 0/2] ACPI: scan: Honor certain device identification rules

[Andy Shevchenko]

On Wed, Oct 27, 2021 at 08:12:20PM +0200, Rafael J. Wysocki wrote:
> On Wed, Oct 27, 2021 at 7:35 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Oct 26, 2021 at 10:33:17PM +0300, Andy Shevchenko wrote:
> > > On Tue, Oct 26, 2021 at 08:51:49PM +0200, Rafael J. Wysocki wrote:

...

> > > From the practice I would wait for some tests. I will try to find any new
> > > information about latest firmware tables on Elkhart Lake machines.
> >
> > So, what I see in Elkhart Lake
> >
> > Case 1 - Sound Wire devices (2 times):
> >
> >     Name (_ADR, 0x40000000)  // _ADR: Address
> 
> No _HID, so the IDs returned by the _CID below won't be used.
> 
> >     Name (_CID, Package (0x02)  // _CID: Compatible ID
> >     {
> >         "PRP00001",
> 
> The above device ID is invalid (one 0 too many).

Probably we have to communicate this to EHL program owners internally...
I dunno what this means in case of Sound Wire.

> >         "PNP0A05" /* Generic Container Device */
> 
> Without the change this causes a container device to be created, but
> the only purpose of it may be offline/online (if the child devices
> support offline/online).
> 
> This change should not be functionally relevant.
> 
> >     })
> >
> > Case 2 - GP DMA devices (3 times):
> >
> >     Name (_ADR, 0x001D0003)  // _ADR: Address
> 
> _ADR will be ignored which may not be expected.  Is this a PCI device?

It depends on the BIOS decision at boot time. No idea if it's only one
possibility (what I have heard is that device is PCI enumerated, that's
why they chose PCI ID in the CSRT, to avoid allocating new IDs for truly
ACPI-enumerated device).

But seems another point to discuss internally.

> >     Name (_HID, "80864BB4")  // _HID: Hardware ID
> >
> > Case 3 - Camera PMIC devices (5 x 2 (CLPn/DSCn) + 1 (PMIC) times = 11x):
> >
> >     Name (_ADR, Zero)  // _ADR: Address
> 
> _ADR will be ignored, which shouldn't matter.
> 
> >     Name (_HID, "INT3472")  // _HID: Hardware ID
> >     Name (_CID, "INT3472")  // _CID: Compatible ID
> >
> > Case 4 - LNK devices (6 times):
> >
> >     Name (_ADR, Zero)  // _ADR: Address
> 
> Same here.
> 
> >     ...
> >
> >     Name (_UID, One)  // _UID: Unique ID
> >     Method (_HID, 0, NotSerialized)  // _HID: Hardware ID
> >     {
> >         Return (HCID (One))
> >     }
> >
> > Case 5 - Camera sensors (2 times):
> >
> >     Name (_ADR, Zero)  // _ADR: Address
> 
> And same here.
> 
> >     Name (_HID, "INT34xx")  // _HID: Hardware ID
> >     Name (_CID, "INT34xx")  // _CID: Compatible ID
> >
> > I have no idea about cameras or audio devices, but what I'm worrying about
> > is GP DMA. This kind of devices are PCI, but due to Microsoft hack, called
> > CSRT, we have to have a possibility to match DSDT with CSRT ot retrieve
> > the crucial information from the latter while being enumerated by the former.
> >
> > While it may be against the specification, there is no other way to achieve
> > that as far as I understand (without either breaking things in Linux or
> > getting yellow bang in Windows).
> 
> I'm not really sure why _HID is needed for this.  The PCI device ID
> could be used for CRST matching just fine.
> 
> > Can you confirm that your change won't modify behaviour for these devices?
> 
> Well, the GP DMA thing may be broken by patch [2/2], but does Windows
> actually use _ADR if _HID is provided?

No idea. Let's discuss internally.

P.S. The issue here is that some BIOS versions are floating around and
we never know who is using what... :-(

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/2] ACPI: scan: Honor certain device identification rules

[Hans de Goede]

Hi,

On 10/26/21 20:51, Rafael J. Wysocki wrote:
> Hi All,
> 
> There are some rules in the ACPI spec regarding which device identification
> objects can be used together etc., but they are not followed by the kernel
> code.
> 
> This series modifies the code to follow the spec more closely (see patch
> changelogs for details).

Both changes seem sensible to me; and since you make _HID take precedence
over _ADR I don't expect this to cause any regressions on BYT / CHT
hardware (the other way around will likely be an issue).

So for the series:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans