acpi_device_notify() binding devices that don't seem like they should be bound

acpi_device_notify() binding devices that don't seem like they should be bound

[Daniel Scally]

Hi Len, Rafael

I'm having some trouble with how the kernel is treating some ACPI
devices, and I'm hoping for some help.

For reference, the DSDT tables for everything I'm talking about below
can be found here. The device in question is a Lenovo Miix 510:

https://gist.githubusercontent.com/djrscally/e64d112180517352fa3392878b0f4a7d/raw/88b90b3ea4204fd7845257b6666fdade47cc2981/dsdt.dsl

So the problem goes like this; we have a bunch of devices with _HID
"INT3472" in those tables. Most of them have _STA=0, but 2 have _STA=15.
I expect both of these devices (INT3472:08 and INT3472:09) to be created
as platform_devices as well as acpi_devices by the kernel, however it
turns out that only INT3472:09 gets a platform_device, :08 doesn't.

The reason :08 gets no platform_device is that by the time
acpi_create_platform_device() is ran for it, it's already gotten a
physical_node. This comes from acpi_bind_one() binding the INT3472:08
acpi_device to a pci device named 0000:00:00.0 during
acpi_device_notify(). The flow goes like this:

acpi_device_notify(0000:00:00.0)
 -> acpi_pci_find_companion(0000:00:00.0)
     -> acpi_find_child_device(PNP0A08:00, 0, false)
	 -> return INT3472:08

The jump to PNP0A08:00 there is acpi_find_companion() fetching the
ACPI_COMPANION() for 0000:00:00.0's _parent_ device, named pci0000:00.
This isn't behaviour that strikes me as particularly desirable.
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.

Any advice on how to go about rectifying this would be very much
appreciated.

Thanks
Dan

Re: acpi_device_notify() binding devices that don't seem like they should be bound

[Daniel Scally]

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.

Re: acpi_device_notify() binding devices that don't seem like they should be bound

[Daniel Scally]

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.

Sorry for the noise, think I'm good now :)

Re: acpi_device_notify() binding devices that don't seem like they should be bound

[Rafael J. Wysocki]

On Wed, Dec 9, 2020 at 10:55 AM Daniel Scally <djrscally@gmail.com> 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.

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.

> Sorry for the noise, think I'm good now :)

OK

Re: acpi_device_notify() binding devices that don't seem like they should be bound

[Daniel Scally]

Hi Rafael

On 09/12/2020 15:43, Rafael J. Wysocki wrote:
> On Wed, Dec 9, 2020 at 10:55 AM Daniel Scally <djrscally@gmail.com> 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.
>
> 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.

>> Sorry for the noise, think I'm good now :)
> OK

Re: acpi_device_notify() binding devices that don't seem like they should be bound

[Rafael J. Wysocki]

On Wed, Dec 9, 2020 at 5:20 PM Daniel Scally <djrscally@gmail.com> 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@gmail.com> 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.

> > 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.

Re: acpi_device_notify() binding devices that don't seem like they should be bound

[Daniel Scally]

On 09/12/2020 16:53, Rafael J. Wysocki wrote:
> On Wed, Dec 9, 2020 at 5:20 PM Daniel Scally <djrscally@gmail.com> 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@gmail.com> 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, so
find_child_checks() throws -ENODEV; and downweights them below the devs
that shouldn't match.

Re: acpi_device_notify() binding devices that don't seem like they should be bound

[Rafael J. Wysocki]

On Thu, Dec 10, 2020 at 1:06 AM Daniel Scally <djrscally@gmail.com> wrote:
>
> On 09/12/2020 16:53, Rafael J. Wysocki wrote:
> > On Wed, Dec 9, 2020 at 5:20 PM Daniel Scally <djrscally@gmail.com> 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@gmail.com> 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)?

Re: acpi_device_notify() binding devices that don't seem like they should be bound

[Daniel Scally]

On 10/12/2020 13:53, Rafael J. Wysocki wrote:
> On Thu, Dec 10, 2020 at 1:06 AM Daniel Scally <djrscally@gmail.com> wrote:
>> On 09/12/2020 16:53, Rafael J. Wysocki wrote:
>>> On Wed, Dec 9, 2020 at 5:20 PM Daniel Scally <djrscally@gmail.com> 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@gmail.com> 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)?

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
[    0.221038] INT3472:08: _STA 0x0f, _ADR=0x00000000, _HID=INT3472
[    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
[    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


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.

Re: acpi_device_notify() binding devices that don't seem like they should be bound

[Rafael J. Wysocki]

On Thu, Dec 10, 2020 at 4:02 PM Daniel Scally <djrscally@gmail.com> wrote:
>
>
> On 10/12/2020 13:53, Rafael J. Wysocki wrote:
> > On Thu, Dec 10, 2020 at 1:06 AM Daniel Scally <djrscally@gmail.com> wrote:
> >> On 09/12/2020 16:53, Rafael J. Wysocki wrote:
> >>> On Wed, Dec 9, 2020 at 5:20 PM Daniel Scally <djrscally@gmail.com> 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@gmail.com> 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)?
>
> 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.

> [    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).

This is a hostbridge special case and let me think about this for a while.

Re: acpi_device_notify() binding devices that don't seem like they should be bound

[Daniel Scally]

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.

Re: acpi_device_notify() binding devices that don't seem like they should be bound

[Rafael J. Wysocki]

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;
 }
 
 /**

Re: acpi_device_notify() binding devices that don't seem like they should be bound

[Daniel Scally]

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. 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.


Really appreciate the help - thank you!

Re: acpi_device_notify() binding devices that don't seem like they should be bound

[Rafael J. Wysocki]

On Thu, Dec 10, 2020 at 11:46 PM Daniel Scally <djrscally@gmail.com> 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".