[BUG] hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22

[BUG] hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22

[Lukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 866 bytes --]

Hi,

I've got HP EliteDesk 800 G6 Tower PC running Linux 6.1 from Debian 12.
I managed to build the hp-wmi-sensors out of tree. When I loaded it I
got EINVAL.

    hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22

I managed to track it down. And it happens in check_wobj() called from
check_platform_events_wobj() because in the for loop when prop==0 the
type is ACPI_TYPE_BUFFER instead of ACPI_TYPE_STRING. When I bypass this
particular check like this

    if (prop == 0 && type == ACPI_TYPE_BUFFER)
            continue;

everything else works like charm and I can read senosrs via sysfs. I'd
like to perpare a proper patch, but I've got no idea how to do properly
work this quirk around. What are your suggestions?

Kind regards,
-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

Re: [BUG] hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22

[Guenter Roeck]

On 10/27/23 08:07, Lukasz Stelmach wrote:
> Hi,
> 
> I've got HP EliteDesk 800 G6 Tower PC running Linux 6.1 from Debian 12.
> I managed to build the hp-wmi-sensors out of tree. When I loaded it I
> got EINVAL.
> 
>      hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22
> 
> I managed to track it down. And it happens in check_wobj() called from
> check_platform_events_wobj() because in the for loop when prop==0 the
> type is ACPI_TYPE_BUFFER instead of ACPI_TYPE_STRING. When I bypass this
> particular check like this
> 
>      if (prop == 0 && type == ACPI_TYPE_BUFFER)
>              continue;
> 
> everything else works like charm and I can read senosrs via sysfs. I'd
> like to perpare a proper patch, but I've got no idea how to do properly
> work this quirk around. What are your suggestions?
> 
> Kind regards,


I really don't know if that is a bug in the driver or a bug in the WMI
description on that system. Given that, I have no idea how to handle this.
I also don't know the impact of ACPI_TYPE_BUFFER vs. ACPI_TYPE_STRING;
from the code in the kernel it seems that those values have a distinctly
different meaning. Is the returned value, when extracted, a string ?
Is it ok to accept ACPI_TYPE_BUFFER instead of ACPI_TYPE_STRING for
property names ? I simply don't know.

Are the type checks really needed ? What do other drivers do when interpreting
WMI data ? Could extract_acpi_value() be more flexible and also accept
ACPI_TYPE_BUFFER, or would that potentially create other problems ?
Lots of questions that at least for my part can not answer.

Guenter

Re: [BUG] hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22

[Armin Wolf]

Am 27.10.23 um 19:07 schrieb Guenter Roeck:

> On 10/27/23 08:07, Lukasz Stelmach wrote:
>> Hi,
>>
>> I've got HP EliteDesk 800 G6 Tower PC running Linux 6.1 from Debian 12.
>> I managed to build the hp-wmi-sensors out of tree. When I loaded it I
>> got EINVAL.
>>
>>      hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A
>> failed with error -22
>>
>> I managed to track it down. And it happens in check_wobj() called from
>> check_platform_events_wobj() because in the for loop when prop==0 the
>> type is ACPI_TYPE_BUFFER instead of ACPI_TYPE_STRING. When I bypass this
>> particular check like this
>>
>>      if (prop == 0 && type == ACPI_TYPE_BUFFER)
>>              continue;
>>
>> everything else works like charm and I can read senosrs via sysfs. I'd
>> like to perpare a proper patch, but I've got no idea how to do properly
>> work this quirk around. What are your suggestions?
>>
>> Kind regards,
>
>
> I really don't know if that is a bug in the driver or a bug in the WMI
> description on that system. Given that, I have no idea how to handle
> this.
> I also don't know the impact of ACPI_TYPE_BUFFER vs. ACPI_TYPE_STRING;
> from the code in the kernel it seems that those values have a distinctly
> different meaning. Is the returned value, when extracted, a string ?
> Is it ok to accept ACPI_TYPE_BUFFER instead of ACPI_TYPE_STRING for
> property names ? I simply don't know.
>
Hi,

could it be that this ACPI buffer object containst UTF16 characters? If yes, then
the reason for this could be that on Windows, the ACPI-WMI mapper convert the whole
ACPI object into a buffer first, take a look at this:

https://learn.microsoft.com/en-us/windows-hardware/drivers/kernel/driver-defined-wmi-data-items

Since the mapper does not know anything about the higher-level content of the ACPI object,
i assume there are conversion rules on how to convert an ACPI object into an WMI buffer.
Maybe the mapper converts an ACPI string object into such a buffer per default?

> Are the type checks really needed ? What do other drivers do when
> interpreting
> WMI data ? Could extract_acpi_value() be more flexible and also accept
> ACPI_TYPE_BUFFER, or would that potentially create other problems ?
> Lots of questions that at least for my part can not answer.
>
> Guenter
>
It would be helpful if i had access to the output of acpidump and the BMOFs of this notebook.
If it is indeed possible to find out which conversion rules the Windows driver uses, then
i could create something like this:

	int wmidev_invoke_method(struct wmi_device *wdev, u8 instance, u32 method_id, u8 *input, size_t input_size, u8 *output);

It would basically wrap wmidev_evalue_method() and convert the resulting ACPI object into
an general WMI buffer.

Armin Wolf

Re: [BUG] hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22

[James Seo]

On Fri, Oct 27, 2023 at 05:07:09PM +0200, Lukasz Stelmach wrote:
> Hi,

Hi Łukasz, thanks for the report.

> I've got HP EliteDesk 800 G6 Tower PC running Linux 6.1 from Debian 12.
> I managed to build the hp-wmi-sensors out of tree. When I loaded it I
> got EINVAL.
> 
>     hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22
> 
> I managed to track it down. And it happens in check_wobj() called from
> check_platform_events_wobj() because in the for loop when prop==0 the
> type is ACPI_TYPE_BUFFER instead of ACPI_TYPE_STRING. When I bypass this
> particular check like this
> 
>     if (prop == 0 && type == ACPI_TYPE_BUFFER)
>             continue;
> 
> everything else works like charm and I can read senosrs via sysfs.

I'm surprised that your bypass is working! That check is intended to make it
safe to assume HPBIOS_BIOSNumericSensor.Name is a ACPI_TYPE_STRING. Which the
driver continues to do, of course.

Maybe .Name is being erroneously reported as an ACPI_TYPE_BUFFER on just that
model, but is in reality still a string? Dealing with that string in e.g.
hp_wmi_strdup() would still work because .string.pointer and .buffer.pointer
seem to end up at the same offset in a union acpi_object, at least for now.

> I'd like to perpare a proper patch, but I've got no idea how to do properly
> work this quirk around. What are your suggestions?

Could you share output from acpidump and sensors before we go any further?

Thanks.

-James Seo

> Kind regards,
> -- 
> Łukasz Stelmach
> Samsung R&D Institute Poland
> Samsung Electronics

Re: [BUG] hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22

[Lukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 3360 bytes --]

It was <2023-10-28 sob 11:37>, when James Seo wrote:
> On Fri, Oct 27, 2023 at 05:07:09PM +0200, Lukasz Stelmach wrote:
>> Hi,
>
> Hi Łukasz, thanks for the report.

I've got sensor I'd like to read (-;

>> I've got HP EliteDesk 800 G6 Tower PC running Linux 6.1 from Debian 12.
>> I managed to build the hp-wmi-sensors out of tree. When I loaded it I
>> got EINVAL.
>> 
>>     hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22
>> 
>> I managed to track it down. And it happens in check_wobj() called from
>> check_platform_events_wobj() because in the for loop when prop==0 the
>> type is ACPI_TYPE_BUFFER instead of ACPI_TYPE_STRING. When I bypass this
>> particular check like this
>> 
>>     if (prop == 0 && type == ACPI_TYPE_BUFFER)
>>             continue;
>> 
>> everything else works like charm and I can read senosrs via sysfs.
>
> I'm surprised that your bypass is working!

Me too, honestly (-; but hey...

> That check is intended to make it safe to assume
> HPBIOS_BIOSNumericSensor.Name is a ACPI_TYPE_STRING. Which the driver
> continues to do, of course.
>
> Maybe .Name is being erroneously reported as an ACPI_TYPE_BUFFER on
> just that model, but is in reality still a string?

I wouldn't be surprised at all.

> Dealing with that string in e.g.  hp_wmi_strdup() would still work
> because .string.pointer and .buffer.pointer seem to end up at the same
> offset in a union acpi_object, at least for now.
>
>> I'd like to perpare a proper patch, but I've got no idea how to do properly
>> work this quirk around. What are your suggestions?
>
> Could you share output from acpidump

I am attaching dsdt.dat. It is the only file that contains the
"HPBIOS_BIOSEvent" string and the above UUID.

[ The attachment is too big for linux-hwmon list to handle. I am sending
this copy of the message only to the list without the attachment. ]

> and sensors before we go any further?

The readings of hp_wmi_sensors-virtual-0 look sensible and they change
reasonably under load.

--8<---------------cut here---------------start------------->8---
coretemp-isa-0000
Adapter: ISA adapter
Package id 0:  +34.0°C  (high = +80.0°C, crit = +100.0°C)
Core 0:        +31.0°C  (high = +80.0°C, crit = +100.0°C)
Core 1:        +32.0°C  (high = +80.0°C, crit = +100.0°C)
Core 2:        +32.0°C  (high = +80.0°C, crit = +100.0°C)
Core 3:        +31.0°C  (high = +80.0°C, crit = +100.0°C)
Core 4:        +34.0°C  (high = +80.0°C, crit = +100.0°C)
Core 5:        +31.0°C  (high = +80.0°C, crit = +100.0°C)
Core 6:        +33.0°C  (high = +80.0°C, crit = +100.0°C)
Core 7:        +33.0°C  (high = +80.0°C, crit = +100.0°C)

pch_cometlake-virtual-0
Adapter: Virtual device
temp1:        +49.0°C  

hp_wmi_sensors-virtual-0
Adapter: Virtual device
CPU Fan Speed:          1006 RPM
Rear Chassis Fan Speed:  901 RPM
Power Supply Fan Speed:  803 RPM
CPU Temperature:         +33.0°C  
Chassis Temperature:     +28.0°C  

ucsi_source_psy_USBC000:001-isa-0000
Adapter: ISA adapter
in0:           0.00 V  (min =  +0.00 V, max =  +0.00 V)
curr1:         0.00 A  (max =  +0.00 A)
--8<---------------cut here---------------end--------------->8---


-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

Re: [BUG] hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22

[James Seo]

On Tue, Oct 31, 2023 at 11:20:00AM +0100, Lukasz Stelmach wrote:
> I am attaching dsdt.dat. It is the only file that contains the
> "HPBIOS_BIOSEvent" string and the above UUID.

OK, I think I've figured it out.

Earlier, I focused on "probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed"
(the GUID of HPBIOS_BIOSNumericSensor), and missed it when you said the failure
is happening when check_wobj() is called from check_platform_events_wobj().
So the issue is actually with HPBIOS_PlatformEvents.Name, since any instances
of that WMI object are examined during driver init as well.

After using iasl to decompile the DSDT you sent to ACPI Source Language (ASL),
everything looks fine with HPBIOS_BIOSNumericSensor. Not that I'm particularly
familiar with ASL, but search for "Name (SEN1, Package (0x06)" and read on to
the "Method (_INI, 0, NotSerialized)" and the "Method (WQAE, 1, Serialized)"
lines below. It certainly looks like that's how the BIOS on that system is
generating HPBIOS_PlatformEvents instances at runtime.

For HPBIOS_PlatformEvents instances, that BIOS seems to generate them in two
places. The driver is interested in those from "Method (WQBC, 1, Serialized)",
which rely on the section above that beginning at "Name (EVNT, Package (0x0D)".
There are also some generated in "Method (WQPE, 1, Serialized)", relying on:

        Name (CBWE, Package (0x02)
        {
            Package (0x05)
            {
                Unicode ("4BIOS Configuration Change"), // Oops.
                "BIOS Settings", 
                0x04, 
                0x05, 
                0x02
            }, 

            Package (0x05)
            {
                "BIOS Configuration Security", 
                "An attempt has been made to Access BIOS features unsuccessfully", 
                0x04, 
                0x0A, 
                0x06
            }
        })

So yes, Armin was right, UTF-16 is the culprit. And it's only for this one
instance of HPBIOS_PlatformEvents on this one system's BIOS.

Can you find the wonky object instance in the debugfs interface? I imagine
its name shows up as either "4" or an empty string, depending on the UTF-16
byte order.

I'm also curious how it looks in Windows (I used WMI Explorer [1] during
development). The WMI namespace should be "\\.\root\HP\InstrumentedBIOS".

> The readings of hp_wmi_sensors-virtual-0 look sensible and they change
> reasonably under load.

Good. I guess that part working wasn't so surprising after all :)

Regarding how we proceed from here, I think the main question is whether a
workaround should be limited to only specific machines (maybe detected using
DMI data), and if so, to specific WMI object types and instances (cf.
hp_wmi_get_wobj(), though limiting on instance number would be fragile if
BIOS updates change instance numbering). Other questions are how thoroughly
to validate UTF-16 in a buffer and whether to convert to UTF-8 internally.

For what it's worth, I personally don't see much value in doing much more than
a machine-limited workaround for now. To me it's clear that this UTF-16 corner
case is a BIOS bug and its consequences are minimal once a workaround is in
place.

Thoughts? Also, if you'd prefer to let me handle it, I'd be glad to write a
patch myself.

[1] https://github.com/vinaypamnani/wmie2

Re: [BUG] hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22

[James Seo]

On Tue, Oct 31, 2023 at 07:00:17AM -0700, James Seo wrote:
> After using iasl to decompile the DSDT you sent to ACPI Source Language (ASL),
> everything looks fine with HPBIOS_BIOSNumericSensor. Not that I'm particularly
> familiar with ASL, but search for "Name (SEN1, Package (0x06)" and read on to
> the "Method (_INI, 0, NotSerialized)" and the "Method (WQAE, 1, Serialized)"
> lines below. It certainly looks like that's how the BIOS on that system is
> generating HPBIOS_PlatformEvents instances at runtime.

Correction: I meant "HPBIOS_BIOSNumericSensor instances" in the last line here.

Sorry about that.

Re: [BUG] hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22

[Guenter Roeck]

On 10/31/23 07:00, James Seo wrote:
> On Tue, Oct 31, 2023 at 11:20:00AM +0100, Lukasz Stelmach wrote:

[ ... ]

> 
> For what it's worth, I personally don't see much value in doing much more than
> a machine-limited workaround for now. To me it's clear that this UTF-16 corner
> case is a BIOS bug and its consequences are minimal once a workaround is in
> place.
> 

A machine limited workaround sounds about right to me. We may have to revise
it later, but better safe than sorry.

Guenter

Re: [BUG] hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22

[Lukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 5098 bytes --]

It was <2023-10-31 wto 07:00>, when James Seo wrote:
> On Tue, Oct 31, 2023 at 11:20:00AM +0100, Lukasz Stelmach wrote:
>> I am attaching dsdt.dat. It is the only file that contains the
>> "HPBIOS_BIOSEvent" string and the above UUID.
>
> OK, I think I've figured it out.
>
> Earlier, I focused on "probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed"
> (the GUID of HPBIOS_BIOSNumericSensor), and missed it when you said the failure
> is happening when check_wobj() is called from check_platform_events_wobj().
> So the issue is actually with HPBIOS_PlatformEvents.Name, since any instances
> of that WMI object are examined during driver init as well.
>
> After using iasl to decompile the DSDT you sent to ACPI Source Language (ASL),
> everything looks fine with HPBIOS_BIOSNumericSensor. Not that I'm particularly
> familiar with ASL, but search for "Name (SEN1, Package (0x06)" and read on to
> the "Method (_INI, 0, NotSerialized)" and the "Method (WQAE, 1, Serialized)"
> lines below. It certainly looks like that's how the BIOS on that system is
> generating HPBIOS_PlatformEvents instances at runtime.
>
> For HPBIOS_PlatformEvents instances, that BIOS seems to generate them in two
> places. The driver is interested in those from "Method (WQBC, 1, Serialized)",
> which rely on the section above that beginning at "Name (EVNT, Package (0x0D)".
> There are also some generated in "Method (WQPE, 1, Serialized)", relying on:
>
>         Name (CBWE, Package (0x02)
>         {
>             Package (0x05)
>             {
>                 Unicode ("4BIOS Configuration Change"), // Oops.
>                 "BIOS Settings", 
>                 0x04, 
>                 0x05, 
>                 0x02
>             }, 
>
>             Package (0x05)
>             {
>                 "BIOS Configuration Security", 
>                 "An attempt has been made to Access BIOS features unsuccessfully", 
>                 0x04, 
>                 0x0A, 
>                 0x06
>             }
>         })
>
> So yes, Armin was right, UTF-16 is the culprit. And it's only for this one
> instance of HPBIOS_PlatformEvents on this one system's BIOS.
>
> Can you find the wonky object instance in the debugfs interface? I imagine
> its name shows up as either "4" or an empty string, depending on the UTF-16
> byte order.

Indeed there is one

--8<---------------cut here---------------start------------->8---
# grep -r "" /sys/kernel/debug/hp-wmi-sensors-0/platform_events/0/
/sys/kernel/debug/hp-wmi-sensors-0/platform_events/0/possible_status:2
/sys/kernel/debug/hp-wmi-sensors-0/platform_events/0/possible_severity:5
/sys/kernel/debug/hp-wmi-sensors-0/platform_events/0/category:4
/sys/kernel/debug/hp-wmi-sensors-0/platform_events/0/source_class:HPBIOS_BIOSEvent
/sys/kernel/debug/hp-wmi-sensors-0/platform_events/0/source_namespace:root\wmi
/sys/kernel/debug/hp-wmi-sensors-0/platform_events/0/description:BIOS Settings
/sys/kernel/debug/hp-wmi-sensors-0/platform_events/0/name:4
--8<---------------cut here---------------end--------------->8---

> I'm also curious how it looks in Windows (I used WMI Explorer [1] during
> development). The WMI namespace should be "\\.\root\HP\InstrumentedBIOS".

I'll see if I can find such machine with Windows. It may take a few days
thou.

>> The readings of hp_wmi_sensors-virtual-0 look sensible and they change
>> reasonably under load.
>
> Good. I guess that part working wasn't so surprising after all :)
>
> Regarding how we proceed from here, I think the main question is
> whether a workaround should be limited to only specific machines
> (maybe detected using DMI data), and if so, to specific WMI object
> types and instances (cf.  hp_wmi_get_wobj(), though limiting on
> instance number would be fragile if BIOS updates change instance
> numbering). Other questions are how thoroughly to validate UTF-16 in a
> buffer and whether to convert to UTF-8 internally.
>
> For what it's worth, I personally don't see much value in doing much
> more than a machine-limited workaround for now. To me it's clear that
> this UTF-16 corner case is a BIOS bug and its consequences are minimal
> once a workaround is in place.
>
> Thoughts?

I am no expert regarding x86 platforms and I don't know how often bugs
like this happen and if making it more generic makes sens. Maybe.

My solution would be to add a module option, let's name it `quirks` and
make it a bit field for future use, that enables the workaound. Plus an
additional error message when probe fails to suggest user to add the
option to kernel command line or whatever file that contains module
options. A nice touch would be to detect if the workaround is still
required.

Thouths.

> Also, if you'd prefer to let me handle it, I'd be glad to write a
> patch myself.

I am fine with that, I know a bit more about device-tree than about ACPI
(-; and PCs in general. I'll be glad to test the patch thou.

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

Re: [BUG] hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22

[Guenter Roeck]

On 10/31/23 12:07, Lukasz Stelmach wrote:

[ ... ]

>> For what it's worth, I personally don't see much value in doing much
>> more than a machine-limited workaround for now. To me it's clear that
>> this UTF-16 corner case is a BIOS bug and its consequences are minimal
>> once a workaround is in place.
>>
>> Thoughts?
> 
> I am no expert regarding x86 platforms and I don't know how often bugs
> like this happen and if making it more generic makes sens. Maybe.
> 

That really depends on the system vendor, less on the CPU architecture.

> My solution would be to add a module option, let's name it `quirks` and
> make it a bit field for future use, that enables the workaound. Plus an
> additional error message when probe fails to suggest user to add the
> option to kernel command line or whatever file that contains module
> options. A nice touch would be to detect if the workaround is still
> required.
> 

Please no module option. Use DMI data or similar.

Guenter

Re: [BUG] hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22

[Lukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 2297 bytes --]

It was <2023-10-31 wto 12:28>, when Guenter Roeck wrote:
> On 10/31/23 12:07, Lukasz Stelmach wrote:
>
> [ ... ]
>
>>> For what it's worth, I personally don't see much value in doing much
>>> more than a machine-limited workaround for now. To me it's clear that
>>> this UTF-16 corner case is a BIOS bug and its consequences are minimal
>>> once a workaround is in place.
>>>
>>> Thoughts?
>> I am no expert regarding x86 platforms and I don't know how often
>> bugs
>> like this happen and if making it more generic makes sens. Maybe.
>> 
>
> That really depends on the system vendor, less on the CPU architecture.

Of course it's not architecture but rather vendor landscape.  My point
is that most embedded platforms I work with can be fixed at this level
by patching device-tree, which is much easier (at least for me) than
patching BIOS/UEFI. And I've seen a number of broken BIOS-es over years
which vendors didn't care to fix. At the end of the day my *impression*
is that x86 platform users more often have to live with quirks like this
that require fixes at higher levels. But this is just my impression.

>> My solution would be to add a module option, let's name it `quirks` and
>> make it a bit field for future use, that enables the workaound. Plus an
>> additional error message when probe fails to suggest user to add the
>> option to kernel command line or whatever file that contains module
>> options. A nice touch would be to detect if the workaround is still
>> required.
>> 
>
> Please no module option. Use DMI data or similar.

DMI data is fine when can you identify broken systems upfront. In this
case we don't know which systems are or will be affected by this bug.

For example, eariler this year I fixed a different quirk in the same
machine[1] but luckily there was a way I could apply the fix without
waiting for the patch to be merged and pulled downstream. In my case
I could use ALSA features to make my machine work before I got a patched
kernel.

Maybe both?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ea24b9953bcd3889f77a66e7f1d7e86e995dd9c3
[2] https://bugzilla.kernel.org/show_bug.cgi?id=217008
-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

Re: [BUG] hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22

[Armin Wolf]

Am 31.10.23 um 22:07 schrieb Lukasz Stelmach:

> It was <2023-10-31 wto 12:28>, when Guenter Roeck wrote:
>> On 10/31/23 12:07, Lukasz Stelmach wrote:
>>
>> [ ... ]
>>
>>>> For what it's worth, I personally don't see much value in doing much
>>>> more than a machine-limited workaround for now. To me it's clear that
>>>> this UTF-16 corner case is a BIOS bug and its consequences are minimal
>>>> once a workaround is in place.
>>>>
>>>> Thoughts?

I think this is no BIOS bug, but valid behavior since the Windows ACPI-WMI mapper
converts the ACPI objects into a common buffer format as described here:

https://learn.microsoft.com/en-us/windows-hardware/drivers/kernel/driver-defined-wmi-data-items

I assume that the mapper converts the ACPI string into a WMI string buffer, and that
a common ACPI buffer is just passed as-is. In this case, the error lies inside the
linux WMI subsystem, which does not do such a conversion.

I can try to find out more about this conversion and its rules, and use this to add
support for that to the WMI subsystem. This would prevent such errors in the future
and would bring us closer to full ACPI WMI support inside the kernel.

This will take quite some time however, so i would suggest adding a quirk handler
first and replace this with the WMI conversion functions later.

Armin Wolf

>>> I am no expert regarding x86 platforms and I don't know how often
>>> bugs
>>> like this happen and if making it more generic makes sens. Maybe.
>>>
>> That really depends on the system vendor, less on the CPU architecture.
> Of course it's not architecture but rather vendor landscape.  My point
> is that most embedded platforms I work with can be fixed at this level
> by patching device-tree, which is much easier (at least for me) than
> patching BIOS/UEFI. And I've seen a number of broken BIOS-es over years
> which vendors didn't care to fix. At the end of the day my *impression*
> is that x86 platform users more often have to live with quirks like this
> that require fixes at higher levels. But this is just my impression.
>
>>> My solution would be to add a module option, let's name it `quirks` and
>>> make it a bit field for future use, that enables the workaound. Plus an
>>> additional error message when probe fails to suggest user to add the
>>> option to kernel command line or whatever file that contains module
>>> options. A nice touch would be to detect if the workaround is still
>>> required.
>>>
>> Please no module option. Use DMI data or similar.
> DMI data is fine when can you identify broken systems upfront. In this
> case we don't know which systems are or will be affected by this bug.
>
> For example, eariler this year I fixed a different quirk in the same
> machine[1] but luckily there was a way I could apply the fix without
> waiting for the patch to be merged and pulled downstream. In my case
> I could use ALSA features to make my machine work before I got a patched
> kernel.
>
> Maybe both?
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ea24b9953bcd3889f77a66e7f1d7e86e995dd9c3
> [2] https://bugzilla.kernel.org/show_bug.cgi?id=217008

Re: [BUG] hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22

[James Seo]

On Tue, Oct 31, 2023 at 11:34:16PM +0100, Armin Wolf wrote:
> Am 31.10.23 um 22:07 schrieb Lukasz Stelmach:
> 
>> It was <2023-10-31 wto 12:28>, when Guenter Roeck wrote:
>>> On 10/31/23 12:07, Lukasz Stelmach wrote:
>>> 
>>> [ ... ]
>>> 
>>>>> For what it's worth, I personally don't see much value in doing much
>>>>> more than a machine-limited workaround for now. To me it's clear that
>>>>> this UTF-16 corner case is a BIOS bug and its consequences are minimal
>>>>> once a workaround is in place.
>>>>> 
>>>>> Thoughts?
> 
> I think this is no BIOS bug, but valid behavior since the Windows ACPI-WMI mapper
> converts the ACPI objects into a common buffer format as described here:
> 
> https://learn.microsoft.com/en-us/windows-hardware/drivers/kernel/driver-defined-wmi-data-items
> 
Hi Armin,

I did see this link when you mentioned it earlier, and I understand that it's
specifying the packed and internally aligned buffer format that WMI on Windows
expects when a Windows driver provides a WMI data block.

This to me is a different question from whether an ACPI object in the BIOS,
one which will be converted to a WMI object by Windows later, should contain
UTF-16. I didn't find a single other example in all the ACPI dumps in the Linux
Hardware Database [1] of such an ACPI object.

So the answer to the question seems like a "SHOULD NOT". And someone at HP
definitely did a bad copy-paste when it came to this BIOS. I feel comfortable
calling it a bug (the leading "4" makes it one in any case).

> I assume that the mapper converts the ACPI string into a WMI string buffer, and that
> a common ACPI buffer is just passed as-is. In this case, the error lies inside the
> linux WMI subsystem, which does not do such a conversion.
>
> I can try to find out more about this conversion and its rules, and use this to add
> support for that to the WMI subsystem. This would prevent such errors in the future
> and would bring us closer to full ACPI WMI support inside the kernel.

Yes, if the ACPI-WMI mapping driver handles already existing UTF-16 in an ACPI
buffer as we have here, it would be good for us to support that as well.
Hopefully Łukasz can find a Windows machine to help determine if it does.

Earlier, you mentioned converting an ACPI object into the packed buffer format
that Windows expects. But is there some reason I'm missing for us to also pack
things like that in the first place? I assume that this format exists for
convenience (returning multiple values) or space reasons, and such a WMI
buffer is eventually unpacked into its various components according to the MOF
anyway. At least for WMI drivers on Linux, I think it would make more sense to
transparently convert the UTF-16 string to UTF-8 and pretend that the property
was an ACPI_TYPE_STRING all along.

And now I'm thinking out loud, but if WMI doesn't allow arbitrary binary data
(and from the WMI buffer spec you linked, it doesn't), and the Windows ACPI-WMI
mapper can indeed handle UTF-16, then ACPI_TYPE_BUFFER in ACPI objects intended
to become WMI objects can only contain UTF-16.

> This will take quite some time however, so i would suggest adding a quirk handler
> first and replace this with the WMI conversion functions later.

No worries.

[1] https://github.com/linuxhw/ACPI/

Re: [BUG] hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22

[Armin Wolf]

Am 01.11.23 um 05:34 schrieb James Seo:

> On Tue, Oct 31, 2023 at 11:34:16PM +0100, Armin Wolf wrote:
>> Am 31.10.23 um 22:07 schrieb Lukasz Stelmach:
>>
>>> It was <2023-10-31 wto 12:28>, when Guenter Roeck wrote:
>>>> On 10/31/23 12:07, Lukasz Stelmach wrote:
>>>>
>>>> [ ... ]
>>>>
>>>>>> For what it's worth, I personally don't see much value in doing much
>>>>>> more than a machine-limited workaround for now. To me it's clear that
>>>>>> this UTF-16 corner case is a BIOS bug and its consequences are minimal
>>>>>> once a workaround is in place.
>>>>>>
>>>>>> Thoughts?
>> I think this is no BIOS bug, but valid behavior since the Windows ACPI-WMI mapper
>> converts the ACPI objects into a common buffer format as described here:
>>
>> https://learn.microsoft.com/en-us/windows-hardware/drivers/kernel/driver-defined-wmi-data-items
>>
> Hi Armin,
>
> I did see this link when you mentioned it earlier, and I understand that it's
> specifying the packed and internally aligned buffer format that WMI on Windows
> expects when a Windows driver provides a WMI data block.
>
> This to me is a different question from whether an ACPI object in the BIOS,
> one which will be converted to a WMI object by Windows later, should contain
> UTF-16. I didn't find a single other example in all the ACPI dumps in the Linux
> Hardware Database [1] of such an ACPI object.
>
> So the answer to the question seems like a "SHOULD NOT". And someone at HP
> definitely did a bad copy-paste when it came to this BIOS. I feel comfortable
> calling it a bug (the leading "4" makes it one in any case).

Not exactly, the leading "4" is a hack:

The string "4BIOS Configuration Change" get converted to "\u0034 \u0042 \u0049 \u004f \u0053 \u0020 \u0043 \u006f \u006e \u0066 \u0069 \u0067
\u0075 \u0072 \u0061 \u0074 \u0069 \u006f \u006e \u0020 \u0043 \u0068 \u0061 \u006e \u0067 \u0065 \u0000",
so without the leading "4", the utf16 string is 52 bytes long.

Since WMI want the utf16 string prefixed with its length (52 = 0x34), the leading "4" was added
since the letter "4" gets converted to \u0034 (0x34 = 52).

So for WMI, the leading "4" gets interpreted as the length of the following utf16 string,
so it is not displayed to WMI data consumers.

>
>> I assume that the mapper converts the ACPI string into a WMI string buffer, and that
>> a common ACPI buffer is just passed as-is. In this case, the error lies inside the
>> linux WMI subsystem, which does not do such a conversion.
>>
>> I can try to find out more about this conversion and its rules, and use this to add
>> support for that to the WMI subsystem. This would prevent such errors in the future
>> and would bring us closer to full ACPI WMI support inside the kernel.
> Yes, if the ACPI-WMI mapping driver handles already existing UTF-16 in an ACPI
> buffer as we have here, it would be good for us to support that as well.
> Hopefully Łukasz can find a Windows machine to help determine if it does.
>
> Earlier, you mentioned converting an ACPI object into the packed buffer format
> that Windows expects. But is there some reason I'm missing for us to also pack
> things like that in the first place? I assume that this format exists for
> convenience (returning multiple values) or space reasons, and such a WMI
> buffer is eventually unpacked into its various components according to the MOF
> anyway. At least for WMI drivers on Linux, I think it would make more sense to
> transparently convert the UTF-16 string to UTF-8 and pretend that the property
> was an ACPI_TYPE_STRING all along.
>
> And now I'm thinking out loud, but if WMI doesn't allow arbitrary binary data
> (and from the WMI buffer spec you linked, it doesn't), and the Windows ACPI-WMI
> mapper can indeed handle UTF-16, then ACPI_TYPE_BUFFER in ACPI objects intended
> to become WMI objects can only contain UTF-16.

On my machine (Dell Inspiron 3505), an ACPI WMI method uses a ACPI_TYPE_BUFFER
to pass an array of u32, so assuming ACPI_TYPE_BUFFER == utf16 is false.

This shows why linux should do this preprocessing too, because WMI marshals
the _converted_ buffer using the MOF definitions, not the ACPI bobject.

According to Microsoft documentation, the ACPI-WMI mapper converts the ACPI object
without having _any_ knowledge of the corresponding MOF definitions. This means
that after conversion, the contents of the WMI buffer are only "normalized" so
the WMI core does not have to know about ACPI data types.

If linux would also do this "normalization" step, then we would also be able to
operate on this "normalized" WMI buffer without relying on the ACPI data types,
which (like in this case) can use different ways to express the same WMI data
(string as ACPI string vs string as utf16 buffer).

Armin Wolf

>> This will take quite some time however, so i would suggest adding a quirk handler
>> first and replace this with the WMI conversion functions later.
> No worries.
>
> [1] https://github.com/linuxhw/ACPI/

Re: [BUG] hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22

[James Seo]

On Thu, Nov 02, 2023 at 02:11:49AM +0100, Armin Wolf wrote:
> Am 01.11.23 um 05:34 schrieb James Seo:
> 
>> On Tue, Oct 31, 2023 at 11:34:16PM +0100, Armin Wolf wrote:
>>> Am 31.10.23 um 22:07 schrieb Lukasz Stelmach:
>>> 
>>>> It was <2023-10-31 wto 12:28>, when Guenter Roeck wrote:
>>>>> On 10/31/23 12:07, Lukasz Stelmach wrote:
>>>>> 
>>>>> [ ... ]
>>>>> 
>>>>>>> For what it's worth, I personally don't see much value in doing much
>>>>>>> more than a machine-limited workaround for now. To me it's clear that
>>>>>>> this UTF-16 corner case is a BIOS bug and its consequences are minimal
>>>>>>> once a workaround is in place.
>>>>>>> 
>>>>>>> Thoughts?
>>> I think this is no BIOS bug, but valid behavior since the Windows ACPI-WMI mapper
>>> converts the ACPI objects into a common buffer format as described here:
>>> 
>>> https://learn.microsoft.com/en-us/windows-hardware/drivers/kernel/driver-defined-wmi-data-items
>>> 
>> Hi Armin,
>> 
>> I did see this link when you mentioned it earlier, and I understand that it's
>> specifying the packed and internally aligned buffer format that WMI on Windows
>> expects when a Windows driver provides a WMI data block.
>> 
>> This to me is a different question from whether an ACPI object in the BIOS,
>> one which will be converted to a WMI object by Windows later, should contain
>> UTF-16. I didn't find a single other example in all the ACPI dumps in the Linux
>> Hardware Database [1] of such an ACPI object.
>> 
>> So the answer to the question seems like a "SHOULD NOT". And someone at HP
>> definitely did a bad copy-paste when it came to this BIOS. I feel comfortable
>> calling it a bug (the leading "4" makes it one in any case).
> 
> Not exactly, the leading "4" is a hack:
> 
> The string "4BIOS Configuration Change" get converted to "\u0034 \u0042 \u0049 \u004f \u0053 \u0020 \u0043 \u006f \u006e \u0066 \u0069 \u0067
> \u0075 \u0072 \u0061 \u0074 \u0069 \u006f \u006e \u0020 \u0043 \u0068 \u0061 \u006e \u0067 \u0065 \u0000",
> so without the leading "4", the utf16 string is 52 bytes long.
> 
> Since WMI want the utf16 string prefixed with its length (52 = 0x34), the leading "4" was added
> since the letter "4" gets converted to \u0034 (0x34 = 52).
> 
> So for WMI, the leading "4" gets interpreted as the length of the following utf16 string,
> so it is not displayed to WMI data consumers.

I see. So the ACPI-WMI mapper doesn't really "handle" UTF-16, then. Not in the
same way it does UTF-8.

I can't remember where, but I know I've dealt with these length-prefixed
UTF-16 strings before when touching other Microsoft offerings. So maybe this
string was originally inserted as 'Buffer (0x00,0x34,0x00,0x42,...0x00,0x00)'
and 'Unicode ("4BIOS...")' [1] is just how iasl decompiles it.

Either way, someone at HP realized all of this, and instead of just changing
this one string back to a regular string like every other similar string in
the same BIOS - and probably every other BIOS they'd ever seen - kept it.

On purpose.

Sigh.

>> And now I'm thinking out loud, but if WMI doesn't allow arbitrary binary data
>> (and from the WMI buffer spec you linked, it doesn't), and the Windows ACPI-WMI
>> mapper can indeed handle UTF-16, then ACPI_TYPE_BUFFER in ACPI objects intended
>> to become WMI objects can only contain UTF-16.
> 
> On my machine (Dell Inspiron 3505), an ACPI WMI method uses a ACPI_TYPE_BUFFER
> to pass an array of u32, so assuming ACPI_TYPE_BUFFER == utf16 is false.
> 
> This shows why linux should do this preprocessing too, because WMI marshals
> the _converted_ buffer using the MOF definitions, not the ACPI bobject.
> 
> According to Microsoft documentation, the ACPI-WMI mapper converts the ACPI object
> without having _any_ knowledge of the corresponding MOF definitions. This means
> that after conversion, the contents of the WMI buffer are only "normalized" so
> the WMI core does not have to know about ACPI data types.
 
Interesting. These HP machines represent a WMI array of strings as individual ACPI
String elements in an ACPI Package, so I assumed arrays of other types would be
individual elements as well.

> If linux would also do this "normalization" step, then we would also be able to
> operate on this "normalized" WMI buffer without relying on the ACPI data types,
> which (like in this case) can use different ways to express the same WMI data
> (string as ACPI string vs string as utf16 buffer).

Thanks for your work on the WMI subsystem side. Looking forward to when this,
BMOF decoding, and an unmarshaling mechanism are in the kernel and drivers can
work with WMI data types only.

[1] https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/19_ASL_Reference/ACPI_Source_Language_Reference.html#unicode-string-to-unicode-conversion-macro

Re: [BUG] hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22

[James Seo]

On Tue, Oct 31, 2023 at 10:07:26PM +0100, Lukasz Stelmach wrote:
> It was <2023-10-31 wto 12:28>, when Guenter Roeck wrote:
>> On 10/31/23 12:07, Lukasz Stelmach wrote:
>>
>> [ ... ]
>>
>>>> For what it's worth, I personally don't see much value in doing much
>>>> more than a machine-limited workaround for now. To me it's clear that
>>>> this UTF-16 corner case is a BIOS bug and its consequences are minimal
>>>> once a workaround is in place.
>>>>
>>>> Thoughts?
>>> I am no expert regarding x86 platforms and I don't know how often
>>> bugs
>>> like this happen and if making it more generic makes sens. Maybe.
>>> 
>>
>> That really depends on the system vendor, less on the CPU architecture.
> 
> Of course it's not architecture but rather vendor landscape.  My point
> is that most embedded platforms I work with can be fixed at this level
> by patching device-tree, which is much easier (at least for me) than
> patching BIOS/UEFI. And I've seen a number of broken BIOS-es over years
> which vendors didn't care to fix. At the end of the day my *impression*
> is that x86 platform users more often have to live with quirks like this
> that require fixes at higher levels. But this is just my impression.

Must be nice being able to patch this sort of thing in devicetree.

>>> My solution would be to add a module option, let's name it `quirks` and
>>> make it a bit field for future use, that enables the workaound. Plus an
>>> additional error message when probe fails to suggest user to add the
>>> option to kernel command line or whatever file that contains module
>>> options. A nice touch would be to detect if the workaround is still
>>> required.
>>> 
>>
>> Please no module option. Use DMI data or similar.
> 
> DMI data is fine when can you identify broken systems upfront. In this
> case we don't know which systems are or will be affected by this bug.

This particular bug seems extremely rare in general, which means I'm further
inclined towards treating it as a one-off. As Günter said, we can always add
more later.

Can you provide the output of `dmidecode -s baseboard-product-name` for now?

Re: [BUG] hp-wmi-sensors: probe of 8F1F6435-9F42-42C8-BADC-0E9424F20C9A failed with error -22

[Lukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 1372 bytes --]

It was <2023-10-31 wto 22:52>, when James Seo wrote:
> On Tue, Oct 31, 2023 at 10:07:26PM +0100, Lukasz Stelmach wrote:
>> It was <2023-10-31 wto 12:28>, when Guenter Roeck wrote:
>>> On 10/31/23 12:07, Lukasz Stelmach wrote:
[...]
>>>> My solution would be to add a module option, let's name it `quirks` and
>>>> make it a bit field for future use, that enables the workaound. Plus an
>>>> additional error message when probe fails to suggest user to add the
>>>> option to kernel command line or whatever file that contains module
>>>> options. A nice touch would be to detect if the workaround is still
>>>> required.
>>>> 
>>>
>>> Please no module option. Use DMI data or similar.
>> 
>> DMI data is fine when can you identify broken systems upfront. In this
>> case we don't know which systems are or will be affected by this bug.
>
> This particular bug seems extremely rare in general, which means I'm further
> inclined towards treating it as a one-off. As Günter said, we can always add
> more later.
>
> Can you provide the output of `dmidecode -s baseboard-product-name` for now?

--8<---------------cut here---------------start------------->8---
# dmidecode -s baseboard-product-name
870C
--8<---------------cut here---------------end--------------->8---

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]