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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 27-02-17 22:49, Rafael J. Wysocki wrote:
> On Mon, Feb 27, 2017 at 10:29 PM, Hans de Goede <hdegoede@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@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@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 ?

Or is there some existing quirk mechanism I should tie into ?

Regards,

Hans
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx