[Intel-gfx] [PATCH] ACPI / bus: Introduce a list of ids for "always present" devices

[Rafael J. Wysocki]

On Mon, Feb 27, 2017 at 10:58 PM, Hans de Goede <hdegoede at redhat.com> wrote:
> Hi,
>
>
> On 27-02-17 22:49, Rafael J. Wysocki wrote:
>>
>> On Mon, Feb 27, 2017 at 10:29 PM, Hans de Goede <hdegoede at redhat.com>
>> wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 27-02-17 22:25, Rafael J. Wysocki wrote:
>>>>
>>>>
>>>> On Mon, Feb 27, 2017 at 3:25 PM, Hans de Goede <hdegoede at redhat.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>>
>>>>> On 27-02-17 14:30, Rafael J. Wysocki wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> +Mika & Andy
>>>>>>
>>>>>> On Saturday, February 25, 2017 07:23:28 PM Hans de Goede wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Several cherrytrail devices (all of which ship with windows 10) hide
>>>>>>> the
>>>>>>> lpss pwm controller in ACPI, typically the _STA method looks like
>>>>>>> this:
>>>>>>>
>>>>>>>     Method (_STA, 0, NotSerialized)  // _STA: Status
>>>>>>>     {
>>>>>>>         If (OSID == One)
>>>>>>>         {
>>>>>>>             Return (Zero)
>>>>>>>         }
>>>>>>>
>>>>>>>         Return (0x0F)
>>>>>>>     }
>>>>>>>
>>>>>>> Where OSID is some dark magic seen in all cherrytrail ACPI tables
>>>>>>> making
>>>>>>> the machine behave differently depending on which OS it *thinks* it
>>>>>>> is
>>>>>>> booting, this gets set in a number of ways which we cannot control,
>>>>>>> on
>>>>>>> some newer machines it simple hardcoded to "One" aka win10.
>>>>>>>
>>>>>>> This causes the PWM controller to get hidden, which means Linux
>>>>>>> cannot
>>>>>>> control the backlight level on cht based tablets / laptops.
>>>>>>>
>>>>>>> Since loading the driver for this does no harm (the only in kernel
>>>>>>> user
>>>>>>> of it is the i915 driver, which will only use it when it needs it),
>>>>>>> this
>>>>>>> commit makes acpi_bus_get_status() always set status to
>>>>>>> ACPI_STA_DEFAULT
>>>>>>> for the 80862288 device, fixing the lack of backlight control.
>>>>>>>
>>>>>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>>>>>> ---
>>>>>>>  drivers/acpi/bus.c | 25 +++++++++++++++++++++++++
>>>>>>>  1 file changed, 25 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>>>>>>> index 95855cb..483d4d0 100644
>>>>>>> --- a/drivers/acpi/bus.c
>>>>>>> +++ b/drivers/acpi/bus.c
>>>>>>> @@ -109,11 +109,36 @@ acpi_status
>>>>>>> acpi_bus_get_status_handle(acpi_handle
>>>>>>> handle,
>>>>>>>         return status;
>>>>>>>  }
>>>>>>>
>>>>>>> +/*
>>>>>>> + * Some ACPI devices are hidden (status == 0x0) in recent BIOS-es
>>>>>>> because
>>>>>>> + * some recent windows drivers bind to one device but poke at
>>>>>>> multiple
>>>>>>> + * devices at the same time, so the others get hidden.
>>>>>>> + * We work around this by always reporting ACPI_STA_DEFAULT for
>>>>>>> these
>>>>>>> + * devices. Note this MUST only be done for devices where this is
>>>>>>> safe.
>>>>>>> + */
>>>>>>> +static const struct acpi_device_id always_present_device_ids[] = {
>>>>>>> +       /*
>>>>>>> +        * Cherrytrail pwm directly poked by GPU driver in win10,
>>>>>>> +        * but Linux uses a separate pwm driver, harmless if not
>>>>>>> used.
>>>>>>> +        */
>>>>>>> +       { "80862288", },
>>>>>>> +       { }
>>>>>>> +};
>>>>>>> +
>>>>>>>  int acpi_bus_get_status(struct acpi_device *device)
>>>>>>>  {
>>>>>>>         acpi_status status;
>>>>>>>         unsigned long long sta;
>>>>>>>
>>>>>>> +       /* acpi_match_device_ids checks status, so start with default
>>>>>>> */
>>>>>>> +       acpi_set_device_status(device, ACPI_STA_DEFAULT);
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> This shouldn't be done in a "get" routine.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> With this you mean the acpi_match_device_ids() check I assume ?
>>>>> (acpi_bus_get_status already calls acpi_set_device_status())
>>>>
>>>>
>>>>
>>>> Yes, the device ID check.
>>>>
>>>>>> Ideally, a scan handler should do that or similar.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> The problem is that drivers/acpi/scan.c: acpi_bus_attach()
>>>>> calls acpi_bus_get_status() and if it does not set
>>>>> the status to present acpi_bus_attach() exits without bothering
>>>>> with attaching scan handlers, which is why I ended up doing this
>>>>> here.
>>>>
>>>>
>>>>
>>>> Fair enough.
>>>>
>>>> Two problems with this approach.
>>>>
>>>> One is that it doesn't prevent _STA from being evaluated as
>>>> acpi_bus_get_status_handle() is called directly from a couple of
>>>> places.
>>>
>>>
>>> Yes I noticed that, but that is not a problem for this
>>> (and I would assume most) devices. Intervening directly
>>> in acpi_bus_get_status_handle is harder as there is no
>>> access to the hid there.
>>
>>
>> But if you modify acpi_set_device_status(), that should make it
>> consistent AFAICS.
>>
>> And this is just a quirk for devices where _STA is known to return
>> garbage sometimes and I'd call it a quirk openly.
>
>
> Ok, currently acpi_set_device_status() is an inline function in
> include/acpi/acpi_bus.h. If I understand you correctly you want me
> to uninline it and have a table with quirks which specify an override
> value to apply for certain acpi_ids in the uninlined version, correct ?

Yes.

Thanks,
Rafael