[PATCH 0/3] hwmon: (dell-smm) Improve init code

[PATCH 0/3] hwmon: (dell-smm) Improve init code

[Armin Wolf]

This patch series improves the init code of the dell_smm_hwmon
driver. The first patch speeds up device initialisation by avoiding
unnecessary SMM calls during init, which might be slow on some
machines. The second patch is a small cleanup patch, while the
third patch allows for easier diagnosis of audio problems caused
by really slow SMM calls.

Tested on a Dell Inspiron 3505.

Armin Wolf (3):
  hwmon: (dell-smm) Avoid unnecessary SMM calls during init
  hwmon: (dell-smm) Cleanup init code
  hwmon: (dell-smm) Warn if SMM call took a very long time to execute

 drivers/hwmon/dell-smm-hwmon.c | 50 +++++++++++++++-------------------
 1 file changed, 22 insertions(+), 28 deletions(-)

--
2.30.2

[PATCH 1/3] hwmon: (dell-smm) Avoid unnecessary SMM calls during init

[Armin Wolf]

When the driver tries to detect the fan multiplier during
module initialisation, it issues one SMM call for each fan.
Those SMM calls are however redundant and also try to query
fans which may not be present.
Fix that by detecting the fan multiplier during hwmon
initialisation when no extra SMM calls are needed.
Also dont assume the last nominal speed entry to be the
biggest and instead check all entries.

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

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 30b6f0c28093..202ee884de9e 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -50,7 +50,7 @@
 #define I8K_SMM_GET_DELL_SIG2	0xffa3

 #define I8K_FAN_MULT		30
-#define I8K_FAN_MAX_RPM		30000
+#define I8K_FAN_RPM_THRESHOLD	1000
 #define I8K_MAX_TEMP		127

 #define I8K_FN_NONE		0x00
@@ -326,7 +326,7 @@ static int __init i8k_get_fan_nominal_speed(const struct dell_smm_data *data, u8
 	if (data->disallow_fan_support)
 		return -EINVAL;

-	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * data->i8k_fan_mult;
+	return i8k_smm(&regs) ? : (regs.eax & 0xffff);
 }

 /*
@@ -776,6 +776,7 @@ static int dell_smm_read(struct device *dev, enum hwmon_sensor_types type, u32 a
 			 long *val)
 {
 	struct dell_smm_data *data = dev_get_drvdata(dev);
+	int mult = data->i8k_fan_mult;
 	int ret;

 	switch (type) {
@@ -804,11 +805,11 @@ static int dell_smm_read(struct device *dev, enum hwmon_sensor_types type, u32 a

 			return 0;
 		case hwmon_fan_min:
-			*val = data->fan_nominal_speed[channel][0];
+			*val = data->fan_nominal_speed[channel][0] * mult;

 			return 0;
 		case hwmon_fan_max:
-			*val = data->fan_nominal_speed[channel][data->i8k_fan_max];
+			*val = data->fan_nominal_speed[channel][data->i8k_fan_max] * mult;

 			return 0;
 		case hwmon_fan_target:
@@ -819,7 +820,7 @@ static int dell_smm_read(struct device *dev, enum hwmon_sensor_types type, u32 a
 			if (ret > data->i8k_fan_max)
 				ret = data->i8k_fan_max;

-			*val = data->fan_nominal_speed[channel][ret];
+			*val = data->fan_nominal_speed[channel][ret] * mult;

 			return 0;
 		default:
@@ -1071,6 +1072,13 @@ static int __init dell_smm_init_hwmon(struct device *dev)
 				break;
 			}
 			data->fan_nominal_speed[i][state] = err;
+			/*
+			 * Autodetect fan multiplier based on nominal rpm if multiplier
+			 * was not specified as module param or in DMI. If fan reports
+			 * rpm value too high then set multiplier to 1.
+			 */
+			if (!fan_mult && err > I8K_FAN_RPM_THRESHOLD)
+				data->i8k_fan_mult = 1;
 		}
 	}

@@ -1359,7 +1367,6 @@ static int __init dell_smm_probe(struct platform_device *pdev)
 	struct dell_smm_data *data;
 	const struct dmi_system_id *id, *fan_control;
 	int ret;
-	u8 fan;

 	data = devm_kzalloc(&pdev->dev, sizeof(struct dell_smm_data), GFP_KERNEL);
 	if (!data)
@@ -1414,24 +1421,8 @@ static int __init dell_smm_probe(struct platform_device *pdev)
 		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
 	}

-	if (!fan_mult) {
-		/*
-		 * Autodetect fan multiplier based on nominal rpm
-		 * If fan reports rpm value too high then set multiplier to 1
-		 */
-		for (fan = 0; fan < DELL_SMM_NO_FANS; ++fan) {
-			ret = i8k_get_fan_nominal_speed(data, fan, data->i8k_fan_max);
-			if (ret < 0)
-				continue;
-
-			if (ret > I8K_FAN_MAX_RPM)
-				data->i8k_fan_mult = 1;
-			break;
-		}
-	} else {
-		/* Fan multiplier was specified in module param or in dmi */
+	if (fan_mult)
 		data->i8k_fan_mult = fan_mult;
-	}

 	ret = dell_smm_init_hwmon(&pdev->dev);
 	if (ret)
--
2.30.2

Re: [PATCH 1/3] hwmon: (dell-smm) Avoid unnecessary SMM calls during init

[Guenter Roeck]

On Tue, Apr 26, 2022 at 10:03:18PM +0200, Armin Wolf wrote:
> When the driver tries to detect the fan multiplier during
> module initialisation, it issues one SMM call for each fan.
> Those SMM calls are however redundant and also try to query
> fans which may not be present.
> Fix that by detecting the fan multiplier during hwmon
> initialisation when no extra SMM calls are needed.
> Also dont assume the last nominal speed entry to be the
> biggest and instead check all entries.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/hwmon/dell-smm-hwmon.c | 37 +++++++++++++---------------------
>  1 file changed, 14 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 30b6f0c28093..202ee884de9e 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -50,7 +50,7 @@
>  #define I8K_SMM_GET_DELL_SIG2	0xffa3
> 
>  #define I8K_FAN_MULT		30
> -#define I8K_FAN_MAX_RPM		30000
> +#define I8K_FAN_RPM_THRESHOLD	1000
>  #define I8K_MAX_TEMP		127
> 
>  #define I8K_FN_NONE		0x00
> @@ -326,7 +326,7 @@ static int __init i8k_get_fan_nominal_speed(const struct dell_smm_data *data, u8
>  	if (data->disallow_fan_support)
>  		return -EINVAL;
> 
> -	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * data->i8k_fan_mult;
> +	return i8k_smm(&regs) ? : (regs.eax & 0xffff);
>  }
> 
>  /*
> @@ -776,6 +776,7 @@ static int dell_smm_read(struct device *dev, enum hwmon_sensor_types type, u32 a
>  			 long *val)
>  {
>  	struct dell_smm_data *data = dev_get_drvdata(dev);
> +	int mult = data->i8k_fan_mult;
>  	int ret;
> 
>  	switch (type) {
> @@ -804,11 +805,11 @@ static int dell_smm_read(struct device *dev, enum hwmon_sensor_types type, u32 a
> 
>  			return 0;
>  		case hwmon_fan_min:
> -			*val = data->fan_nominal_speed[channel][0];
> +			*val = data->fan_nominal_speed[channel][0] * mult;
> 
>  			return 0;
>  		case hwmon_fan_max:
> -			*val = data->fan_nominal_speed[channel][data->i8k_fan_max];
> +			*val = data->fan_nominal_speed[channel][data->i8k_fan_max] * mult;
> 
>  			return 0;
>  		case hwmon_fan_target:
> @@ -819,7 +820,7 @@ static int dell_smm_read(struct device *dev, enum hwmon_sensor_types type, u32 a
>  			if (ret > data->i8k_fan_max)
>  				ret = data->i8k_fan_max;
> 
> -			*val = data->fan_nominal_speed[channel][ret];
> +			*val = data->fan_nominal_speed[channel][ret] * mult;
> 
>  			return 0;
>  		default:
> @@ -1071,6 +1072,13 @@ static int __init dell_smm_init_hwmon(struct device *dev)
>  				break;
>  			}
>  			data->fan_nominal_speed[i][state] = err;
> +			/*
> +			 * Autodetect fan multiplier based on nominal rpm if multiplier
> +			 * was not specified as module param or in DMI. If fan reports
> +			 * rpm value too high then set multiplier to 1.
> +			 */
> +			if (!fan_mult && err > I8K_FAN_RPM_THRESHOLD)
> +				data->i8k_fan_mult = 1;
>  		}
>  	}
> 
> @@ -1359,7 +1367,6 @@ static int __init dell_smm_probe(struct platform_device *pdev)
>  	struct dell_smm_data *data;
>  	const struct dmi_system_id *id, *fan_control;
>  	int ret;
> -	u8 fan;
> 
>  	data = devm_kzalloc(&pdev->dev, sizeof(struct dell_smm_data), GFP_KERNEL);
>  	if (!data)
> @@ -1414,24 +1421,8 @@ static int __init dell_smm_probe(struct platform_device *pdev)
>  		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
>  	}
> 
> -	if (!fan_mult) {
> -		/*
> -		 * Autodetect fan multiplier based on nominal rpm
> -		 * If fan reports rpm value too high then set multiplier to 1
> -		 */
> -		for (fan = 0; fan < DELL_SMM_NO_FANS; ++fan) {
> -			ret = i8k_get_fan_nominal_speed(data, fan, data->i8k_fan_max);
> -			if (ret < 0)
> -				continue;
> -
> -			if (ret > I8K_FAN_MAX_RPM)
> -				data->i8k_fan_mult = 1;
> -			break;
> -		}
> -	} else {
> -		/* Fan multiplier was specified in module param or in dmi */
> +	if (fan_mult)

Might as well drop the if statement here.

>  		data->i8k_fan_mult = fan_mult;
> -	}
> 
>  	ret = dell_smm_init_hwmon(&pdev->dev);
>  	if (ret)
> --
> 2.30.2
> 

Re: [PATCH 1/3] hwmon: (dell-smm) Avoid unnecessary SMM calls during init

[Guenter Roeck]

On Tue, Apr 26, 2022 at 01:20:48PM -0700, Guenter Roeck wrote:
> On Tue, Apr 26, 2022 at 10:03:18PM +0200, Armin Wolf wrote:
> > When the driver tries to detect the fan multiplier during
> > module initialisation, it issues one SMM call for each fan.
> > Those SMM calls are however redundant and also try to query
> > fans which may not be present.
> > Fix that by detecting the fan multiplier during hwmon
> > initialisation when no extra SMM calls are needed.
> > Also dont assume the last nominal speed entry to be the
> > biggest and instead check all entries.
> > 
> > Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> > ---
> >  drivers/hwmon/dell-smm-hwmon.c | 37 +++++++++++++---------------------
> >  1 file changed, 14 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> > index 30b6f0c28093..202ee884de9e 100644
> > --- a/drivers/hwmon/dell-smm-hwmon.c
> > +++ b/drivers/hwmon/dell-smm-hwmon.c
> > @@ -50,7 +50,7 @@
> >  #define I8K_SMM_GET_DELL_SIG2	0xffa3
> > 
> >  #define I8K_FAN_MULT		30
> > -#define I8K_FAN_MAX_RPM		30000
> > +#define I8K_FAN_RPM_THRESHOLD	1000
> >  #define I8K_MAX_TEMP		127
> > 
> >  #define I8K_FN_NONE		0x00
> > @@ -326,7 +326,7 @@ static int __init i8k_get_fan_nominal_speed(const struct dell_smm_data *data, u8
> >  	if (data->disallow_fan_support)
> >  		return -EINVAL;
> > 
> > -	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * data->i8k_fan_mult;
> > +	return i8k_smm(&regs) ? : (regs.eax & 0xffff);
> >  }
> > 
> >  /*
> > @@ -776,6 +776,7 @@ static int dell_smm_read(struct device *dev, enum hwmon_sensor_types type, u32 a
> >  			 long *val)
> >  {
> >  	struct dell_smm_data *data = dev_get_drvdata(dev);
> > +	int mult = data->i8k_fan_mult;
> >  	int ret;
> > 
> >  	switch (type) {
> > @@ -804,11 +805,11 @@ static int dell_smm_read(struct device *dev, enum hwmon_sensor_types type, u32 a
> > 
> >  			return 0;
> >  		case hwmon_fan_min:
> > -			*val = data->fan_nominal_speed[channel][0];
> > +			*val = data->fan_nominal_speed[channel][0] * mult;
> > 
> >  			return 0;
> >  		case hwmon_fan_max:
> > -			*val = data->fan_nominal_speed[channel][data->i8k_fan_max];
> > +			*val = data->fan_nominal_speed[channel][data->i8k_fan_max] * mult;
> > 
> >  			return 0;
> >  		case hwmon_fan_target:
> > @@ -819,7 +820,7 @@ static int dell_smm_read(struct device *dev, enum hwmon_sensor_types type, u32 a
> >  			if (ret > data->i8k_fan_max)
> >  				ret = data->i8k_fan_max;
> > 
> > -			*val = data->fan_nominal_speed[channel][ret];
> > +			*val = data->fan_nominal_speed[channel][ret] * mult;
> > 
> >  			return 0;
> >  		default:
> > @@ -1071,6 +1072,13 @@ static int __init dell_smm_init_hwmon(struct device *dev)
> >  				break;
> >  			}
> >  			data->fan_nominal_speed[i][state] = err;
> > +			/*
> > +			 * Autodetect fan multiplier based on nominal rpm if multiplier
> > +			 * was not specified as module param or in DMI. If fan reports
> > +			 * rpm value too high then set multiplier to 1.
> > +			 */
> > +			if (!fan_mult && err > I8K_FAN_RPM_THRESHOLD)
> > +				data->i8k_fan_mult = 1;
> >  		}
> >  	}
> > 
> > @@ -1359,7 +1367,6 @@ static int __init dell_smm_probe(struct platform_device *pdev)
> >  	struct dell_smm_data *data;
> >  	const struct dmi_system_id *id, *fan_control;
> >  	int ret;
> > -	u8 fan;
> > 
> >  	data = devm_kzalloc(&pdev->dev, sizeof(struct dell_smm_data), GFP_KERNEL);
> >  	if (!data)
> > @@ -1414,24 +1421,8 @@ static int __init dell_smm_probe(struct platform_device *pdev)
> >  		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
> >  	}
> > 
> > -	if (!fan_mult) {
> > -		/*
> > -		 * Autodetect fan multiplier based on nominal rpm
> > -		 * If fan reports rpm value too high then set multiplier to 1
> > -		 */
> > -		for (fan = 0; fan < DELL_SMM_NO_FANS; ++fan) {
> > -			ret = i8k_get_fan_nominal_speed(data, fan, data->i8k_fan_max);
> > -			if (ret < 0)
> > -				continue;
> > -
> > -			if (ret > I8K_FAN_MAX_RPM)
> > -				data->i8k_fan_mult = 1;
> > -			break;
> > -		}
> > -	} else {
> > -		/* Fan multiplier was specified in module param or in dmi */
> > +	if (fan_mult)
> 
> Might as well drop the if statement here.
> 
Never mind, I see that is removed later anyway.

Guenter

> >  		data->i8k_fan_mult = fan_mult;
> > -	}
> > 
> >  	ret = dell_smm_init_hwmon(&pdev->dev);
> >  	if (ret)
> > --
> > 2.30.2
> > 

[PATCH 2/3] hwmon: (dell-smm) Cleanup init code

[Armin Wolf]

The default values for i8k_fan_mult and i8k_fan_max
should be assigend only if the values specified as
module params or in DMI are invalid/missing.

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

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 202ee884de9e..f13902414615 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -1373,8 +1373,6 @@ static int __init dell_smm_probe(struct platform_device *pdev)
 		return -ENOMEM;

 	mutex_init(&data->i8k_mutex);
-	data->i8k_fan_mult = I8K_FAN_MULT;
-	data->i8k_fan_max = I8K_FAN_HIGH;
 	platform_set_drvdata(pdev, data);

 	if (dmi_check_system(i8k_blacklist_fan_support_dmi_table)) {
@@ -1409,7 +1407,9 @@ static int __init dell_smm_probe(struct platform_device *pdev)
 			fan_max = conf->fan_max;
 	}

-	data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
+	/* All options must not be 0 */
+	data->i8k_fan_mult = fan_mult ? : I8K_FAN_MULT;
+	data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;
 	data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max);

 	fan_control = dmi_first_match(i8k_whitelist_fan_control);
@@ -1421,9 +1421,6 @@ static int __init dell_smm_probe(struct platform_device *pdev)
 		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
 	}

-	if (fan_mult)
-		data->i8k_fan_mult = fan_mult;
-
 	ret = dell_smm_init_hwmon(&pdev->dev);
 	if (ret)
 		return ret;
--
2.30.2

[PATCH 3/3] hwmon: (dell-smm) Warn if SMM call took a very long time to execute

[Armin Wolf]

If a particular SMM call takes a very long time to execute,
the user might experience audio problems. Print a warning
if a particular SMM call took over 0.250 seconds to execute,
so the user can check whether or not possible audio problems
are caused by this driver.

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

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index f13902414615..b960330eaded 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -49,6 +49,9 @@
 #define I8K_SMM_GET_DELL_SIG1	0xfea3
 #define I8K_SMM_GET_DELL_SIG2	0xffa3

+/* in usecs */
+#define DELL_SMM_MAX_DURATION  250000
+
 #define I8K_FAN_MULT		30
 #define I8K_FAN_RPM_THRESHOLD	1000
 #define I8K_MAX_TEMP		127
@@ -239,6 +242,9 @@ static int i8k_smm_func(void *par)
 	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x  (took %7lld usecs)\n", eax, ebx,
 		 (rc ? 0xffff : regs->eax & 0xffff), duration);

+	if (duration > DELL_SMM_MAX_DURATION)
+		pr_warn("SMM call took %lld usecs!\n", duration);
+
 	return rc;
 }

--
2.30.2

Re: [PATCH 3/3] hwmon: (dell-smm) Warn if SMM call took a very long time to execute

[Guenter Roeck]

On Tue, Apr 26, 2022 at 10:03:20PM +0200, Armin Wolf wrote:
> If a particular SMM call takes a very long time to execute,
> the user might experience audio problems. Print a warning
> if a particular SMM call took over 0.250 seconds to execute,
> so the user can check whether or not possible audio problems
> are caused by this driver.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/hwmon/dell-smm-hwmon.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index f13902414615..b960330eaded 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -49,6 +49,9 @@
>  #define I8K_SMM_GET_DELL_SIG1	0xfea3
>  #define I8K_SMM_GET_DELL_SIG2	0xffa3
> 
> +/* in usecs */
> +#define DELL_SMM_MAX_DURATION  250000
> +
>  #define I8K_FAN_MULT		30
>  #define I8K_FAN_RPM_THRESHOLD	1000
>  #define I8K_MAX_TEMP		127
> @@ -239,6 +242,9 @@ static int i8k_smm_func(void *par)
>  	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x  (took %7lld usecs)\n", eax, ebx,
>  		 (rc ? 0xffff : regs->eax & 0xffff), duration);
> 
> +	if (duration > DELL_SMM_MAX_DURATION)
> +		pr_warn("SMM call took %lld usecs!\n", duration);

pr_warn_once, maybe ? If this happens, it will happen a lot, and it doesn't
add much if any value to keep reporting the problem.

Guenter