ibm-acpi-devel — ibm-acpi Linux driver development

[ibm-acpi-devel] [PATCH 1/2] platform/x86: thinkpad_acpi: take mutex for hotkey_mask_{set, get}

[Thomas W. <li...@we...>]

hotkey_mask_set() and hotkey_mask_get() expect hotkey_mutex to be held.
While it should not matter during initialization a following patch will
enable lockdep for hotkey_mutex which would produce warnings here.

Signed-off-by: Thomas Weißschuh <li...@we...>
---
 drivers/platform/x86/thinkpad_acpi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 187018ffb068..b796e65c99e0 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3478,7 +3478,9 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 	if (tp_features.hotkey_mask) {
 		/* hotkey_source_mask *must* be zero for
 		 * the first hotkey_mask_get to return hotkey_orig_mask */
+		mutex_lock(&hotkey_mutex);
 		res = hotkey_mask_get();
+		mutex_unlock(&hotkey_mutex);
 		if (res)
 			return res;
 
@@ -3577,9 +3579,11 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 		hotkey_exit();
 		return res;
 	}
+	mutex_lock(&hotkey_mutex);
 	res = hotkey_mask_set(((hotkey_all_mask & ~hotkey_reserved_mask)
 			       | hotkey_driver_mask)
 			      & ~hotkey_source_mask);
+	mutex_unlock(&hotkey_mutex);
 	if (res < 0 && res != -ENXIO) {
 		hotkey_exit();
 		return res;

-- 
2.41.0

[ibm-acpi-devel] [PATCH 2/2] platform/x86: thinkpad_acpi: use lockdep annotations

[Thomas W. <li...@we...>]

Instead of relying on free-form comments to document locking
requirements use well-known lockdep annotations.
These can also be validated at runtime make sure the invariants are not
violated.

Signed-off-by: Thomas Weißschuh <li...@we...>
---
 drivers/platform/x86/thinkpad_acpi.c | 41 +++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index b796e65c99e0..25cc86a4a36a 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -50,6 +50,7 @@
 #include <linux/kthread.h>
 #include <linux/leds.h>
 #include <linux/list.h>
+#include <linux/lockdep.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/nvram.h>
@@ -2071,11 +2072,11 @@ static int hotkey_get_tablet_mode(int *status)
  * hotkey_acpi_mask accordingly.  Also resets any bits
  * from hotkey_user_mask that are unavailable to be
  * delivered (shadow requirement of the userspace ABI).
- *
- * Call with hotkey_mutex held
  */
 static int hotkey_mask_get(void)
 {
+	lockdep_assert_held(&hotkey_mutex);
+
 	if (tp_features.hotkey_mask) {
 		u32 m = 0;
 
@@ -2111,8 +2112,6 @@ static void hotkey_mask_warn_incomplete_mask(void)
  * Also calls hotkey_mask_get to update hotkey_acpi_mask.
  *
  * NOTE: does not set bits in hotkey_user_mask, but may reset them.
- *
- * Call with hotkey_mutex held
  */
 static int hotkey_mask_set(u32 mask)
 {
@@ -2121,6 +2120,8 @@ static int hotkey_mask_set(u32 mask)
 
 	const u32 fwmask = mask & ~hotkey_source_mask;
 
+	lockdep_assert_held(&hotkey_mutex);
+
 	if (tp_features.hotkey_mask) {
 		for (i = 0; i < 32; i++) {
 			if (!acpi_evalf(hkey_handle,
@@ -2152,13 +2153,13 @@ static int hotkey_mask_set(u32 mask)
 
 /*
  * Sets hotkey_user_mask and tries to set the firmware mask
- *
- * Call with hotkey_mutex held
  */
 static int hotkey_user_mask_set(const u32 mask)
 {
 	int rc;
 
+	lockdep_assert_held(&hotkey_mutex);
+
 	/* Give people a chance to notice they are doing something that
 	 * is bound to go boom on their users sooner or later */
 	if (!tp_warned.hotkey_mask_ff &&
@@ -2519,21 +2520,23 @@ static int hotkey_kthread(void *data)
 	return 0;
 }
 
-/* call with hotkey_mutex held */
 static void hotkey_poll_stop_sync(void)
 {
+	lockdep_assert_held(&hotkey_mutex);
+
 	if (tpacpi_hotkey_task) {
 		kthread_stop(tpacpi_hotkey_task);
 		tpacpi_hotkey_task = NULL;
 	}
 }
 
-/* call with hotkey_mutex held */
 static void hotkey_poll_setup(const bool may_warn)
 {
 	const u32 poll_driver_mask = hotkey_driver_mask & hotkey_source_mask;
 	const u32 poll_user_mask = hotkey_user_mask & hotkey_source_mask;
 
+	lockdep_assert_held(&hotkey_mutex);
+
 	if (hotkey_poll_freq > 0 &&
 	    (poll_driver_mask ||
 	     (poll_user_mask && tpacpi_inputdev->users > 0))) {
@@ -2562,9 +2565,10 @@ static void hotkey_poll_setup_safe(const bool may_warn)
 	mutex_unlock(&hotkey_mutex);
 }
 
-/* call with hotkey_mutex held */
 static void hotkey_poll_set_freq(unsigned int freq)
 {
+	lockdep_assert_held(&hotkey_mutex);
+
 	if (!freq)
 		hotkey_poll_stop_sync();
 
@@ -6664,12 +6668,13 @@ static unsigned int brightness_enable = 2; /* 2 = auto, 0 = no, 1 = yes */
 
 static struct mutex brightness_mutex;
 
-/* NVRAM brightness access,
- * call with brightness_mutex held! */
+/* NVRAM brightness access */
 static unsigned int tpacpi_brightness_nvram_get(void)
 {
 	u8 lnvram;
 
+	lockdep_assert_held(&brightness_mutex);
+
 	lnvram = (nvram_read_byte(TP_NVRAM_ADDR_BRIGHTNESS)
 		  & TP_NVRAM_MASK_LEVEL_BRIGHTNESS)
 		  >> TP_NVRAM_POS_LEVEL_BRIGHTNESS;
@@ -6717,11 +6722,12 @@ static void tpacpi_brightness_checkpoint_nvram(void)
 }
 
 
-/* call with brightness_mutex held! */
 static int tpacpi_brightness_get_raw(int *status)
 {
 	u8 lec = 0;
 
+	lockdep_assert_held(&brightness_mutex);
+
 	switch (brightness_mode) {
 	case TPACPI_BRGHT_MODE_UCMS_STEP:
 		*status = tpacpi_brightness_nvram_get();
@@ -6737,12 +6743,13 @@ static int tpacpi_brightness_get_raw(int *status)
 	}
 }
 
-/* call with brightness_mutex held! */
 /* do NOT call with illegal backlight level value */
 static int tpacpi_brightness_set_ec(unsigned int value)
 {
 	u8 lec = 0;
 
+	lockdep_assert_held(&brightness_mutex);
+
 	if (unlikely(!acpi_ec_read(TP_EC_BACKLIGHT, &lec)))
 		return -EIO;
 
@@ -6754,12 +6761,13 @@ static int tpacpi_brightness_set_ec(unsigned int value)
 	return 0;
 }
 
-/* call with brightness_mutex held! */
 static int tpacpi_brightness_set_ucmsstep(unsigned int value)
 {
 	int cmos_cmd, inc;
 	unsigned int current_value, i;
 
+	lockdep_assert_held(&brightness_mutex);
+
 	current_value = tpacpi_brightness_nvram_get();
 
 	if (value == current_value)
@@ -8208,11 +8216,10 @@ static bool fan_select_fan2(void)
 	return true;
 }
 
-/*
- * Call with fan_mutex held
- */
 static void fan_update_desired_level(u8 status)
 {
+	lockdep_assert_held(&fan_mutex);
+
 	if ((status &
 	     (TP_EC_FAN_AUTO | TP_EC_FAN_FULLSPEED)) == 0) {
 		if (status > 7)

-- 
2.41.0

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

[Hans de G. <hde...@re...>]

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 <hde...@re...> 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 da...@da...v
>>>>>>> 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 st...@vg... 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

[Hans de G. <hde...@re...>]

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 <da...@da...v>

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

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

[Hans de G. <hde...@re...>]

Hi,

On 4/15/23 16:22, Daniel Bertalan wrote:
> Hi,
> 
> On Saturday, April 15th, 2023 at 15:30, Hans de Goede <hde...@re...> 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 da...@da...v
>>>>> 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 st...@vg... 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: [ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: Fix Embedded Controller access on X380 Yoga

[Hans de G. <hde...@re...>]

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 <da...@da...v>
>> 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: [ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: Fix Embedded Controller access on X380 Yoga

[Hans de G. <hde...@re...>]

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 <da...@da...v>

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: [ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: Add missing T14s Gen1 type to s2idle quirk list

[Hans de G. <hde...@re...>]

Hi Mark,

On 4/3/23 16:41, Mark Pearson wrote:
> Hi Hans
> 
> On Mon, Apr 3, 2023, at 6:03 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 4/1/23 01:24, Benjamin Asbach wrote:
>>>> Lenovo laptops that contain NVME SSDs across a variety of generations have
>>>> trouble resuming from suspend to idle when the IOMMU translation layer is
>>>> active for the NVME storage device.
>>>>
>>>> This generally manifests as a large resume delay or page faults. These
>>>> delays and page faults occur as a result of a Lenovo BIOS specific SMI
>>>> that runs during the D3->D0 transition on NVME devices.
>>>
>>> Link: https://lore.kernel.org/all/202...@am.../
>>>
>>> As Lenovo distributes T14s Gen1 laptops with different product names
>>> a missing one is added by this patch.
>>>
>>> Note: Based on lenovo support page there might be some more variants which
>>> are not represented in s2idle quirk list.
>>
>> Can you provide some more in info on this? Then Mark can maybe check
>> if we need to add more models ?
>>
>> Mark, generally speaking it may help to do a DMI_EXACT_MATCH on
>> DMI_PRODUCT_VERSION with ThinkPads ? That contains the human
>> readable model string instead of things like "20UJ", and I guess
>> that we want to e.g. apply the s2idle quirk to all "T14s Gen1 AMD"
>> ThinkPads.
> 
> Sadly that won't work :(
>  - The same ID is used for multiple platform names and those can change by geography (for instance China often calls things differently) or if WWAN supported, etc. 
>  - They use the same platform name for Intel and AMD in a few cases (not all). And this match should only be done for the AMD platforms.
> 
> For every platform there are two IDs. In this case the T14s G1 has 20UH and 20UJ. I need to figure out when each is used - I thought only the first one was in released platforms but it seems that's not the case from this patch. I need to understand how/why.
> 
> For models impacted - there are a couple missing from the list that I would expect to see there as they're the same generation: X13 G1 and L15 G2 (and a possible ? against L14/L15 G1). I'm also a bit cautious as the E-series might need to show up here - but I don't know those platforms as well..
> And depending on the two IDs...some of the platforms may need doubling up. Urgh.

Ok.

Stating the obvious here: Please send a patch adding
the necessary extra IDs once you know which ids to add.

Regards,

Hans

Re: [ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: Add missing T14s Gen1 type to s2idle quirk list

[Hans de G. <hde...@re...>]

Hi,

On 4/1/23 01:24, Benjamin Asbach wrote:
>> Lenovo laptops that contain NVME SSDs across a variety of generations have
>> trouble resuming from suspend to idle when the IOMMU translation layer is
>> active for the NVME storage device.
>>
>> This generally manifests as a large resume delay or page faults. These
>> delays and page faults occur as a result of a Lenovo BIOS specific SMI
>> that runs during the D3->D0 transition on NVME devices.
> 
> Link: https://lore.kernel.org/all/202...@am.../
> 
> As Lenovo distributes T14s Gen1 laptops with different product names
> a missing one is added by this patch.
> 
> Note: Based on lenovo support page there might be some more variants which
> are not represented in s2idle quirk list.

Can you provide some more in info on this? Then Mark can maybe check
if we need to add more models ?

Mark, generally speaking it may help to do a DMI_EXACT_MATCH on
DMI_PRODUCT_VERSION with ThinkPads ? That contains the human
readable model string instead of things like "20UJ", and I guess
that we want to e.g. apply the s2idle quirk to all "T14s Gen1 AMD"
ThinkPads.

> 
> Signed-off-by: Benjamin Asbach <asb...@im...>
> Tested-by: Benjamin Asbach <asb...@im...>

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

I will include this patch in my next fixes pull-req to Linus
for the current kernel development cycle.

Regards,

Hans





> 
> Link: https://bbs.archlinux.org/viewtopic.php?pid=2084655#p2084655
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 32c10457399e..57428d7a392d 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -4478,6 +4478,15 @@ static const struct dmi_system_id fwbug_list[] __initconst = {
>  			DMI_MATCH(DMI_PRODUCT_NAME, "20UH"),
>  		}
>  	},
> +	{
> +		.ident = "T14s Gen1 AMD",
> +		.driver_data = &quirk_s2idle_bug,
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_VENDOR, "LENOVO"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "20UJ"),
> +		}
> +	},
> +
>  	{
>  		.ident = "P14s Gen1 AMD",
>  		.driver_data = &quirk_s2idle_bug,

Re: [ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: Add missing T14s Gen1 type to s2idle quirk list

[Hans de G. <hde...@re...>]

Hi,

On 4/1/23 01:24, Benjamin Asbach wrote:
>> Lenovo laptops that contain NVME SSDs across a variety of generations have
>> trouble resuming from suspend to idle when the IOMMU translation layer is
>> active for the NVME storage device.
>>
>> This generally manifests as a large resume delay or page faults. These
>> delays and page faults occur as a result of a Lenovo BIOS specific SMI
>> that runs during the D3->D0 transition on NVME devices.
> 
> Link: https://lore.kernel.org/all/202...@am.../
> 
> As Lenovo distributes T14s Gen1 laptops with different product names
> a missing one is added by this patch.
> 
> Note: Based on lenovo support page there might be some more variants which
> are not represented in s2idle quirk list.

Can you provide some more in info on this? Then Mark can maybe check
if we need to add more models ?

Mark, generally speaking it may help to do a DMI_EXACT_MATCH on
DMI_PRODUCT_VERSION with ThinkPads ? That contains the human
readable model string instead of things like "20UJ", and I guess
that we want to e.g. apply the s2idle quirk to all "T14s Gen1 AMD"
ThinkPads.

> 
> Signed-off-by: Benjamin Asbach <asb...@im...>
> Tested-by: Benjamin Asbach <asb...@im...>

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

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

I will include this patch in my next fixes pull-req to Linus
for the current kernel development cycle.

Regards,

Hans





> 
> Link: https://bbs.archlinux.org/viewtopic.php?pid=2084655#p2084655
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 32c10457399e..57428d7a392d 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -4478,6 +4478,15 @@ static const struct dmi_system_id fwbug_list[] __initconst = {
>  			DMI_MATCH(DMI_PRODUCT_NAME, "20UH"),
>  		}
>  	},
> +	{
> +		.ident = "T14s Gen1 AMD",
> +		.driver_data = &quirk_s2idle_bug,
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_VENDOR, "LENOVO"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "20UJ"),
> +		}
> +	},
> +
>  	{
>  		.ident = "P14s Gen1 AMD",
>  		.driver_data = &quirk_s2idle_bug,

[ibm-acpi-devel] ThinkPad neo 14 (Intel): Unknown/reserved multi mode value 0x0000 for type 4

[Slide R. <Rai...@ou...>]

OS: Fedora 37
kernel: 6.1.5-200.fc37.x86_64

From `journalctl --boot=-1 -g thinkpad_acpi`:

thinkpad_acpi: ThinkPad ACPI Extras v0.26
thinkpad_acpi: http://ibm-acpi.sf.net/
thinkpad_acpi: ThinkPad BIOS R1RET46W(1.18), EC R1RHT46W
thinkpad_acpi: Lenovo ThinkPad neo 14, model 21DN0009CD
thinkpad_acpi: radio switch found; radios are enabled
thinkpad_acpi: Unknown/reserved multi mode value 0x0000 for type 4, please report this to ibm...@li...
thinkpad_acpi: Tablet mode switch found (type: GMMS), currently in laptop mode
thinkpad_acpi: This ThinkPad has standard ACPI backlight brightness control, supported by the ACPI video driver
thinkpad_acpi: Disabling thinkpad-acpi brightness events by default...
thinkpad_acpi: rfkill switch tpacpi_bluetooth_sw: radio is blocked
thinkpad_acpi: secondary fan control detected & enabled
thinkpad_acpi: battery 1 registered (start 0, stop 100, behaviours: 0x7)
input: ThinkPad Extra Buttons as /devices/platform/thinkpad_acpi/input/input15

Re: [ibm-acpi-devel] [PATCH] x86/thinkpad_acpi: Enable s2idle quirk for 21A1 machine type

[Hans de G. <hde...@re...>]

Hi,

On 11/8/22 13:47, Limonciello, Mario wrote:
> On 11/8/2022 01:20, Lennard Gäher wrote:
>> Previously, the s2idle quirk was only active for the 21A0 machine type
>> of the P14s Gen2a product. This also enables it for the second 21A1 type,
>> thus reducing wake-up times from s2idle.
>>
>> Signed-off-by: Lennard Gäher <ga...@mp...>
>> Suggested-by: Mario Limonciello <mar...@am...>
> 
> Reviewed-by: Mario Limonciello <mar...@am...>
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2181

Thank you, I've applied the patch with both tags added:

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

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

I will include this patch in my next fixes pull-req to Linus
for the current kernel development cycle.

Regards,

Hans



> 
>> ---
>>   drivers/platform/x86/thinkpad_acpi.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index 20e5c043a8e8..8476dfef4e62 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -4497,6 +4497,14 @@ static const struct dmi_system_id fwbug_list[] __initconst = {
>>               DMI_MATCH(DMI_PRODUCT_NAME, "21A0"),
>>           }
>>       },
>> +    {
>> +        .ident = "P14s Gen2 AMD",
>> +        .driver_data = &quirk_s2idle_bug,
>> +        .matches = {
>> +            DMI_MATCH(DMI_BOARD_VENDOR, "LENOVO"),
>> +            DMI_MATCH(DMI_PRODUCT_NAME, "21A1"),
>> +        }
>> +    },
>>       {}
>>   };
>>   
> 

Re: [ibm-acpi-devel] Fw: [External] [PATCH] platform/x86: thinkpad_acpi: Add fan quirk to X1 Extreme 4th Gen

[Bryan H. <bry...@gm...>]

Mark,

I rebooted back into the default Debian (sid) 6.0.6-2 linux image and I do
see the `thinkpad_acpi: secondary fan control detected & enabled` message.
Is there a recommendation from lenovo for thinkfan (
https://github.com/vmatare/thinkfan) config - or another fan control tool -
for X1 Extreme Gen 4i (20Y5007HUS)?
I have tried quite a few different configurations, and figured that maybe
the quirk needed to be specifically enabled. It either runs really hot or
sounds like a jet engine.

-Bryan

On Mon, Nov 7, 2022 at 2:09 PM Mark Pearson <mpe...@sq...>
wrote:

> Hi Bryan
>
> In theory that shouldn't be needed - I added some auto detection code to
> the driver a little while back:
>
> https://github.com/torvalds/linux/commit/bf779aaf56ea23864e39e9862b3b3a8436236e07
> And specifically removed the P1 G4 there as I'd tested it and the 2nd fan
> was auto-detected on my system.
>
> I take it that's not working on your system? Can we debug it and
> understand why? I really wanted to get away from having to add more and
> more quirks as more platforms were added.
>
> Mark
> Note - moved to a new email address as I was fedup with Outlook. I'm the
> same person as mar...@le... :)
>
>
> ------------------------------
>
> *From:* Bryan Hundven <bry...@gm...>
> *Sent:* November 7, 2022 14:16
> *Subject:* [External] [ibm-acpi-devel] [PATCH] platform/x86:
> thinkpad_acpi: Add fan quirk to X1 Extreme 4th Gen
>
> P1/X1 Extreme 4th Gen needs 2nd Controller fan quirk.
>
> Signed-off-by: Bryan Hundven <bry...@gm...>
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index 20e5c043a8e8..2c8a25950fc9 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -8825,6 +8825,7 @@ static const struct tpacpi_quirk fan_quirk_table[]
> __initconst = {
>          TPACPI_Q_LNV3('N', '2', 'N', TPACPI_FAN_2CTL),  /* P53 / P73 */
>          TPACPI_Q_LNV3('N', '2', 'E', TPACPI_FAN_2CTL),  /* P1 / X1
> Extreme (1st gen) */
>          TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2CTL),  /* P1 / X1
> Extreme (2nd gen) */
> +       TPACPI_Q_LNV3('N', '4', '0', TPACPI_FAN_2CTL),  /* P1 / X1 Extreme
> (4th gen) */
>          TPACPI_Q_LNV3('N', '3', '0', TPACPI_FAN_2CTL),  /* P15 (1st gen)
> / P15v (1st gen) */
>          TPACPI_Q_LNV3('N', '3', '7', TPACPI_FAN_2CTL),  /* T15g (2nd gen)
> */
>          TPACPI_Q_LNV3('N', '1', 'O', TPACPI_FAN_NOFAN), /* X1 Tablet (2nd
> gen) */
> --
> 2.38.1
>
>
>
>

[ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: Add fan quirk to X1 Extreme 4th Gen

[Bryan H. <bry...@gm...>]

P1/X1 Extreme 4th Gen needs 2nd Controller fan quirk.

Signed-off-by: Bryan Hundven <bry...@gm...>
---
 drivers/platform/x86/thinkpad_acpi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 20e5c043a8e8..2c8a25950fc9 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -8825,6 +8825,7 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
 	TPACPI_Q_LNV3('N', '2', 'N', TPACPI_FAN_2CTL),	/* P53 / P73 */
 	TPACPI_Q_LNV3('N', '2', 'E', TPACPI_FAN_2CTL),	/* P1 / X1 Extreme (1st gen) */
 	TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2CTL),	/* P1 / X1 Extreme (2nd gen) */
+	TPACPI_Q_LNV3('N', '4', '0', TPACPI_FAN_2CTL),  /* P1 / X1 Extreme (4th gen) */
 	TPACPI_Q_LNV3('N', '3', '0', TPACPI_FAN_2CTL),	/* P15 (1st gen) / P15v (1st gen) */
 	TPACPI_Q_LNV3('N', '3', '7', TPACPI_FAN_2CTL),  /* T15g (2nd gen) */
 	TPACPI_Q_LNV3('N', '1', 'O', TPACPI_FAN_NOFAN),	/* X1 Tablet (2nd gen) */
-- 
2.38.1

Re: [ibm-acpi-devel] [PATCH v1] platform/x86: thinkpad_acpi: use strstarts()

[Hans de G. <hde...@re...>]

Hi,

On 10/21/22 20:17, Barnabás Pőcze wrote:
> There is a function, `strstarts()`, in linux/string.h
> to check if a string is prefix of another. So remove
> the custom version from the driver.
> 
> Signed-off-by: Barnabás Pőcze <po...@pr...>

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 | 58 +++++++++++++---------------
>  1 file changed, 27 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 6a823b850a77..7a1d5402697e 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -263,9 +263,6 @@ enum tpacpi_hkey_event_t {
>  #define TPACPI_DBG_BRGHT	0x0020
>  #define TPACPI_DBG_MIXER	0x0040
> 
> -#define strlencmp(a, b) (strncmp((a), (b), strlen(b)))
> -
> -
>  /****************************************************************************
>   * Driver-wide structs and misc. variables
>   */
> @@ -1333,9 +1330,9 @@ static int tpacpi_rfk_procfs_write(const enum tpacpi_rfk_id id, char *buf)
>  		return -ENODEV;
> 
>  	while ((cmd = strsep(&buf, ","))) {
> -		if (strlencmp(cmd, "enable") == 0)
> +		if (strstarts(cmd, "enable"))
>  			status = TPACPI_RFK_RADIO_ON;
> -		else if (strlencmp(cmd, "disable") == 0)
> +		else if (strstarts(cmd, "disable"))
>  			status = TPACPI_RFK_RADIO_OFF;
>  		else
>  			return -EINVAL;
> @@ -4196,12 +4193,12 @@ static int hotkey_write(char *buf)
> 
>  	res = 0;
>  	while ((cmd = strsep(&buf, ","))) {
> -		if (strlencmp(cmd, "enable") == 0) {
> +		if (strstarts(cmd, "enable")) {
>  			hotkey_enabledisable_warn(1);
> -		} else if (strlencmp(cmd, "disable") == 0) {
> +		} else if (strstarts(cmd, "disable")) {
>  			hotkey_enabledisable_warn(0);
>  			res = -EPERM;
> -		} else if (strlencmp(cmd, "reset") == 0) {
> +		} else if (strstarts(cmd, "reset")) {
>  			mask = (hotkey_all_mask | hotkey_source_mask)
>  				& ~hotkey_reserved_mask;
>  		} else if (sscanf(cmd, "0x%x", &mask) == 1) {
> @@ -5223,33 +5220,33 @@ static int video_write(char *buf)
>  	disable = 0;
> 
>  	while ((cmd = strsep(&buf, ","))) {
> -		if (strlencmp(cmd, "lcd_enable") == 0) {
> +		if (strstarts(cmd, "lcd_enable")) {
>  			enable |= TP_ACPI_VIDEO_S_LCD;
> -		} else if (strlencmp(cmd, "lcd_disable") == 0) {
> +		} else if (strstarts(cmd, "lcd_disable")) {
>  			disable |= TP_ACPI_VIDEO_S_LCD;
> -		} else if (strlencmp(cmd, "crt_enable") == 0) {
> +		} else if (strstarts(cmd, "crt_enable")) {
>  			enable |= TP_ACPI_VIDEO_S_CRT;
> -		} else if (strlencmp(cmd, "crt_disable") == 0) {
> +		} else if (strstarts(cmd, "crt_disable")) {
>  			disable |= TP_ACPI_VIDEO_S_CRT;
>  		} else if (video_supported == TPACPI_VIDEO_NEW &&
> -			   strlencmp(cmd, "dvi_enable") == 0) {
> +			   strstarts(cmd, "dvi_enable")) {
>  			enable |= TP_ACPI_VIDEO_S_DVI;
>  		} else if (video_supported == TPACPI_VIDEO_NEW &&
> -			   strlencmp(cmd, "dvi_disable") == 0) {
> +			   strstarts(cmd, "dvi_disable")) {
>  			disable |= TP_ACPI_VIDEO_S_DVI;
> -		} else if (strlencmp(cmd, "auto_enable") == 0) {
> +		} else if (strstarts(cmd, "auto_enable")) {
>  			res = video_autosw_set(1);
>  			if (res)
>  				return res;
> -		} else if (strlencmp(cmd, "auto_disable") == 0) {
> +		} else if (strstarts(cmd, "auto_disable")) {
>  			res = video_autosw_set(0);
>  			if (res)
>  				return res;
> -		} else if (strlencmp(cmd, "video_switch") == 0) {
> +		} else if (strstarts(cmd, "video_switch")) {
>  			res = video_outputsw_cycle();
>  			if (res)
>  				return res;
> -		} else if (strlencmp(cmd, "expand_toggle") == 0) {
> +		} else if (strstarts(cmd, "expand_toggle")) {
>  			res = video_expand_toggle();
>  			if (res)
>  				return res;
> @@ -5642,9 +5639,9 @@ static int light_write(char *buf)
>  		return -ENODEV;
> 
>  	while ((cmd = strsep(&buf, ","))) {
> -		if (strlencmp(cmd, "on") == 0) {
> +		if (strstarts(cmd, "on")) {
>  			newstatus = 1;
> -		} else if (strlencmp(cmd, "off") == 0) {
> +		} else if (strstarts(cmd, "off")) {
>  			newstatus = 0;
>  		} else
>  			return -EINVAL;
> @@ -7115,10 +7112,10 @@ static int brightness_write(char *buf)
>  		return level;
> 
>  	while ((cmd = strsep(&buf, ","))) {
> -		if (strlencmp(cmd, "up") == 0) {
> +		if (strstarts(cmd, "up")) {
>  			if (level < bright_maxlvl)
>  				level++;
> -		} else if (strlencmp(cmd, "down") == 0) {
> +		} else if (strstarts(cmd, "down")) {
>  			if (level > 0)
>  				level--;
>  		} else if (sscanf(cmd, "level %d", &level) == 1 &&
> @@ -7867,13 +7864,13 @@ static int volume_write(char *buf)
> 
>  	while ((cmd = strsep(&buf, ","))) {
>  		if (!tp_features.mixer_no_level_control) {
> -			if (strlencmp(cmd, "up") == 0) {
> +			if (strstarts(cmd, "up")) {
>  				if (new_mute)
>  					new_mute = 0;
>  				else if (new_level < TP_EC_VOLUME_MAX)
>  					new_level++;
>  				continue;
> -			} else if (strlencmp(cmd, "down") == 0) {
> +			} else if (strstarts(cmd, "down")) {
>  				if (new_mute)
>  					new_mute = 0;
>  				else if (new_level > 0)
> @@ -7885,9 +7882,9 @@ static int volume_write(char *buf)
>  				continue;
>  			}
>  		}
> -		if (strlencmp(cmd, "mute") == 0)
> +		if (strstarts(cmd, "mute"))
>  			new_mute = TP_EC_AUDIO_MUTESW_MSK;
> -		else if (strlencmp(cmd, "unmute") == 0)
> +		else if (strstarts(cmd, "unmute"))
>  			new_mute = 0;
>  		else
>  			return -EINVAL;
> @@ -9110,10 +9107,9 @@ static int fan_write_cmd_level(const char *cmd, int *rc)
>  {
>  	int level;
> 
> -	if (strlencmp(cmd, "level auto") == 0)
> +	if (strstarts(cmd, "level auto"))
>  		level = TP_EC_FAN_AUTO;
> -	else if ((strlencmp(cmd, "level disengaged") == 0) ||
> -			(strlencmp(cmd, "level full-speed") == 0))
> +	else if (strstarts(cmd, "level disengaged") || strstarts(cmd, "level full-speed"))
>  		level = TP_EC_FAN_FULLSPEED;
>  	else if (sscanf(cmd, "level %d", &level) != 1)
>  		return 0;
> @@ -9131,7 +9127,7 @@ static int fan_write_cmd_level(const char *cmd, int *rc)
> 
>  static int fan_write_cmd_enable(const char *cmd, int *rc)
>  {
> -	if (strlencmp(cmd, "enable") != 0)
> +	if (!strstarts(cmd, "enable"))
>  		return 0;
> 
>  	*rc = fan_set_enable();
> @@ -9146,7 +9142,7 @@ static int fan_write_cmd_enable(const char *cmd, int *rc)
> 
>  static int fan_write_cmd_disable(const char *cmd, int *rc)
>  {
> -	if (strlencmp(cmd, "disable") != 0)
> +	if (!strstarts(cmd, "disable"))
>  		return 0;
> 
>  	*rc = fan_set_disable();
> --
> 2.38.1
> 
> 

Re: [ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: Fix a max_brightness of thinklight

[Hans de G. <hde...@re...>]

Hi Michał,

On 10/8/22 20:49, Michał Szczepaniak wrote:
> Thinklight has only two values, on/off so it's reasonable for max_brightness to be 0 and 1 as if you write anything between 0 and 255 it will be 255 anyway so there's no point for it to be 255.
> 
> Also sorry for repeated messages, I got no reply so I'm trying to figure out what I did wrong.

Your patch is missing a Signed-off-by line in the commit-message. I can only
accept patches with a Signed-off-by line in the commit-message like this:

Signed-off-by: Your Real Name <em...@yo...main>

See:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

Also your patch is malformed (it has been reflowed by your email client)
and it does not apply properly.

Since this is only a trivial single line change I've just manually recreated
the patch myself, with me as author to avoid the S-o-b problem.

I have given you credit by adding:

Reported-by: Michał Szczepaniak <m.s...@gm...>

to the commit message of the patch.

Regards,

Hans



> 
> 
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 22d4e8633e30e9..775c2f327dc3a8 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -5562,6 +5562,7 @@ static enum led_brightness light_sysfs_get(struct led_classdev *led_cdev)
>  static struct tpacpi_led_classdev tpacpi_led_thinklight = {
>      .led_classdev = {
>          .name        = "tpacpi::thinklight",
> +        .max_brightness    = 1,
>          .brightness_set_blocking = &light_sysfs_set,
>          .brightness_get    = &light_sysfs_get,
>      }
> 
> 
> 
> 

[ibm-acpi-devel] [PATCH RESEND] platform/x86: thinkpad_acpi: Fix a max_brightness of thinklight

[Michał S. <m.s...@gm...>]

Thinklight has only two values, on/off so it's reasonable for 
max_brightness to be 0 and 1 as if you write anything between 0 and 255 
it will be 255 anyway so there's no point for it to be 255.

Signed-off-by: Michał Szczepaniak<m.s...@gm...>
---
  drivers/platform/x86/thinkpad_acpi.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 6a823b850a77..4a7369868f2e 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -5562,6 +5562,7 @@ static enum led_brightness light_sysfs_get(struct led_classdev *led_cdev)
  static struct tpacpi_led_classdev tpacpi_led_thinklight = {
  	.led_classdev = {
  		.name		= "tpacpi::thinklight",
+		.max_brightness = 1,
  		.brightness_set_blocking = &light_sysfs_set,
  		.brightness_get	= &light_sysfs_get,
  	}
-- 
2.37.3

Re: [ibm-acpi-devel] [PATCH 00/22] Fallback to native backlight

[Hans de G. <hde...@re...>]

Hi,

On 10/24/22 16:31, Akihiko Odaki wrote:
> On 2022/10/24 23:06, Akihiko Odaki wrote:
>> On 2022/10/24 22:21, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 10/24/22 14:58, Akihiko Odaki wrote:
>>>> On 2022/10/24 20:53, Hans de Goede wrote:
>>>>> Hi Akihiko,
>>>>>
>>>>> On 10/24/22 13:34, Akihiko Odaki wrote:
>>>>>> Commit 2600bfa3df99 ("ACPI: video: Add acpi_video_backlight_use_native()
>>>>>> helper") and following commits made native backlight unavailable if
>>>>>> CONFIG_ACPI_VIDEO is set and the backlight feature of ACPI video is
>>>>>> unavailable, which broke the backlight functionality on Lenovo ThinkPad
>>>>>> C13 Yoga Chromebook. Allow to fall back to native backlight in such
>>>>>> cases.
>>>>>
>>>>> I appreciate your work on this, but what this in essence does is
>>>>> it allows 2 backlight drivers (vendor + native) to get registered
>>>>> for the same panel again. While the whole goal of the backlight refactor
>>>>> series landing in 6.1 was to make it so that there always is only
>>>>> *1* backlight device registered instead of (possibly) registering
>>>>> multiple and letting userspace figure it out. It is also important
>>>>> to only always have 1 backlight device per panel for further
>>>>> upcoming changes.
>>>>>
>>>>> So nack for this solution, sorry.
>>>>>
>>>>> I am aware that this breaks backlight control on some Chromebooks,
>>>>> this was already reported and I wrote a long reply explaining why
>>>>> things are done the way they are done now and also suggesting
>>>>> 2 possible (much simpler) fixes, see:
>>>>> https://lore.kernel.org/linux-acpi/42a...@re.../
>>>>>
>>>>> Unfortunately the reported has not followed-up on this and
>>>>> I don't have the hardware to test this myself.
>>>>>
>>>>> Can you please try implementing 1 of the fixes suggested there
>>>>> and then submit that upstream ?
>>>>>
>>>>> Regards,
>>>>>
>>>>> Hans
>>>>>
>>>>
>>>> Hi Hans,
>>>>
>>>> Thanks for reviewing and letting me know the prior attempt.
>>>>
>>>> In this case, there is only a native backlight device and no vendor backlight device so the duplication of backlight devices does not happen. I think it is better to handle such a case without quirks.
>>>
>>> Adding a single heuristic for all chromebooks is something completely different
>>> then adding per model quirks which indeed ideally should be avoided (but this
>>> is not always possible).
>>>
>>>> I understand it is still questionable to cover the case by allowing duplication when both of a vendor backlight device and native one. To explain my understanding and reasoning for *not* trying to apply the de-duplication rule to the vendor/native combination, let me first describe that the de-duplication which happens in acpi_video_get_backlight_type() is a heuristics and limited.
>>>>
>>>> As the background of acpi_video_get_backlight_type(), there is an assumption that it should be common that both of the firmware, implementing ACPI, and the kernel have code to drive backlight. In the case, the more reliable one should be picked instead of using both, and that is what the statements in `if (video_caps & ACPI_VIDEO_BACKLIGHT)` does.
>>>>
>>>> However, the method has two limitations:
>>>> 1. It does not cover the case where two backlight devices with the same type exist.
>>>
>>> This only happens when there are 2 panels; or 2 gpus driving a single panel
>>> which are both special cases where we actually want 2 backlight devices.
>>>
>>>> 2. The underlying assumption does not apply to vendor/native combination.
>>>>
>>>> Regarding the second limitation, I don't even understand the difference between vendor and native. My guess is that a vendor backlight device uses vendor-specific ACPI interface, and a native one directly uses hardware registers. If my guess is correct, the difference between vendor and native does not imply that both of them are likely to exist at the same time. As the conclusion, there is no more motivation to try to de-duplicate the vendor/native combination than to try to de-duplicate combination of devices with a single type.
>>>>
>>>> Of course, it is better if we could also avoid registering two devices with one type for one physical device. Possibly we can do so by providing a parameter to indicate that it is for the same "internal" backlight to devm_backlight_device_register(), and let the function check for the duplication. However, this rule may be too restrict, or may have problems I missed.
>>>>
>>>> Based on the discussion above, we can deduce three possibilities:
>>>> a. There is no reason to distinguish vendor and native in this case, and we can stick to my current proposal.
>>>> b. There is a valid reason to distinguish vendor and native, and we can adopt the same strategy that already adopted for ACPI video/vendor combination.
>>>> c. We can implement de-duplication in devm_backlight_device_register().
>>>> d. The other possible options are not worth, and we can just implement quirks specific to Chromebook/coreboot.
>>>>
>>>> In case b, it should be noted that vendor and native backlight device do not require ACPI video, and CONFIG_ACPI_VIDEO may not be enabled. In the case, the de-duplication needs to be implemented in backlight class device.
>>>
>>> I have been working on the ACPI/x86 backlight detection code since 2015, please trust
>>> me when I say that allowing both vendor + native backlight devices at the same time
>>> is a bad idea.
>>>
>>> I'm currently in direct contact with the ChromeOS team about fixing the Chromebook
>>> backlight issue introduced in 6.1-rc1.
>>>
>>> If you wan to help, please read:
>>>
>>> https://lore.kernel.org/linux-acpi/42a...@re.../
>>>
>>> And try implementing 1 if the 2 solutions suggested there.
>>>
>>> Regards,
>>>
>>> Hans
>>
>> Hi,
>>
>> I just wanted to confirm your intention that we should distinguish vendor and native. In the case I think it is better to modify __acpi_video_get_backlight_type() and add "native_available" check in case of no ACPI video as already done for the ACPI video/native combination.
>>
>> Unfortunately this has one pitfall though: it does not work if CONFIG_ACPI_VIDEO is disabled. If it is, acpi_video_get_backlight_type() always return acpi_backlight_vendor, and acpi_video_backlight_use_native() always returns true. It is not a regression but the current behavior. Fixing it requires also refactoring touching both of ACPI video and backlight class driver so I guess I'm not an appropriate person to do that, and I should just add "native_available" check to __acpi_video_get_backlight_type().
>>
>> Does that sound good?
> 
> Well, it would not be that easy since just adding native_available cannot handle the case where the vendor driver gets registered first. Checking with "native_available" was possible for ACPI video/vendor combination because ACPI video registers its backlight after some delay. I still think it does not overcomplicate things to modify __acpi_video_get_backlight_type() so that it can use both of vendor and native as fallback while preventing duplicate registration.

In the mean time this has already been fixed by a single patch of just a few
lines, rather then requiring 22 patches, see:

https://lore.kernel.org/dri-devel/202...@co.../

Regards,

Hans




>>>>>> Akihiko Odaki (22):
>>>>>>     drm/i915/opregion: Improve backlight request condition
>>>>>>     ACPI: video: Introduce acpi_video_get_backlight_types()
>>>>>>     LoongArch: Use acpi_video_get_backlight_types()
>>>>>>     platform/x86: acer-wmi: Use acpi_video_get_backlight_types()
>>>>>>     platform/x86: asus-laptop: Use acpi_video_get_backlight_types()
>>>>>>     platform/x86: asus-wmi: Use acpi_video_get_backlight_types()
>>>>>>     platform/x86: compal-laptop: Use acpi_video_get_backlight_types()
>>>>>>     platform/x86: eeepc-laptop: Use acpi_video_get_backlight_types()
>>>>>>     platform/x86: fujitsu-laptop: Use acpi_video_get_backlight_types()
>>>>>>     platform/x86: ideapad-laptop: Use acpi_video_get_backlight_types()
>>>>>>     platform/x86: msi-laptop: Use acpi_video_get_backlight_types()
>>>>>>     platform/x86: msi-wmi: Use acpi_video_get_backlight_types()
>>>>>>     platform/x86: nvidia-wmi-ec-backlight: Use
>>>>>>       acpi_video_get_backlight_types()
>>>>>>     platform/x86: panasonic-laptop: Use acpi_video_get_backlight_types()
>>>>>>     platform/x86: samsung-laptop: Use acpi_video_get_backlight_types()
>>>>>>     platform/x86: sony-laptop: Use acpi_video_get_backlight_types()
>>>>>>     platform/x86: thinkpad_acpi: Use acpi_video_get_backlight_types()
>>>>>>     platform/x86: toshiba_acpi: Use acpi_video_get_backlight_types()
>>>>>>     platform/x86: dell-laptop: Use acpi_video_get_backlight_types()
>>>>>>     platform/x86: intel_oaktrail: Use acpi_video_get_backlight_types()
>>>>>>     ACPI: video: Remove acpi_video_get_backlight_type()
>>>>>>     ACPI: video: Fallback to native backlight
>>>>>>
>>>>>>    Documentation/gpu/todo.rst                    |  8 +--
>>>>>>    drivers/acpi/acpi_video.c                     |  2 +-
>>>>>>    drivers/acpi/video_detect.c                   | 54 ++++++++++---------
>>>>>>    drivers/gpu/drm/i915/display/intel_opregion.c |  3 +-
>>>>>>    drivers/platform/loongarch/loongson-laptop.c  |  4 +-
>>>>>>    drivers/platform/x86/acer-wmi.c               |  2 +-
>>>>>>    drivers/platform/x86/asus-laptop.c            |  2 +-
>>>>>>    drivers/platform/x86/asus-wmi.c               |  4 +-
>>>>>>    drivers/platform/x86/compal-laptop.c          |  2 +-
>>>>>>    drivers/platform/x86/dell/dell-laptop.c       |  2 +-
>>>>>>    drivers/platform/x86/eeepc-laptop.c           |  2 +-
>>>>>>    drivers/platform/x86/fujitsu-laptop.c         |  4 +-
>>>>>>    drivers/platform/x86/ideapad-laptop.c         |  2 +-
>>>>>>    drivers/platform/x86/intel/oaktrail.c         |  2 +-
>>>>>>    drivers/platform/x86/msi-laptop.c             |  2 +-
>>>>>>    drivers/platform/x86/msi-wmi.c                |  2 +-
>>>>>>    .../platform/x86/nvidia-wmi-ec-backlight.c    |  2 +-
>>>>>>    drivers/platform/x86/panasonic-laptop.c       |  2 +-
>>>>>>    drivers/platform/x86/samsung-laptop.c         |  2 +-
>>>>>>    drivers/platform/x86/sony-laptop.c            |  2 +-
>>>>>>    drivers/platform/x86/thinkpad_acpi.c          |  4 +-
>>>>>>    drivers/platform/x86/toshiba_acpi.c           |  2 +-
>>>>>>    drivers/video/backlight/backlight.c           | 18 +++++++
>>>>>>    include/acpi/video.h                          | 21 ++++----
>>>>>>    include/linux/backlight.h                     |  1 +
>>>>>>    25 files changed, 85 insertions(+), 66 deletions(-)
>>>>>>
>>>>>
>>>>
>>>
> 

Re: [ibm-acpi-devel] [PATCH 00/22] Fallback to native backlight

[Hans de G. <hde...@re...>]

Hi,

On 10/24/22 15:14, Pali Rohár wrote:
> On Monday 24 October 2022 21:58:57 Akihiko Odaki wrote:
>> Regarding the second limitation, I don't even understand the difference
>> between vendor and native. My guess is that a vendor backlight device uses
>> vendor-specific ACPI interface, and a native one directly uses hardware
>> registers. If my guess is correct, the difference between vendor and native
>> does not imply that both of them are likely to exist at the same time. As
>> the conclusion, there is no more motivation to try to de-duplicate the
>> vendor/native combination than to try to de-duplicate combination of devices
>> with a single type.
> 
> Hello! I just want to point one thing. On some Dell laptops there are
> 3 different ways (= 3 different APIs) how to control display backlight.
> There is ACPI driver (uses ACPI), GPU/DRM driver (i915.ko; uses directly
> HW) and platform vendor driver (dell-laptop.ko; uses vendor BIOS or
> firmware API).

Right and that is just one example of laptops which can register both
vendor + native backlight devices, which is why this whole series is
a bad idea.

Regards,

Hans



> Just every driver has different pre-calculated scaling
> values. So sometimes user wants to choose different driver just because
> it allows to set backlight level with "better" granularity. Registering
> all 3 device drivers is bad as user does not want to see 3 display
> panels and forcing registration of specific one without runtime option
> is also bad (some of those drivers do not have to be suitable or has
> worse granularity as other).

Re: [ibm-acpi-devel] [PATCH 00/22] Fallback to native backlight

[Hans de G. <hde...@re...>]

Hi,

On 10/24/22 14:58, Akihiko Odaki wrote:
> On 2022/10/24 20:53, Hans de Goede wrote:
>> Hi Akihiko,
>>
>> On 10/24/22 13:34, Akihiko Odaki wrote:
>>> Commit 2600bfa3df99 ("ACPI: video: Add acpi_video_backlight_use_native()
>>> helper") and following commits made native backlight unavailable if
>>> CONFIG_ACPI_VIDEO is set and the backlight feature of ACPI video is
>>> unavailable, which broke the backlight functionality on Lenovo ThinkPad
>>> C13 Yoga Chromebook. Allow to fall back to native backlight in such
>>> cases.
>>
>> I appreciate your work on this, but what this in essence does is
>> it allows 2 backlight drivers (vendor + native) to get registered
>> for the same panel again. While the whole goal of the backlight refactor
>> series landing in 6.1 was to make it so that there always is only
>> *1* backlight device registered instead of (possibly) registering
>> multiple and letting userspace figure it out. It is also important
>> to only always have 1 backlight device per panel for further
>> upcoming changes.
>>
>> So nack for this solution, sorry.
>>
>> I am aware that this breaks backlight control on some Chromebooks,
>> this was already reported and I wrote a long reply explaining why
>> things are done the way they are done now and also suggesting
>> 2 possible (much simpler) fixes, see:
>> https://lore.kernel.org/linux-acpi/42a...@re.../
>>
>> Unfortunately the reported has not followed-up on this and
>> I don't have the hardware to test this myself.
>>
>> Can you please try implementing 1 of the fixes suggested there
>> and then submit that upstream ?
>>
>> Regards,
>>
>> Hans
>>
> 
> Hi Hans,
> 
> Thanks for reviewing and letting me know the prior attempt.
> 
> In this case, there is only a native backlight device and no vendor backlight device so the duplication of backlight devices does not happen. I think it is better to handle such a case without quirks.

Adding a single heuristic for all chromebooks is something completely different
then adding per model quirks which indeed ideally should be avoided (but this
is not always possible).

> I understand it is still questionable to cover the case by allowing duplication when both of a vendor backlight device and native one. To explain my understanding and reasoning for *not* trying to apply the de-duplication rule to the vendor/native combination, let me first describe that the de-duplication which happens in acpi_video_get_backlight_type() is a heuristics and limited.
> 
> As the background of acpi_video_get_backlight_type(), there is an assumption that it should be common that both of the firmware, implementing ACPI, and the kernel have code to drive backlight. In the case, the more reliable one should be picked instead of using both, and that is what the statements in `if (video_caps & ACPI_VIDEO_BACKLIGHT)` does.
> 
> However, the method has two limitations:
> 1. It does not cover the case where two backlight devices with the same type exist.

This only happens when there are 2 panels; or 2 gpus driving a single panel
which are both special cases where we actually want 2 backlight devices.

> 2. The underlying assumption does not apply to vendor/native combination.
> 
> Regarding the second limitation, I don't even understand the difference between vendor and native. My guess is that a vendor backlight device uses vendor-specific ACPI interface, and a native one directly uses hardware registers. If my guess is correct, the difference between vendor and native does not imply that both of them are likely to exist at the same time. As the conclusion, there is no more motivation to try to de-duplicate the vendor/native combination than to try to de-duplicate combination of devices with a single type.
> 
> Of course, it is better if we could also avoid registering two devices with one type for one physical device. Possibly we can do so by providing a parameter to indicate that it is for the same "internal" backlight to devm_backlight_device_register(), and let the function check for the duplication. However, this rule may be too restrict, or may have problems I missed.
> 
> Based on the discussion above, we can deduce three possibilities:
> a. There is no reason to distinguish vendor and native in this case, and we can stick to my current proposal.
> b. There is a valid reason to distinguish vendor and native, and we can adopt the same strategy that already adopted for ACPI video/vendor combination.
> c. We can implement de-duplication in devm_backlight_device_register().
> d. The other possible options are not worth, and we can just implement quirks specific to Chromebook/coreboot.
> 
> In case b, it should be noted that vendor and native backlight device do not require ACPI video, and CONFIG_ACPI_VIDEO may not be enabled. In the case, the de-duplication needs to be implemented in backlight class device.

I have been working on the ACPI/x86 backlight detection code since 2015, please trust
me when I say that allowing both vendor + native backlight devices at the same time
is a bad idea.

I'm currently in direct contact with the ChromeOS team about fixing the Chromebook
backlight issue introduced in 6.1-rc1.

If you wan to help, please read:

https://lore.kernel.org/linux-acpi/42a...@re.../

And try implementing 1 if the 2 solutions suggested there.

Regards,

Hans



>>> Akihiko Odaki (22):
>>>    drm/i915/opregion: Improve backlight request condition
>>>    ACPI: video: Introduce acpi_video_get_backlight_types()
>>>    LoongArch: Use acpi_video_get_backlight_types()
>>>    platform/x86: acer-wmi: Use acpi_video_get_backlight_types()
>>>    platform/x86: asus-laptop: Use acpi_video_get_backlight_types()
>>>    platform/x86: asus-wmi: Use acpi_video_get_backlight_types()
>>>    platform/x86: compal-laptop: Use acpi_video_get_backlight_types()
>>>    platform/x86: eeepc-laptop: Use acpi_video_get_backlight_types()
>>>    platform/x86: fujitsu-laptop: Use acpi_video_get_backlight_types()
>>>    platform/x86: ideapad-laptop: Use acpi_video_get_backlight_types()
>>>    platform/x86: msi-laptop: Use acpi_video_get_backlight_types()
>>>    platform/x86: msi-wmi: Use acpi_video_get_backlight_types()
>>>    platform/x86: nvidia-wmi-ec-backlight: Use
>>>      acpi_video_get_backlight_types()
>>>    platform/x86: panasonic-laptop: Use acpi_video_get_backlight_types()
>>>    platform/x86: samsung-laptop: Use acpi_video_get_backlight_types()
>>>    platform/x86: sony-laptop: Use acpi_video_get_backlight_types()
>>>    platform/x86: thinkpad_acpi: Use acpi_video_get_backlight_types()
>>>    platform/x86: toshiba_acpi: Use acpi_video_get_backlight_types()
>>>    platform/x86: dell-laptop: Use acpi_video_get_backlight_types()
>>>    platform/x86: intel_oaktrail: Use acpi_video_get_backlight_types()
>>>    ACPI: video: Remove acpi_video_get_backlight_type()
>>>    ACPI: video: Fallback to native backlight
>>>
>>>   Documentation/gpu/todo.rst                    |  8 +--
>>>   drivers/acpi/acpi_video.c                     |  2 +-
>>>   drivers/acpi/video_detect.c                   | 54 ++++++++++---------
>>>   drivers/gpu/drm/i915/display/intel_opregion.c |  3 +-
>>>   drivers/platform/loongarch/loongson-laptop.c  |  4 +-
>>>   drivers/platform/x86/acer-wmi.c               |  2 +-
>>>   drivers/platform/x86/asus-laptop.c            |  2 +-
>>>   drivers/platform/x86/asus-wmi.c               |  4 +-
>>>   drivers/platform/x86/compal-laptop.c          |  2 +-
>>>   drivers/platform/x86/dell/dell-laptop.c       |  2 +-
>>>   drivers/platform/x86/eeepc-laptop.c           |  2 +-
>>>   drivers/platform/x86/fujitsu-laptop.c         |  4 +-
>>>   drivers/platform/x86/ideapad-laptop.c         |  2 +-
>>>   drivers/platform/x86/intel/oaktrail.c         |  2 +-
>>>   drivers/platform/x86/msi-laptop.c             |  2 +-
>>>   drivers/platform/x86/msi-wmi.c                |  2 +-
>>>   .../platform/x86/nvidia-wmi-ec-backlight.c    |  2 +-
>>>   drivers/platform/x86/panasonic-laptop.c       |  2 +-
>>>   drivers/platform/x86/samsung-laptop.c         |  2 +-
>>>   drivers/platform/x86/sony-laptop.c            |  2 +-
>>>   drivers/platform/x86/thinkpad_acpi.c          |  4 +-
>>>   drivers/platform/x86/toshiba_acpi.c           |  2 +-
>>>   drivers/video/backlight/backlight.c           | 18 +++++++
>>>   include/acpi/video.h                          | 21 ++++----
>>>   include/linux/backlight.h                     |  1 +
>>>   25 files changed, 85 insertions(+), 66 deletions(-)
>>>
>>
> 

Re: [ibm-acpi-devel] [PATCH 00/22] Fallback to native backlight

[Hans de G. <hde...@re...>]

Hi,

On 10/24/22 13:56, Akihiko Odaki wrote:
> On 2022/10/24 20:48, Jani Nikula wrote:
>> On Mon, 24 Oct 2022, Akihiko Odaki <aki...@da...> wrote:
>>> Commit 2600bfa3df99 ("ACPI: video: Add acpi_video_backlight_use_native()
>>> helper") and following commits made native backlight unavailable if
>>> CONFIG_ACPI_VIDEO is set and the backlight feature of ACPI video is
>>> unavailable, which broke the backlight functionality on Lenovo ThinkPad
>>> C13 Yoga Chromebook. Allow to fall back to native backlight in such
>>> cases.
>>
>> Where's the bug report with relevant logs, kconfigs, etc?
> 
> I haven't filed one. Should I? Please tell me where to report and what information you would need (to bugzilla.kernel.org with things mentioned in Documentation/admin-guide/reporting-issues.rst?)

As mentioned in my other email this is a known issue, and your effort
to fix this is appreciated very much, but I don't believe your solution
to be the right one.

See: https://lore.kernel.org/linux-acpi/42a...@re.../
for more details and possible solutions. Please try implementing one of
those solutions for your Chromebook. I unfortunately do not have hw to
test this myself.

Regards,

Hans



> 
> Regards,
> Akihiko Odaki
> 
>>
>> BR,
>> Jani.
>>
>>>
>>> Signed-off-by: Akihiko Odaki <aki...@da...>
>>>
>>> Akihiko Odaki (22):
>>>    drm/i915/opregion: Improve backlight request condition
>>>    ACPI: video: Introduce acpi_video_get_backlight_types()
>>>    LoongArch: Use acpi_video_get_backlight_types()
>>>    platform/x86: acer-wmi: Use acpi_video_get_backlight_types()
>>>    platform/x86: asus-laptop: Use acpi_video_get_backlight_types()
>>>    platform/x86: asus-wmi: Use acpi_video_get_backlight_types()
>>>    platform/x86: compal-laptop: Use acpi_video_get_backlight_types()
>>>    platform/x86: eeepc-laptop: Use acpi_video_get_backlight_types()
>>>    platform/x86: fujitsu-laptop: Use acpi_video_get_backlight_types()
>>>    platform/x86: ideapad-laptop: Use acpi_video_get_backlight_types()
>>>    platform/x86: msi-laptop: Use acpi_video_get_backlight_types()
>>>    platform/x86: msi-wmi: Use acpi_video_get_backlight_types()
>>>    platform/x86: nvidia-wmi-ec-backlight: Use
>>>      acpi_video_get_backlight_types()
>>>    platform/x86: panasonic-laptop: Use acpi_video_get_backlight_types()
>>>    platform/x86: samsung-laptop: Use acpi_video_get_backlight_types()
>>>    platform/x86: sony-laptop: Use acpi_video_get_backlight_types()
>>>    platform/x86: thinkpad_acpi: Use acpi_video_get_backlight_types()
>>>    platform/x86: toshiba_acpi: Use acpi_video_get_backlight_types()
>>>    platform/x86: dell-laptop: Use acpi_video_get_backlight_types()
>>>    platform/x86: intel_oaktrail: Use acpi_video_get_backlight_types()
>>>    ACPI: video: Remove acpi_video_get_backlight_type()
>>>    ACPI: video: Fallback to native backlight
>>>
>>>   Documentation/gpu/todo.rst                    |  8 +--
>>>   drivers/acpi/acpi_video.c                     |  2 +-
>>>   drivers/acpi/video_detect.c                   | 54 ++++++++++---------
>>>   drivers/gpu/drm/i915/display/intel_opregion.c |  3 +-
>>>   drivers/platform/loongarch/loongson-laptop.c  |  4 +-
>>>   drivers/platform/x86/acer-wmi.c               |  2 +-
>>>   drivers/platform/x86/asus-laptop.c            |  2 +-
>>>   drivers/platform/x86/asus-wmi.c               |  4 +-
>>>   drivers/platform/x86/compal-laptop.c          |  2 +-
>>>   drivers/platform/x86/dell/dell-laptop.c       |  2 +-
>>>   drivers/platform/x86/eeepc-laptop.c           |  2 +-
>>>   drivers/platform/x86/fujitsu-laptop.c         |  4 +-
>>>   drivers/platform/x86/ideapad-laptop.c         |  2 +-
>>>   drivers/platform/x86/intel/oaktrail.c         |  2 +-
>>>   drivers/platform/x86/msi-laptop.c             |  2 +-
>>>   drivers/platform/x86/msi-wmi.c                |  2 +-
>>>   .../platform/x86/nvidia-wmi-ec-backlight.c    |  2 +-
>>>   drivers/platform/x86/panasonic-laptop.c       |  2 +-
>>>   drivers/platform/x86/samsung-laptop.c         |  2 +-
>>>   drivers/platform/x86/sony-laptop.c            |  2 +-
>>>   drivers/platform/x86/thinkpad_acpi.c          |  4 +-
>>>   drivers/platform/x86/toshiba_acpi.c           |  2 +-
>>>   drivers/video/backlight/backlight.c           | 18 +++++++
>>>   include/acpi/video.h                          | 21 ++++----
>>>   include/linux/backlight.h                     |  1 +
>>>   25 files changed, 85 insertions(+), 66 deletions(-)
>>
> 

Re: [ibm-acpi-devel] [PATCH 00/22] Fallback to native backlight

[Hans de G. <hde...@re...>]

Hi Akihiko,

On 10/24/22 13:34, Akihiko Odaki wrote:
> Commit 2600bfa3df99 ("ACPI: video: Add acpi_video_backlight_use_native()
> helper") and following commits made native backlight unavailable if
> CONFIG_ACPI_VIDEO is set and the backlight feature of ACPI video is
> unavailable, which broke the backlight functionality on Lenovo ThinkPad
> C13 Yoga Chromebook. Allow to fall back to native backlight in such
> cases.

I appreciate your work on this, but what this in essence does is
it allows 2 backlight drivers (vendor + native) to get registered
for the same panel again. While the whole goal of the backlight refactor
series landing in 6.1 was to make it so that there always is only
*1* backlight device registered instead of (possibly) registering
multiple and letting userspace figure it out. It is also important
to only always have 1 backlight device per panel for further
upcoming changes.

So nack for this solution, sorry.

I am aware that this breaks backlight control on some Chromebooks,
this was already reported and I wrote a long reply explaining why
things are done the way they are done now and also suggesting
2 possible (much simpler) fixes, see:
https://lore.kernel.org/linux-acpi/42a...@re.../

Unfortunately the reported has not followed-up on this and
I don't have the hardware to test this myself.

Can you please try implementing 1 of the fixes suggested there
and then submit that upstream ?

Regards,

Hans









> 
> Signed-off-by: Akihiko Odaki <aki...@da...>
> 
> Akihiko Odaki (22):
>   drm/i915/opregion: Improve backlight request condition
>   ACPI: video: Introduce acpi_video_get_backlight_types()
>   LoongArch: Use acpi_video_get_backlight_types()
>   platform/x86: acer-wmi: Use acpi_video_get_backlight_types()
>   platform/x86: asus-laptop: Use acpi_video_get_backlight_types()
>   platform/x86: asus-wmi: Use acpi_video_get_backlight_types()
>   platform/x86: compal-laptop: Use acpi_video_get_backlight_types()
>   platform/x86: eeepc-laptop: Use acpi_video_get_backlight_types()
>   platform/x86: fujitsu-laptop: Use acpi_video_get_backlight_types()
>   platform/x86: ideapad-laptop: Use acpi_video_get_backlight_types()
>   platform/x86: msi-laptop: Use acpi_video_get_backlight_types()
>   platform/x86: msi-wmi: Use acpi_video_get_backlight_types()
>   platform/x86: nvidia-wmi-ec-backlight: Use
>     acpi_video_get_backlight_types()
>   platform/x86: panasonic-laptop: Use acpi_video_get_backlight_types()
>   platform/x86: samsung-laptop: Use acpi_video_get_backlight_types()
>   platform/x86: sony-laptop: Use acpi_video_get_backlight_types()
>   platform/x86: thinkpad_acpi: Use acpi_video_get_backlight_types()
>   platform/x86: toshiba_acpi: Use acpi_video_get_backlight_types()
>   platform/x86: dell-laptop: Use acpi_video_get_backlight_types()
>   platform/x86: intel_oaktrail: Use acpi_video_get_backlight_types()
>   ACPI: video: Remove acpi_video_get_backlight_type()
>   ACPI: video: Fallback to native backlight
> 
>  Documentation/gpu/todo.rst                    |  8 +--
>  drivers/acpi/acpi_video.c                     |  2 +-
>  drivers/acpi/video_detect.c                   | 54 ++++++++++---------
>  drivers/gpu/drm/i915/display/intel_opregion.c |  3 +-
>  drivers/platform/loongarch/loongson-laptop.c  |  4 +-
>  drivers/platform/x86/acer-wmi.c               |  2 +-
>  drivers/platform/x86/asus-laptop.c            |  2 +-
>  drivers/platform/x86/asus-wmi.c               |  4 +-
>  drivers/platform/x86/compal-laptop.c          |  2 +-
>  drivers/platform/x86/dell/dell-laptop.c       |  2 +-
>  drivers/platform/x86/eeepc-laptop.c           |  2 +-
>  drivers/platform/x86/fujitsu-laptop.c         |  4 +-
>  drivers/platform/x86/ideapad-laptop.c         |  2 +-
>  drivers/platform/x86/intel/oaktrail.c         |  2 +-
>  drivers/platform/x86/msi-laptop.c             |  2 +-
>  drivers/platform/x86/msi-wmi.c                |  2 +-
>  .../platform/x86/nvidia-wmi-ec-backlight.c    |  2 +-
>  drivers/platform/x86/panasonic-laptop.c       |  2 +-
>  drivers/platform/x86/samsung-laptop.c         |  2 +-
>  drivers/platform/x86/sony-laptop.c            |  2 +-
>  drivers/platform/x86/thinkpad_acpi.c          |  4 +-
>  drivers/platform/x86/toshiba_acpi.c           |  2 +-
>  drivers/video/backlight/backlight.c           | 18 +++++++
>  include/acpi/video.h                          | 21 ++++----
>  include/linux/backlight.h                     |  1 +
>  25 files changed, 85 insertions(+), 66 deletions(-)
> 

[ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: Fix a max_brightness of thinklight

[Michał S. <m.s...@gm...>]

Thinklight has only two values, on/off so it's reasonable for 
max_brightness to be 0 and 1 as if you write anything between 0 and 255 
it will be 255 anyway so there's no point for it to be 255.

Sending again as I noticed some issues with patch.

---
  drivers/platform/x86/thinkpad_acpi.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 6a823b850a77..4a7369868f2e 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -5562,6 +5562,7 @@ static enum led_brightness light_sysfs_get(struct led_classdev *led_cdev)
  static struct tpacpi_led_classdev tpacpi_led_thinklight = {
  	.led_classdev = {
  		.name		= "tpacpi::thinklight",
+		.max_brightness = 1,
  		.brightness_set_blocking = &light_sysfs_set,
  		.brightness_get	= &light_sysfs_get,
  	}
-- 
2.37.3

[ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: Fix a max_brightness of thinklight

[Michał S. <m.s...@gm...>]

Thinklight has only two values, on/off so it's reasonable for 
max_brightness to be 0 and 1 as if you write anything between 0 and 255 
it will be 255 anyway so there's no point for it to be 255.

Also sorry for repeated messages, I got no reply so I'm trying to figure 
out what I did wrong.


---
  drivers/platform/x86/thinkpad_acpi.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c 
b/drivers/platform/x86/thinkpad_acpi.c
index 22d4e8633e30e9..775c2f327dc3a8 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -5562,6 +5562,7 @@ static enum led_brightness light_sysfs_get(struct 
led_classdev *led_cdev)
  static struct tpacpi_led_classdev tpacpi_led_thinklight = {
      .led_classdev = {
          .name        = "tpacpi::thinklight",
+        .max_brightness    = 1,
          .brightness_set_blocking = &light_sysfs_set,
          .brightness_get    = &light_sysfs_get,
      }

[ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: Fix a max_brightness of thinklight

[Michał S. <m.s...@gm...>]

Thinklight has only two values, on/off so it's reasonable for 
max_brightness to be 0 and 1 as if you write anything between 0 and 255 
it will be 255 anyway so there's no point for it to be 255.

---
  drivers/platform/x86/thinkpad_acpi.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c 
b/drivers/platform/x86/thinkpad_acpi.c
index 22d4e8633e30e9..775c2f327dc3a8 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -5562,6 +5562,7 @@ static enum led_brightness light_sysfs_get(struct 
led_classdev *led_cdev)
  static struct tpacpi_led_classdev tpacpi_led_thinklight = {
      .led_classdev = {
          .name        = "tpacpi::thinklight",
+        .max_brightness    = 1,
          .brightness_set_blocking = &light_sysfs_set,
          .brightness_get    = &light_sysfs_get,
      }