linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Fix detection of 2nd fan on X1C9
@ 2022-04-29 21:14 Lyude Paul
  2022-04-29 21:14 ` [PATCH 1/2] platform/x86: thinkpad_acpi: Restore X1 Carbon 9th Gen dual fan quirk Lyude Paul
  2022-04-29 21:14 ` [PATCH 2/2] platform/x86: thinkpad_acpi: Don't probe 2nd fan if enabled by quirk Lyude Paul
  0 siblings, 2 replies; 9+ messages in thread
From: Lyude Paul @ 2022-04-29 21:14 UTC (permalink / raw)
  To: ibm-acpi-devel, platform-driver-x86
  Cc: linux-kernel, linux-hwmon, Hans de Goede, Mark Pearson

Some recent changes broke detection of the second fan on the X1 Carbon
9th generation, so here's some patches to fix it.

Lyude Paul (2):
  platform/x86: thinkpad_acpi: Restore X1 Carbon 9th Gen dual fan quirk
  platform/x86: thinkpad_acpi: Don't probe 2nd fan if enabled by quirk

 drivers/platform/x86/thinkpad_acpi.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

-- 
2.35.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] platform/x86: thinkpad_acpi: Restore X1 Carbon 9th Gen dual fan quirk
  2022-04-29 21:14 [PATCH 0/2] Fix detection of 2nd fan on X1C9 Lyude Paul
@ 2022-04-29 21:14 ` Lyude Paul
  2022-04-30  1:25   ` [External] " Mark Pearson
  2022-04-29 21:14 ` [PATCH 2/2] platform/x86: thinkpad_acpi: Don't probe 2nd fan if enabled by quirk Lyude Paul
  1 sibling, 1 reply; 9+ messages in thread
From: Lyude Paul @ 2022-04-29 21:14 UTC (permalink / raw)
  To: ibm-acpi-devel, platform-driver-x86
  Cc: linux-kernel, linux-hwmon, Hans de Goede, Mark Pearson,
	Henrique de Moraes Holschuh, Mark Gross

The new method of probing dual fan support introduced in:

bf779aaf56ea ("platform/x86: thinkpad_acpi: Add dual fan probe")

While this commit says this works on the X1 Carbon 9th Gen, it actually
just results in hiding the second fan on my local machine. Additionally,
I'm fairly sure this machine powers on quite often without either of the
two fans spinning.

So let's fix this by adding back the dual fan quirk for the X1 Carbon 9th
Gen.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Fixes: bf779aaf56ea ("platform/x86: thinkpad_acpi: Add dual fan probe")
Cc: Mark Pearson <markpearson@lenovo.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Mark Gross <markgross@kernel.org>
Cc: ibm-acpi-devel@lists.sourceforge.net
Cc: platform-driver-x86@vger.kernel.org
---
 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 c568fae56db2..9067fd0a945c 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -8699,6 +8699,7 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
 	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', '3', '0', TPACPI_FAN_2CTL),	/* P15 (1st gen) / P15v (1st gen) */
+	TPACPI_Q_LNV3('N', '3', '2', TPACPI_FAN_2CTL),  /* X1 Carbon (9th 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.35.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] platform/x86: thinkpad_acpi: Don't probe 2nd fan if enabled by quirk
  2022-04-29 21:14 [PATCH 0/2] Fix detection of 2nd fan on X1C9 Lyude Paul
  2022-04-29 21:14 ` [PATCH 1/2] platform/x86: thinkpad_acpi: Restore X1 Carbon 9th Gen dual fan quirk Lyude Paul
@ 2022-04-29 21:14 ` Lyude Paul
  1 sibling, 0 replies; 9+ messages in thread
From: Lyude Paul @ 2022-04-29 21:14 UTC (permalink / raw)
  To: ibm-acpi-devel, platform-driver-x86
  Cc: linux-kernel, linux-hwmon, Hans de Goede, Mark Pearson,
	Henrique de Moraes Holschuh, Mark Gross

If we already know that the system in question has a quirk telling us that
the system has a second fan, there's no purpose in probing the second fan -
especially when probing the second fan may not work properly with systems
relying on quirks.

Also, convert all of the conditionals here into a single group of if/else
statements. This is because there's no situations where there's more then
one quirk on a device.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Fixes: bf779aaf56ea ("platform/x86: thinkpad_acpi: Add dual fan probe")
Cc: Mark Pearson <markpearson@lenovo.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Mark Gross <markgross@kernel.org>
Cc: ibm-acpi-devel@lists.sourceforge.net
Cc: platform-driver-x86@vger.kernel.org
---
 drivers/platform/x86/thinkpad_acpi.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 9067fd0a945c..677822b5d4b4 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -8747,26 +8747,25 @@ static int __init fan_init(struct ibm_init_struct *iibm)
 			unsigned int speed;
 
 			fan_status_access_mode = TPACPI_FAN_RD_TPEC;
-			if (quirks & TPACPI_FAN_Q1)
+			if (quirks & TPACPI_FAN_Q1) {
 				fan_quirk1_setup();
-			if (quirks & TPACPI_FAN_2FAN) {
+			} else if (quirks & TPACPI_FAN_2FAN) {
 				tp_features.second_fan = 1;
 				pr_info("secondary fan support enabled\n");
-			}
-			if (quirks & TPACPI_FAN_2CTL) {
+			} else if (quirks & TPACPI_FAN_2CTL) {
 				tp_features.second_fan = 1;
 				tp_features.second_fan_ctl = 1;
 				pr_info("secondary fan control enabled\n");
+			} else {
+				/* Try and probe the 2nd fan */
+				res = fan2_get_speed(&speed);
+				if (res >= 0) {
+					/* It responded - so let's assume it's there */
+					tp_features.second_fan = 1;
+					tp_features.second_fan_ctl = 1;
+					pr_info("secondary fan control detected & enabled\n");
+				}
 			}
-			/* Try and probe the 2nd fan */
-			res = fan2_get_speed(&speed);
-			if (res >= 0) {
-				/* It responded - so let's assume it's there */
-				tp_features.second_fan = 1;
-				tp_features.second_fan_ctl = 1;
-				pr_info("secondary fan control detected & enabled\n");
-			}
-
 		} else {
 			pr_err("ThinkPad ACPI EC access misbehaving, fan status and control unavailable\n");
 			return -ENODEV;
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [External] [PATCH 1/2] platform/x86: thinkpad_acpi: Restore X1 Carbon 9th Gen dual fan quirk
  2022-04-29 21:14 ` [PATCH 1/2] platform/x86: thinkpad_acpi: Restore X1 Carbon 9th Gen dual fan quirk Lyude Paul
@ 2022-04-30  1:25   ` Mark Pearson
  2022-04-30 12:13     ` Thomas Weißschuh
  2022-05-02 17:42     ` Lyude Paul
  0 siblings, 2 replies; 9+ messages in thread
From: Mark Pearson @ 2022-04-30  1:25 UTC (permalink / raw)
  To: Lyude Paul, ibm-acpi-devel, platform-driver-x86
  Cc: linux-kernel, linux-hwmon, Hans de Goede,
	Henrique de Moraes Holschuh, Mark Gross

Hi Lyude

On 4/29/22 17:14, Lyude Paul wrote:
> The new method of probing dual fan support introduced in:
> 
> bf779aaf56ea ("platform/x86: thinkpad_acpi: Add dual fan probe")
> 
> While this commit says this works on the X1 Carbon 9th Gen, it actually
> just results in hiding the second fan on my local machine. Additionally,
> I'm fairly sure this machine powers on quite often without either of the
> two fans spinning.
> 
> So let's fix this by adding back the dual fan quirk for the X1 Carbon 9th
> Gen.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Fixes: bf779aaf56ea ("platform/x86: thinkpad_acpi: Add dual fan probe")
> Cc: Mark Pearson <markpearson@lenovo.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Mark Gross <markgross@kernel.org>
> Cc: ibm-acpi-devel@lists.sourceforge.net
> Cc: platform-driver-x86@vger.kernel.org
> ---
>  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 c568fae56db2..9067fd0a945c 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -8699,6 +8699,7 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
>  	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', '3', '0', TPACPI_FAN_2CTL),	/* P15 (1st gen) / P15v (1st gen) */
> +	TPACPI_Q_LNV3('N', '3', '2', TPACPI_FAN_2CTL),  /* X1 Carbon (9th 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) */
>  };
I just double checked this on my X1C9 - and it's working correctly. 2nd
fan is detected correctly.

I'd rather understand why it's not working on your setup then just
re-introduce the quirk.

What happens on your system when the
  res = fan2_get_speed(&speed);
is called? If that is failing it means your 2nd fan isn't responding and
that's not supposed to happen. Could you let me know if you get an error
code, if it happens every boot, etc
I assume when the function is called later it works successfully?

Also please confirm which BIOS and EC version you have.

Thanks
Mark

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [External] [PATCH 1/2] platform/x86: thinkpad_acpi: Restore X1 Carbon 9th Gen dual fan quirk
  2022-04-30  1:25   ` [External] " Mark Pearson
@ 2022-04-30 12:13     ` Thomas Weißschuh
  2022-05-02 13:03       ` Mark Pearson
  2022-05-02 17:42     ` Lyude Paul
  1 sibling, 1 reply; 9+ messages in thread
From: Thomas Weißschuh @ 2022-04-30 12:13 UTC (permalink / raw)
  To: Mark Pearson
  Cc: Lyude Paul, ibm-acpi-devel, platform-driver-x86, linux-kernel,
	linux-hwmon, Hans de Goede, Henrique de Moraes Holschuh,
	Mark Gross

Hi Mark,

On 2022-04-29 21:25-0400, Mark Pearson wrote:
> Hi Lyude
> 
> On 4/29/22 17:14, Lyude Paul wrote:
> > The new method of probing dual fan support introduced in:
> > 
> > bf779aaf56ea ("platform/x86: thinkpad_acpi: Add dual fan probe")
> > 
> > While this commit says this works on the X1 Carbon 9th Gen, it actually
> > just results in hiding the second fan on my local machine. Additionally,
> > I'm fairly sure this machine powers on quite often without either of the
> > two fans spinning.
> > 
> > So let's fix this by adding back the dual fan quirk for the X1 Carbon 9th
> > Gen.
> > 
> [..]
>
> I just double checked this on my X1C9 - and it's working correctly. 2nd
> fan is detected correctly.
> 
> I'd rather understand why it's not working on your setup then just
> re-introduce the quirk.
> 
> What happens on your system when the
>   res = fan2_get_speed(&speed);
> is called? If that is failing it means your 2nd fan isn't responding and
> that's not supposed to happen. Could you let me know if you get an error
> code, if it happens every boot, etc
> I assume when the function is called later it works successfully?

I have the same issue.

To me it looks like this:

Probing for the second fan calls fan2_get_speed(), this calls
fan_select_fan2() which in turn checks that tp_features.second_fan is set.
But at this point in the tp_features.second_fan can not yet be set because it
is either set from quirks or *after* the probing.

Maybe some of the matches for the quirk TPACPI_FAN_2FAN should also have
matched this device?
It doesn't do so on my device.

> Also please confirm which BIOS and EC version you have.

Linux: 5.17.5
BIOS Revision: 1.51
Firmware Revision: 1.32

Thomas

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [External] [PATCH 1/2] platform/x86: thinkpad_acpi: Restore X1 Carbon 9th Gen dual fan quirk
  2022-04-30 12:13     ` Thomas Weißschuh
@ 2022-05-02 13:03       ` Mark Pearson
  0 siblings, 0 replies; 9+ messages in thread
From: Mark Pearson @ 2022-05-02 13:03 UTC (permalink / raw)
  To: Thomas Weißschuh
  Cc: Lyude Paul, ibm-acpi-devel, platform-driver-x86, linux-kernel,
	linux-hwmon, Hans de Goede, Henrique de Moraes Holschuh,
	Mark Gross



On 4/30/22 08:13, Thomas Weißschuh wrote:
> Hi Mark,
> 
> On 2022-04-29 21:25-0400, Mark Pearson wrote:
>> Hi Lyude
>>
>> On 4/29/22 17:14, Lyude Paul wrote:
>>> The new method of probing dual fan support introduced in:
>>>
>>> bf779aaf56ea ("platform/x86: thinkpad_acpi: Add dual fan probe")
>>>
>>> While this commit says this works on the X1 Carbon 9th Gen, it actually
>>> just results in hiding the second fan on my local machine. Additionally,
>>> I'm fairly sure this machine powers on quite often without either of the
>>> two fans spinning.
>>>
>>> So let's fix this by adding back the dual fan quirk for the X1 Carbon 9th
>>> Gen.
>>>
>> [..]
>>
>> I just double checked this on my X1C9 - and it's working correctly. 2nd
>> fan is detected correctly.
>>
>> I'd rather understand why it's not working on your setup then just
>> re-introduce the quirk.
>>
>> What happens on your system when the
>>   res = fan2_get_speed(&speed);
>> is called? If that is failing it means your 2nd fan isn't responding and
>> that's not supposed to happen. Could you let me know if you get an error
>> code, if it happens every boot, etc
>> I assume when the function is called later it works successfully?
> 
> I have the same issue.
> 
> To me it looks like this:
> 
> Probing for the second fan calls fan2_get_speed(), this calls
> fan_select_fan2() which in turn checks that tp_features.second_fan is set.
> But at this point in the tp_features.second_fan can not yet be set because it
> is either set from quirks or *after* the probing.
> 
> Maybe some of the matches for the quirk TPACPI_FAN_2FAN should also have
> matched this device?
> It doesn't do so on my device.
> 
>> Also please confirm which BIOS and EC version you have.
> 
> Linux: 5.17.5
> BIOS Revision: 1.51
> Firmware Revision: 1.32
> 
Thanks Thomas,
I'll go do some more digging on my system and see what I've missed.
Mark

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [External] [PATCH 1/2] platform/x86: thinkpad_acpi: Restore X1 Carbon 9th Gen dual fan quirk
  2022-04-30  1:25   ` [External] " Mark Pearson
  2022-04-30 12:13     ` Thomas Weißschuh
@ 2022-05-02 17:42     ` Lyude Paul
  2022-05-02 17:44       ` Mark Pearson
  1 sibling, 1 reply; 9+ messages in thread
From: Lyude Paul @ 2022-05-02 17:42 UTC (permalink / raw)
  To: Mark Pearson, ibm-acpi-devel, platform-driver-x86
  Cc: linux-kernel, linux-hwmon, Hans de Goede,
	Henrique de Moraes Holschuh, Mark Gross

Some answers/comments down below

On Fri, 2022-04-29 at 21:25 -0400, Mark Pearson wrote:
> Hi Lyude
> 
> On 4/29/22 17:14, Lyude Paul wrote:
> > The new method of probing dual fan support introduced in:
> > 
> > bf779aaf56ea ("platform/x86: thinkpad_acpi: Add dual fan probe")
> > 
> > While this commit says this works on the X1 Carbon 9th Gen, it actually
> > just results in hiding the second fan on my local machine. Additionally,
> > I'm fairly sure this machine powers on quite often without either of the
> > two fans spinning.
> > 
> > So let's fix this by adding back the dual fan quirk for the X1 Carbon 9th
> > Gen.
> > 
> > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > Fixes: bf779aaf56ea ("platform/x86: thinkpad_acpi: Add dual fan probe")
> > Cc: Mark Pearson <markpearson@lenovo.com>
> > Cc: Hans de Goede <hdegoede@redhat.com>
> > Cc: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> > Cc: Mark Gross <markgross@kernel.org>
> > Cc: ibm-acpi-devel@lists.sourceforge.net
> > Cc: platform-driver-x86@vger.kernel.org
> > ---
> >  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 c568fae56db2..9067fd0a945c 100644
> > --- a/drivers/platform/x86/thinkpad_acpi.c
> > +++ b/drivers/platform/x86/thinkpad_acpi.c
> > @@ -8699,6 +8699,7 @@ static const struct tpacpi_quirk fan_quirk_table[]
> > __initconst = {
> >         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', '3', '0', TPACPI_FAN_2CTL),  /* P15 (1st gen) /
> > P15v (1st gen) */
> > +       TPACPI_Q_LNV3('N', '3', '2', TPACPI_FAN_2CTL),  /* X1 Carbon (9th
> > 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) */
> >  };
> I just double checked this on my X1C9 - and it's working correctly. 2nd
> fan is detected correctly.
> 
> I'd rather understand why it's not working on your setup then just
> re-introduce the quirk.

Of course! I figured as much, it's just easy to start conversations with a
revert :P

> 
> What happens on your system when the
>   res = fan2_get_speed(&speed);
> is called? If that is failing it means your 2nd fan isn't responding and
> that's not supposed to happen. Could you let me know if you get an error
> code, if it happens every boot, etc
> I assume when the function is called later it works successfully?

It definitely seems to happen every boot, not sure about the error code it
returns. I will check and get you this info asap

> 
> Also please confirm which BIOS and EC version you have.

BIOS version N32ET75W (1.51) release date 12/02/2021, embedded controller is
0.1.32


> 
> Thanks
> Mark
> 

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [External] [PATCH 1/2] platform/x86: thinkpad_acpi: Restore X1 Carbon 9th Gen dual fan quirk
  2022-05-02 17:42     ` Lyude Paul
@ 2022-05-02 17:44       ` Mark Pearson
  2022-05-02 19:13         ` Mark Pearson
  0 siblings, 1 reply; 9+ messages in thread
From: Mark Pearson @ 2022-05-02 17:44 UTC (permalink / raw)
  To: Lyude Paul, ibm-acpi-devel, platform-driver-x86
  Cc: linux-kernel, linux-hwmon, Hans de Goede,
	Henrique de Moraes Holschuh, Mark Gross



On 5/2/22 13:42, Lyude Paul wrote:
> Some answers/comments down below
> 
> On Fri, 2022-04-29 at 21:25 -0400, Mark Pearson wrote:
>> Hi Lyude
>>
>> On 4/29/22 17:14, Lyude Paul wrote:
>>> The new method of probing dual fan support introduced in:
>>>
>>> bf779aaf56ea ("platform/x86: thinkpad_acpi: Add dual fan probe")
>>>
>>> While this commit says this works on the X1 Carbon 9th Gen, it actually
>>> just results in hiding the second fan on my local machine. Additionally,
>>> I'm fairly sure this machine powers on quite often without either of the
>>> two fans spinning.
>>>
>>> So let's fix this by adding back the dual fan quirk for the X1 Carbon 9th
>>> Gen.
>>>
>>> Signed-off-by: Lyude Paul <lyude@redhat.com>
>>> Fixes: bf779aaf56ea ("platform/x86: thinkpad_acpi: Add dual fan probe")
>>> Cc: Mark Pearson <markpearson@lenovo.com>
>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>> Cc: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
>>> Cc: Mark Gross <markgross@kernel.org>
>>> Cc: ibm-acpi-devel@lists.sourceforge.net
>>> Cc: platform-driver-x86@vger.kernel.org
>>> ---
>>>  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 c568fae56db2..9067fd0a945c 100644
>>> --- a/drivers/platform/x86/thinkpad_acpi.c
>>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>>> @@ -8699,6 +8699,7 @@ static const struct tpacpi_quirk fan_quirk_table[]
>>> __initconst = {
>>>         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', '3', '0', TPACPI_FAN_2CTL),  /* P15 (1st gen) /
>>> P15v (1st gen) */
>>> +       TPACPI_Q_LNV3('N', '3', '2', TPACPI_FAN_2CTL),  /* X1 Carbon (9th
>>> 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) */
>>>  };
>> I just double checked this on my X1C9 - and it's working correctly. 2nd
>> fan is detected correctly.
>>
>> I'd rather understand why it's not working on your setup then just
>> re-introduce the quirk.
> 
> Of course! I figured as much, it's just easy to start conversations with a
> revert :P
> 
>>
>> What happens on your system when the
>>   res = fan2_get_speed(&speed);
>> is called? If that is failing it means your 2nd fan isn't responding and
>> that's not supposed to happen. Could you let me know if you get an error
>> code, if it happens every boot, etc
>> I assume when the function is called later it works successfully?
> 
> It definitely seems to happen every boot, not sure about the error code it
> returns. I will check and get you this info asap
> 
>>
>> Also please confirm which BIOS and EC version you have.
> 
> BIOS version N32ET75W (1.51) release date 12/02/2021, embedded controller is
> 0.1.32
> 
> 
Thanks!

Along with Thomas' notes I think I've found the problem (though still
bemused why I don't see the problem on my X1C9 and I tested on multiple
platforms previously...so it is somewhat weird).

Working on a fix - will try and have that out for review later today or
tomorrow.

Mark

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [External] [PATCH 1/2] platform/x86: thinkpad_acpi: Restore X1 Carbon 9th Gen dual fan quirk
  2022-05-02 17:44       ` Mark Pearson
@ 2022-05-02 19:13         ` Mark Pearson
  0 siblings, 0 replies; 9+ messages in thread
From: Mark Pearson @ 2022-05-02 19:13 UTC (permalink / raw)
  To: Lyude Paul, ibm-acpi-devel, platform-driver-x86
  Cc: linux-kernel, linux-hwmon, Hans de Goede,
	Henrique de Moraes Holschuh, Mark Gross



On 5/2/22 13:44, Mark Pearson wrote:
> 
> 
> On 5/2/22 13:42, Lyude Paul wrote:
>> Some answers/comments down below
>>
>> On Fri, 2022-04-29 at 21:25 -0400, Mark Pearson wrote:
>>> Hi Lyude
>>>
>>> On 4/29/22 17:14, Lyude Paul wrote:
>>>> The new method of probing dual fan support introduced in:
>>>>
>>>> bf779aaf56ea ("platform/x86: thinkpad_acpi: Add dual fan probe")
>>>>
>>>> While this commit says this works on the X1 Carbon 9th Gen, it actually
>>>> just results in hiding the second fan on my local machine. Additionally,
>>>> I'm fairly sure this machine powers on quite often without either of the
>>>> two fans spinning.
>>>>
>>>> So let's fix this by adding back the dual fan quirk for the X1 Carbon 9th
>>>> Gen.
>>>>
>>>> Signed-off-by: Lyude Paul <lyude@redhat.com>
>>>> Fixes: bf779aaf56ea ("platform/x86: thinkpad_acpi: Add dual fan probe")
>>>> Cc: Mark Pearson <markpearson@lenovo.com>
>>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>>> Cc: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
>>>> Cc: Mark Gross <markgross@kernel.org>
>>>> Cc: ibm-acpi-devel@lists.sourceforge.net
>>>> Cc: platform-driver-x86@vger.kernel.org
>>>> ---
>>>>  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 c568fae56db2..9067fd0a945c 100644
>>>> --- a/drivers/platform/x86/thinkpad_acpi.c
>>>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>>>> @@ -8699,6 +8699,7 @@ static const struct tpacpi_quirk fan_quirk_table[]
>>>> __initconst = {
>>>>         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', '3', '0', TPACPI_FAN_2CTL),  /* P15 (1st gen) /
>>>> P15v (1st gen) */
>>>> +       TPACPI_Q_LNV3('N', '3', '2', TPACPI_FAN_2CTL),  /* X1 Carbon (9th
>>>> 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) */
>>>>  };
>>> I just double checked this on my X1C9 - and it's working correctly. 2nd
>>> fan is detected correctly.
>>>
>>> I'd rather understand why it's not working on your setup then just
>>> re-introduce the quirk.
>>
>> Of course! I figured as much, it's just easy to start conversations with a
>> revert :P
>>
>>>
>>> What happens on your system when the
>>>   res = fan2_get_speed(&speed);
>>> is called? If that is failing it means your 2nd fan isn't responding and
>>> that's not supposed to happen. Could you let me know if you get an error
>>> code, if it happens every boot, etc
>>> I assume when the function is called later it works successfully?
>>
>> It definitely seems to happen every boot, not sure about the error code it
>> returns. I will check and get you this info asap
>>
>>>
>>> Also please confirm which BIOS and EC version you have.
>>
>> BIOS version N32ET75W (1.51) release date 12/02/2021, embedded controller is
>> 0.1.32
>>
>>
> Thanks!
> 
> Along with Thomas' notes I think I've found the problem (though still
> bemused why I don't see the problem on my X1C9 and I tested on multiple
> platforms previously...so it is somewhat weird).
> 
> Working on a fix - will try and have that out for review later today or
> tomorrow.
> 
Not sure exactly what the etiquette here is for the mailing list but I
just posted an updated patch "platform/x86: thinkpad_acpi: Correct dual
fan probe" that I think addresses all the issues raised in this patch
sequence.

Please let me know any feedback or concerns. And thanks for raising this!

Mark

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-05-02 19:14 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-29 21:14 [PATCH 0/2] Fix detection of 2nd fan on X1C9 Lyude Paul
2022-04-29 21:14 ` [PATCH 1/2] platform/x86: thinkpad_acpi: Restore X1 Carbon 9th Gen dual fan quirk Lyude Paul
2022-04-30  1:25   ` [External] " Mark Pearson
2022-04-30 12:13     ` Thomas Weißschuh
2022-05-02 13:03       ` Mark Pearson
2022-05-02 17:42     ` Lyude Paul
2022-05-02 17:44       ` Mark Pearson
2022-05-02 19:13         ` Mark Pearson
2022-04-29 21:14 ` [PATCH 2/2] platform/x86: thinkpad_acpi: Don't probe 2nd fan if enabled by quirk Lyude Paul

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).