Re: [PATCH] platform/x86: thinkpad_acpi: Fix Embedded Controller access on X380 Yoga

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 4/15/23 16:22, Daniel Bertalan wrote:
> Hi,
> 
> On Saturday, April 15th, 2023 at 15:30, Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Hi,
>>
>> On 4/15/23 15:24, Liav Albani wrote:
>>
>>> On 4/15/23 13:12, Hans de Goede wrote:
>>>
>>>> Hi,
>>>>
>>>> On 4/14/23 20:02, Daniel Bertalan wrote:
>>>>
>>>>> On the X380 Yoga, the `ECRD` and `ECWR` ACPI objects cannot be used for
>>>>> accessing the Embedded Controller: instead of a method that reads from
>>>>> the EC's memory, `ECRD` is the name of a location in high memory. This
>>>>> meant that trying to call them would fail with the following message:
>>>>>
>>>>> ACPI: \_SB.PCI0.LPCB.EC.ECRD: 1 arguments were passed to a non-method
>>>>> ACPI object (RegionField)
>>>>>
>>>>> With this commit, it is now possible to access the EC and read
>>>>> temperature and fan speed information. Note that while writes to the
>>>>> HFSP register do go through (as indicated by subsequent reads showing
>>>>> the new value), the fan does not actually change its speed.
>>>>>
>>>>> Signed-off-by: Daniel Bertalan dani@danielbertalan.dev
>>>>> Interestig, this looks like a pretty clean solution to me.
>>>
>>> Daniel and I have looked in the DSDT ASL code and found a bunch of registers in high physical memory location (which is an ACPI OpRegion),
>>> and one of the registers had a bit called ECRD.
>>> However, there were many other registers that might be interesting as well, the problem is the short names in the ASL code (so we only see abbreviations essentially).
>>>
>>> While I do agree that adding this code is indeed a clean solution, if you (people that are dealing with Thinkpad laptops) know about cleaner way to access the embedded controller, I think it's preferable, because this way Daniel might be able to trigger the fan on that laptop so it will actually spin and will dissipate-out heat from the system, without the relying on the embedded controller to get into some sort of thermal state and then to trigger the fan.
>>
>>
>> Have you tried falling back to the ec_read() and ec_write() helpers
>> exported by the ACPI EC code ?
>>
>> The "first_ec" pointer used by these functions is exported too,
>> so you could try modifying thinkpad_acpi to use ec_read() and
>> ec_write() as fallback when first_ec is set (and the quirk
>> added by this patch triggers).
> 
> This is basically what my patch does. If the ECRD/ECWR method handles
> are NULL, thinkpad_acpi's acpi_ec_{read,write} functions fall back to
> the regular ACPI EC helpers you mentioned.

Ah I did not realize that. Ok that sounds good to me.

I'll go and apply the patch then. To be on the save side I'm going
to only add this to -next, so that it gets some testing before
showing up in stable series. Once 6.4-rc1 is out you can then
send it to stable@vger.kernel.org to get it backported.

> Speaking of which, does anyone know why these private helper functions
> exist in the first place? The code seems to use them interchangeably.
> Even in the fan control code, there are places where the regular EC
> helpers are called directly. Can we get away with always doing that?

I assume that on some older models there is no standard ACPI EC device
in the ACPI tables, so there only ECRD/ECWR work. I guess that code-paths
which directly call ec_read() / ec_write() are only used on newer
models. But this does indeed seem inconsistent.

> Back to the issue at hand, is there someone we could ask if the X380Y
> even supports manual fan control in the first place? My debugging
> efforts are starting to look like a wild goose chase.
> 
> The thermal sensors and fan PWM readings now work, which is better
> than nothing, but it would be good to get to the bottom of this.

Mark Pearson from Lenovo can hopefully help answer this, but I know
that he is quite busy atm. Hopefully Mark will get back to you when
he has some more bandwidth.

Regards,

Hans



>>>>> ---
>>>>> drivers/platform/x86/thinkpad_acpi.c | 16 ++++++++++++++++
>>>>> 1 file changed, 16 insertions(+)
>>>>>
>>>>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>>>>> index 7191ff2625b1..6fe82f805ea8 100644
>>>>> --- a/drivers/platform/x86/thinkpad_acpi.c
>>>>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>>>>> @@ -11699,6 +11699,7 @@ static int __init thinkpad_acpi_module_init(void)
>>>>> {
>>>>> const struct dmi_system_id *dmi_id;
>>>>> int ret, i;
>>>>> + acpi_object_type obj_type;
>>>>>
>>>>> tpacpi_lifecycle = TPACPI_LIFE_INIT;
>>>>>
>>>>> @@ -11724,6 +11725,21 @@ static int __init thinkpad_acpi_module_init(void)
>>>>> TPACPI_ACPIHANDLE_INIT(ecrd);
>>>>> TPACPI_ACPIHANDLE_INIT(ecwr);
>>>>>
>>>>> + /*
>>>>> + * Quirk: in some models (e.g. X380 Yoga), an object named ECRD
>>>>> + * exists, but it is a register, not a method.
>>>>> + */
>>>>> + if (ecrd_handle) {
>>>>> + acpi_get_type(ecrd_handle, &obj_type);
>>>>> + if (obj_type != ACPI_TYPE_METHOD)
>>>>> + ecrd_handle = NULL;
>>>>> + }
>>>>> + if (ecwr_handle) {
>>>>> + acpi_get_type(ecwr_handle, &obj_type);
>>>>> + if (obj_type != ACPI_TYPE_METHOD)
>>>>> + ecwr_handle = NULL;
>>>>> + }
>>>>> +
>>>>> tpacpi_wq = create_singlethread_workqueue(TPACPI_WORKQUEUE_NAME);
>>>>> if (!tpacpi_wq) {
>>>>> thinkpad_acpi_module_exit();
>