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

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

[Hans de Goede]

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);
+	if (acpi_match_device_ids(device, always_present_device_ids) == 0) {
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device [%s] is in always present list setting status [%08x]\n",
+				  device->pnp.bus_id, ACPI_STA_DEFAULT));
+		return 0;
+	}
+	acpi_set_device_status(device, 0);
+
 	status = acpi_bus_get_status_handle(device->handle, &sta);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
-- 
2.9.3

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

[Rafael J. Wysocki]

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

Ideally, a scan handler should do that or similar.

> +	if (acpi_match_device_ids(device, always_present_device_ids) == 0) {
> +		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device [%s] is in always present list setting status [%08x]\n",
> +				  device->pnp.bus_id, ACPI_STA_DEFAULT));

pr_debug() please.  The ACPI_DEBUG_PRINT() stuff is basically for ACPICA
(yes, I know it is used elsewhere and no, it is not a good idea to do that).

> +		return 0;
> +	}
> +	acpi_set_device_status(device, 0);
> +
>  	status = acpi_bus_get_status_handle(device->handle, &sta);
>  	if (ACPI_FAILURE(status))
>  		return -ENODEV;
> 

Thanks,
Rafael

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

[Hans de Goede]

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())

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

>> +	if (acpi_match_device_ids(device, always_present_device_ids) == 0) {
>> +		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device [%s] is in always present list setting status [%08x]\n",
>> +				  device->pnp.bus_id, ACPI_STA_DEFAULT));
>
> pr_debug() please.  The ACPI_DEBUG_PRINT() stuff is basically for ACPICA
> (yes, I know it is used elsewhere and no, it is not a good idea to do that).

Ok, that is easy to fix :)


>> +		return 0;
>> +	}
>> +	acpi_set_device_status(device, 0);
>> +
>>  	status = acpi_bus_get_status_handle(device->handle, &sta);
>>  	if (ACPI_FAILURE(status))
>>  		return -ENODEV;
>>
>
> Thanks,
> Rafael

Regards,

Hans

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

[Takashi Iwai]

On Mon, 27 Feb 2017 15:25:32 +0100,
Hans de Goede 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())
> 
> > 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.

Oh, this is interesting, please let me join to the party.

We've hit a similar problem, but for other one: namely, it's INT0002
that is found on a few CHT devices.  It's never bound properly by a
similar reason, where _STA is always zero on Linux:

            Method (_STA, 0, NotSerialized)  // _STA: Status
            {
                If (SOCS <= 0x04)
                {
                    Return (0x0F)
                }
                Else
                {
                    Return (Zero)
                }
            }

The device is supposed to be a "virtual GPIO" stuff, and the driver
hasn't been upstreamed from Intel.  Due to the lack of this driver
(and it's binding), the machine gets spurious IRQ#9 after the PM
resume, and it locks up at the second time of PM.


thanks,

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

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

[Rafael J. Wysocki]

On Mon, Feb 27, 2017 at 3:40 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Mon, 27 Feb 2017 15:25:32 +0100,
> Hans de Goede 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())
>>
>> > 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.
>
> Oh, this is interesting, please let me join to the party.
>
> We've hit a similar problem, but for other one: namely, it's INT0002
> that is found on a few CHT devices.  It's never bound properly by a
> similar reason, where _STA is always zero on Linux:
>
>             Method (_STA, 0, NotSerialized)  // _STA: Status
>             {
>                 If (SOCS <= 0x04)
>                 {
>                     Return (0x0F)
>                 }
>                 Else
>                 {
>                     Return (Zero)
>                 }
>             }
>
> The device is supposed to be a "virtual GPIO" stuff, and the driver
> hasn't been upstreamed from Intel.  Due to the lack of this driver
> (and it's binding), the machine gets spurious IRQ#9 after the PM
> resume, and it locks up at the second time of PM.

Well, the solution here seems to be to acquire the missing driver
instead of adding quirks to the ACPI core.

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

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

[Takashi Iwai]

On Mon, 27 Feb 2017 22:27:58 +0100,
Rafael J. Wysocki wrote:
> 
> On Mon, Feb 27, 2017 at 3:40 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > On Mon, 27 Feb 2017 15:25:32 +0100,
> > Hans de Goede 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())
> >>
> >> > 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.
> >
> > Oh, this is interesting, please let me join to the party.
> >
> > We've hit a similar problem, but for other one: namely, it's INT0002
> > that is found on a few CHT devices.  It's never bound properly by a
> > similar reason, where _STA is always zero on Linux:
> >
> >             Method (_STA, 0, NotSerialized)  // _STA: Status
> >             {
> >                 If (SOCS <= 0x04)
> >                 {
> >                     Return (0x0F)
> >                 }
> >                 Else
> >                 {
> >                     Return (Zero)
> >                 }
> >             }
> >
> > The device is supposed to be a "virtual GPIO" stuff, and the driver
> > hasn't been upstreamed from Intel.  Due to the lack of this driver
> > (and it's binding), the machine gets spurious IRQ#9 after the PM
> > resume, and it locks up at the second time of PM.
> 
> Well, the solution here seems to be to acquire the missing driver
> instead of adding quirks to the ACPI core.

The driver is available (not upstreamed, though), but it's not bound
due to _STA above.  That's why I'm interested in this thread.


thanks,

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

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

[Hans de Goede]

Hi,

On 27-02-17 23:53, Takashi Iwai wrote:
> On Mon, 27 Feb 2017 22:27:58 +0100,
> Rafael J. Wysocki wrote:
>>
>> On Mon, Feb 27, 2017 at 3:40 PM, Takashi Iwai <tiwai@suse.de> wrote:

<snip>

>>> Oh, this is interesting, please let me join to the party.
>>>
>>> We've hit a similar problem, but for other one: namely, it's INT0002
>>> that is found on a few CHT devices.  It's never bound properly by a
>>> similar reason, where _STA is always zero on Linux:
>>>
>>>             Method (_STA, 0, NotSerialized)  // _STA: Status
>>>             {
>>>                 If (SOCS <= 0x04)
>>>                 {
>>>                     Return (0x0F)
>>>                 }
>>>                 Else
>>>                 {
>>>                     Return (Zero)
>>>                 }
>>>             }
>>>
>>> The device is supposed to be a "virtual GPIO" stuff, and the driver
>>> hasn't been upstreamed from Intel.  Due to the lack of this driver
>>> (and it's binding), the machine gets spurious IRQ#9 after the PM
>>> resume, and it locks up at the second time of PM.
>>
>> Well, the solution here seems to be to acquire the missing driver
>> instead of adding quirks to the ACPI core.
>
> The driver is available (not upstreamed, though), but it's not bound
> due to _STA above.  That's why I'm interested in this thread.

Takashi thanks for pointing me to the INT0002 device / driver to
fix the spurious IRQ#9 after the PM resume issue. I was seeing this
on the GPD-win too (when suspending with power-button + wakeup with
spacebar). I've added a patch to add the INT0002 device to the
always present device list + cleaned up the INT0002 driver. I plan
to submit this upstream soon.

If you want to test this on your device you need these patches:

https://github.com/jwrdegoede/linux-sunxi/commit/a1a6e92f2665376ed72f575553238a93e88bb037
https://github.com/jwrdegoede/linux-sunxi/commit/4946f68f8eaa300f42289bf767722d78cf7fa9e2
https://github.com/jwrdegoede/linux-sunxi/commit/32640c816dd60d17f003ae8306863da01c215afb
https://github.com/jwrdegoede/linux-sunxi/commit/abb6a9d69690bb2a1a00b184b06cdae43d6ad001

Regards,

Hans

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

[Takashi Iwai]

On Sun, 09 Apr 2017 22:32:46 +0200,
Hans de Goede wrote:
> 
> Hi,
> 
> On 27-02-17 23:53, Takashi Iwai wrote:
> > On Mon, 27 Feb 2017 22:27:58 +0100,
> > Rafael J. Wysocki wrote:
> >>
> >> On Mon, Feb 27, 2017 at 3:40 PM, Takashi Iwai <tiwai@suse.de> wrote:
> 
> <snip>
> 
> >>> Oh, this is interesting, please let me join to the party.
> >>>
> >>> We've hit a similar problem, but for other one: namely, it's INT0002
> >>> that is found on a few CHT devices.  It's never bound properly by a
> >>> similar reason, where _STA is always zero on Linux:
> >>>
> >>>             Method (_STA, 0, NotSerialized)  // _STA: Status
> >>>             {
> >>>                 If (SOCS <= 0x04)
> >>>                 {
> >>>                     Return (0x0F)
> >>>                 }
> >>>                 Else
> >>>                 {
> >>>                     Return (Zero)
> >>>                 }
> >>>             }
> >>>
> >>> The device is supposed to be a "virtual GPIO" stuff, and the driver
> >>> hasn't been upstreamed from Intel.  Due to the lack of this driver
> >>> (and it's binding), the machine gets spurious IRQ#9 after the PM
> >>> resume, and it locks up at the second time of PM.
> >>
> >> Well, the solution here seems to be to acquire the missing driver
> >> instead of adding quirks to the ACPI core.
> >
> > The driver is available (not upstreamed, though), but it's not bound
> > due to _STA above.  That's why I'm interested in this thread.
> 
> Takashi thanks for pointing me to the INT0002 device / driver to
> fix the spurious IRQ#9 after the PM resume issue. I was seeing this
> on the GPD-win too (when suspending with power-button + wakeup with
> spacebar). I've added a patch to add the INT0002 device to the
> always present device list + cleaned up the INT0002 driver. I plan
> to submit this upstream soon.
> 
> If you want to test this on your device you need these patches:
> 
> https://github.com/jwrdegoede/linux-sunxi/commit/a1a6e92f2665376ed72f575553238a93e88bb037
> https://github.com/jwrdegoede/linux-sunxi/commit/4946f68f8eaa300f42289bf767722d78cf7fa9e2
> https://github.com/jwrdegoede/linux-sunxi/commit/32640c816dd60d17f003ae8306863da01c215afb
> https://github.com/jwrdegoede/linux-sunxi/commit/abb6a9d69690bb2a1a00b184b06cdae43d6ad001

Thanks Hans, these look promising!

One remaining concern is that INT0002 seems serving for other purpose,
too, in drivers/platform/x86/intel_menlow.c for the thermal
management.  I wonder whether we can enable INT0002 unconditionally.


Takashi

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

[Hans de Goede]

Hi,

On 10-04-17 15:56, Takashi Iwai wrote:
> On Sun, 09 Apr 2017 22:32:46 +0200,
> Hans de Goede wrote:
>>
>> Hi,
>>
>> On 27-02-17 23:53, Takashi Iwai wrote:
>>> On Mon, 27 Feb 2017 22:27:58 +0100,
>>> Rafael J. Wysocki wrote:
>>>>
>>>> On Mon, Feb 27, 2017 at 3:40 PM, Takashi Iwai <tiwai@suse.de> wrote:
>>
>> <snip>
>>
>>>>> Oh, this is interesting, please let me join to the party.
>>>>>
>>>>> We've hit a similar problem, but for other one: namely, it's INT0002
>>>>> that is found on a few CHT devices.  It's never bound properly by a
>>>>> similar reason, where _STA is always zero on Linux:
>>>>>
>>>>>             Method (_STA, 0, NotSerialized)  // _STA: Status
>>>>>             {
>>>>>                 If (SOCS <= 0x04)
>>>>>                 {
>>>>>                     Return (0x0F)
>>>>>                 }
>>>>>                 Else
>>>>>                 {
>>>>>                     Return (Zero)
>>>>>                 }
>>>>>             }
>>>>>
>>>>> The device is supposed to be a "virtual GPIO" stuff, and the driver
>>>>> hasn't been upstreamed from Intel.  Due to the lack of this driver
>>>>> (and it's binding), the machine gets spurious IRQ#9 after the PM
>>>>> resume, and it locks up at the second time of PM.
>>>>
>>>> Well, the solution here seems to be to acquire the missing driver
>>>> instead of adding quirks to the ACPI core.
>>>
>>> The driver is available (not upstreamed, though), but it's not bound
>>> due to _STA above.  That's why I'm interested in this thread.
>>
>> Takashi thanks for pointing me to the INT0002 device / driver to
>> fix the spurious IRQ#9 after the PM resume issue. I was seeing this
>> on the GPD-win too (when suspending with power-button + wakeup with
>> spacebar). I've added a patch to add the INT0002 device to the
>> always present device list + cleaned up the INT0002 driver. I plan
>> to submit this upstream soon.
>>
>> If you want to test this on your device you need these patches:
>>
>> https://github.com/jwrdegoede/linux-sunxi/commit/a1a6e92f2665376ed72f575553238a93e88bb037
>> https://github.com/jwrdegoede/linux-sunxi/commit/4946f68f8eaa300f42289bf767722d78cf7fa9e2
>> https://github.com/jwrdegoede/linux-sunxi/commit/32640c816dd60d17f003ae8306863da01c215afb
>> https://github.com/jwrdegoede/linux-sunxi/commit/abb6a9d69690bb2a1a00b184b06cdae43d6ad001
>
> Thanks Hans, these look promising!
>
> One remaining concern is that INT0002 seems serving for other purpose,
> too, in drivers/platform/x86/intel_menlow.c for the thermal
> management.  I wonder whether we can enable INT0002 unconditionally.

Oh, good point I already had the following in the driver to deal with this:

static const struct x86_cpu_id int0002_cpu_ids[] = {
/*
  * Limit ourselves to Cherry Trail for now, until testing shows we
  * need to handle the INT0002 device on Baytrail too.
  *      ICPU(INTEL_FAM6_ATOM_SILVERMONT1),       * Valleyview, Bay Trail *
  */
         ICPU(INTEL_FAM6_ATOM_AIRMONT),          /* Braswell, Cherry Trail */
         {}
};


probe()
{
         /* Menlow has a different INT0002 device? <sigh> */
         data->cpu_id = x86_match_cpu(int0002_cpu_ids);
         if (!data->cpu_id)
                 return -ENODEV;
}

But I guess we need to do the same for the always present stuff to force the
status to present, so now I've :

#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_id[2];
};

#define ENTRY(hid, cpu_model) {                                         \
         { { hid, }, {} },                                               \
         { { X86_VENDOR_INTEL, 6, cpu_model, X86_FEATURE_ANY, }, {} }    \
}

static const struct always_present_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.
          */
         ENTRY("80862288", 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", INTEL_FAM6_ATOM_AIRMONT),
};
#endif

void acpi_set_device_status(struct acpi_device *adev, u32 sta)
{
         u32 *status = (u32 *)&adev->status;
#ifdef CONFIG_X86
         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 &&
                     x86_match_cpu(always_present_device_ids[i].cpu_id)) {
                         dev_info(&adev->dev, "Device [%s] is in always present l
                                  adev->pnp.bus_id, ACPI_STA_DEFAULT);
                         return;
                 }
         }
#endif
         *status = sta;
}



Which limits the forcing of status to present for INT0002 to cherrytrail
systems (and this probably is a good idea for the "80862288" pwm device too,
so it is done there too).

You can find the latest version of my patches, with these changes here:

https://github.com/jwrdegoede/linux-sunxi/commits/master

Regards,

Hans

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

[Rafael J. Wysocki]

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.

Second, what if the same device ID is used on another system where
_STA actually works correctly for that device?

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

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

[Hans de Goede]

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.

> Second, what if the same device ID is used on another system where
> _STA actually works correctly for that device?

You mean where _STA shoul actually report 0 because the
device is really unused. That is why the comment above
the table says:

+ * devices. Note this MUST only be done for devices where this is safe.

In case of the device in question this will cause the pwm driver
to bind to it, but nothing will in turn bind to the pwm interface
so nothing will be done with it.

Regards,

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

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

[Rafael J. Wysocki]

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

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

[Hans de Goede]

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

Re: [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@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@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 ?

Yes.

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