On Fri, Feb 16, 2024 at 9:20 PM Mateusz Jończyk <mat.jonczyk@xxxxx> wrote: > > W dniu 16.02.2024 o 19:49, Bjorn Helgaas pisze: > > On Fri, Feb 16, 2024 at 07:26:06PM +0100, Rafael J. Wysocki wrote: > >> On Tue, Dec 26, 2023 at 1:50 PM Mateusz Jończyk <mat.jonczyk@xxxxx> wrote: > >>> On some platforms, the ACPI _PRT function returns duplicate interrupt > >>> routing entries. Linux uses the first matching entry, but sometimes the > >>> second matching entry contains the correct interrupt vector. > >>> > >>> As a debugging aid, print a warning to dmesg if duplicate interrupt > >>> routing entries are present. This way, we could check how many models > >>> are affected. > >>> > >>> This happens on a Dell Latitude E6500 laptop with the i2c-i801 Intel > >>> SMBus controller. This controller is nonfunctional unless its interrupt > >>> usage is disabled (using the "disable_features=0x10" module parameter). > >>> > >>> After investigation, it turned out that the driver was using an > >>> incorrect interrupt vector: in lspci output for this device there was: > >>> Interrupt: pin B routed to IRQ 19 > >>> but after running i2cdetect (without using any i2c-i801 module > >>> parameters) the following was logged to dmesg: > >>> > >>> [...] > >>> i801_smbus 0000:00:1f.3: Timeout waiting for interrupt! > >>> i801_smbus 0000:00:1f.3: Transaction timeout > >>> i801_smbus 0000:00:1f.3: Timeout waiting for interrupt! > >>> i801_smbus 0000:00:1f.3: Transaction timeout > >>> irq 17: nobody cared (try booting with the "irqpoll" option) > >>> > >>> Existence of duplicate entries in a table returned by the _PRT method > >>> was confirmed by disassembling the ACPI DSDT table. > >>> > >>> Windows XP is using IRQ3 (as reported by HWiNFO32 and in the Device > >>> Manager), which is neither of the two vectors returned by _PRT. > >>> As HWiNFO32 decoded contents of the SPD EEPROMs, the i2c-i801 device is > >>> working under Windows. It appears that Windows has reconfigured the > >>> chipset independently to use another interrupt vector for the device. > >>> This is possible, according to the chipset datasheet [1], page 436 for > >>> example (PIRQ[n]_ROUT—PIRQ[A,B,C,D] Routing Control Register). > >>> > >>> [1] https://www.intel.com/content/dam/doc/datasheet/io-controller-hub-9-datasheet.pdf > >>> > >>> Signed-off-by: Mateusz Jończyk <mat.jonczyk@xxxxx> > >>> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> > >>> Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx> > >>> Cc: Len Brown <lenb@xxxxxxxxxx> > >>> Cc: Borislav Petkov <bp@xxxxxxx> > >>> Cc: Jean Delvare <jdelvare@xxxxxxx> > >>> Previously-reviewed-by: Jean Delvare <jdelvare@xxxxxxx> > >>> Previously-tested-by: Jean Delvare <jdelvare@xxxxxxx> > >>> > >>> --- > >>> Hello, > >>> > >>> I'm resurrecting an older patch that was discussed back in January: > >>> > >>> https://lore.kernel.org/lkml/20230121153314.6109-1-mat.jonczyk@xxxxx/T/#u > >>> > >>> To consider: should we print a warning or an error in case of duplicate > >>> entries? This may not be serious enough to disturb the user with an > >>> error message at boot. > >>> > >>> I'm also looking into modifying the i2c-i801 driver to disable its usage > >>> of interrupts if one did not fire. > >>> > >>> v2: - add a newline at the end of the kernel log message, > >>> - replace: "if (match == NULL)" -> "if (!match)" > >>> - patch description tweaks. > >>> v3: - fix C style issues pointed by Jean Delvare, > >>> - switch severity from warning to error. > >>> v3 RESEND: retested on top of v6.2-rc4 > >>> v4: - rebase and retest on top of v6.7-rc7 > >>> - switch severity back to warning, > >>> - change pr_err() to dev_warn() and simplify the code, > >>> - modify patch description (describe Windows behaviour etc.) > >>> --- > >>> drivers/acpi/pci_irq.c | 25 ++++++++++++++++++++++--- > >>> 1 file changed, 22 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c > >>> index ff30ceca2203..1fcf72e335b0 100644 > >>> --- a/drivers/acpi/pci_irq.c > >>> +++ b/drivers/acpi/pci_irq.c > >>> @@ -203,6 +203,8 @@ static int acpi_pci_irq_find_prt_entry(struct pci_dev *dev, > >>> struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > >>> struct acpi_pci_routing_table *entry; > >>> acpi_handle handle = NULL; > >>> + struct acpi_prt_entry *match = NULL; > >>> + const char *match_int_source = NULL; > >>> > >>> if (dev->bus->bridge) > >>> handle = ACPI_HANDLE(dev->bus->bridge); > >>> @@ -219,13 +221,30 @@ static int acpi_pci_irq_find_prt_entry(struct pci_dev *dev, > >>> > >>> entry = buffer.pointer; > >>> while (entry && (entry->length > 0)) { > >>> - if (!acpi_pci_irq_check_entry(handle, dev, pin, > >>> - entry, entry_ptr)) > >>> - break; > >>> + struct acpi_prt_entry *curr; > >>> + > >>> + if (!acpi_pci_irq_check_entry(handle, dev, pin, entry, &curr)) { > >>> + if (!match) { > >>> + match = curr; > >>> + match_int_source = entry->source; > >>> + } else { > >>> + dev_warn(&dev->dev, FW_BUG > >> dev_info() would be sufficient here IMV. > >> > >>> + "ACPI _PRT returned duplicate IRQ routing entries for INT%c: %s[%d] and %s[%d]\n", > >>> + pin_name(curr->pin), > >>> + match_int_source, match->index, > >>> + entry->source, curr->index); > >>> + /* We use the first matching entry nonetheless, > >>> + * for compatibility with older kernels. > > The usual comment style in this file is: > > > > /* > > * We use ... > > */ > > > >>> + */ > >>> + } > >>> + } > >>> + > >>> entry = (struct acpi_pci_routing_table *) > >>> ((unsigned long)entry + entry->length); > >>> } > >>> > >>> + *entry_ptr = match; > >>> + > >>> kfree(buffer.pointer); > >>> return 0; > >>> } > >>> > >>> base-commit: 861deac3b092f37b2c5e6871732f3e11486f7082 > >>> -- > >> Bjorn, any concerns regarding this one? > > No concerns from me. > > > > I guess this only adds a message, right? It doesn't actually fix > > anything or change any behavior? > Exactly. > > This talks about "duplicate" entries, which suggests to me that they > > are identical, but I don't think they are. It sounds like it's two > > "matching" entries, i.e., two entries for the same (device, pin)? > > Right. > > > And neither of the two _PRT entries yields a working i801 device? > > Unpatched Linux uses the first matching entry, but the second one gives > a working i801 device. The point is to print a warning message to see > how many devices are affected and whether it is safe to switch the code > to use the last matching entry in all instances. > > Therefore I used dev_warn(). I don't quite see a connection between the above and the log level.
