On 9/24/19 10:47 AM, Nick Desaulniers wrote:
Fixes the following 2 issues in the driver:
1. Left shifting a signed integer is undefined behavior. Unsigned
integral types should be used for bitwise operations.
2. The delay scales from 0x0010 to 0x20000 by powers of 2, but udelay
will result in a linkage failure when given a constant that's greater
than 20000 (0x4E20). Agressive loop unrolling can fully unroll the
loop, resulting in later iterations overflowing the call to udelay.
2 is fixed via splitting the loop in two, iterating the first up to the
point where udelay would overflow, then switching to mdelay, as
suggested in Documentation/timers/timers-howto.rst.
Reported-by: Tomasz Paweł Gajc <tpg...@gmail.com>
Link: https://github.com/ClangBuiltLinux/linux/issues/678
Debugged-by: Nathan Chancellor <natechancel...@gmail.com>
Signed-off-by: Nick Desaulniers <ndesaulni...@google.com>
---
Changes V1 -> V2:
* The first loop in send_byte() needs to break out on the same condition
now. Technically, the loop condition could even be removed. The diff
looks funny because of the duplicated logic between existing and newly
added for loops.
That is a delay()-internal dependency, and completely undocumented. This code
will fall apart if the implementation of udelay() is ever changed. This
also depends on the architecture - in some cases, mdelay() is implemented
as udelay(n * 1000).
drivers/hwmon/applesmc.c | 35 +++++++++++++++++++++++++++++++----
1 file changed, 31 insertions(+), 4 deletions(-)
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 183ff3d25129..c76adb504dff 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -46,6 +46,7 @@
#define APPLESMC_MIN_WAIT 0x0010
#define APPLESMC_RETRY_WAIT 0x0100
#define APPLESMC_MAX_WAIT 0x20000
+#define APPLESMC_UDELAY_MAX 20000
This is not really a problem in this driver; it is a system problem.
Anyone can call udelay() with a parameter longer than 20,000 us.
We can't add code like this all over the place because the implementation
of delay() is broken.
Besides, calling delay() with a parameter of 20,000 or more is a strong
indication that something is really wrong with the code. More on that
see below.
#define APPLESMC_READ_CMD 0x10
#define APPLESMC_WRITE_CMD 0x11
@@ -157,14 +158,23 @@ static struct workqueue_struct *applesmc_led_wq;
static int wait_read(void)
{
u8 status;
- int us;
- for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
+ unsigned int us;
+
+ for (us = APPLESMC_MIN_WAIT; us < APPLESMC_UDELAY_MAX; us <<= 1) {
udelay(us);
status = inb(APPLESMC_CMD_PORT);
/* read: wait for smc to settle */
if (status & 0x01)
return 0;
}
+ /* switch to mdelay for longer sleeps */
+ for (; us < APPLESMC_MAX_WAIT; us <<= 1) {
+ mdelay(us);
Shouldn't that be us / 1000 ? Seems to me the above will wait for
at least 20000 ms, which is a a tiny bit long.
Also, mdelay(n) is by default implemented as udelay(n * 1000).
Also, at the very least, this should be something like
if (us < limit)
delay(us);
else
mdelay(us / 1000);
instead of introducing a second loop. But more on that below.
+ status = inb(APPLESMC_CMD_PORT);
+ /* read: wait for smc to settle */
+ if (status & 0x01)
+ return 0;
+ }
pr_warn("wait_read() fail: 0x%02x\n", status);
return -EIO;
@@ -177,10 +187,10 @@ static int wait_read(void)
static int send_byte(u8 cmd, u16 port)
{
u8 status;
- int us;
+ unsigned int us;
outb(cmd, port);
- for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
+ for (us = APPLESMC_MIN_WAIT; us < APPLESMC_UDELAY_MAX; us <<= 1) {
udelay(us);
status = inb(APPLESMC_CMD_PORT);
/* write: wait for smc to settle */
@@ -190,6 +200,23 @@ static int send_byte(u8 cmd, u16 port)
if (status & 0x04)
return 0;
/* timeout: give up */
+ if (us << 1 == APPLESMC_UDELAY_MAX)
+ break;
+ /* busy: long wait and resend */
+ udelay(APPLESMC_RETRY_WAIT);
+ outb(cmd, port);
+ }
+ /* switch to mdelay for longer sleeps */
+ for (; us < APPLESMC_MAX_WAIT; us <<= 1) {
+ mdelay(us);
Again, I fail to understand why waiting for a multiple of 20 seconds
under any circumstances would make any sense. Maybe the idea was
to divide us by 1000 before entering the second loop ?
Looking into the code, there is no need to use udelay() in the first
place. It should be possible to replace the longer waits with
usleep_range(). Something like
if (us < some_low_value) // eg. 0x80
delay(us)
else
usleep_range(us, us * 2);
should do, and at the same time prevent the system from turning
into a space heater.
Thanks,
Guenter
+ status = inb(APPLESMC_CMD_PORT);
+ /* write: wait for smc to settle */
+ if (status & 0x02)
+ continue;
+ /* ready: cmd accepted, return */
+ if (status & 0x04)
+ return 0;
+ /* timeout: give up */
if (us << 1 == APPLESMC_MAX_WAIT)
break;
/* busy: long wait and resend */
