All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] hwmon: (tacho) Fix a condition in aspeed_get_fan_tach_ch_rpm()
@ 2017-04-10 13:46 ` Dan Carpenter
  0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2017-04-10 13:46 UTC (permalink / raw)
  To: Joel Stanley, Jaghathiswari Rankappagounder Natarajan
  Cc: Jean Delvare, Guenter Roeck, linux-hwmon, kernel-janitors

What we want is for regmap_read() to return zero meaning success and
for RESULT_STATUS_MASK which is BIT(31) to be set set.

The current code is buggy in two ways.  It requires both failure
conditions should be met before it fails.  And if it times out then it
returns zero instead of -EIO.

Fixes: cc96f6f6d766 ("hwmon: Support for ASPEED PWM/Fan tach")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Static analysis.  Not tested.

diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
index 29010ad94208..2f22add5c73b 100644
--- a/drivers/hwmon/aspeed-pwm-tacho.c
+++ b/drivers/hwmon/aspeed-pwm-tacho.c
@@ -512,11 +512,11 @@ static u32 aspeed_get_fan_tach_ch_rpm(struct aspeed_pwm_tacho_data *priv,
 
 	msleep(sec);
 
-	while (!(regmap_read(priv->regmap, ASPEED_PTCR_RESULT, &val))
-			& !(val & RESULT_STATUS_MASK)) {
+	while (regmap_read(priv->regmap, ASPEED_PTCR_RESULT, &val) ||
+	       !(val & RESULT_STATUS_MASK)) {
 		timeout++;
 		if (timeout > 1)
-			return 0;
+			return -EIO;
 		msleep(sec);
 	}
 

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* [PATCH 1/2] hwmon: (tacho) Fix a condition in aspeed_get_fan_tach_ch_rpm()
@ 2017-04-10 13:46 ` Dan Carpenter
  0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2017-04-10 13:46 UTC (permalink / raw)
  To: Joel Stanley, Jaghathiswari Rankappagounder Natarajan
  Cc: Jean Delvare, Guenter Roeck, linux-hwmon, kernel-janitors

What we want is for regmap_read() to return zero meaning success and
for RESULT_STATUS_MASK which is BIT(31) to be set set.

The current code is buggy in two ways.  It requires both failure
conditions should be met before it fails.  And if it times out then it
returns zero instead of -EIO.

Fixes: cc96f6f6d766 ("hwmon: Support for ASPEED PWM/Fan tach")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Static analysis.  Not tested.

diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
index 29010ad94208..2f22add5c73b 100644
--- a/drivers/hwmon/aspeed-pwm-tacho.c
+++ b/drivers/hwmon/aspeed-pwm-tacho.c
@@ -512,11 +512,11 @@ static u32 aspeed_get_fan_tach_ch_rpm(struct aspeed_pwm_tacho_data *priv,
 
 	msleep(sec);
 
-	while (!(regmap_read(priv->regmap, ASPEED_PTCR_RESULT, &val))
-			& !(val & RESULT_STATUS_MASK)) {
+	while (regmap_read(priv->regmap, ASPEED_PTCR_RESULT, &val) ||
+	       !(val & RESULT_STATUS_MASK)) {
 		timeout++;
 		if (timeout > 1)
-			return 0;
+			return -EIO;
 		msleep(sec);
 	}
 

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] hwmon: (tacho) Fix a condition in aspeed_get_fan_tach_ch_rpm()
  2017-04-10 13:46 ` Dan Carpenter
@ 2017-04-10 20:31   ` Guenter Roeck
  -1 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2017-04-10 20:31 UTC (permalink / raw)
  To: Dan Carpenter, Joel Stanley, Jaghathiswari Rankappagounder Natarajan
  Cc: Jean Delvare, linux-hwmon, kernel-janitors

Hi Dan,

On 04/10/2017 06:46 AM, Dan Carpenter wrote:
> What we want is for regmap_read() to return zero meaning success and
> for RESULT_STATUS_MASK which is BIT(31) to be set set.
>
> The current code is buggy in two ways.  It requires both failure
> conditions should be met before it fails.  And if it times out then it
> returns zero instead of -EIO.
>
> Fixes: cc96f6f6d766 ("hwmon: Support for ASPEED PWM/Fan tach")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Static analysis.  Not tested.

Thanks for the report. My fault - I had asked for this and the problem
in the next patch to be fixed, I got a new version of the patch with the fix,
but I did not find the time to apply it. Sorry for the noise.

Guenter

>
> diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
> index 29010ad94208..2f22add5c73b 100644
> --- a/drivers/hwmon/aspeed-pwm-tacho.c
> +++ b/drivers/hwmon/aspeed-pwm-tacho.c
> @@ -512,11 +512,11 @@ static u32 aspeed_get_fan_tach_ch_rpm(struct aspeed_pwm_tacho_data *priv,
>
>  	msleep(sec);
>
> -	while (!(regmap_read(priv->regmap, ASPEED_PTCR_RESULT, &val))
> -			& !(val & RESULT_STATUS_MASK)) {
> +	while (regmap_read(priv->regmap, ASPEED_PTCR_RESULT, &val) ||
> +	       !(val & RESULT_STATUS_MASK)) {
>  		timeout++;
>  		if (timeout > 1)
> -			return 0;
> +			return -EIO;
>  		msleep(sec);
>  	}
>
>


^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] hwmon: (tacho) Fix a condition in aspeed_get_fan_tach_ch_rpm()
@ 2017-04-10 20:31   ` Guenter Roeck
  0 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2017-04-10 20:31 UTC (permalink / raw)
  To: Dan Carpenter, Joel Stanley, Jaghathiswari Rankappagounder Natarajan
  Cc: Jean Delvare, linux-hwmon, kernel-janitors

Hi Dan,

On 04/10/2017 06:46 AM, Dan Carpenter wrote:
> What we want is for regmap_read() to return zero meaning success and
> for RESULT_STATUS_MASK which is BIT(31) to be set set.
>
> The current code is buggy in two ways.  It requires both failure
> conditions should be met before it fails.  And if it times out then it
> returns zero instead of -EIO.
>
> Fixes: cc96f6f6d766 ("hwmon: Support for ASPEED PWM/Fan tach")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Static analysis.  Not tested.

Thanks for the report. My fault - I had asked for this and the problem
in the next patch to be fixed, I got a new version of the patch with the fix,
but I did not find the time to apply it. Sorry for the noise.

Guenter

>
> diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
> index 29010ad94208..2f22add5c73b 100644
> --- a/drivers/hwmon/aspeed-pwm-tacho.c
> +++ b/drivers/hwmon/aspeed-pwm-tacho.c
> @@ -512,11 +512,11 @@ static u32 aspeed_get_fan_tach_ch_rpm(struct aspeed_pwm_tacho_data *priv,
>
>  	msleep(sec);
>
> -	while (!(regmap_read(priv->regmap, ASPEED_PTCR_RESULT, &val))
> -			& !(val & RESULT_STATUS_MASK)) {
> +	while (regmap_read(priv->regmap, ASPEED_PTCR_RESULT, &val) ||
> +	       !(val & RESULT_STATUS_MASK)) {
>  		timeout++;
>  		if (timeout > 1)
> -			return 0;
> +			return -EIO;
>  		msleep(sec);
>  	}
>
>


^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-04-10 20:31 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-10 13:46 [PATCH 1/2] hwmon: (tacho) Fix a condition in aspeed_get_fan_tach_ch_rpm() Dan Carpenter
2017-04-10 13:46 ` Dan Carpenter
2017-04-10 20:31 ` Guenter Roeck
2017-04-10 20:31   ` Guenter Roeck

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.