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

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

[Hans de Goede]

Several 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 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>
---
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
---
 drivers/acpi/bus.c      | 65 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/acpi/acpi_bus.h |  6 +----
 2 files changed, 66 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 34fbe02..eb30630 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -34,6 +34,8 @@
 #include <linux/reboot.h>
 #include <linux/delay.h>
 #ifdef CONFIG_X86
+#include <asm/cpu_device_id.h>
+#include <asm/intel-family.h>
 #include <asm/mpspec.h>
 #endif
 #include <linux/acpi_iort.h>
@@ -132,6 +134,69 @@ int acpi_bus_get_status(struct acpi_device *device)
 }
 EXPORT_SYMBOL(acpi_bus_get_status);
 
+#ifdef CONFIG_X86
+/*
+ * 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.
+ *
+ * This forcing of devices to be present is limited to specific CPU (SoC)
+ * models both to avoid potentially causing trouble on other models and
+ * because some HIDs are re-used on different SoCs for completely
+ * different devices.
+ */
+struct always_present_device_id {
+	struct acpi_device_id hid[2];
+	struct x86_cpu_id cpu_ids[2];
+	const char *uid;
+};
+
+#define ICPU(model)	{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, }
+
+#define ENTRY(hid, uid, cpu_models) {					\
+	{ { hid, }, {} },						\
+	{ cpu_models, {} },						\
+	uid,								\
+}
+
+static const struct always_present_device_id always_present_device_ids[] = {
+	/*
+	 * Cherry Trail PWM directly poked by GPU driver in win10,
+	 * but Linux uses a separate PWM driver, harmless if not used.
+	 */
+	ENTRY("80862288", "1", ICPU(INTEL_FAM6_ATOM_AIRMONT)),
+};
+#endif
+
+void acpi_set_device_status(struct acpi_device *adev, u32 sta)
+{
+	u32 *status = (u32 *)&adev->status;
+#ifdef CONFIG_X86
+	u32 old_status = *status;
+	int i;
+
+	/* acpi_match_device_ids checks status, so start with default */
+	*status = ACPI_STA_DEFAULT;
+	for (i = 0; i < ARRAY_SIZE(always_present_device_ids); i++) {
+		if (acpi_match_device_ids(adev,
+			always_present_device_ids[i].hid) == 0 &&
+		    adev->pnp.unique_id &&
+		    strcmp(adev->pnp.unique_id,
+			   always_present_device_ids[i].uid) == 0 &&
+		    x86_match_cpu(always_present_device_ids[i].cpu_ids)) {
+			if (old_status != ACPI_STA_DEFAULT)
+				dev_info(&adev->dev,
+					 "Device [%s] is in always present list setting status [%08x]\n",
+					 adev->pnp.bus_id, ACPI_STA_DEFAULT);
+			return;
+		}
+	}
+#endif
+	*status = sta;
+}
+
 void acpi_bus_private_data_handler(acpi_handle handle,
 				   void *context)
 {
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index b53c058..ef0794a 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -441,11 +441,6 @@ static inline void *acpi_driver_data(struct acpi_device *d)
 #define to_acpi_device(d)	container_of(d, struct acpi_device, dev)
 #define to_acpi_driver(d)	container_of(d, struct acpi_driver, drv)
 
-static inline void acpi_set_device_status(struct acpi_device *adev, u32 sta)
-{
-	*((u32 *)&adev->status) = sta;
-}
-
 static inline void acpi_set_hp_context(struct acpi_device *adev,
 				       struct acpi_hotplug_context *hp)
 {
@@ -494,6 +489,7 @@ void acpi_bus_put_acpi_device(struct acpi_device *adev);
 acpi_status acpi_bus_get_status_handle(acpi_handle handle,
 				       unsigned long long *sta);
 int acpi_bus_get_status(struct acpi_device *device);
+void acpi_set_device_status(struct acpi_device *adev, u32 sta);
 
 int acpi_bus_set_power(acpi_handle handle, int state);
 const char *acpi_power_state_string(int state);
-- 
2.9.3

[PATCH v6 2/3] ACPI / bus: Add INT0002 to list of always-present devices

[Hans de Goede]

The INT0002 device is necessary to clear wakeup interrupt sources
on Cherry Trail devices, without it we get nobody cared IRQ msgs
and some systems don't properly resume at all without it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v6:
-This is a new patch in v6 of this patch-set
---
 drivers/acpi/bus.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index eb30630..6fa177c 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -167,6 +167,11 @@ static const struct always_present_device_id always_present_device_ids[] = {
 	 * but Linux uses a separate PWM driver, harmless if not used.
 	 */
 	ENTRY("80862288", "1", ICPU(INTEL_FAM6_ATOM_AIRMONT)),
+	/*
+	 * The INT0002 device is necessary to clear wakeup interrupt sources
+	 * on Cherry Trail devices, without it we get nobody cared IRQ msgs.
+	 */
+	ENTRY("INT0002", "1", ICPU(INTEL_FAM6_ATOM_AIRMONT)),
 };
 #endif
 
-- 
2.9.3

[PATCH v6 3/3] ACPI / bus: Add Bay Trail PWM controller to list of always-present devices

[Hans de Goede]

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

Add an entry for the the Bay Trail PWM controller to list of
always-present devices to fix backlight control not working on
some Bay Trail devices.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v6:
-This is a new patch in v6 of this patch-set
---
 drivers/acpi/bus.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 6fa177c..5f9fe3f 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -163,9 +163,10 @@ struct always_present_device_id {
 
 static const struct always_present_device_id always_present_device_ids[] = {
 	/*
-	 * Cherry Trail PWM directly poked by GPU driver in win10,
+	 * Bay / Cherry Trail PWM directly poked by GPU driver in win10,
 	 * but Linux uses a separate PWM driver, harmless if not used.
 	 */
+	ENTRY("80860F09", "1", ICPU(INTEL_FAM6_ATOM_SILVERMONT1)),
 	ENTRY("80862288", "1", ICPU(INTEL_FAM6_ATOM_AIRMONT)),
 	/*
 	 * The INT0002 device is necessary to clear wakeup interrupt sources
-- 
2.9.3

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

[Rafael J. Wysocki]

On Tue, Apr 18, 2017 at 1:54 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Several 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 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>
> ---
> 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
> ---
>  drivers/acpi/bus.c      | 65 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/acpi/acpi_bus.h |  6 +----
>  2 files changed, 66 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 34fbe02..eb30630 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -34,6 +34,8 @@
>  #include <linux/reboot.h>
>  #include <linux/delay.h>
>  #ifdef CONFIG_X86
> +#include <asm/cpu_device_id.h>
> +#include <asm/intel-family.h>
>  #include <asm/mpspec.h>
>  #endif
>  #include <linux/acpi_iort.h>
> @@ -132,6 +134,69 @@ int acpi_bus_get_status(struct acpi_device *device)
>  }
>  EXPORT_SYMBOL(acpi_bus_get_status);
>
> +#ifdef CONFIG_X86
> +/*
> + * 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.
> + *
> + * This forcing of devices to be present is limited to specific CPU (SoC)
> + * models both to avoid potentially causing trouble on other models and
> + * because some HIDs are re-used on different SoCs for completely
> + * different devices.
> + */
> +struct always_present_device_id {
> +       struct acpi_device_id hid[2];
> +       struct x86_cpu_id cpu_ids[2];

This really is x86-specific, so it should go into somewhere like
arch/x86/kernel/acpi/ or drivers/acpi/x86/ (not present yet).

> +       const char *uid;
> +};
> +
> +#define ICPU(model)    { X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, }
> +
> +#define ENTRY(hid, uid, cpu_models) {                                  \
> +       { { hid, }, {} },                                               \
> +       { cpu_models, {} },                                             \
> +       uid,                                                            \
> +}
> +
> +static const struct always_present_device_id always_present_device_ids[] = {
> +       /*
> +        * Cherry Trail PWM directly poked by GPU driver in win10,
> +        * but Linux uses a separate PWM driver, harmless if not used.
> +        */
> +       ENTRY("80862288", "1", ICPU(INTEL_FAM6_ATOM_AIRMONT)),
> +};
> +#endif
> +
> +void acpi_set_device_status(struct acpi_device *adev, u32 sta)
> +{
> +       u32 *status = (u32 *)&adev->status;
> +#ifdef CONFIG_X86
> +       u32 old_status = *status;
> +       int i;
> +
> +       /* acpi_match_device_ids checks status, so start with default */
> +       *status = ACPI_STA_DEFAULT;
> +       for (i = 0; i < ARRAY_SIZE(always_present_device_ids); i++) {
> +               if (acpi_match_device_ids(adev,
> +                       always_present_device_ids[i].hid) == 0 &&
> +                   adev->pnp.unique_id &&
> +                   strcmp(adev->pnp.unique_id,
> +                          always_present_device_ids[i].uid) == 0 &&
> +                   x86_match_cpu(always_present_device_ids[i].cpu_ids)) {

Split this condition please.  It is almost unreadable as is.

> +                       if (old_status != ACPI_STA_DEFAULT)
> +                               dev_info(&adev->dev,
> +                                        "Device [%s] is in always present list setting status [%08x]\n",
> +                                        adev->pnp.bus_id, ACPI_STA_DEFAULT);
> +                       return;
> +               }
> +       }
> +#endif
> +       *status = sta;
> +}
> +
>  void acpi_bus_private_data_handler(acpi_handle handle,
>                                    void *context)
>  {

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

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

[Hans de Goede]

Hi,

On 18-04-17 15:34, Rafael J. Wysocki wrote:
> On Tue, Apr 18, 2017 at 1:54 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Several 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 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>
>> ---
>> 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
>> ---
>>  drivers/acpi/bus.c      | 65 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/acpi/acpi_bus.h |  6 +----
>>  2 files changed, 66 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> index 34fbe02..eb30630 100644
>> --- a/drivers/acpi/bus.c
>> +++ b/drivers/acpi/bus.c
>> @@ -34,6 +34,8 @@
>>  #include <linux/reboot.h>
>>  #include <linux/delay.h>
>>  #ifdef CONFIG_X86
>> +#include <asm/cpu_device_id.h>
>> +#include <asm/intel-family.h>
>>  #include <asm/mpspec.h>
>>  #endif
>>  #include <linux/acpi_iort.h>
>> @@ -132,6 +134,69 @@ int acpi_bus_get_status(struct acpi_device *device)
>>  }
>>  EXPORT_SYMBOL(acpi_bus_get_status);
>>
>> +#ifdef CONFIG_X86
>> +/*
>> + * 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.
>> + *
>> + * This forcing of devices to be present is limited to specific CPU (SoC)
>> + * models both to avoid potentially causing trouble on other models and
>> + * because some HIDs are re-used on different SoCs for completely
>> + * different devices.
>> + */
>> +struct always_present_device_id {
>> +       struct acpi_device_id hid[2];
>> +       struct x86_cpu_id cpu_ids[2];
>
> This really is x86-specific, so it should go into somewhere like
> arch/x86/kernel/acpi/ or drivers/acpi/x86/ (not present yet).

Ok, but then how do you want to hook this up, before you said
that you wanted to deal with this in acpi_set_device_status(),
which belongs in drivers/acpi/bus.c, do you want the x86
code to provide something like a

bool acpi_device_always_present(struct acpi_device *adev) {
}

Helper function and use that in the drivers/acpi/bus.c
acpi_set_device_status() implementation ?


>
>> +       const char *uid;
>> +};
>> +
>> +#define ICPU(model)    { X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, }
>> +
>> +#define ENTRY(hid, uid, cpu_models) {                                  \
>> +       { { hid, }, {} },                                               \
>> +       { cpu_models, {} },                                             \
>> +       uid,                                                            \
>> +}
>> +
>> +static const struct always_present_device_id always_present_device_ids[] = {
>> +       /*
>> +        * Cherry Trail PWM directly poked by GPU driver in win10,
>> +        * but Linux uses a separate PWM driver, harmless if not used.
>> +        */
>> +       ENTRY("80862288", "1", ICPU(INTEL_FAM6_ATOM_AIRMONT)),
>> +};
>> +#endif
>> +
>> +void acpi_set_device_status(struct acpi_device *adev, u32 sta)
>> +{
>> +       u32 *status = (u32 *)&adev->status;
>> +#ifdef CONFIG_X86
>> +       u32 old_status = *status;
>> +       int i;
>> +
>> +       /* acpi_match_device_ids checks status, so start with default */
>> +       *status = ACPI_STA_DEFAULT;
>> +       for (i = 0; i < ARRAY_SIZE(always_present_device_ids); i++) {
>> +               if (acpi_match_device_ids(adev,
>> +                       always_present_device_ids[i].hid) == 0 &&
>> +                   adev->pnp.unique_id &&
>> +                   strcmp(adev->pnp.unique_id,
>> +                          always_present_device_ids[i].uid) == 0 &&
>> +                   x86_match_cpu(always_present_device_ids[i].cpu_ids)) {
>
> Split this condition please.  It is almost unreadable as is.

Not sure what you want me to do here ? Use multiple if's with "continue"
if the check fails, or add whitespace (empty lines) between the conditions,
or ... ?


>
>> +                       if (old_status != ACPI_STA_DEFAULT)
>> +                               dev_info(&adev->dev,
>> +                                        "Device [%s] is in always present list setting status [%08x]\n",
>> +                                        adev->pnp.bus_id, ACPI_STA_DEFAULT);
>> +                       return;
>> +               }
>> +       }
>> +#endif
>> +       *status = sta;
>> +}
>> +
>>  void acpi_bus_private_data_handler(acpi_handle handle,
>>                                    void *context)
>>  {
>
> Thanks,
> Rafael
>

Regards,

Hans

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

[Rafael J. Wysocki]

On Wed, Apr 19, 2017 at 10:59 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 18-04-17 15:34, Rafael J. Wysocki wrote:
>>
>> On Tue, Apr 18, 2017 at 1:54 PM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>
>>> Several 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 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>
>>> ---
>>> 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
>>> ---
>>>  drivers/acpi/bus.c      | 65
>>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  include/acpi/acpi_bus.h |  6 +----
>>>  2 files changed, 66 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>>> index 34fbe02..eb30630 100644
>>> --- a/drivers/acpi/bus.c
>>> +++ b/drivers/acpi/bus.c
>>> @@ -34,6 +34,8 @@
>>>  #include <linux/reboot.h>
>>>  #include <linux/delay.h>
>>>  #ifdef CONFIG_X86
>>> +#include <asm/cpu_device_id.h>
>>> +#include <asm/intel-family.h>
>>>  #include <asm/mpspec.h>
>>>  #endif
>>>  #include <linux/acpi_iort.h>
>>> @@ -132,6 +134,69 @@ int acpi_bus_get_status(struct acpi_device *device)
>>>  }
>>>  EXPORT_SYMBOL(acpi_bus_get_status);
>>>
>>> +#ifdef CONFIG_X86
>>> +/*
>>> + * 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.
>>> + *
>>> + * This forcing of devices to be present is limited to specific CPU
>>> (SoC)
>>> + * models both to avoid potentially causing trouble on other models and
>>> + * because some HIDs are re-used on different SoCs for completely
>>> + * different devices.
>>> + */
>>> +struct always_present_device_id {
>>> +       struct acpi_device_id hid[2];
>>> +       struct x86_cpu_id cpu_ids[2];
>>
>>
>> This really is x86-specific, so it should go into somewhere like
>> arch/x86/kernel/acpi/ or drivers/acpi/x86/ (not present yet).
>
>
> Ok, but then how do you want to hook this up, before you said
> that you wanted to deal with this in acpi_set_device_status(),
> which belongs in drivers/acpi/bus.c, do you want the x86
> code to provide something like a
>
> bool acpi_device_always_present(struct acpi_device *adev) {
> }
>
> Helper function and use that in the drivers/acpi/bus.c
> acpi_set_device_status() implementation ?

Yes, something like that.

In a header file you can make it depend on CONFIG_X86 and provide and
empty static inline stub for the "not defined" case.

>>
>>> +       const char *uid;
>>> +};
>>> +
>>> +#define ICPU(model)    { X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, }
>>> +
>>> +#define ENTRY(hid, uid, cpu_models) {                                  \
>>> +       { { hid, }, {} },                                               \
>>> +       { cpu_models, {} },                                             \
>>> +       uid,                                                            \
>>> +}
>>> +
>>> +static const struct always_present_device_id always_present_device_ids[]
>>> = {
>>> +       /*
>>> +        * Cherry Trail PWM directly poked by GPU driver in win10,
>>> +        * but Linux uses a separate PWM driver, harmless if not used.
>>> +        */
>>> +       ENTRY("80862288", "1", ICPU(INTEL_FAM6_ATOM_AIRMONT)),
>>> +};
>>> +#endif
>>> +
>>> +void acpi_set_device_status(struct acpi_device *adev, u32 sta)
>>> +{
>>> +       u32 *status = (u32 *)&adev->status;
>>> +#ifdef CONFIG_X86
>>> +       u32 old_status = *status;
>>> +       int i;
>>> +
>>> +       /* acpi_match_device_ids checks status, so start with default */
>>> +       *status = ACPI_STA_DEFAULT;
>>> +       for (i = 0; i < ARRAY_SIZE(always_present_device_ids); i++) {
>>> +               if (acpi_match_device_ids(adev,
>>> +                       always_present_device_ids[i].hid) == 0 &&
>>> +                   adev->pnp.unique_id &&
>>> +                   strcmp(adev->pnp.unique_id,
>>> +                          always_present_device_ids[i].uid) == 0 &&
>>> +                   x86_match_cpu(always_present_device_ids[i].cpu_ids))
>>> {
>>
>>
>> Split this condition please.  It is almost unreadable as is.
>
>
> Not sure what you want me to do here ? Use multiple if's with "continue"
> if the check fails, or add whitespace (empty lines) between the conditions,
> or ... ?

I would do the multiple if()s variant.

The compiler should coalesce them anyway if it is smart enough.

Thanks,
Rafael

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

[Lukas Wunner]

On Wed, Apr 19, 2017 at 12:28:50PM +0200, Rafael J. Wysocki wrote:
> On Wed, Apr 19, 2017 at 10:59 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> > On 18-04-17 15:34, Rafael J. Wysocki wrote:
> >> On Tue, Apr 18, 2017 at 1:54 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> >>>
> >>> Several 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 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      | 65
> >>> +++++++++++++++++++++++++++++++++++++++++++++++++
> >>>  include/acpi/acpi_bus.h |  6 +----
> >>>  2 files changed, 66 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> >>> index 34fbe02..eb30630 100644
> >>> --- a/drivers/acpi/bus.c
> >>> +++ b/drivers/acpi/bus.c
> >>> @@ -34,6 +34,8 @@
> >>>  #include <linux/reboot.h>
> >>>  #include <linux/delay.h>
> >>>  #ifdef CONFIG_X86
> >>> +#include <asm/cpu_device_id.h>
> >>> +#include <asm/intel-family.h>
> >>>  #include <asm/mpspec.h>
> >>>  #endif
> >>>  #include <linux/acpi_iort.h>
> >>> @@ -132,6 +134,69 @@ int acpi_bus_get_status(struct acpi_device *device)
> >>>  }
> >>>  EXPORT_SYMBOL(acpi_bus_get_status);
> >>>
> >>> +#ifdef CONFIG_X86
> >>> +/*
> >>> + * 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.
> >>> + *
> >>> + * This forcing of devices to be present is limited to specific CPU
> >>> (SoC)
> >>> + * models both to avoid potentially causing trouble on other models and
> >>> + * because some HIDs are re-used on different SoCs for completely
> >>> + * different devices.
> >>> + */
> >>> +struct always_present_device_id {
> >>> +       struct acpi_device_id hid[2];
> >>> +       struct x86_cpu_id cpu_ids[2];
> >>
> >> This really is x86-specific, so it should go into somewhere like
> >> arch/x86/kernel/acpi/ or drivers/acpi/x86/ (not present yet).
> >
> > Ok, but then how do you want to hook this up, before you said
> > that you wanted to deal with this in acpi_set_device_status(),
> > which belongs in drivers/acpi/bus.c, do you want the x86
> > code to provide something like a
> >
> > bool acpi_device_always_present(struct acpi_device *adev) {
> > }
> >
> > Helper function and use that in the drivers/acpi/bus.c
> > acpi_set_device_status() implementation ?
> 
> Yes, something like that.
> 
> In a header file you can make it depend on CONFIG_X86 and provide and
> empty static inline stub for the "not defined" case.

The PCI subsystem uses __weak stubs such as pcibios_add_bus() for arch-
specific behaviour, or quirks which can be declared in arch/x86 to
constrain them to this specific platform.

Perhaps it makes sense to introduce ACPI quirks which can be declared
the same way as PCI quirks to avoid cluttering the generic code with
arch- or device-specific special cases.  So in this case we'd have
something like:

DECLARE_ACPI_FIXUP_STATUS(const char *hid, const char *uid, hook);

And before calling _STA we'd check for presence of a fixup for the
device in question.  Any additional stuff such as _HRV, CPU ID,
DMI data could be checked in the fixup hook.  Thoughts?

By the way:

> >>> +void acpi_set_device_status(struct acpi_device *adev, u32 sta)
> >>> +{
> >>> +       u32 *status = (u32 *)&adev->status;
> >>> +#ifdef CONFIG_X86
> >>> +       u32 old_status = *status;
> >>> +       int i;
> >>> +
> >>> +       /* acpi_match_device_ids checks status, so start with default */
> >>> +       *status = ACPI_STA_DEFAULT;
> >>> +       for (i = 0; i < ARRAY_SIZE(always_present_device_ids); i++) {
> >>> +               if (acpi_match_device_ids(adev,
> >>> +                       always_present_device_ids[i].hid) == 0 &&
> >>> +                   adev->pnp.unique_id &&
> >>> +                   strcmp(adev->pnp.unique_id,
> >>> +                          always_present_device_ids[i].uid) == 0 &&
> >>> +                   x86_match_cpu(always_present_device_ids[i].cpu_ids))
> >>> {

Why is this added to acpi_set_device_status(), rather than
acpi_bus_get_status_handle() ?  That way you're gratuitously calling
_STA even though you ignore its return value afterwards.

Thanks,

Lukas

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

[Rafael J. Wysocki]

On Wed, Apr 19, 2017 at 6:17 PM, Lukas Wunner <lukas@wunner.de> wrote:
> On Wed, Apr 19, 2017 at 12:28:50PM +0200, Rafael J. Wysocki wrote:
>> On Wed, Apr 19, 2017 at 10:59 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> > On 18-04-17 15:34, Rafael J. Wysocki wrote:
>> >> On Tue, Apr 18, 2017 at 1:54 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> >>>
>> >>> Several 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 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      | 65
>> >>> +++++++++++++++++++++++++++++++++++++++++++++++++
>> >>>  include/acpi/acpi_bus.h |  6 +----
>> >>>  2 files changed, 66 insertions(+), 5 deletions(-)
>> >>>
>> >>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> >>> index 34fbe02..eb30630 100644
>> >>> --- a/drivers/acpi/bus.c
>> >>> +++ b/drivers/acpi/bus.c
>> >>> @@ -34,6 +34,8 @@
>> >>>  #include <linux/reboot.h>
>> >>>  #include <linux/delay.h>
>> >>>  #ifdef CONFIG_X86
>> >>> +#include <asm/cpu_device_id.h>
>> >>> +#include <asm/intel-family.h>
>> >>>  #include <asm/mpspec.h>
>> >>>  #endif
>> >>>  #include <linux/acpi_iort.h>
>> >>> @@ -132,6 +134,69 @@ int acpi_bus_get_status(struct acpi_device *device)
>> >>>  }
>> >>>  EXPORT_SYMBOL(acpi_bus_get_status);
>> >>>
>> >>> +#ifdef CONFIG_X86
>> >>> +/*
>> >>> + * 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.
>> >>> + *
>> >>> + * This forcing of devices to be present is limited to specific CPU
>> >>> (SoC)
>> >>> + * models both to avoid potentially causing trouble on other models and
>> >>> + * because some HIDs are re-used on different SoCs for completely
>> >>> + * different devices.
>> >>> + */
>> >>> +struct always_present_device_id {
>> >>> +       struct acpi_device_id hid[2];
>> >>> +       struct x86_cpu_id cpu_ids[2];
>> >>
>> >> This really is x86-specific, so it should go into somewhere like
>> >> arch/x86/kernel/acpi/ or drivers/acpi/x86/ (not present yet).
>> >
>> > Ok, but then how do you want to hook this up, before you said
>> > that you wanted to deal with this in acpi_set_device_status(),
>> > which belongs in drivers/acpi/bus.c, do you want the x86
>> > code to provide something like a
>> >
>> > bool acpi_device_always_present(struct acpi_device *adev) {
>> > }
>> >
>> > Helper function and use that in the drivers/acpi/bus.c
>> > acpi_set_device_status() implementation ?
>>
>> Yes, something like that.
>>
>> In a header file you can make it depend on CONFIG_X86 and provide and
>> empty static inline stub for the "not defined" case.
>
> The PCI subsystem uses __weak stubs such as pcibios_add_bus() for arch-
> specific behaviour, or quirks which can be declared in arch/x86 to
> constrain them to this specific platform.
>
> Perhaps it makes sense to introduce ACPI quirks which can be declared
> the same way as PCI quirks to avoid cluttering the generic code with
> arch- or device-specific special cases.  So in this case we'd have
> something like:
>
> DECLARE_ACPI_FIXUP_STATUS(const char *hid, const char *uid, hook);
>
> And before calling _STA we'd check for presence of a fixup for the
> device in question.  Any additional stuff such as _HRV, CPU ID,
> DMI data could be checked in the fixup hook.  Thoughts?

Why don't we do that later if need be?

BTW, I'm not a big fan of __weak declarations as they result in dead
code being added to binaries.

> By the way:
>
>> >>> +void acpi_set_device_status(struct acpi_device *adev, u32 sta)
>> >>> +{
>> >>> +       u32 *status = (u32 *)&adev->status;
>> >>> +#ifdef CONFIG_X86
>> >>> +       u32 old_status = *status;
>> >>> +       int i;
>> >>> +
>> >>> +       /* acpi_match_device_ids checks status, so start with default */
>> >>> +       *status = ACPI_STA_DEFAULT;
>> >>> +       for (i = 0; i < ARRAY_SIZE(always_present_device_ids); i++) {
>> >>> +               if (acpi_match_device_ids(adev,
>> >>> +                       always_present_device_ids[i].hid) == 0 &&
>> >>> +                   adev->pnp.unique_id &&
>> >>> +                   strcmp(adev->pnp.unique_id,
>> >>> +                          always_present_device_ids[i].uid) == 0 &&
>> >>> +                   x86_match_cpu(always_present_device_ids[i].cpu_ids))
>> >>> {
>
> Why is this added to acpi_set_device_status(), rather than
> acpi_bus_get_status_handle() ?  That way you're gratuitously calling
> _STA even though you ignore its return value afterwards.

Because it doesn't make much sense for power resources.

Thanks,
Rafael