| Message ID | 3c72ccc3-4de1-b5d0-423d-7b8c80991254@fnarfbargle.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | applesmc: Re-work SMC comms v2 | expand |
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@kemnade.info> > Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()") > Signed-off-by: Brad Campbell <brad@fnarfbargle.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; > } >
On Thu, 5 Nov 2020 18:26:24 +1100 Brad Campbell <brad@fnarfbargle.com> 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@kemnade.info> > Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()") > Signed-off-by: Brad Campbell <brad@fnarfbargle.com> > Still works here: Tested-by: Andreas Kemnade <andreas@kemnade.info> # MacBookAir6,2
On Thu, 5 Nov 2020 08:56:04 +0100 Henrik Rydberg <rydberg@bitmath.org> wrote: > 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. > Well, the issue has arisen because of a change in kernel to make clang happy. So it is not a new Apple device causing trouble. Regards, Andreas
On 5/11/20 6:56 pm, Henrik Rydberg wrote: > 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. G'day Henrik, Unfortunately I didn't make these changes to accommodate a "new generation". Changes made in kernel 5.9 broke it on my machine and in looking at why didn't identify any obvious causes, so I re-worked some of the comms. I can't guarantee it won't break older machines which is why I've asked for help testing it. I only have a MacbookPro 11,1 and an iMac 12,2. It fixes both of those. Help testing would be much appreciated. Regards, Brad
On 2020-11-05 09:30, Brad Campbell wrote: > On 5/11/20 6:56 pm, Henrik Rydberg wrote: >> 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. > > G'day Henrik, > > Unfortunately I didn't make these changes to accommodate a "new generation". Changes made in kernel 5.9 broke it on my machine and in looking at why didn't identify any obvious causes, so I re-worked some of the comms. > > I can't guarantee it won't break older machines which is why I've asked for help testing it. I only have a MacbookPro 11,1 and an iMac 12,2. It fixes both of those. > > Help testing would be much appreciated. I see, this makes much more sense. I may be able to run some tests tonight. Meanwhile, looking at the patch, the status variable in send_command looks superfluous now that there is a wait_status() before it. Thanks, Henrik
On 11/4/20 11:26 PM, 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. > Still a few formatting problems, but I like this version. Id take care of the formatting myself, but send_command() will need a change. Subject should be [PATCH v<version>] hwmon: (applesmc) ... > v2 : Address logic and coding style Change log should be after '---' > > Reported-by: Andreas Kemnade <andreas@kemnade.info> > Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()") > Signed-off-by: Brad Campbell <brad@fnarfbargle.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 */ > + Hah, tricked you here ;-). Using "BIT()" requires #include <linux/bits.h> > /* 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); > + } Bad indentation of "}" > 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); > + Is this read necessary ? 'status' isn't used subsequently, meaning we'll probably get static analyzer warnings about a variable which is assigned but unused. If the read is necessary, just don't assign it to a variable. > + 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; > } >
On 6/11/20 3:12 am, Guenter Roeck wrote: > On 11/4/20 11:26 PM, 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. >> > > Still a few formatting problems, but I like this version. Id take > care of the formatting myself, but send_command() will need a change. Nope, I'm more than happy to sort it all out. It's a learning process. I'd still like this to get some wider testing before I consider it ready to go so extra revisions don't worry me. > Subject should be > [PATCH v<version>] hwmon: (applesmc) ... Thanks. >> v2 : Address logic and coding style > > Change log should be after '---' No worries, can do. > >> >> Reported-by: Andreas Kemnade <andreas@kemnade.info> >> Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()") >> Signed-off-by: Brad Campbell <brad@fnarfbargle.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 */ >> + > > Hah, tricked you here ;-). Using "BIT()" requires > > #include <linux/bits.h> "requires" ?? It compiles and tests without warning, but I'll certainly add it in. >> /* 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); >> + } > > Bad indentation of "}" Yeah, I've found my editor "less than optimal" and I've had to correct a few tab related indent problems manually. Thanks. >> 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); >> + > > Is this read necessary ? 'status' isn't used subsequently, meaning we'll > probably get static analyzer warnings about a variable which is assigned > but unused. If the read is necessary, just don't assign it to a variable. No it's not. It's hangover from incompletely remove debug statements. Henrik Rydberg picked that one up yesterday. >> + 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; >> } >> > > I'll get a v3 in when I get some more test results. Much appreciated. Regards, Brad
On 11/5/20 4:02 PM, Brad Campbell wrote: [ ... ] >>> +/* 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 */ >>> + >> >> Hah, tricked you here ;-). Using "BIT()" requires >> >> #include <linux/bits.h> > > "requires" ?? > It compiles and tests without warning, but I'll certainly add it in. > Each driver should include the files with the declarations it needs, and not depend on some indirect includes. Those indirect includes are not guaranteed to exist and may be removed at some point in the future. "It compiles" is most definitely not a valid argument. Guenter
>> I can't guarantee it won't break older machines which is why I've >> asked for help testing it. I only have a MacbookPro 11,1 and an iMac >> 12,2. It fixes both of those. >> >> Help testing would be much appreciated. > > I see, this makes much more sense. I may be able to run some tests > tonight. Meanwhile, looking at the patch, the status variable in > send_command looks superfluous now that there is a wait_status() before it. Ok, it took some time to get the machines up to speed, but so far I have managed to run some tests on an MacBookAir1,1. I only managed to upgrade up to 4.15, so I had to revert the inputdev polling patch, but the rest applied without problems. I recovered an old test program I used to use (attached), and checked for failures and reads per second *** hwmon: (applesmc) switch to using input device polling mode At this point in the tree, with this reverted, I see 0 failures and 55 reads per second. *** hwmon: (applesmc) avoid overlong udelay() With this applied, I see 0 failures and 16 reads per second. *** hwmon: (applesmc) check status earlier. With this applied, I see 0 failures and 16 reads per second. *** (HEAD -> stable) applesmc: Re-work SMC comms v2 With this applied, the kernel hangs in module probe, and the kernel log is flooded with read failures. So as it stands, it does not work at all. I will continue to check another machine, and see if I can get something working. Henrik
> So as it stands, it does not work at all. I will continue to check > another machine, and see if I can get something working. On the MacBookAir3,1 the situation is somewhat better. The first three tree positions result in zero failures and 10 reads per second. The fourth yields zero failues and 11 reads per second, within the margin of similarity. So, the patch appears to have no apparent effect on the 3,1 series. Now onto fixing the 1,1 behavior. Henrik
On 7/11/20 3:26 am, Henrik Rydberg wrote: >>> I can't guarantee it won't break older machines which is why I've asked for help testing it. I only have a MacbookPro 11,1 and an iMac 12,2. It fixes both of those. >>> >>> Help testing would be much appreciated. >> >> I see, this makes much more sense. I may be able to run some tests tonight. Meanwhile, looking at the patch, the status variable in send_command looks superfluous now that there is a wait_status() before it. > > Ok, it took some time to get the machines up to speed, but so far I have managed to run some tests on an MacBookAir1,1. I only managed to upgrade up to 4.15, so I had to revert the inputdev polling patch, but the rest applied without problems. I recovered an old test program I used to use (attached), and checked for failures and reads per second > > *** hwmon: (applesmc) switch to using input device polling mode > > At this point in the tree, with this reverted, I see 0 failures and 55 reads per second. > > *** hwmon: (applesmc) avoid overlong udelay() > > With this applied, I see 0 failures and 16 reads per second. > > *** hwmon: (applesmc) check status earlier. > > With this applied, I see 0 failures and 16 reads per second. > > *** (HEAD -> stable) applesmc: Re-work SMC comms v2 > > With this applied, the kernel hangs in module probe, and the kernel log is flooded with read failures. > > So as it stands, it does not work at all. I will continue to check another machine, and see if I can get something working. > > Henrik G'day Heinrik, If you could try this earlier version which still had all the failure logging it in we might be able to get a handle on the failure. Regards, Brad --- 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; } }
On 2020-11-06 21:02, Henrik Rydberg wrote: >> So as it stands, it does not work at all. I will continue to check >> another machine, and see if I can get something working. > > On the MacBookAir3,1 the situation is somewhat better. > > The first three tree positions result in zero failures and 10 reads per > second. The fourth yields zero failues and 11 reads per second, within > the margin of similarity. > > So, the patch appears to have no apparent effect on the 3,1 series. > > Now onto fixing the 1,1 behavior. Hi again, This patch, v3, works for me, on both MBA1,1 and MBA3,1. Both machines yields 25 reads per second. It turns out that the origin code has a case that was not carried over to the v2 patch; the command byte needs to be resent upon the wrong status code. I added that back. Also, there seems to be a basic response time that needs to be respected, so I added back a small fixed delay after each write operation. I also took the liberty to reduce the number of status reads, and clean up error handling. Checkpatch is happy with this version. The code obviously needs to be retested on the other machines, but the logic still follows what you wrote, Brad, and I have also checked it against the VirtualSMC code. It appears to make sense, so hopefully there wont be additional issues. Thanks, Henrik From be4a32620b2b611472af3e35f9b50004e678efd5 Mon Sep 17 00:00:00 2001 From: Brad Campbell <brad@fnarfbargle.com> Date: Thu, 5 Nov 2020 18:26:24 +1100 Subject: [PATCH] applesmc: Re-work SMC comms v3 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 v3 : Modifications for MacBookAir1,1 Reported-by: Andreas Kemnade <andreas@kemnade.info> Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()") Signed-off-by: Brad Campbell <brad@fnarfbargle.com> Signed-off-by: Henrik Rydberg <rydberg@bitmath.org> --- drivers/hwmon/applesmc.c | 132 +++++++++++++++++++++------------------ 1 file changed, 70 insertions(+), 62 deletions(-) diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c index a18887990f4a..08289827da1e 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,76 @@ 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; + usleep_range(us, us * 16); } - pr_warn("wait_read() fail: 0x%02x\n", status); + if (debug) + pr_warn("%s fail: 0x%02x 0x%02x 0x%02x\n", __func__, val, mask, status); 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) +{ + outb(cmd, port); + udelay(APPLESMC_MIN_WAIT); + return wait_status(SMC_STATUS_BUSY, SMC_STATUS_BUSY | SMC_STATUS_IB_CLOSED); +} + +/* + * 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) { + int ret; + int i; u8 status; - int us; - unsigned long end = jiffies + (APPLESMC_MAX_WAIT * HZ) / USEC_PER_SEC; - outb(cmd, port); - for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) { - usleep_range(us, us * 16); + ret = wait_status(0, SMC_STATUS_IB_CLOSED); + if (ret) + return ret; + + for (i = 0; i < 16; i++) { + outb(cmd, APPLESMC_CMD_PORT); + udelay(APPLESMC_MIN_WAIT); + ret = wait_status(0, SMC_STATUS_IB_CLOSED); + if (ret) + return ret; status = inb(APPLESMC_CMD_PORT); - /* write: wait for smc to settle */ - if (status & 0x02) - continue; - /* ready: cmd accepted, return */ - if (status & 0x04) + if (status & SMC_STATUS_BUSY) return 0; - /* timeout: give up */ - if (time_after(jiffies, end)) - break; - /* busy: long wait and resend */ - udelay(APPLESMC_RETRY_WAIT); - outb(cmd, port); + usleep_range(APPLESMC_RETRY_WAIT, APPLESMC_RETRY_WAIT * 16); } - pr_warn("send_byte(0x%02x, 0x%04x) fail: 0x%02x\n", cmd, port, status); - return -EIO; -} + if (debug) + pr_warn("%s fail: 0x%02x\n", __func__, status); -static int send_command(u8 cmd) -{ - return send_byte(cmd, APPLESMC_CMD_PORT); + return -EIO; } static int send_argument(const char *key) @@ -217,32 +233,28 @@ static int send_argument(const char *key) int i; for (i = 0; i < 4; i++) - if (send_byte(key[i], APPLESMC_DATA_PORT)) + if (send_byte_data(key[i], APPLESMC_DATA_PORT, false)) return -EIO; return 0; } +static int send_length(u8 len, bool skip) +{ + return send_byte_data(len, APPLESMC_DATA_PORT, skip); +} + static int read_smc(u8 cmd, const char *key, u8 *buffer, u8 len) { u8 status, data = 0; int i; - if (send_command(cmd) || send_argument(key)) { - pr_warn("%.4s: read arg fail\n", key); - return -EIO; - } - - /* This has no effect on newer (2012) SMCs */ - if (send_byte(len, APPLESMC_DATA_PORT)) { - pr_warn("%.4s: read len fail\n", key); - return -EIO; - } + if (send_command(cmd) || send_argument(key) || send_length(len, 1)) + goto err; for (i = 0; i < len; i++) { - if (wait_read()) { - pr_warn("%.4s: read data[%d] fail\n", key, i); - return -EIO; - } + if (wait_status(SMC_STATUS_AWAITING_DATA, + SMC_STATUS_AWAITING_DATA | SMC_STATUS_IB_CLOSED)) + goto err; buffer[i] = inb(APPLESMC_DATA_PORT); } @@ -250,7 +262,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); } @@ -258,30 +270,26 @@ static int read_smc(u8 cmd, const char *key, u8 *buffer, u8 len) pr_warn("flushed %d bytes, last value is: %d\n", i, data); return 0; +err: + pr_warn("read cmd fail: %x %.4s %d\n", cmd, key, len); + return -EIO; } static int write_smc(u8 cmd, const char *key, const u8 *buffer, u8 len) { int i; - if (send_command(cmd) || send_argument(key)) { - pr_warn("%s: write arg fail\n", key); - return -EIO; - } + if (send_command(cmd) || send_argument(key) || send_length(len, 0)) + goto err; - if (send_byte(len, APPLESMC_DATA_PORT)) { - 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); - return -EIO; - } - } + for (i = 0; i < len; i++) + if (send_byte_data(buffer[i], APPLESMC_DATA_PORT, i == len - 1)) + goto err; return 0; +err: + pr_warn("write cmd fail: %x %.4s %d\n", cmd, key, len); + return -EIO; } static int read_register_count(unsigned int *count)
On 8/11/20 5:31 am, Henrik Rydberg wrote: > On 2020-11-06 21:02, Henrik Rydberg wrote: >>> So as it stands, it does not work at all. I will continue to check another machine, and see if I can get something working. >> >> On the MacBookAir3,1 the situation is somewhat better. >> >> The first three tree positions result in zero failures and 10 reads per second. The fourth yields zero failues and 11 reads per second, within the margin of similarity. >> >> So, the patch appears to have no apparent effect on the 3,1 series. >> >> Now onto fixing the 1,1 behavior. > > Hi again, > > This patch, v3, works for me, on both MBA1,1 and MBA3,1. Both machines yields 25 reads per second. > > It turns out that the origin code has a case that was not carried over to the v2 patch; the command byte needs to be resent upon the wrong status code. I added that back. Also, there seems to be a basic response time that needs to be respected, so I added back a small fixed delay after each write operation. I also took the liberty to reduce the number of status reads, and clean up error handling. Checkpatch is happy with this version. > > The code obviously needs to be retested on the other machines, but the logic still follows what you wrote, Brad, and I have also checked it against the VirtualSMC code. It appears to make sense, so hopefully there wont be additional issues. > > Thanks, > Henrik > G'day Henrik, Which kernel was this based on? It won't apply to my 5.9 tree. I assume the sprinkling of udelay(APPLESMC_MIN_WAIT) means the SMC is slow in getting its status register set up. Could we instead just put a single one of those up-front in wait_status? Any chance you could try this one? I've added a retry to send_command and added a single global APPLESMC_MIN_WAIT before each status read. From looking at your modified send_command, it appears the trigger for a retry is sending a command and the SMC doing absolutely nothing. This should do the same thing. Interestingly enough, by adding the udelay to wait_status on my machine I've gone from 24 reads/s to 50 reads/s. I've left out the remainder of the cleanups. Once we get a minimally working patch I was going to look at a few cleanups, and I have some patches pending to allow writing to the SMC from userspace (for setting BCLM and BFCL mainly) diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c index a18887990f4a..2190de78b5f5 100644 --- a/drivers/hwmon/applesmc.c +++ b/drivers/hwmon/applesmc.c @@ -32,6 +32,7 @@ #include <linux/hwmon.h> #include <linux/workqueue.h> #include <linux/err.h> +#include <linux/bits.h> /* data port used by Apple SMC */ #define APPLESMC_DATA_PORT 0x300 @@ -42,6 +43,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 +157,73 @@ 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; + udelay(APPLESMC_MIN_WAIT); 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; + usleep_range(us, us * 16); } - - pr_warn("wait_read() fail: 0x%02x\n", status); 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); + int ret; + int i; + + for (i=0; i < 16; i++) { + ret = wait_status(0, SMC_STATUS_IB_CLOSED); + if (ret) + return ret; + + outb(cmd, APPLESMC_CMD_PORT); + ret = wait_status(SMC_STATUS_BUSY, SMC_STATUS_BUSY); + if (!ret) + return ret; + } + return -EIO; } static int send_argument(const char *key) @@ -239,7 +253,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 +266,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 +291,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; }
Hi Brad, > G'day Henrik, > > Which kernel was this based on? It won't apply to my 5.9 tree. I was being lazy and applied the diff to linus/master on top of my current stable branch. More importantly, I sent the mail out from an email client that may not format the patch properly; I'll fix that. > I assume the sprinkling of udelay(APPLESMC_MIN_WAIT) means the SMC is > slow in getting its status register set up. Could we instead just put > a single one of those up-front in wait_status? That works fine, just a matter of taste. > Any chance you could try this one? I've added a retry to send_command and > added a single global APPLESMC_MIN_WAIT before each status read. > > From looking at your modified send_command, it appears the trigger for a > retry is sending a command and the SMC doing absolutely nothing. This > should do the same thing. Not quite, unfortunately. The patch that works waits for a drop of IB_CLOSED, then checks the BUSY status. If not seen, it resends immediately, never expecting to see it. The patch in this email creates a dreadfully sluggish probe, and the occasional failure. > Interestingly enough, by adding the udelay to wait_status on my machine I've > gone from 24 reads/s to 50 reads/s. Yep, I experience the same positive effect. > I've left out the remainder of the cleanups. Once we get a minimally working > patch I was going to look at a few cleanups, and I have some patches pending > to allow writing to the SMC from userspace (for setting BCLM and BFCL mainly) All fine. I will respond to the v3 mail separately. Henrik
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; }
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@kemnade.info> Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()") Signed-off-by: Brad Campbell <brad@fnarfbargle.com> ---