Re: [PATCH] applesmc: Re-work SMC comms v2
From: Henrik Rydberg
Date: Thu Nov 05 2020 - 03:12:03 EST
Hi Brad,
Great to see this effort, it is certainly an area which could be
improved. After having seen several generations of Macbooks while
modifying much of that code, it became clear that the SMC communication
got refreshed a few times over the years. Every tiny change had to be
tested on all machines, or kept separate for a particular generation, or
something would break.
I have not followed the back story here, but I imagine the need has
arisen because of a new refresh, and so this patch only needs to
strictly apply to a new generation. I would therefore advice that you
write the patch in that way, reducing the actual change to zero for
earlier generations. It also makes it easier to test the effect of the
new approach on older systems. I should be able to help testing on a
2008 and 2011 model once we get to that stage.
Thanks,
Henrik
On 2020-11-05 08:26, Brad Campbell wrote:
Commit fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()") introduced
an issue whereby communication with the SMC became unreliable with write
errors like :
[ 120.378614] applesmc: send_byte(0x00, 0x0300) fail: 0x40
[ 120.378621] applesmc: LKSB: write data fail
[ 120.512782] applesmc: send_byte(0x00, 0x0300) fail: 0x40
[ 120.512787] applesmc: LKSB: write data fail
The original code appeared to be timing sensitive and was not reliable with
the timing changes in the aforementioned commit.
This patch re-factors the SMC communication to remove the timing
dependencies and restore function with the changes previously committed.
v2 : Address logic and coding style
Reported-by: Andreas Kemnade <andreas@xxxxxxxxxxxx>
Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
Signed-off-by: Brad Campbell <brad@xxxxxxxxxxxxxxx>
---
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index a18887990f4a..de890f3ec12f 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -42,6 +42,11 @@
#define APPLESMC_MAX_DATA_LENGTH 32
+/* Apple SMC status bits */
+#define SMC_STATUS_AWAITING_DATA BIT(0) /* SMC has data waiting */
+#define SMC_STATUS_IB_CLOSED BIT(1) /* Will ignore any input */
+#define SMC_STATUS_BUSY BIT(2) /* Command in progress */
+
/* wait up to 128 ms for a status change. */
#define APPLESMC_MIN_WAIT 0x0010
#define APPLESMC_RETRY_WAIT 0x0100
@@ -151,65 +156,69 @@ static unsigned int key_at_index;
static struct workqueue_struct *applesmc_led_wq;
/*
- * wait_read - Wait for a byte to appear on SMC port. Callers must
- * hold applesmc_lock.
+ * Wait for specific status bits with a mask on the SMC
+ * Used before and after writes, and before reads
*/
-static int wait_read(void)
+
+static int wait_status(u8 val, u8 mask)
{
unsigned long end = jiffies + (APPLESMC_MAX_WAIT * HZ) / USEC_PER_SEC;
u8 status;
int us;
for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
- usleep_range(us, us * 16);
status = inb(APPLESMC_CMD_PORT);
- /* read: wait for smc to settle */
- if (status & 0x01)
+ if ((status & mask) == val)
return 0;
/* timeout: give up */
if (time_after(jiffies, end))
break;
- }
-
- pr_warn("wait_read() fail: 0x%02x\n", status);
+ usleep_range(us, us * 16);
+ }
return -EIO;
}
/*
- * send_byte - Write to SMC port, retrying when necessary. Callers
+ * send_byte_data - Write to SMC data port. Callers
* must hold applesmc_lock.
+ * Parameter skip must be true on the last write of any
+ * command or it'll time out.
*/
-static int send_byte(u8 cmd, u16 port)
+
+static int send_byte_data(u8 cmd, u16 port, bool skip)
{
- u8 status;
- int us;
- unsigned long end = jiffies + (APPLESMC_MAX_WAIT * HZ) / USEC_PER_SEC;
+ int ret;
+ ret = wait_status(SMC_STATUS_BUSY, SMC_STATUS_BUSY | SMC_STATUS_IB_CLOSED);
+ if (ret)
+ return ret;
outb(cmd, port);
- for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
- usleep_range(us, us * 16);
- 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 (time_after(jiffies, end))
- break;
- /* busy: long wait and resend */
- udelay(APPLESMC_RETRY_WAIT);
- outb(cmd, port);
- }
+ return wait_status(skip ? 0 : SMC_STATUS_BUSY, SMC_STATUS_BUSY);
+}
- pr_warn("send_byte(0x%02x, 0x%04x) fail: 0x%02x\n", cmd, port, status);
- return -EIO;
+static int send_byte(u8 cmd, u16 port)
+{
+ return send_byte_data(cmd, port, false);
}
+/*
+ * send_command - Write a command to the SMC. Callers must hold applesmc_lock.
+ * If SMC is in undefined state, any new command write resets the state machine.
+ */
+
static int send_command(u8 cmd)
{
- return send_byte(cmd, APPLESMC_CMD_PORT);
+ u8 status;
+ int ret;
+
+ ret = wait_status(0, SMC_STATUS_IB_CLOSED);
+ if (ret)
+ return ret;
+
+ status = inb(APPLESMC_CMD_PORT);
+
+ outb(cmd, APPLESMC_CMD_PORT);
+ return wait_status(SMC_STATUS_BUSY, SMC_STATUS_BUSY);
}
static int send_argument(const char *key)
@@ -239,7 +248,9 @@ static int read_smc(u8 cmd, const char *key, u8 *buffer, u8 len)
}
for (i = 0; i < len; i++) {
- if (wait_read()) {
+ if (wait_status(SMC_STATUS_AWAITING_DATA | SMC_STATUS_BUSY,
+ SMC_STATUS_AWAITING_DATA | SMC_STATUS_BUSY |
+ SMC_STATUS_IB_CLOSED)) {
pr_warn("%.4s: read data[%d] fail\n", key, i);
return -EIO;
}
@@ -250,7 +261,7 @@ static int read_smc(u8 cmd, const char *key, u8 *buffer, u8 len)
for (i = 0; i < 16; i++) {
udelay(APPLESMC_MIN_WAIT);
status = inb(APPLESMC_CMD_PORT);
- if (!(status & 0x01))
+ if (!(status & SMC_STATUS_AWAITING_DATA))
break;
data = inb(APPLESMC_DATA_PORT);
}
@@ -275,7 +286,7 @@ static int write_smc(u8 cmd, const char *key, const u8 *buffer, u8 len)
}
for (i = 0; i < len; i++) {
- if (send_byte(buffer[i], APPLESMC_DATA_PORT)) {
+ if (send_byte_data(buffer[i], APPLESMC_DATA_PORT, i == len - 1)) {
pr_warn("%s: write data fail\n", key);
return -EIO;
}