On Thu, Dec 10, 2020 at 11:46 PM Daniel Scally <djrscally@xxxxxxxxx> wrote:
>
>
> On 10/12/2020 16:59, Rafael J. Wysocki wrote:
> > On Thursday, December 10, 2020 5:07:56 PM CET Daniel Scally wrote:
> >> On 10/12/2020 16:05, Rafael J. Wysocki wrote:
> >>> All the adevs with matching _ADR also have both _STA and _HID
> >>> unfortunately. Sorry; let me stop half-arsing this and show you
> >>> something useful:
> >>>
> >>> [ 0.219953] acpi_find_child_device(PNP0A08:00, 0x00, false)
> >>> [ 0.220818] INT3472:00: _STA 0x00, _ADR=0x00000000, _HID=INT3472
> >>> [ 0.220821] INT3472:01: _STA 0x00, _ADR=0x00000000, _HID=INT3472
> >>> [ 0.220870] INT3472:02: _STA 0x00, _ADR=0x00000000, _HID=INT3472
> >>> [ 0.220892] INT3472:03: _STA 0x00, _ADR=0x00000000, _HID=INT3472
> >>> [ 0.220916] INT3472:04: _STA 0x00, _ADR=0x00000000, _HID=INT3472
> >>> [ 0.220941] INT3472:05: _STA 0x00, _ADR=0x00000000, _HID=INT3472
> >>> [ 0.220965] INT3472:06: _STA 0x00, _ADR=0x00000000, _HID=INT3472
> >>> [ 0.220990] INT3472:07: _STA 0x00, _ADR=0x00000000, _HID=INT3472
> >>> These will be ignored with -ENODEV.
> >>>
> >>>> [ 0.221038] INT3472:08: _STA 0x0f, _ADR=0x00000000, _HID=INT3472
> >>> For this acpi_find_child_device() will return FIND_CHILD_MIN_SCORE if
> >>> I'm not mistaken.
> >> It does - this is the one that binds, being the first.
> >>>> [ 0.221051] OVTI5648:00: _STA 0x0f, _ADR=0x00000000, _HID=OVTI5648
> >>>> [ 0.221061] INT3472:09: _STA 0x0f, _ADR=0x00000000, _HID=INT3472
> >>>> [ 0.221070] OVTI2680:00: _STA 0x0f, _ADR=0x00000000, _HID=OVTI2680
> >>> As well as for the three above.
> >>>
> >>>> [ 0.221079] INT3471:00: _STA 0x00, _ADR=0x00000000, _HID=INT3471
> >>>> [ 0.221105] INT33BE:00: _STA 0x00, _ADR=0x00000000, _HID=INT33BE
> >>>> [ 0.221130] INT3471:01: _STA 0x00, _ADR=0x00000000, _HID=INT3471
> >>>> [ 0.221156] INT33BE:01: _STA 0x00, _ADR=0x00000000, _HID=INT33BE
> >>> And the rest will be ignored.
> >>>
> >>>> That's the debug output I included for each adev that's assessed as a
> >>>> child of PNP0A08:00. _STA, _ADR and _HID present for all, _ADR 0x00 for
> >>>> all, _STA 0x0f for the 2 sensors and their PMIC's and 0x00 for the rest.
> >>>> The same situation holds on both of my devices.
> >>> So in fact we don't want to have an ACPI companion for (PNP0A08:00,
> >>> 0x00, false).
> >> Yeah, I think that's right
> >>> This is a hostbridge special case and let me think about this for a while.
> >> Sure - thanks very much for your help.
> > I've come up with the following patch.
> >
> > Please let me know if it works for you.
> >
> > ---
> > Index: linux-pm/drivers/pci/pci-acpi.c
> > ===================================================================
> > --- linux-pm.orig/drivers/pci/pci-acpi.c
> > +++ linux-pm/drivers/pci/pci-acpi.c
> > @@ -1162,14 +1162,32 @@ void acpi_pci_remove_bus(struct pci_bus
> > static struct acpi_device *acpi_pci_find_companion(struct device *dev)
> > {
> > struct pci_dev *pci_dev = to_pci_dev(dev);
> > + struct acpi_device *adev;
> > bool check_children;
> > u64 addr;
> >
> > check_children = pci_is_bridge(pci_dev);
> > /* Please ref to ACPI spec for the syntax of _ADR */
> > addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn);
> > - return acpi_find_child_device(ACPI_COMPANION(dev->parent), addr,
> > + adev = acpi_find_child_device(ACPI_COMPANION(dev->parent), addr,
> > check_children);
> > + /*
> > + * There may be ACPI device objects in the ACPI namesoace that are
> > + * children of the device object representing the host bridge, but don't
> > + * represent PCI devices. Both _HID and _ADR may be present for them,
> > + * even though that is against the specification (for example, see
> > + * Section 6.1 of ACPI 6.3), but in many cases the _ADR returns 0 which
> > + * appears to indicate that they should not be taken into consideration
> > + * as potential companions of PCI devices on the root bus.
> > + *
> > + * To catch this special case, disregard the returned device object if
> > + * it has a valid _HID, addr is 0 and the PCI device at hand is on the
> > + * root bus.
> > + */
> > + if (adev->pnp.type.platform_id && !addr && !pci_dev->bus->parent)
> > + return NULL;
> > +
> > + return adev;
> > }
> >
> > /**
> >
> Thanks - this needs to check adev for NULL too; acpi_find_child_device()
> does return that sometimes.
Yes, it does, sorry for the mistake.
> When changed to:
>
> + if (adev && adev->pnp.type.platform_id && !addr && !pci_dev->bus->parent)
> + return NULL;
>
> Then it boots properly, and fixes the original problem. Also;
> s/namesoace/namespace in the comment.
Right.
> Really appreciate the help - thank you!
YW
Let me fix the patch and post it "officially".
