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

["Rafael J. Wysocki" <rafael@kernel.org>]

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.

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