| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 5 Nov 2020 08:56:04 +0100
From: Henrik Rydberg <rydberg@...math.org>
To: Brad Campbell <brad@...rfbargle.com>,
Guenter Roeck <linux@...ck-us.net>,
Andreas Kemnade <andreas@...nade.info>,
Jean Delvare <jdelvare@...e.com>
Cc: Arnd Bergmann <arnd@...db.de>, linux-hwmon@...r.kernel.org,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
hns@...delico.com
Subject: Re: [PATCH] applesmc: Re-work SMC comms v2
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@...nade.info>
> Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
> Signed-off-by: Brad Campbell <brad@...rfbargle.com>
>
> ---
> 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;
> }
>
Powered by blists - more mailing lists