[PATCH 0/3] hwmon: (dell-smm) Minor fixes

[PATCH 0/3] hwmon: (dell-smm) Minor fixes

[Armin Wolf]

This patch series fixes some minor issues inside dell-smm-hwmon.
The first patch causes the driver to fail probing when registration
of a cooling device fails, so that userspace can actually depend
on those cooling devices representing all fans.

The other two patches change some warning messages to be more
informative/unambiguous.

All changes where tested on a Dell Inspiron 3505.

Armin Wolf (3):
  hwmon: (dell-smm) Fail probing when cooling device registration fails
  hwmon: (dell-smm) Add FW_BUG to SMM warning message
  hwmon: (dell-smm) Improve warning messages

 drivers/hwmon/dell-smm-hwmon.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

--
2.30.2

[PATCH 1/3] hwmon: (dell-smm) Fail probing when cooling device registration fails

[Armin Wolf]

Previously, it was thought that failing to register a cooling device
would not be critical, so the probing was not aborted in such a case.
This however would lead to userspace being unable to rely on those
cooling devices, since they might not represent all fans being present.
Fix that by failing probing when cooling device registration fails.

Tested on a Dell Inspiron 3505.

Fixes: e0d3f7cb2606 ("hwmon: (dell-smm) Add cooling device support")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/dell-smm-hwmon.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 7f8d95dd2717..1dab7591576a 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -1013,12 +1013,10 @@ static int __init dell_smm_init_hwmon(struct device *dev)

 		data->fan[i] = true;

-		/* the cooling device is not critical, ignore failures */
 		if (IS_REACHABLE(CONFIG_THERMAL)) {
 			err = dell_smm_init_cdev(dev, i);
 			if (err < 0)
-				dev_warn(dev, "Failed to register cooling device for fan %u\n",
-					 i + 1);
+				return err;
 		}

 		data->fan_nominal_speed[i] = devm_kmalloc_array(dev, data->i8k_fan_max + 1,
--
2.30.2

Re: [PATCH 1/3] hwmon: (dell-smm) Fail probing when cooling device registration fails

[Pali Rohár]

On Sunday 21 August 2022 17:17:11 Armin Wolf wrote:
> Previously, it was thought that failing to register a cooling device
> would not be critical, so the probing was not aborted in such a case.
> This however would lead to userspace being unable to rely on those
> cooling devices, since they might not represent all fans being present.
> Fix that by failing probing when cooling device registration fails.

This patch does not fix address this issue fully. CONFIG_THERMAL can be
disabled during compile time and then cooling device would not be
registered too.

> Tested on a Dell Inspiron 3505.
> 
> Fixes: e0d3f7cb2606 ("hwmon: (dell-smm) Add cooling device support")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/hwmon/dell-smm-hwmon.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 7f8d95dd2717..1dab7591576a 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -1013,12 +1013,10 @@ static int __init dell_smm_init_hwmon(struct device *dev)
> 
>  		data->fan[i] = true;
> 
> -		/* the cooling device is not critical, ignore failures */
>  		if (IS_REACHABLE(CONFIG_THERMAL)) {
>  			err = dell_smm_init_cdev(dev, i);
>  			if (err < 0)
> -				dev_warn(dev, "Failed to register cooling device for fan %u\n",
> -					 i + 1);
> +				return err;
>  		}
> 
>  		data->fan_nominal_speed[i] = devm_kmalloc_array(dev, data->i8k_fan_max + 1,
> --
> 2.30.2
> 

Re: [PATCH 1/3] hwmon: (dell-smm) Fail probing when cooling device registration fails

[Armin Wolf]

Am 21.08.22 um 17:41 schrieb Pali Rohár:

> On Sunday 21 August 2022 17:17:11 Armin Wolf wrote:
>> Previously, it was thought that failing to register a cooling device
>> would not be critical, so the probing was not aborted in such a case.
>> This however would lead to userspace being unable to rely on those
>> cooling devices, since they might not represent all fans being present.
>> Fix that by failing probing when cooling device registration fails.
> This patch does not fix address this issue fully. CONFIG_THERMAL can be
> disabled during compile time and then cooling device would not be
> registered too.

I though of the cooling device feature as being optional "as a whole".
So when CONFIG_THERMAL is disabled during compile time, the driver does
not create any cooling devices. If however CONFIG_THERMAL was enabled
during compile time, the driver should fail probing if it cannot register
all cooling devices.

Armin Wolf

>> Tested on a Dell Inspiron 3505.
>>
>> Fixes: e0d3f7cb2606 ("hwmon: (dell-smm) Add cooling device support")
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   drivers/hwmon/dell-smm-hwmon.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>> index 7f8d95dd2717..1dab7591576a 100644
>> --- a/drivers/hwmon/dell-smm-hwmon.c
>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>> @@ -1013,12 +1013,10 @@ static int __init dell_smm_init_hwmon(struct device *dev)
>>
>>   		data->fan[i] = true;
>>
>> -		/* the cooling device is not critical, ignore failures */
>>   		if (IS_REACHABLE(CONFIG_THERMAL)) {
>>   			err = dell_smm_init_cdev(dev, i);
>>   			if (err < 0)
>> -				dev_warn(dev, "Failed to register cooling device for fan %u\n",
>> -					 i + 1);
>> +				return err;
>>   		}
>>
>>   		data->fan_nominal_speed[i] = devm_kmalloc_array(dev, data->i8k_fan_max + 1,
>> --
>> 2.30.2
>>

Re: [PATCH 1/3] hwmon: (dell-smm) Fail probing when cooling device registration fails

[Guenter Roeck]

On Sun, Aug 21, 2022 at 09:11:24PM +0200, Armin Wolf wrote:
> Am 21.08.22 um 17:41 schrieb Pali Rohár:
> 
> > On Sunday 21 August 2022 17:17:11 Armin Wolf wrote:
> > > Previously, it was thought that failing to register a cooling device
> > > would not be critical, so the probing was not aborted in such a case.
> > > This however would lead to userspace being unable to rely on those
> > > cooling devices, since they might not represent all fans being present.
> > > Fix that by failing probing when cooling device registration fails.
> > This patch does not fix address this issue fully. CONFIG_THERMAL can be
> > disabled during compile time and then cooling device would not be
> > registered too.
> 
> I though of the cooling device feature as being optional "as a whole".
> So when CONFIG_THERMAL is disabled during compile time, the driver does
> not create any cooling devices. If however CONFIG_THERMAL was enabled
> during compile time, the driver should fail probing if it cannot register
> all cooling devices.
> 

I disagree. The primary objective of this driver is to report environmental
data. Support for the thermal subsystem is an add-on. If instantiating
the thermal device fails, the driver should at least report temperatures
and fan speeds, as it did before thermal support was added.

Thanks,
Guenter

> Armin Wolf
> 
> > > Tested on a Dell Inspiron 3505.
> > > 
> > > Fixes: e0d3f7cb2606 ("hwmon: (dell-smm) Add cooling device support")
> > > Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> > > ---
> > >   drivers/hwmon/dell-smm-hwmon.c | 4 +---
> > >   1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> > > index 7f8d95dd2717..1dab7591576a 100644
> > > --- a/drivers/hwmon/dell-smm-hwmon.c
> > > +++ b/drivers/hwmon/dell-smm-hwmon.c
> > > @@ -1013,12 +1013,10 @@ static int __init dell_smm_init_hwmon(struct device *dev)
> > > 
> > >   		data->fan[i] = true;
> > > 
> > > -		/* the cooling device is not critical, ignore failures */
> > >   		if (IS_REACHABLE(CONFIG_THERMAL)) {
> > >   			err = dell_smm_init_cdev(dev, i);
> > >   			if (err < 0)
> > > -				dev_warn(dev, "Failed to register cooling device for fan %u\n",
> > > -					 i + 1);
> > > +				return err;
> > >   		}
> > > 
> > >   		data->fan_nominal_speed[i] = devm_kmalloc_array(dev, data->i8k_fan_max + 1,
> > > --
> > > 2.30.2
> > > 

[PATCH 2/3] hwmon: (dell-smm) Add FW_BUG to SMM warning message

[Armin Wolf]

When a SMM call takes very long to execute, then it definitely
is a firmware bug which should be marked with FW_BUG.
Also add the number of the buggy SMM call to the warning message
so BIOS developers, etc immediately know which part of the SMM
interface is buggy.

Tested on a Dell Inspiron 3505.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/dell-smm-hwmon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 1dab7591576a..f7bab1a91b93 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -26,6 +26,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/platform_device.h>
+#include <linux/printk.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
@@ -198,7 +199,7 @@ static int i8k_smm_func(void *par)
 		 eax, ebx, regs->eax & 0xffff, carry, duration);

 	if (duration > DELL_SMM_MAX_DURATION)
-		pr_warn_once("SMM call took %lld usecs!\n", duration);
+		pr_warn_once(FW_BUG "SMM call 0x%.4x took %lld usecs!\n", eax, duration);

 	if (carry || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
 		return -EINVAL;
--
2.30.2

Re: [PATCH 2/3] hwmon: (dell-smm) Add FW_BUG to SMM warning message

[Pali Rohár]

On Sunday 21 August 2022 17:17:12 Armin Wolf wrote:
> When a SMM call takes very long to execute, then it definitely
> is a firmware bug which should be marked with FW_BUG.
> Also add the number of the buggy SMM call to the warning message
> so BIOS developers, etc immediately know which part of the SMM
> interface is buggy.
> 
> Tested on a Dell Inspiron 3505.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Acked-by: Pali Rohár <pali@kernel.org>

Ideally this change should be CCed to Dell BIOS developers :D (hehe)

> ---
>  drivers/hwmon/dell-smm-hwmon.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 1dab7591576a..f7bab1a91b93 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -26,6 +26,7 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/platform_device.h>
> +#include <linux/printk.h>
>  #include <linux/proc_fs.h>
>  #include <linux/seq_file.h>
>  #include <linux/slab.h>
> @@ -198,7 +199,7 @@ static int i8k_smm_func(void *par)
>  		 eax, ebx, regs->eax & 0xffff, carry, duration);
> 
>  	if (duration > DELL_SMM_MAX_DURATION)
> -		pr_warn_once("SMM call took %lld usecs!\n", duration);
> +		pr_warn_once(FW_BUG "SMM call 0x%.4x took %lld usecs!\n", eax, duration);
> 
>  	if (carry || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
>  		return -EINVAL;
> --
> 2.30.2
> 

[PATCH 3/3] hwmon: (dell-smm) Improve warning messages

[Armin Wolf]

When dell-smm-hwmon is loaded on a machine with a buggy BIOS
with the option "force" being enabled, it wrongly prints
what the buggy features where disabled. This may cause
users to wrongly assume that the driver still protects them
from these BIOS bugs even with "force" being enabled.
Change the warning message to avoid such a misunderstanding.

Tested on a Dell Inspiron 3505.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/dell-smm-hwmon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index f7bab1a91b93..bf13852afe48 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -1354,13 +1354,13 @@ static int __init dell_smm_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, data);

 	if (dmi_check_system(i8k_blacklist_fan_support_dmi_table)) {
-		dev_warn(&pdev->dev, "broken Dell BIOS detected, disallow fan support\n");
+		dev_warn(&pdev->dev, "BIOS has broken fan support\n");
 		if (!force)
 			data->disallow_fan_support = true;
 	}

 	if (dmi_check_system(i8k_blacklist_fan_type_dmi_table)) {
-		dev_warn(&pdev->dev, "broken Dell BIOS detected, disallow fan type call\n");
+		dev_warn(&pdev->dev, "BIOS has broken fan type call\n");
 		if (!force)
 			data->disallow_fan_type_call = true;
 	}
--
2.30.2

Re: [PATCH 3/3] hwmon: (dell-smm) Improve warning messages

[Pali Rohár]

On Sunday 21 August 2022 17:17:13 Armin Wolf wrote:
> When dell-smm-hwmon is loaded on a machine with a buggy BIOS
> with the option "force" being enabled, it wrongly prints
> what the buggy features where disabled. This may cause
> users to wrongly assume that the driver still protects them
> from these BIOS bugs even with "force" being enabled.
> Change the warning message to avoid such a misunderstanding.

Should not there be also FW_BUG too?

I'm thinking more about message, would not it be better to print also
information if fan support and fan type call is allowed or disallowed
(based on force argument) when broken BIOS is detected?

> Tested on a Dell Inspiron 3505.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/hwmon/dell-smm-hwmon.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index f7bab1a91b93..bf13852afe48 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -1354,13 +1354,13 @@ static int __init dell_smm_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, data);
> 
>  	if (dmi_check_system(i8k_blacklist_fan_support_dmi_table)) {
> -		dev_warn(&pdev->dev, "broken Dell BIOS detected, disallow fan support\n");
> +		dev_warn(&pdev->dev, "BIOS has broken fan support\n");
>  		if (!force)
>  			data->disallow_fan_support = true;
>  	}
> 
>  	if (dmi_check_system(i8k_blacklist_fan_type_dmi_table)) {
> -		dev_warn(&pdev->dev, "broken Dell BIOS detected, disallow fan type call\n");
> +		dev_warn(&pdev->dev, "BIOS has broken fan type call\n");
>  		if (!force)
>  			data->disallow_fan_type_call = true;
>  	}
> --
> 2.30.2
> 

Re: [PATCH 3/3] hwmon: (dell-smm) Improve warning messages

[Armin Wolf]

Am 21.08.22 um 17:46 schrieb Pali Rohár:

> On Sunday 21 August 2022 17:17:13 Armin Wolf wrote:
>> When dell-smm-hwmon is loaded on a machine with a buggy BIOS
>> with the option "force" being enabled, it wrongly prints
>> what the buggy features where disabled. This may cause
>> users to wrongly assume that the driver still protects them
>> from these BIOS bugs even with "force" being enabled.
>> Change the warning message to avoid such a misunderstanding.
> Should not there be also FW_BUG too?

Since the driver itself is unable to detect if a buggy BIOS was fixed,
i would not use FW_BUG here since it may cause confusion if such a message
was printed on an already fixed BIOS.

> I'm thinking more about message, would not it be better to print also
> information if fan support and fan type call is allowed or disallowed
> (based on force argument) when broken BIOS is detected?

Something like "Disabling fan support due to BIOS bugs" and "Enabling fan support despite BIOS bugs"?
The first message could only be a notice message, while the second message could be a warning message.

Armin Wolf

>> Tested on a Dell Inspiron 3505.
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   drivers/hwmon/dell-smm-hwmon.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>> index f7bab1a91b93..bf13852afe48 100644
>> --- a/drivers/hwmon/dell-smm-hwmon.c
>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>> @@ -1354,13 +1354,13 @@ static int __init dell_smm_probe(struct platform_device *pdev)
>>   	platform_set_drvdata(pdev, data);
>>
>>   	if (dmi_check_system(i8k_blacklist_fan_support_dmi_table)) {
>> -		dev_warn(&pdev->dev, "broken Dell BIOS detected, disallow fan support\n");
>> +		dev_warn(&pdev->dev, "BIOS has broken fan support\n");
>>   		if (!force)
>>   			data->disallow_fan_support = true;
>>   	}
>>
>>   	if (dmi_check_system(i8k_blacklist_fan_type_dmi_table)) {
>> -		dev_warn(&pdev->dev, "broken Dell BIOS detected, disallow fan type call\n");
>> +		dev_warn(&pdev->dev, "BIOS has broken fan type call\n");
>>   		if (!force)
>>   			data->disallow_fan_type_call = true;
>>   	}
>> --
>> 2.30.2
>>

Re: [PATCH 3/3] hwmon: (dell-smm) Improve warning messages

[Guenter Roeck]

On Sun, Aug 21, 2022 at 05:17:13PM +0200, Armin Wolf wrote:
> When dell-smm-hwmon is loaded on a machine with a buggy BIOS
> with the option "force" being enabled, it wrongly prints
> what the buggy features where disabled. This may cause
> users to wrongly assume that the driver still protects them
> from these BIOS bugs even with "force" being enabled.
> Change the warning message to avoid such a misunderstanding.
> 
> Tested on a Dell Inspiron 3505.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/hwmon/dell-smm-hwmon.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index f7bab1a91b93..bf13852afe48 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -1354,13 +1354,13 @@ static int __init dell_smm_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, data);
> 
>  	if (dmi_check_system(i8k_blacklist_fan_support_dmi_table)) {
> -		dev_warn(&pdev->dev, "broken Dell BIOS detected, disallow fan support\n");
> +		dev_warn(&pdev->dev, "BIOS has broken fan support\n");

I don't see that as improvement. It no longer mentions that fan support
is disabled if it is disabled.

>  		if (!force)
>  			data->disallow_fan_support = true;
>  	}
> 
>  	if (dmi_check_system(i8k_blacklist_fan_type_dmi_table)) {
> -		dev_warn(&pdev->dev, "broken Dell BIOS detected, disallow fan type call\n");
> +		dev_warn(&pdev->dev, "BIOS has broken fan type call\n");

Same as above.

>  		if (!force)
>  			data->disallow_fan_type_call = true;
>  	}
> --
> 2.30.2
>