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

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

[Daniel Bertalan]

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

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

[Hans de Goede]

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.

Mark Pearson, do you have any remarks on this ?

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

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

[Hans de Goede]

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

Regards,

Hans



>> Mark Pearson, do you have any remarks on this ?
>>
>> 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();

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

[Daniel Bertalan]

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.

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?

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.

All the best,

Daniel

> Regards,
> 
> Hans
> 
> 
> > > Mark Pearson, do you have any remarks on this ?
> > > 
> > > 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();

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

[Hans de Goede]

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

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

[Mark Pearson]

On Mon, Apr 17, 2023, at 6:19 AM, Hans de Goede wrote:
> 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.
>

Apologies - I thought I had already replied to thread, but seems I hadn't.

I'm checking with the FW team and will see what I can find out. It may take a little while, especially as this is an older platform and the question is a bit more non-standard than normal.
Internal ticket is LO-2411

Thanks!
Mark

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

[Mark Pearson]

> Subject: [External] Re: [ibm-acpi-devel] [PATCH] platform/x86: 
> thinkpad_acpi: Fix Embedded Controller access on X380 Yoga
>
> 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.
>
Sorry for the slow reply on this one.

I checked with the FW team and they confirmed on the x380 Yoga that the implementation is unique and not using the ECRD/WCWR ACPI methods. They didn't say why...but it's not expected to be something done again.

I had missed the question about fan control so didn't ask about that. Is there a reason you need to change the fans? It's generally not recommended as it can violate temperature specs and leads to all sorts of issues.

I don't know if the fact this is a one off makes this a better candidate as a quirk? To reduce the risk of breaking something on other platforms? But the code changes looked sensible to me.

I'll aim to do some builds with it in and test it on a few platforms - but I'm travelling next week so this week (and almost certainly the week after) are a bit hectic.

Thanks
Mark

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

[Hans de Goede]

Hi,

On 4/18/23 15:16, Mark Pearson wrote:
>> Subject: [External] Re: [ibm-acpi-devel] [PATCH] platform/x86: 
>> thinkpad_acpi: Fix Embedded Controller access on X380 Yoga
>>
>> 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.
>>
> Sorry for the slow reply on this one.
> 
> I checked with the FW team and they confirmed on the x380 Yoga that the implementation is unique and not using the ECRD/WCWR ACPI methods. They didn't say why...but it's not expected to be something done again.
> 
> I had missed the question about fan control so didn't ask about that. Is there a reason you need to change the fans? It's generally not recommended as it can violate temperature specs and leads to all sorts of issues.
> 
> I don't know if the fact this is a one off makes this a better candidate as a quirk? To reduce the risk of breaking something on other platforms? But the code changes looked sensible to me.
> 
> I'll aim to do some builds with it in and test it on a few platforms - but I'm travelling next week so this week (and almost certainly the week after) are a bit hectic.

I just remembered that making thinkpad_acpi fan-control
actually requires passing a module-parameter, because as
said generally speaking leaving the fan on auto mode is best.

I wonder if that parameter was set when testing on
the X380 ?  I guess it was set since the actual
register was inspected and the changes were visible
there, right ?

Regards,

Hans

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

[Daniel Bertalan]

Hi Mark and Hans,

please see my reply inline.

On Tuesday, April 18th, 2023 at 15:23, Hans de Goede wrote:


> On 4/18/23 15:16, Mark Pearson wrote:
>
> > 
> > Sorry for the slow reply on this one.
> > 
> > I checked with the FW team and they confirmed on the x380 Yoga that the implementation is unique and not using the ECRD/WCWR ACPI methods. They didn't say why...but it's not expected to be something done again.

Thank you and the FW team for the quick reply! As a first time
kernel hacker, I find my experience top-notch :)

> > I had missed the question about fan control so didn't ask about that. Is there a reason you need to change the fans? It's generally not recommended as it can violate temperature specs and leads to all sorts of issues.

My end goal is to hopefully get a bit more performance out of
my machine by making the fan kick in earlier using Thinkfan.

I have been doing a lot of compile-heavy development lately on
my X380 Yoga, and I noticed that the CPU will throttle down to
around 1.4 GHz at full load. This might just be normal for such
a small ultrabook, but I want to get the most out of it, as I'm
likely going to be stuck with this laptop for my first year of
CS studies.

Additionally, I remember the fans being louder at full load back
when I used Windows. With this patch, I can see that the highest
it gets is 4800 RPM. My memory might just be wrong though, and if so,
I apologize for wasting your time.

I have set "Adaptive Thermal Management" to "Maximize Performance"
in the BIOS, but I don't see a significant improvement.

> > I don't know if the fact this is a one off makes this a better candidate as a quirk? To reduce the risk of breaking something on other platforms? But the code changes looked sensible to me.
> >
> > I'll aim to do some builds with it in and test it on a few platforms - but I'm travelling next week so this week (and almost certainly the week after) are a bit hectic.

Thank you again. In no way is this patch urgent, so feel free to
take your time.

> 
> 
> I just remembered that making thinkpad_acpi fan-control
> actually requires passing a module-parameter, because as
> said generally speaking leaving the fan on auto mode is best.
> 
> I wonder if that parameter was set when testing on
> the X380 ? I guess it was set since the actual
> register was inspected and the changes were visible
> there, right ?

This is correct, I did set fan_control=1 for my testing.
Writes to /proc/acpi/ibm/fan are rejected otherwise.

> 
> Regards,
> 
> Hans

Regards,

Daniel

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

[Hans de Goede]

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>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

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