[PATCH] hwmon: (applesmc) Fix smc_sane() function

[PATCH] hwmon: (applesmc) Fix smc_sane() function

[Dan Carpenter]

This test is reversed so the function will return without sending
the APPLESMC_READ_CMD or completing the rest of the function.

Fixes: 4d64bb4ba5ec ("hwmon: (applesmc) Re-work SMC comms")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/hwmon/applesmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 79b498f816fe..289b39537683 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -227,7 +227,7 @@ static int smc_sane(void)
 	int ret;
 
 	ret = wait_status(0, SMC_STATUS_BUSY);
-	if (!ret)
+	if (ret)
 		return ret;
 	ret = send_command(APPLESMC_READ_CMD);
 	if (ret)
-- 
2.29.2

Re: [PATCH] hwmon: (applesmc) Fix smc_sane() function

[Brad Campbell]

G’day Dan,

Have you tested that change on hardware?

Sent from an annoyingly small mobile device with no keyboard.

> On 17 Nov 2020, at 7:52 pm, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> This test is reversed so the function will return without sending
> the APPLESMC_READ_CMD or completing the rest of the function.
> 
> Fixes: 4d64bb4ba5ec ("hwmon: (applesmc) Re-work SMC comms")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> drivers/hwmon/applesmc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 79b498f816fe..289b39537683 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -227,7 +227,7 @@ static int smc_sane(void)
>    int ret;
> 
>    ret = wait_status(0, SMC_STATUS_BUSY);
> -    if (!ret)
> +    if (ret)
>        return ret;
>    ret = send_command(APPLESMC_READ_CMD);
>    if (ret)
> -- 
> 2.29.2
> 
> 

Re: [PATCH] hwmon: (applesmc) Fix smc_sane() function

[Brad Campbell]

On 17/11/20 6:24 pm, Dan Carpenter wrote:
> This test is reversed so the function will return without sending
> the APPLESMC_READ_CMD or completing the rest of the function.

That is as designed. The routine looks at the busy line and if it's already in the right state then it simply ends. If not then it tries to "re-align" the state machine by sending a new command.

> Fixes: 4d64bb4ba5ec ("hwmon: (applesmc) Re-work SMC comms")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/hwmon/applesmc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 79b498f816fe..289b39537683 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -227,7 +227,7 @@ static int smc_sane(void)
>  	int ret;
>  
>  	ret = wait_status(0, SMC_STATUS_BUSY);
> -	if (!ret)
> +	if (ret)
>  		return ret;
>  	ret = send_command(APPLESMC_READ_CMD);
>  	if (ret)
> 

Re: [PATCH] hwmon: (applesmc) Fix smc_sane() function

[Dan Carpenter]

On Tue, Nov 17, 2020 at 08:58:47PM +1100, Brad Campbell wrote:
> On 17/11/20 6:24 pm, Dan Carpenter wrote:
> > This test is reversed so the function will return without sending
> > the APPLESMC_READ_CMD or completing the rest of the function.
> 
> That is as designed. The routine looks at the busy line and if it's
> already in the right state then it simply ends. If not then it tries
> to "re-align" the state machine by sending a new command.

Ah.  Ok.  It looked like a typo.  These "if (!ret) return ret;" typos
are surprisingly common so I review them every time they are added.
It's a static analysis warning, that I haven't published.  I kind of
feel like it would be more clearly intentional if it were written like
so:

	ret = wait_status(0, SMC_STATUS_BUSY);
	if (!ret)
		return 0;

But I try not to get too bogged down with style so let's leave it.

regards,
dan carpenter