linux-kernel - Re: [PATCH v3 1/1] hwmon: (gpio-fan) Add regulator support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <851fe398-28da-4ca7-8b0d-b8111811779c@roeck-us.net>
Date: Mon, 24 Mar 2025 07:23:34 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Alexander Stein <alexander.stein@...tq-group.com>,
 Jean Delvare <jdelvare@...e.com>
Cc: linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/1] hwmon: (gpio-fan) Add regulator support

On 3/24/25 05:45, Alexander Stein wrote:
> FANs might be supplied by a regulator which needs to be enabled as well.
> This is implemented using runtime PM. Every time speed_index changes from
> 0 to non-zero and vise versa RPM is resumed or suspended.
> Intitial RPM state is determined by initial value of speed_index.
> 
> Signed-off-by: Alexander Stein <alexander.stein@...tq-group.com>
> ---
> Patch 1 & 2 from v1 [1] have already been applied, although number 2 [2] is not
> yet showing in next-20250305. Patches 3 & 4 (just removing comments) from v1
> have been dropped, so only this patch remains.
> 
> Changes in v3:
> * Remove noisy dev_err calls related to runtime pm
> * Properly propagate return codes from set_fan_speed
> 
> Changes in v2:
> * Make regulator non-optional
> 
> [1] https://lore.kernel.org/all/20250210145934.761280-1-alexander.stein@ew.tq-group.com/
> [2] https://web.git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git/commit/?h=hwmon-next&id=9fee7d19bab635f89223cc40dfd2c8797fdc4988
> ---
>   drivers/hwmon/gpio-fan.c | 104 +++++++++++++++++++++++++++++++++------
>   1 file changed, 90 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
> index cee3fa146d69a..4c736b7eb5473 100644
> --- a/drivers/hwmon/gpio-fan.c
> +++ b/drivers/hwmon/gpio-fan.c
> @@ -20,6 +20,9 @@
>   #include <linux/gpio/consumer.h>
>   #include <linux/of.h>
>   #include <linux/of_platform.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
>   #include <linux/thermal.h>
>   
>   struct gpio_fan_speed {
> @@ -42,6 +45,7 @@ struct gpio_fan_data {
>   	bool			pwm_enable;
>   	struct gpio_desc	*alarm_gpio;
>   	struct work_struct	alarm_work;
> +	struct regulator	*supply;
>   };
>   
>   /*
> @@ -125,13 +129,32 @@ static int __get_fan_ctrl(struct gpio_fan_data *fan_data)
>   }
>   
>   /* Must be called with fan_data->lock held, except during initialization. */
> -static void set_fan_speed(struct gpio_fan_data *fan_data, int speed_index)
> +static int set_fan_speed(struct gpio_fan_data *fan_data, int speed_index)
>   {
>   	if (fan_data->speed_index == speed_index)
> -		return;
> +		return 0;
> +
> +	if (fan_data->speed_index == 0 && speed_index > 0) {
> +		int ret;
> +
> +		ret = pm_runtime_resume_and_get(fan_data->dev);
> +		if (ret < 0)
> +			return ret;
> +	}
>   
>   	__set_fan_ctrl(fan_data, fan_data->speed[speed_index].ctrl_val);
> +
> +	if (fan_data->speed_index > 0 && speed_index == 0) {
> +		int ret;
> +
> +		ret = pm_runtime_put_sync(fan_data->dev);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>   	fan_data->speed_index = speed_index;
> +
> +	return 0;
>   }
>   
>   static int get_fan_speed_index(struct gpio_fan_data *fan_data)
> @@ -189,7 +212,9 @@ static ssize_t pwm1_store(struct device *dev, struct device_attribute *attr,
>   	}
>   
>   	speed_index = DIV_ROUND_UP(pwm * (fan_data->num_speed - 1), 255);
> -	set_fan_speed(fan_data, speed_index);
> +	ret = set_fan_speed(fan_data, speed_index);

That means pre-initializing ret is no longer necessary.

> +	if (!ret)
> +		ret = count;
>   

This is confusing. Please use the same error handling everywhere.
I would suggest
	return ret ? : count;
or
	return ret ? ret : count;

>   exit_unlock:
>   	mutex_unlock(&fan_data->lock);
> @@ -211,6 +236,7 @@ static ssize_t pwm1_enable_store(struct device *dev,
>   {
>   	struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
>   	unsigned long val;
> +	int ret;
>   
>   	if (kstrtoul(buf, 10, &val) || val > 1)
>   		return -EINVAL;
> @@ -224,11 +250,14 @@ static ssize_t pwm1_enable_store(struct device *dev,
>   
>   	/* Disable manual control mode: set fan at full speed. */
>   	if (val == 0)
> -		set_fan_speed(fan_data, fan_data->num_speed - 1);
> +		ret = set_fan_speed(fan_data, fan_data->num_speed - 1);
>   
>   	mutex_unlock(&fan_data->lock);
>   
> -	return count;
> +	if (ret)
> +		return ret;
> +	else
> +		return count;

Static analyzers will rightfully complain that else after return is pointless.
Try
	return ret ? : count;
or at least
	return ret ? ret : count;

Thanks,
Guenter