| Message ID | 11860508.O9o76ZdvQC@kreacher (mailing list archive) |
|---|---|
| Headers | show |
| Series | ACPI: scan: Honor certain device identification rules | expand |
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.
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
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?
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).
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?
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.
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... :-(