diff mbox series

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

Message ID 20220426200320.399435-2-W_Armin@gmx.de (mailing list archive)
State Superseded
Headers show
Series hwmon: (dell-smm) Improve init code | expand

Commit Message

Armin Wolf April 26, 2022, 8:03 p.m. UTC 
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(-)

--
2.30.2

Comments

Guenter Roeck April 26, 2022, 8:20 p.m. UTC | #1 
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
>
Guenter Roeck April 26, 2022, 8:33 p.m. UTC | #2 
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
> >
diff mbox series

Patch

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)