| Message ID | 20201117072401.GE1111239@mwanda (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | hwmon: (applesmc) Fix smc_sane() function | expand |
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 > >
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) >
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
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)
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(-)