Re: [REGRESSION] hwmon: (applesmc) avoid overlong udelay()

[Brad Campbell <brad@fnarfbargle.com>]

[-- Attachment #1: Type: text/plain, Size: 2274 bytes --]

On 5/11/20 3:43 pm, Guenter Roeck wrote:
> On 11/4/20 6:18 PM, Brad Campbell wrote:
>> On 5/11/20 12:20 am, Andreas Kemnade wrote:
>>> On Tue, 3 Nov 2020 16:56:32 +1100
>>> Brad Campbell <brad@fnarfbargle.com> wrote:
>>
>>>> If anyone with a Mac having a conventional SMC and seeing issues on 5.9 could test this it'd be appreciated. I'm not saying this code is "correct", but it "works for me".
>>>>
>>> Seems to work here.
>>>    dmesg  | grep applesmc
>>>
>>> [    1.350782] applesmc: key=561 fan=1 temp=33 index=33 acc=0 lux=2 kbd=1
>>> [    1.350922] applesmc applesmc.768: hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
>>> [   17.748504] applesmc: wait_status looping 2: 0x4a, 0x4c, 0x4f
>>> [  212.008952] applesmc: wait_status looping 2: 0x44, 0x40, 0x4e
>>> [  213.033930] applesmc: wait_status looping 2: 0x44, 0x40, 0x4e
>>> [  213.167908] applesmc: wait_status looping 2: 0x44, 0x40, 0x4e
>>> [  219.087854] applesmc: wait_status looping 2: 0x44, 0x40, 0x4e
>>>
>>> Tested it on top of 5.9
>>
>> Much appreciated Andreas.
>>
>> I'm not entirely sure where to go from here. I'd really like some wider testing before cleaning this up and submitting it. It puts extra checks & constraints on the comms with the SMC that weren't there previously.
>>
>> I guess given there doesn't appear to have been a major outcry that the driver broke in 5.9 might indicate that nobody is using it, or that it only broke on certain machines?
>>
>> Can we get some guidance from the hwmon maintainers on what direction they'd like to take? I don't really want to push this forward without broader testing only to find it breaks a whole heap of machines on the basis that it fixes mine.
>>
> 
> Trick question ;-).
> 
> I'd suggest to keep it simple. Your patch seems to be quite complicated
> and checks a lot of bits. Reducing that to a minimum would help limiting
> the risk that some of those bits are interpreted differently on other
> systems.
> 
> Guenter
> 
> 
Appreciate the feedback.

This would be the bare minimum based on the bits use in the original code. If the original code worked "well enough" then this should be relatively safe.

Tested on both machines I have access to.

Regards,
Brad

[-- Attachment #2: smc.patch.9 --]
[-- Type: text/plain, Size: 5474 bytes --]

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index a18887990f4a..22cc5122ce9a 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 from VirtualSMC */
+#define SMC_STATUS_AWAITING_DATA  0x01  ///< Data waiting to be read
+#define SMC_STATUS_IB_CLOSED      0x02  /// A write is pending / will ignore input
+#define SMC_STATUS_BUSY           0x04  ///< Busy in the middle of a command.
+
 /* wait up to 128 ms for a status change. */
 #define APPLESMC_MIN_WAIT	0x0010
 #define APPLESMC_RETRY_WAIT	0x0100
@@ -151,65 +156,77 @@ 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);
+		}
+	pr_warn("wait_status timeout: 0x%02x, 0x%02x, 0x%02x\n", status, val, mask);
 	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;
+	u8 wstat = SMC_STATUS_BUSY;
 
+	if (skip)
+		wstat = 0;
+	if (wait_status(SMC_STATUS_BUSY,
+	SMC_STATUS_BUSY | SMC_STATUS_IB_CLOSED))
+		goto fail;
 	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);
-	}
-
-	pr_warn("send_byte(0x%02x, 0x%04x) fail: 0x%02x\n", cmd, port, status);
+	if (!wait_status(wstat,
+	SMC_STATUS_BUSY))
+		return 0;
+fail:
+	pr_warn("send_byte_data(0x%02x, 0x%04x) fail\n", cmd, APPLESMC_CMD_PORT);
 	return -EIO;
 }
 
+/*
+ * 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;
+
+	if (wait_status(0,
+	SMC_STATUS_IB_CLOSED)) {
+		pr_warn("send_command SMC was busy\n");
+		goto fail; }
+
+	status = inb(APPLESMC_CMD_PORT);
+
+	outb(cmd, APPLESMC_CMD_PORT);
+	if (!wait_status(SMC_STATUS_BUSY,
+	SMC_STATUS_BUSY))
+		return 0;
+fail:
+	pr_warn("send_cmd(0x%02x, 0x%04x) fail\n", cmd, APPLESMC_CMD_PORT);
+	return -EIO;
 }
 
 static int send_argument(const char *key)
@@ -217,7 +234,8 @@ static int send_argument(const char *key)
 	int i;
 
 	for (i = 0; i < 4; i++)
-		if (send_byte(key[i], APPLESMC_DATA_PORT))
+	/* Parameter skip is false as we always send data after an argument */
+		if (send_byte_data(key[i], APPLESMC_DATA_PORT, false))
 			return -EIO;
 	return 0;
 }
@@ -233,13 +251,15 @@ static int read_smc(u8 cmd, const char *key, u8 *buffer, u8 len)
 	}
 
 	/* This has no effect on newer (2012) SMCs */
-	if (send_byte(len, APPLESMC_DATA_PORT)) {
+	if (send_byte_data(len, APPLESMC_DATA_PORT, false)) {
 		pr_warn("%.4s: read len fail\n", key);
 		return -EIO;
 	}
 
 	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 +270,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);
 	}
@@ -263,20 +283,21 @@ static int read_smc(u8 cmd, const char *key, u8 *buffer, u8 len)
 static int write_smc(u8 cmd, const char *key, const u8 *buffer, u8 len)
 {
 	int i;
+	u8 end = len-1;
 
 	if (send_command(cmd) || send_argument(key)) {
 		pr_warn("%s: write arg fail\n", key);
 		return -EIO;
 	}
 
-	if (send_byte(len, APPLESMC_DATA_PORT)) {
+	if (send_byte_data(len, APPLESMC_DATA_PORT, false)) {
 		pr_warn("%.4s: write len fail\n", key);
 		return -EIO;
 	}
 
 	for (i = 0; i < len; i++) {
-		if (send_byte(buffer[i], APPLESMC_DATA_PORT)) {
-			pr_warn("%s: write data fail\n", key);
+		if (send_byte_data(buffer[i], APPLESMC_DATA_PORT, (i == end))) {
+			pr_warn("%s: write data fail at %i\n", key, i);
 			return -EIO;
 		}
 	}