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

[Hans de Goede]

Hi,

On 19-04-17 22:14, Rafael J. Wysocki wrote:
> On Wed, Apr 19, 2017 at 2:04 PM, Hans de Goede <hdegoede at redhat.com> wrote:
>> Several Bay / Cherry Trail 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 Cherry Trail 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 uses it when it needs it), this
>> commit makes acpi_bus_get_status() always set status to ACPI_STA_DEFAULT
>> for the LPSS PWM device, fixing the lack of backlight control.
>>
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>> ---
>> Changes in v2:
>> -Use pr_debug instead of ACPI_DEBUG_PRINT
>> Changes in v3:
>> -Un-inline acpi_set_device_status and do the always_present_device_ids
>>   table check inside the un-inlined version of it
>> Changes in v4:
>> -Use dev_info instead of pr_debug
>> -Not only check for ACPI HID but also for CPU (SoC) model so as to not
>>   for devices present on other models then for which the quirk is intended and
>>   to avoid enabling unrelated ACPI devices which happen to use the same HID
>> Changes in v5:
>> -Only do the dev_info once per device (acpi_set_device_status gets called
>>   multiple times per device during boot)
>> Changes in v6:
>> -Allow specifying more then one CPU-model for a single HID
>> -Not only match the HID but also the UID, like on Cherry Trail, on some Bay
>>   Trail Windows 10 tablets we need to enable the PWM controller to get working
>>   backlight even though _STA returns 0. The Bay Trail SoC has 2 PWM controllers
>>   and we only need the first one. UID matching will allows adding an entry for
>>   Bay Trail which only enables the first PWM controller
>> Changes in v7:
>> -Put the actual x86 specific matching code and table with always present
>>   device HID + UID + CPU model entries in a new x86/x86_utils.c file which
>>   provides an acpi_device_always_present() helper function on x86, on
>>   non x86 a stub which always returns false is provided
>> -Squash in the addition of the Bay Trail PWM entry in the table as it
>>   is there for the same reasons as the Cherry Trail entry is there

<snip>

>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> index 34fbe02..4d952cc 100644
>> --- a/drivers/acpi/bus.c
>> +++ b/drivers/acpi/bus.c
>> @@ -132,6 +132,16 @@ int acpi_bus_get_status(struct acpi_device *device)
>>   }
>>   EXPORT_SYMBOL(acpi_bus_get_status);
>>
>> +void acpi_set_device_status(struct acpi_device *adev, u32 sta)
>> +{
>> +       u32 *status = (u32 *)&adev->status;
>> +
>> +       if (acpi_device_always_present(adev))
>> +               *status = ACPI_STA_DEFAULT;
>> +       else
>> +               *status = sta;
> 
> *((u32 *)&adev->status) = acpi_device_always_present(adev) ?
> ACPI_STA_DEFAULT : sta;
> 
> and I guess it could still be static inline?
> 
> But that said, evaluating _STA for the always present devices is
> pointless (as Lukas noticed), so why not to modify
> acpi_bus_get_status() to do something like:
> 
>      if (acpi_device_always_present(adev)) {
>          acpi_set_device_status(adev, ACPI_STA_DEFAULT);
>          return 0;
>      }
> 
> upfront

Hehe, my v1 of this patch actually did the check in acpi_bus_get_status()
I moved it to acpi_set_device_status() on your request. Moving it back
is fine with me and indeed seems cleaner.

> and modify the other path invoking acpi_set_device_status() accordingly.
> 
> And in that path, which I seem to have overlooked before, the
> acpi_set_device_status() call is too early for invoking
> acpi_device_always_present(adev), so the latter should be called
> directly from acpi_add_single_object() like
> 
>      acpi_init_device_object(device, handle, type, sta);
>      if (acpi_device_always_present(adev))
>          acpi_set_device_status(adev, ACPI_STA_DEFAULT);

That is not necessary, the place(s) where we care about status being
fixed-up for always-present devices, so that they get scanned / their
platform device initiated, is in acpi_bus_attach() which
already calls acpi_bus_get_status() and thus gets the
acpi_device_always_present() check applied before checking.

For hotplugged devices there are acpi_scan_bus_check and
acpi_scan_device_check but those both also already call
acpi_bus_get_status() before checking adev->status.

So just adding the check to acpi_bus_get_status(), as I did in
v1 is sufficient.

Note that the end result is still significantly cleaner then v1
as we're now using the acpi_device_always_present() which makes
the drivers/acpi/bus.c changes a lot cleaner.

I'll go and test this and then send a v8.

Regards,

Hans