On Thu, Dec 10, 2020 at 1:06 AM Daniel Scally <djrscally@xxxxxxxxx> wrote: > > On 09/12/2020 16:53, Rafael J. Wysocki wrote: > > On Wed, Dec 9, 2020 at 5:20 PM Daniel Scally <djrscally@xxxxxxxxx> wrote: > >> Hi Rafael > >> > >> On 09/12/2020 15:43, Rafael J. Wysocki wrote: > >>> On Wed, Dec 9, 2020 at 10:55 AM Daniel Scally <djrscally@xxxxxxxxx> wrote: > >>>> > >>>> On 08/12/2020 23:48, Daniel Scally wrote: > >>>>> Hello again > >>>>> > >>>>> On 06/12/2020 00:00, Daniel Scally wrote: > >>>>>> INT3472:08 is not an acpi device that seems to be a good candidate for > >>>>>> binding to 0000:00:00.0; it just happens to be the first child of > >>>>>> PNP0A08:08 that shares _ADR 0 and has _STA not set to 0. > >>>>>> > >>>>>> The comment within acpi_find_child_device() does imply that there should > >>>>>> only ever be a single child device with the same _ADR as the parent, so > >>>>>> I suppose this is possibly a case of poor ACPI tables confusing the code > >>>>>> a bit; given both PNP0A08:00 and _all_ of the INT3472 devices have _ADR > >>>>>> set to zero (as indeed do the machine's cameras), but I'm not > >>>>>> knowledgeable enough on ACPI to know whether that's to spec (or at least > >>>>>> accounted for). The INT3472 devices themselves do not actually seem to > >>>>>> represent a physical device (atleast, not in this case...sometimes they > >>>>>> do...), rather they're a dummy being used to simply group some GPIO > >>>>>> lines under a common _CRS. The sensors are called out as dependent on > >>>>>> these "devices" in their _DEP method, which is already a horrible way of > >>>>>> doing things so more broken ACPI being to blame wouldn't surprise me. > >>>>>> > >>>>>> The other problem that that raises is that there seems to be _no_ good > >>>>>> candidate for binding to 0000:00:00.0 that's a child of PNP0A08:00 - the > >>>>>> only devices sharing _ADR 0 and having _STA != 0 are those two INT3472 > >>>>>> entries and the machine's cameras. > >>>>> After some more reading, I'm pretty confident that this is the problem > >>>>> now - I.E. that those devices having _ADR of 0 is what's causing this > >>>>> issue to materialise, and that those values should be set to something > >>>>> more appropriate. Still unsure about the best approach to fix it though > >>>>> from a kernel point of view; there doesn't seem to be anything out of > >>>>> whack in the logic, and I believe (correct me if I'm wrong) there can be > >>>>> legitimate instances of child devices sharing _ADR=0 with the parent, so > >>>>> the problem becomes how to identify the illegitimate instances so that > >>>>> they can be discarded. My experience in this is really limited, so I > >>>>> lean towards the conclusion that hard-coding exceptions somewhere might > >>>>> be necessary to handle this without resorting to patched ACPI tables. > >>>>> Whether that's within acpi_find_child_device() to prevent matching > >>>>> occurring there, or else setting the adev->pnp.bus_address to some > >>>>> alternate value after creation to compensate. > >>>>> > >>>>> I recognise that that's a horrible answer though, so I'm really hoping > >>>>> that someone has an idea for how to handle this in a better way. > >>>> Oops, missed this crucial line from the spec: > >>>> > >>>> "A device object must contain either an _HID object or an _ADR object, > >>>> but should not contain both." > >>>> > >>>> And here's the Device declaration for these objects: > >>>> > >>>> Device (PMI0) > >>>> { > >>>> Name (_ADR, Zero) // _ADR: Address > >>>> Name (_HID, "INT3472") // _HID: Hardware ID > >>>> Name (_CID, "INT3472") // _CID: Compatible ID > >>>> Name (_DDN, "INCL-CRDD") // _DDN: DOS Device Name > >>>> Name (_UID, Zero) // _UID: Unique ID > >>>> > >>>> So that's the broken part rather than the _ADR value of 0 specifically. > >>>> That at least gives a jumping off point for some logic to fix rather > >>>> than a hardcoded anything, so I'll try to work out a nice way to handle > >>>> that (probably ignoring adevs in acpi_find_child_device() with addr=0 > >>>> and a valid _HID) and submit a patch. > >>> Please see the comment in find_child_checks(), though - it kind of > >>> tries to handle this case already. > >> It down-weights them currently yes, but does still allow them to match. > >> I think it makes more sense to not allow a match at all, at least in the > >> situation I've encountered, but I suppose the implication of the logic > >> in this check is that at some point we've encountered ACPI entries with > >> both _HID and _ADR that were potentially correct matches, which kinda > >> re-complicates things again. > > That's correct. > OK, that definitely makes it harder then. Sort of clutching at straws > here; is _ADR=0 a special case in any way? As far as I can tell it's > only a problem on my devices for that address but that could easily be > coincidence. > >>> I guess what happens is that _STA is not present under the device that > >>> is expected to be matched, so maybe the logic regarding this may be > >>> changed somewhat. > >> Hmm yeah I guess so, so this is kinda a combination of two problems > >> probably. And if the actual device that is expected to match had a _STA > >>> 0 then presumably the down-weighting of devices with a _HID in > >> find_child_checks() would ensure the correct dev was matched. > > That's the intended outcome. > > > > We may need another value (between the min and the max) to return when > > adev->pnp.type.platform_id is not set and _STA is not present. > > > Unfortunately this turns out not to be the problem in this case; on > checking for _STA too, all the potential devices except the 2 cameras > and their dependee PMICs have a _STA present but set 0, Which means that they shouldn't be used. > so find_child_checks() throws -ENODEV; and downweights them below the devs > that shouldn't match. OK, so we want acpi_find_child_device() to return NULL in this case. What about making it return NULL if there is a matching device with _ADR and without _HID that is unusable (ie. _STA == 0)?