* Re: [PATCH] applesmc: Re-work SMC comms v1
@ 2020-11-05 6:32 Guenter Roeck
0 siblings, 0 replies; 3+ messages in thread
From: Guenter Roeck @ 2020-11-05 6:32 UTC (permalink / raw)
To: Brad Campbell
Cc: Andreas Kemnade, Jean Delvare, Arnd Bergmann, rydberg,
linux-hwmon, linux-kernel, hns
On Thu, Nov 05, 2020 at 04:47:25PM +1100, Brad Campbell wrote:
> Commit fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()") introduced
> an issue whereby communication with the SMC became unreliable with write
> errors :
>
> [ 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.
>
> Reported-by: Andreas Kemnade <andreas@kemnade.info>
Add
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..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.
> +
Maybe consider using BIT() while at it.
/* Please use standard comments */
Also, what does the "<" mean ? Is that supposed to be negated
(ie bit set means not busy) ? If so, that isn't a standard notation
that I am aware of. Maybe "not set if busy in the middle of a command"
would be better in this case.
> /* 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)
I would suggest to keep send_byte() and change it to the following.
static int send_byte(u8 cmd, u16 port)
{
return send_byte_data(cmd, port, false);
}
That would limit the number of changes needed later in the code
(it is never called with a hard 'true' as parameter).
> +
> +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;
u8 wstat = skip ? 0 : SMC_STATUS_BUSY;
> + if (wait_status(SMC_STATUS_BUSY,
> + SMC_STATUS_BUSY | SMC_STATUS_IB_CLOSED))
This fits one line, and the error code
should really not be overwritten.
ret = wait_status(SMC_STATUS_BUSY, SMC_STATUS_BUSY | SMC_STATUS_IB_CLOSED);
if (ret)
return ret;
> + 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))
That really fits into one line.
> + return 0;
> +fail:
> + pr_warn("send_byte_data(0x%02x, 0x%04x) fail\n", cmd, APPLESMC_CMD_PORT);
Can you drop this message ? wait_status() already displays a message,
after all. Also, please reverse error handling, and don't overwrite
error codes.
ret = wait_status(wstat, SMC_STATUS_BUSY)
if (ret)
return ret;
Actually, this can be simplified to
return wait_status(wstat, SMC_STATUS_BUSY);
or, since wstat is only used once,
return wait_status(skip ? 0 : SMC_STATUS_BUSY, SMC_STATUS_BUSY);
> 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)) {
Another one of those odd continuation lines.
> + pr_warn("send_command SMC was busy\n");
and logging noise. As for error handling, same as above, please
ret = wait_status(0, SMC_STATUS_IB_CLOSED);
if (ret)
return ret;
> + goto fail; }
> +
> + status = inb(APPLESMC_CMD_PORT);
> +
> + outb(cmd, APPLESMC_CMD_PORT);
> + if (!wait_status(SMC_STATUS_BUSY,
> + SMC_STATUS_BUSY))
Odd/unnecessary continuation line again.
> + return 0;
> +fail:
> + pr_warn("send_cmd(0x%02x, 0x%04x) fail\n", cmd, APPLESMC_CMD_PORT);
Wow, up to three messages on failure. Please, don't do that.
One message per failure is really enough. Please simplify to
return wait_status(SMC_STATUS_BUSY, SMC_STATUS_BUSY);
Actually, I notice that the callers of send_command()
log yet again. Maybe it is time to drop all the messages
from here and from send_argument() and only log in the
calling code.
> + 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 */
Please align comments with code. Maybe move the comment ahead
of the for statement. Or drop it entirely - it doesn't add that
much value. Actually, this blob would go away if you keep
send_byte().
> + 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)) {
Align continuatiuon lines with preceding '('. "checkpatch --strict"
reports all those alignment issues.
> 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;
space before and after '-', please. checkpatch --strict will tell.
>
> if (send_command(cmd) || send_argument(key)) {
> pr_warn("%s: write arg fail\n", key);
> return -EIO;
I notice the driver keeps overwriting error codes. Oh well.
I can't expect you to fix that, and it should not be fixed as part
of this patch, but please don't make it worse (not here, but above
where calls are changed).
> }
>
> - 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))) {
Unnecessary ( ) around i == end. Not sure if the 'end' variable
is worth it. Might as well make it "i == len - 1" and let the compiler
optimize it at will.
> + pr_warn("%s: write data fail at %i\n", key, i);
> return -EIO;
> }
> }
^ permalink raw reply [flat|nested] 3+ messages in thread
* [REGRESSION] hwmon: (applesmc) avoid overlong udelay()
@ 2020-09-30 8:54 Andreas Kemnade
2020-09-30 16:44 ` Guenter Roeck
0 siblings, 1 reply; 3+ messages in thread
From: Andreas Kemnade @ 2020-09-30 8:54 UTC (permalink / raw)
To: rydberg, jdelvare, linux, linux-hwmon, linux-kernel
Hi,
after the $subject patch I get lots of errors like this:
[ 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
CPU sticks at low speed and no fan is turning on.
Reverting this patch on top of 5.9-rc6 solves this problem.
Some information from dmidecode:
Base Board Information
Manufacturer: Apple Inc.
Product Name: Mac-7DF21CB3ED6977E5
Version: MacBookAir6,2
Handle 0x0020, DMI type 11, 5 bytes OEM Strings String 1: Apple ROM Version. Model: …,
Handle 0x0020, DMI type 11, 5 bytes
OEM Strings
String 1: Apple ROM Version. Model: MBA61. EFI Version: 122.0.0
String 2: .0.0. Built by: root@saumon. Date: Wed Jun 10 18:
String 3: 10:36 PDT 2020. Revision: 122 (B&I). ROM Version: F000_B
String 4: 00. Build Type: Official Build, Release. Compiler: Appl
String 5: e clang version 3.0 (tags/Apple/clang-211.10.1) (based on LLVM
String 6: 3.0svn).
Writing to things in /sys/devices/platform/applesmc.768 gives also the
said errors.
But writing 1 to fan1_maunal and 5000 to fan1_output turns the fan on
despite error messages.
Config used is: https://misc.andi.de1.cc/mac-config.gz
Regards,
Andreas
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [REGRESSION] hwmon: (applesmc) avoid overlong udelay()
2020-09-30 8:54 [REGRESSION] hwmon: (applesmc) avoid overlong udelay() Andreas Kemnade
@ 2020-09-30 16:44 ` Guenter Roeck
2020-09-30 20:00 ` Arnd Bergmann
0 siblings, 1 reply; 3+ messages in thread
From: Guenter Roeck @ 2020-09-30 16:44 UTC (permalink / raw)
To: Andreas Kemnade
Cc: rydberg, jdelvare, linux-hwmon, linux-kernel, Arnd Bergmann
On Wed, Sep 30, 2020 at 10:54:42AM +0200, Andreas Kemnade wrote:
> Hi,
>
> after the $subject patch I get lots of errors like this:
For reference, this refers to commit fff2d0f701e6 ("hwmon: (applesmc)
avoid overlong udelay()").
> [ 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
>
> CPU sticks at low speed and no fan is turning on.
> Reverting this patch on top of 5.9-rc6 solves this problem.
>
> Some information from dmidecode:
>
> Base Board Information
> Manufacturer: Apple Inc.
> Product Name: Mac-7DF21CB3ED6977E5
> Version: MacBookAir6,2
>
> Handle 0x0020, DMI type 11, 5 bytes OEM Strings String 1: Apple ROM Version. Model: …,
> Handle 0x0020, DMI type 11, 5 bytes
> OEM Strings
> String 1: Apple ROM Version. Model: MBA61. EFI Version: 122.0.0
> String 2: .0.0. Built by: root@saumon. Date: Wed Jun 10 18:
> String 3: 10:36 PDT 2020. Revision: 122 (B&I). ROM Version: F000_B
> String 4: 00. Build Type: Official Build, Release. Compiler: Appl
> String 5: e clang version 3.0 (tags/Apple/clang-211.10.1) (based on LLVM
> String 6: 3.0svn).
>
> Writing to things in /sys/devices/platform/applesmc.768 gives also the
> said errors.
> But writing 1 to fan1_maunal and 5000 to fan1_output turns the fan on
> despite error messages.
>
Not really sure what to do here. I could revert the patch, but then we'd gain
clang compile failures. Arnd, any idea ?
Guenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [REGRESSION] hwmon: (applesmc) avoid overlong udelay()
2020-09-30 16:44 ` Guenter Roeck
@ 2020-09-30 20:00 ` Arnd Bergmann
2020-10-01 22:22 ` Andreas Kemnade
0 siblings, 1 reply; 3+ messages in thread
From: Arnd Bergmann @ 2020-09-30 20:00 UTC (permalink / raw)
To: Guenter Roeck
Cc: Andreas Kemnade, rydberg, Jean Delvare, linux-hwmon, linux-kernel
On Wed, Sep 30, 2020 at 6:44 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Wed, Sep 30, 2020 at 10:54:42AM +0200, Andreas Kemnade wrote:
> > Hi,
> >
> > after the $subject patch I get lots of errors like this:
>
> For reference, this refers to commit fff2d0f701e6 ("hwmon: (applesmc)
> avoid overlong udelay()").
>
> > [ 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
> >
> > CPU sticks at low speed and no fan is turning on.
> > Reverting this patch on top of 5.9-rc6 solves this problem.
> >
> > Some information from dmidecode:
> >
> > Base Board Information
> > Manufacturer: Apple Inc.
> > Product Name: Mac-7DF21CB3ED6977E5
> > Version: MacBookAir6,2
> >
> > Handle 0x0020, DMI type 11, 5 bytes OEM Strings String 1: Apple ROM Version. Model: …,
> > Handle 0x0020, DMI type 11, 5 bytes
> > OEM Strings
> > String 1: Apple ROM Version. Model: MBA61. EFI Version: 122.0.0
> > String 2: .0.0. Built by: root@saumon. Date: Wed Jun 10 18:
> > String 3: 10:36 PDT 2020. Revision: 122 (B&I). ROM Version: F000_B
> > String 4: 00. Build Type: Official Build, Release. Compiler: Appl
> > String 5: e clang version 3.0 (tags/Apple/clang-211.10.1) (based on LLVM
> > String 6: 3.0svn).
> >
> > Writing to things in /sys/devices/platform/applesmc.768 gives also the
> > said errors.
> > But writing 1 to fan1_maunal and 5000 to fan1_output turns the fan on
> > despite error messages.
> >
> Not really sure what to do here. I could revert the patch, but then we'd gain
> clang compile failures. Arnd, any idea ?
It seems that either I made a mistake in the conversion and it sleeps for
less time than before, or my assumption was wrong that converting a delay to
a sleep is safe here.
The error message indicates that the write fails, not the read, so that
is what I'd look at first. Right away I can see that the maximum time to
retry is only half of what it used to be, as we used to wait for
0x10, 0x20, 0x40, 0x80, ..., 0x20000 microseconds for a total of
0x3fff0 microseconds (262ms), while my patch went with the 131ms
total delay based on the comment saying "/* wait up to 128 ms for a
status change. */".
Since there is sleeping wait, I see no reason the timeout couldn't
be extended a lot, e.g. to a second, as in
#define APPLESMC_MAX_WAIT 0x100000
If that doesn't work, I'd try using mdelay() in place of
usleep_range(), such as
mdelay(DIV_ROUND_UP(us, USEC_PER_MSEC)));
This adds back a really nasty latency, but it should avoid the
compile-time problem.
Andreas, can you try those two things? (one at a time,
not both)
Arnd
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [REGRESSION] hwmon: (applesmc) avoid overlong udelay()
2020-09-30 20:00 ` Arnd Bergmann
@ 2020-10-01 22:22 ` Andreas Kemnade
2020-10-02 4:07 ` Guenter Roeck
0 siblings, 1 reply; 3+ messages in thread
From: Andreas Kemnade @ 2020-10-01 22:22 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Guenter Roeck, rydberg, Jean Delvare, linux-hwmon, linux-kernel
On Wed, 30 Sep 2020 22:00:09 +0200
Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, Sep 30, 2020 at 6:44 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On Wed, Sep 30, 2020 at 10:54:42AM +0200, Andreas Kemnade wrote:
> > > Hi,
> > >
> > > after the $subject patch I get lots of errors like this:
> >
> > For reference, this refers to commit fff2d0f701e6 ("hwmon: (applesmc)
> > avoid overlong udelay()").
> >
> > > [ 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
> > >
> > > CPU sticks at low speed and no fan is turning on.
> > > Reverting this patch on top of 5.9-rc6 solves this problem.
> > >
> > > Some information from dmidecode:
> > >
> > > Base Board Information
> > > Manufacturer: Apple Inc.
> > > Product Name: Mac-7DF21CB3ED6977E5
> > > Version: MacBookAir6,2
> > >
> > > Handle 0x0020, DMI type 11, 5 bytes OEM Strings String 1: Apple ROM Version. Model: …,
> > > Handle 0x0020, DMI type 11, 5 bytes
> > > OEM Strings
> > > String 1: Apple ROM Version. Model: MBA61. EFI Version: 122.0.0
> > > String 2: .0.0. Built by: root@saumon. Date: Wed Jun 10 18:
> > > String 3: 10:36 PDT 2020. Revision: 122 (B&I). ROM Version: F000_B
> > > String 4: 00. Build Type: Official Build, Release. Compiler: Appl
> > > String 5: e clang version 3.0 (tags/Apple/clang-211.10.1) (based on LLVM
> > > String 6: 3.0svn).
> > >
> > > Writing to things in /sys/devices/platform/applesmc.768 gives also the
> > > said errors.
> > > But writing 1 to fan1_maunal and 5000 to fan1_output turns the fan on
> > > despite error messages.
> > >
> > Not really sure what to do here. I could revert the patch, but then we'd gain
> > clang compile failures. Arnd, any idea ?
>
> It seems that either I made a mistake in the conversion and it sleeps for
> less time than before, or my assumption was wrong that converting a delay to
> a sleep is safe here.
>
> The error message indicates that the write fails, not the read, so that
> is what I'd look at first. Right away I can see that the maximum time to
> retry is only half of what it used to be, as we used to wait for
> 0x10, 0x20, 0x40, 0x80, ..., 0x20000 microseconds for a total of
> 0x3fff0 microseconds (262ms), while my patch went with the 131ms
> total delay based on the comment saying "/* wait up to 128 ms for a
> status change. */".
>
Yes, that is also what I read from the code. I just thought there must
be something simple, which just needs a short look from another pair of
eyes.
> Since there is sleeping wait, I see no reason the timeout couldn't
> be extended a lot, e.g. to a second, as in
>
> #define APPLESMC_MAX_WAIT 0x100000
>
> If that doesn't work, I'd try using mdelay() in place of
> usleep_range(), such as
>
> mdelay(DIV_ROUND_UP(us, USEC_PER_MSEC)));
>
> This adds back a really nasty latency, but it should avoid the
> compile-time problem.
>
> Andreas, can you try those two things? (one at a time,
> not both)
Ok, I tried. None of them works. I rechecked my work and created real
git commits out of them and CONFIG_LOCALVERSION_AUTO is also set so
the usual stupid things are rules out.
In detail:
On top of 5.9-rc6 + *reverted* patch:
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index fd99c9df8a00..2a9bd7f2b71b 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -45,7 +45,7 @@
/* wait up to 128 ms for a status change. */
#define APPLESMC_MIN_WAIT 0x0010
#define APPLESMC_RETRY_WAIT 0x0100
-#define APPLESMC_MAX_WAIT 0x20000
+#define APPLESMC_MAX_WAIT 0x8000
#define APPLESMC_READ_CMD 0x10
#define APPLESMC_WRITE_CMD 0x11
--
2.20.1
-> no trouble found, but I have not tested very long, just some
sysfs writes.
On top of 5.9-rc6:
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index a18887990f4a..3b0108b75a24 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -161,7 +161,7 @@ static int wait_read(void)
int us;
for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
- usleep_range(us, us * 16);
+ mdelay(DIV_ROUND_UP(us, USEC_PER_MSEC));
status = inb(APPLESMC_CMD_PORT);
/* read: wait for smc to settle */
if (status & 0x01)
@@ -187,7 +187,7 @@ static int send_byte(u8 cmd, u16 port)
outb(cmd, port);
for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
- usleep_range(us, us * 16);
+ mdelay(DIV_ROUND_UP(us, USEC_PER_MSEC));
status = inb(APPLESMC_CMD_PORT);
/* write: wait for smc to settle */
if (status & 0x02)
--
2.20.1
-> write errors:
[ 2.472801] applesmc: key=561 fan=1 temp=33 index=33 acc=0 lux=2 kbd=1
[ 2.472961] applesmc applesmc.768: hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
[ 18.535659] applesmc: send_byte(0x00, 0x0300) fail: 0x40
[ 18.538171] applesmc: LKSB: write data fail
[ 45.260307] applesmc: send_byte(0x01, 0x0300) fail: 0x40
[ 45.260324] applesmc: FS! : write data fail
[ 47.870135] applesmc: send_byte(0x20, 0x0300) fail: 0x40
[ 47.870193] applesmc: F0Tg: write data fail
On top of 5.9-rc6:
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index a18887990f4a..f67a25651d03 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -45,7 +45,7 @@
/* wait up to 128 ms for a status change. */
#define APPLESMC_MIN_WAIT 0x0010
#define APPLESMC_RETRY_WAIT 0x0100
-#define APPLESMC_MAX_WAIT 0x20000
+#define APPLESMC_MAX_WAIT 0x100000
#define APPLESMC_READ_CMD 0x10
#define APPLESMC_WRITE_CMD 0x11
--
2.20.1
-> write errors:
[ 1.428726] applesmc: key=561 fan=1 temp=33 index=33 acc=0 lux=2 kbd=1
[ 1.428869] applesmc applesmc.768: hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
[ 19.672561] applesmc: send_byte(0x00, 0x0300) fail: 0x40
[ 19.674641] applesmc: LKSB: write data fail
[ 34.266216] applesmc: send_byte(0x01, 0x0300) fail: 0x40
[ 34.266277] applesmc: FS! : write data fail
[ 37.357023] applesmc: send_byte(0x20, 0x0300) fail: 0x40
[ 37.357082] applesmc: F0Tg: write data fail
Accessing things in sysfs took longer, so the increase seems to be in effect.
Conclusion:
head->scratch();
So something requires really exact timings.
Regards,
Andreas
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [REGRESSION] hwmon: (applesmc) avoid overlong udelay()
2020-10-01 22:22 ` Andreas Kemnade
@ 2020-10-02 4:07 ` Guenter Roeck
2020-10-06 7:02 ` Andreas Kemnade
0 siblings, 1 reply; 3+ messages in thread
From: Guenter Roeck @ 2020-10-02 4:07 UTC (permalink / raw)
To: Andreas Kemnade, Arnd Bergmann
Cc: rydberg, Jean Delvare, linux-hwmon, linux-kernel
On 10/1/20 3:22 PM, Andreas Kemnade wrote:
> On Wed, 30 Sep 2020 22:00:09 +0200
> Arnd Bergmann <arnd@arndb.de> wrote:
>
>> On Wed, Sep 30, 2020 at 6:44 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>> On Wed, Sep 30, 2020 at 10:54:42AM +0200, Andreas Kemnade wrote:
>>>> Hi,
>>>>
>>>> after the $subject patch I get lots of errors like this:
>>>
>>> For reference, this refers to commit fff2d0f701e6 ("hwmon: (applesmc)
>>> avoid overlong udelay()").
>>>
>>>> [ 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
>>>>
>>>> CPU sticks at low speed and no fan is turning on.
>>>> Reverting this patch on top of 5.9-rc6 solves this problem.
>>>>
>>>> Some information from dmidecode:
>>>>
>>>> Base Board Information
>>>> Manufacturer: Apple Inc.
>>>> Product Name: Mac-7DF21CB3ED6977E5
>>>> Version: MacBookAir6,2
>>>>
>>>> Handle 0x0020, DMI type 11, 5 bytes OEM Strings String 1: Apple ROM Version. Model: …,
>>>> Handle 0x0020, DMI type 11, 5 bytes
>>>> OEM Strings
>>>> String 1: Apple ROM Version. Model: MBA61. EFI Version: 122.0.0
>>>> String 2: .0.0. Built by: root@saumon. Date: Wed Jun 10 18:
>>>> String 3: 10:36 PDT 2020. Revision: 122 (B&I). ROM Version: F000_B
>>>> String 4: 00. Build Type: Official Build, Release. Compiler: Appl
>>>> String 5: e clang version 3.0 (tags/Apple/clang-211.10.1) (based on LLVM
>>>> String 6: 3.0svn).
>>>>
>>>> Writing to things in /sys/devices/platform/applesmc.768 gives also the
>>>> said errors.
>>>> But writing 1 to fan1_maunal and 5000 to fan1_output turns the fan on
>>>> despite error messages.
>>>>
>>> Not really sure what to do here. I could revert the patch, but then we'd gain
>>> clang compile failures. Arnd, any idea ?
>>
>> It seems that either I made a mistake in the conversion and it sleeps for
>> less time than before, or my assumption was wrong that converting a delay to
>> a sleep is safe here.
>>
>> The error message indicates that the write fails, not the read, so that
>> is what I'd look at first. Right away I can see that the maximum time to
>> retry is only half of what it used to be, as we used to wait for
>> 0x10, 0x20, 0x40, 0x80, ..., 0x20000 microseconds for a total of
>> 0x3fff0 microseconds (262ms), while my patch went with the 131ms
>> total delay based on the comment saying "/* wait up to 128 ms for a
>> status change. */".
>>
> Yes, that is also what I read from the code. I just thought there must
> be something simple, which just needs a short look from another pair of
> eyes.
>
>> Since there is sleeping wait, I see no reason the timeout couldn't
>> be extended a lot, e.g. to a second, as in
>>
>> #define APPLESMC_MAX_WAIT 0x100000
>>
>> If that doesn't work, I'd try using mdelay() in place of
>> usleep_range(), such as
>>
>> mdelay(DIV_ROUND_UP(us, USEC_PER_MSEC)));
>>
>> This adds back a really nasty latency, but it should avoid the
>> compile-time problem.
>>
>> Andreas, can you try those two things? (one at a time,
>> not both)
>
> Ok, I tried. None of them works. I rechecked my work and created real
> git commits out of them and CONFIG_LOCALVERSION_AUTO is also set so
> the usual stupid things are rules out.
> In detail:
> On top of 5.9-rc6 + *reverted* patch:
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index fd99c9df8a00..2a9bd7f2b71b 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -45,7 +45,7 @@
> /* wait up to 128 ms for a status change. */
> #define APPLESMC_MIN_WAIT 0x0010
> #define APPLESMC_RETRY_WAIT 0x0100
> -#define APPLESMC_MAX_WAIT 0x20000
> +#define APPLESMC_MAX_WAIT 0x8000
>
> #define APPLESMC_READ_CMD 0x10
> #define APPLESMC_WRITE_CMD 0x11
>
Oh man, that code is so badlys broken.
send_byte() repeats sending the data if it was not immediately successful.
That is done for both data and commands. Effectively that happens if
the command is not immediately accepted. However, send_argument()
clearly assumes that each data byte is sent exactly once. Sending
it more than once will mess up the key that is supposed to be sent.
The Apple SMC emulation code in qemu confirms that data bytes can not
be written more than once.
Of course, theoretically it may be that the first data byte was not
accepted (after all, the ACK bit is not set), but the ACK bit is
not checked again after udelay(APPLESMC_RETRY_WAIT), so it may
well have been set in the 256 uS between its check and re-writing
the data.
In other words, this entire code only works accidentally to start with.
If you like, you could play around with the code and find out if and
when exactly bit 1 (busy) is set, if and when bit 2 (ack) is set, and
if and when any other bit is set. We could also try to read port 0x31e
(the error port). Maybe the we can figure out what the error actually
is. But then I don't really know what we could do with that information.
Other than that, the only useful idea I have is something crazy like
if (us < 10000)
udelay(us);
else
mdelay(DIV_ROUND_CLOSEST(udelay, 1000));
in the hope that clang doesn't convert that back into a
compile-time constant and udelay().
Overall it seems like the apple protocol may expect to receive data
bytes faster than 1ms apart, because that is the only real difference
between the original code and the new code using mdelay().
Guenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [REGRESSION] hwmon: (applesmc) avoid overlong udelay()
2020-10-02 4:07 ` Guenter Roeck
@ 2020-10-06 7:02 ` Andreas Kemnade
2020-11-02 23:56 ` Brad Campbell
0 siblings, 1 reply; 3+ messages in thread
From: Andreas Kemnade @ 2020-10-06 7:02 UTC (permalink / raw)
To: Guenter Roeck
Cc: Arnd Bergmann, rydberg, Jean Delvare, linux-hwmon, linux-kernel, hns
[-- Attachment #1: Type: text/plain, Size: 6459 bytes --]
On Thu, 1 Oct 2020 21:07:51 -0700
Guenter Roeck <linux@roeck-us.net> wrote:
> On 10/1/20 3:22 PM, Andreas Kemnade wrote:
> > On Wed, 30 Sep 2020 22:00:09 +0200
> > Arnd Bergmann <arnd@arndb.de> wrote:
> >
> >> On Wed, Sep 30, 2020 at 6:44 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>>
> >>> On Wed, Sep 30, 2020 at 10:54:42AM +0200, Andreas Kemnade wrote:
> >>>> Hi,
> >>>>
> >>>> after the $subject patch I get lots of errors like this:
> >>>
> >>> For reference, this refers to commit fff2d0f701e6 ("hwmon: (applesmc)
> >>> avoid overlong udelay()").
> >>>
> >>>> [ 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
> >>>>
> >>>> CPU sticks at low speed and no fan is turning on.
> >>>> Reverting this patch on top of 5.9-rc6 solves this problem.
> >>>>
> >>>> Some information from dmidecode:
> >>>>
> >>>> Base Board Information
> >>>> Manufacturer: Apple Inc.
> >>>> Product Name: Mac-7DF21CB3ED6977E5
> >>>> Version: MacBookAir6,2
> >>>>
> >>>> Handle 0x0020, DMI type 11, 5 bytes OEM Strings String 1: Apple ROM Version. Model: …,
> >>>> Handle 0x0020, DMI type 11, 5 bytes
> >>>> OEM Strings
> >>>> String 1: Apple ROM Version. Model: MBA61. EFI Version: 122.0.0
> >>>> String 2: .0.0. Built by: root@saumon. Date: Wed Jun 10 18:
> >>>> String 3: 10:36 PDT 2020. Revision: 122 (B&I). ROM Version: F000_B
> >>>> String 4: 00. Build Type: Official Build, Release. Compiler: Appl
> >>>> String 5: e clang version 3.0 (tags/Apple/clang-211.10.1) (based on LLVM
> >>>> String 6: 3.0svn).
> >>>>
> >>>> Writing to things in /sys/devices/platform/applesmc.768 gives also the
> >>>> said errors.
> >>>> But writing 1 to fan1_maunal and 5000 to fan1_output turns the fan on
> >>>> despite error messages.
> >>>>
> >>> Not really sure what to do here. I could revert the patch, but then we'd gain
> >>> clang compile failures. Arnd, any idea ?
> >>
> >> It seems that either I made a mistake in the conversion and it sleeps for
> >> less time than before, or my assumption was wrong that converting a delay to
> >> a sleep is safe here.
> >>
> >> The error message indicates that the write fails, not the read, so that
> >> is what I'd look at first. Right away I can see that the maximum time to
> >> retry is only half of what it used to be, as we used to wait for
> >> 0x10, 0x20, 0x40, 0x80, ..., 0x20000 microseconds for a total of
> >> 0x3fff0 microseconds (262ms), while my patch went with the 131ms
> >> total delay based on the comment saying "/* wait up to 128 ms for a
> >> status change. */".
> >>
> > Yes, that is also what I read from the code. I just thought there must
> > be something simple, which just needs a short look from another pair of
> > eyes.
> >
> >> Since there is sleeping wait, I see no reason the timeout couldn't
> >> be extended a lot, e.g. to a second, as in
> >>
> >> #define APPLESMC_MAX_WAIT 0x100000
> >>
> >> If that doesn't work, I'd try using mdelay() in place of
> >> usleep_range(), such as
> >>
> >> mdelay(DIV_ROUND_UP(us, USEC_PER_MSEC)));
> >>
> >> This adds back a really nasty latency, but it should avoid the
> >> compile-time problem.
> >>
> >> Andreas, can you try those two things? (one at a time,
> >> not both)
> >
> > Ok, I tried. None of them works. I rechecked my work and created real
> > git commits out of them and CONFIG_LOCALVERSION_AUTO is also set so
> > the usual stupid things are rules out.
> > In detail:
> > On top of 5.9-rc6 + *reverted* patch:
> > diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> > index fd99c9df8a00..2a9bd7f2b71b 100644
> > --- a/drivers/hwmon/applesmc.c
> > +++ b/drivers/hwmon/applesmc.c
> > @@ -45,7 +45,7 @@
> > /* wait up to 128 ms for a status change. */
> > #define APPLESMC_MIN_WAIT 0x0010
> > #define APPLESMC_RETRY_WAIT 0x0100
> > -#define APPLESMC_MAX_WAIT 0x20000
> > +#define APPLESMC_MAX_WAIT 0x8000
> >
> > #define APPLESMC_READ_CMD 0x10
> > #define APPLESMC_WRITE_CMD 0x11
> >
>
> Oh man, that code is so badlys broken.
>
> send_byte() repeats sending the data if it was not immediately successful.
> That is done for both data and commands. Effectively that happens if
> the command is not immediately accepted. However, send_argument()
> clearly assumes that each data byte is sent exactly once. Sending
> it more than once will mess up the key that is supposed to be sent.
> The Apple SMC emulation code in qemu confirms that data bytes can not
> be written more than once.
>
> Of course, theoretically it may be that the first data byte was not
> accepted (after all, the ACK bit is not set), but the ACK bit is
> not checked again after udelay(APPLESMC_RETRY_WAIT), so it may
> well have been set in the 256 uS between its check and re-writing
> the data.
>
> In other words, this entire code only works accidentally to start with.
>
> If you like, you could play around with the code and find out if and
> when exactly bit 1 (busy) is set, if and when bit 2 (ack) is set, and
> if and when any other bit is set. We could also try to read port 0x31e
> (the error port). Maybe the we can figure out what the error actually
> is. But then I don't really know what we could do with that information.
>
Smoe research results: the second data byte seems to cause problems, not the
command byte.
> Other than that, the only useful idea I have is something crazy like
> if (us < 10000)
> udelay(us);
> else
> mdelay(DIV_ROUND_CLOSEST(udelay, 1000));
> in the hope that clang doesn't convert that back into a
> compile-time constant and udelay().
>
> Overall it seems like the apple protocol may expect to receive data
> bytes faster than 1ms apart, because that is the only real difference
> between the original code and the new code using mdelay().
Yes, that explanation makes sense. If I am trying something like that, only
the last byte requires more than APPLESMC_MIN_WAIT. I have seen max. 256us.
So we could probably even use msleep for us > 1000 and udelay for anything below.
Regards,
Andreas
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [REGRESSION] hwmon: (applesmc) avoid overlong udelay()
2020-10-06 7:02 ` Andreas Kemnade
@ 2020-11-02 23:56 ` Brad Campbell
2020-11-03 5:56 ` Brad Campbell
0 siblings, 1 reply; 3+ messages in thread
From: Brad Campbell @ 2020-11-02 23:56 UTC (permalink / raw)
To: Andreas Kemnade, Guenter Roeck
Cc: Arnd Bergmann, rydberg, Jean Delvare, linux-hwmon, linux-kernel, hns
[-- Attachment #1: Type: text/plain, Size: 7392 bytes --]
On 6/10/20 6:02 pm, Andreas Kemnade wrote:
> On Thu, 1 Oct 2020 21:07:51 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
>
>> On 10/1/20 3:22 PM, Andreas Kemnade wrote:
>>> On Wed, 30 Sep 2020 22:00:09 +0200
>>> Arnd Bergmann <arnd@arndb.de> wrote:
>>>
>>>> On Wed, Sep 30, 2020 at 6:44 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>
>>>>> On Wed, Sep 30, 2020 at 10:54:42AM +0200, Andreas Kemnade wrote:
>>>>>> Hi,
>>>>>>
>>>>>> after the $subject patch I get lots of errors like this:
>>>>>
>>>>> For reference, this refers to commit fff2d0f701e6 ("hwmon: (applesmc)
>>>>> avoid overlong udelay()").
>>>>>
>>>>>> [ 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
>>>>>>
>>>>>> CPU sticks at low speed and no fan is turning on.
>>>>>> Reverting this patch on top of 5.9-rc6 solves this problem.
>>>>>>
>>>>>> Some information from dmidecode:
>>>>>>
>>>>>> Base Board Information
>>>>>> Manufacturer: Apple Inc.
>>>>>> Product Name: Mac-7DF21CB3ED6977E5
>>>>>> Version: MacBookAir6,2
>>>>>>
>>>>>> Handle 0x0020, DMI type 11, 5 bytes OEM Strings String 1: Apple ROM Version. Model: …,
>>>>>> Handle 0x0020, DMI type 11, 5 bytes
>>>>>> OEM Strings
>>>>>> String 1: Apple ROM Version. Model: MBA61. EFI Version: 122.0.0
>>>>>> String 2: .0.0. Built by: root@saumon. Date: Wed Jun 10 18:
>>>>>> String 3: 10:36 PDT 2020. Revision: 122 (B&I). ROM Version: F000_B
>>>>>> String 4: 00. Build Type: Official Build, Release. Compiler: Appl
>>>>>> String 5: e clang version 3.0 (tags/Apple/clang-211.10.1) (based on LLVM
>>>>>> String 6: 3.0svn).
>>>>>>
>>>>>> Writing to things in /sys/devices/platform/applesmc.768 gives also the
>>>>>> said errors.
>>>>>> But writing 1 to fan1_maunal and 5000 to fan1_output turns the fan on
>>>>>> despite error messages.
>>>>>>
>>>>> Not really sure what to do here. I could revert the patch, but then we'd gain
>>>>> clang compile failures. Arnd, any idea ?
>>>>
>>>> It seems that either I made a mistake in the conversion and it sleeps for
>>>> less time than before, or my assumption was wrong that converting a delay to
>>>> a sleep is safe here.
>>>>
>>>> The error message indicates that the write fails, not the read, so that
>>>> is what I'd look at first. Right away I can see that the maximum time to
>>>> retry is only half of what it used to be, as we used to wait for
>>>> 0x10, 0x20, 0x40, 0x80, ..., 0x20000 microseconds for a total of
>>>> 0x3fff0 microseconds (262ms), while my patch went with the 131ms
>>>> total delay based on the comment saying "/* wait up to 128 ms for a
>>>> status change. */".
>>>>
>>> Yes, that is also what I read from the code. I just thought there must
>>> be something simple, which just needs a short look from another pair of
>>> eyes.
>>>
>>>> Since there is sleeping wait, I see no reason the timeout couldn't
>>>> be extended a lot, e.g. to a second, as in
>>>>
>>>> #define APPLESMC_MAX_WAIT 0x100000
>>>>
>>>> If that doesn't work, I'd try using mdelay() in place of
>>>> usleep_range(), such as
>>>>
>>>> mdelay(DIV_ROUND_UP(us, USEC_PER_MSEC)));
>>>>
>>>> This adds back a really nasty latency, but it should avoid the
>>>> compile-time problem.
>>>>
>>>> Andreas, can you try those two things? (one at a time,
>>>> not both)
>>>
>>> Ok, I tried. None of them works. I rechecked my work and created real
>>> git commits out of them and CONFIG_LOCALVERSION_AUTO is also set so
>>> the usual stupid things are rules out.
>>> In detail:
>>> On top of 5.9-rc6 + *reverted* patch:
>>> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
>>> index fd99c9df8a00..2a9bd7f2b71b 100644
>>> --- a/drivers/hwmon/applesmc.c
>>> +++ b/drivers/hwmon/applesmc.c
>>> @@ -45,7 +45,7 @@
>>> /* wait up to 128 ms for a status change. */
>>> #define APPLESMC_MIN_WAIT 0x0010
>>> #define APPLESMC_RETRY_WAIT 0x0100
>>> -#define APPLESMC_MAX_WAIT 0x20000
>>> +#define APPLESMC_MAX_WAIT 0x8000
>>>
>>> #define APPLESMC_READ_CMD 0x10
>>> #define APPLESMC_WRITE_CMD 0x11
>>>
>>
>> Oh man, that code is so badlys broken.
>>
>> send_byte() repeats sending the data if it was not immediately successful.
>> That is done for both data and commands. Effectively that happens if
>> the command is not immediately accepted. However, send_argument()
>> clearly assumes that each data byte is sent exactly once. Sending
>> it more than once will mess up the key that is supposed to be sent.
>> The Apple SMC emulation code in qemu confirms that data bytes can not
>> be written more than once.
>>
>> Of course, theoretically it may be that the first data byte was not
>> accepted (after all, the ACK bit is not set), but the ACK bit is
>> not checked again after udelay(APPLESMC_RETRY_WAIT), so it may
>> well have been set in the 256 uS between its check and re-writing
>> the data.
>>
>> In other words, this entire code only works accidentally to start with.
>>
>> If you like, you could play around with the code and find out if and
>> when exactly bit 1 (busy) is set, if and when bit 2 (ack) is set, and
>> if and when any other bit is set. We could also try to read port 0x31e
>> (the error port). Maybe the we can figure out what the error actually
>> is. But then I don't really know what we could do with that information.
>>
> Smoe research results: the second data byte seems to cause problems, not the
> command byte.
>
>> Other than that, the only useful idea I have is something crazy like
>> if (us < 10000)
>> udelay(us);
>> else
>> mdelay(DIV_ROUND_CLOSEST(udelay, 1000));
>> in the hope that clang doesn't convert that back into a
>> compile-time constant and udelay().
>>
>> Overall it seems like the apple protocol may expect to receive data
>> bytes faster than 1ms apart, because that is the only real difference
>> between the original code and the new code using mdelay().
>
> Yes, that explanation makes sense. If I am trying something like that, only
> the last byte requires more than APPLESMC_MIN_WAIT. I have seen max. 256us.
> So we could probably even use msleep for us > 1000 and udelay for anything below.
>
> Regards,
> Andreas
>
G'day Andreas,
I've examined the code in VirtualSMC and I'm not convinced we were not waiting on the wrong bits.
#define SMC_STATUS_AWAITING_DATA BIT0 ///< Ready to read data.
#define SMC_STATUS_IB_CLOSED BIT1 /// A write is pending.
#define SMC_STATUS_BUSY BIT2 ///< Busy processing a command.
#define SMC_STATUS_GOT_COMMAND BIT3 ///< The last input was a command.
#define SMC_STATUS_UKN_0x16 BIT4
#define SMC_STATUS_KEY_DONE BIT5
#define SMC_STATUS_READY BIT6 // Ready to work
#define SMC_STATUS_UKN_0x80 BIT7 // error
Any chance you could try this patch? It's ugly, hacked together and currently fairly undocumented, but if it works I'll figure out how to clean it up (suggestions welcome).
It works on my MacbookPro 11,1.
I've also attached a script I adapted from https://github.com/floe/smc_util.git that I use to hammer the SMC with reads.
The behavior pre 5.9 and post this patch is the same.
Regards,
Brad
[-- Attachment #2: smc.patch.2 --]
[-- Type: text/plain, Size: 3897 bytes --]
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index a18887990f4a..2500cc7ebca5 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -151,65 +151,58 @@ 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 on the SMC
*/
-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;
+ unsigned long end = jiffies + (APPLESMC_MAX_WAIT * HZ) / USEC_PER_SEC;
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\n", status);
return -EIO;
}
-
+
/*
* send_byte - Write to SMC port, retrying when necessary. Callers
* must hold applesmc_lock.
*/
-static int send_byte(u8 cmd, u16 port)
-{
- u8 status;
- int us;
- unsigned long end = jiffies + (APPLESMC_MAX_WAIT * HZ) / USEC_PER_SEC;
+static int send_byte_data(u8 cmd, u16 port, bool skip)
+{
+ u8 wstat=0x44;
+ if (skip)
+ wstat=0x40;
+ if (wait_status(0x44, 0xF6))
+ 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, 0xFE))
+ return 0;
+fail:
+ pr_warn("send_byte_data(0x%02x, 0x%04x) fail\n", cmd, APPLESMC_CMD_PORT);
return -EIO;
}
static int send_command(u8 cmd)
{
- return send_byte(cmd, APPLESMC_CMD_PORT);
+ if (wait_status(0x40, 0xF2)) { pr_warn("send_command fail 1\n");
+ goto fail;}
+ outb(cmd, APPLESMC_CMD_PORT);
+ if (wait_status(0x4C, 0xFF)) { pr_warn("send_command fail 2\n");
+ goto fail;}
+ 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 +210,7 @@ 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;
}
@@ -233,13 +226,13 @@ 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(0x45,0xFF)) {
pr_warn("%.4s: read data[%d] fail\n", key, i);
return -EIO;
}
@@ -269,14 +262,14 @@ static int write_smc(u8 cmd, const char *key, const u8 *buffer, u8 len)
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==(len-1)))) {
+ pr_warn("%s: write data fail at %i\n", key, i);
return -EIO;
}
}
[-- Attachment #3: smc_dump_linux.sh --]
[-- Type: application/x-shellscript, Size: 797 bytes --]
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [REGRESSION] hwmon: (applesmc) avoid overlong udelay()
2020-11-02 23:56 ` Brad Campbell
@ 2020-11-03 5:56 ` Brad Campbell
2020-11-04 13:20 ` Andreas Kemnade
0 siblings, 1 reply; 3+ messages in thread
From: Brad Campbell @ 2020-11-03 5:56 UTC (permalink / raw)
To: Andreas Kemnade, Guenter Roeck
Cc: Arnd Bergmann, rydberg, Jean Delvare, linux-hwmon, linux-kernel, hns
[-- Attachment #1: Type: text/plain, Size: 3909 bytes --]
On 3/11/20 10:56 am, Brad Campbell wrote:
>
> I've examined the code in VirtualSMC and I'm not convinced we were not waiting on the wrong bits.
>
> #define SMC_STATUS_AWAITING_DATA BIT0 ///< Ready to read data.
> #define SMC_STATUS_IB_CLOSED BIT1 /// A write is pending.
> #define SMC_STATUS_BUSY BIT2 ///< Busy processing a command.
> #define SMC_STATUS_GOT_COMMAND BIT3 ///< The last input was a command.
> #define SMC_STATUS_UKN_0x16 BIT4
> #define SMC_STATUS_KEY_DONE BIT5
> #define SMC_STATUS_READY BIT6 // Ready to work
> #define SMC_STATUS_UKN_0x80 BIT7 // error
>
> Any chance you could try this patch? It's ugly, hacked together and currently fairly undocumented, but if it works I'll figure out how to clean it up (suggestions welcome).
> It works on my MacbookPro 11,1.
I had some time so I spent a bit of time refactoring and trying to clarify the magic numbers.
I also did some fuzzing of the SMC and figured out where we can loosen the masks.
This has some debug code in it to identify if any wait loops exceed 1 loop and if the SMC is reporting anything other than a clear "I'm waiting" prior to each command.
You might see some of these :
[ 51.316202] applesmc: wait_status looping 2: 0x44, 0x40, 0x4e
[ 60.002547] applesmc: wait_status looping 2: 0x44, 0x40, 0x4e
[ 60.130754] applesmc: wait_status looping 2: 0x44, 0x40, 0x4e
I did some heavy tests and found that with the delays at the bottom of the loop about 50% of calls required no delay at all before a read or write and the other 50% require a single delay.
I can count on one hand the number of times it's exceeded 1 loop, and the max thus far has been 5 loops.
We've been treating bit 0x04 as an ack, but from my testing on the machine and what I've seen in the SMC emulator code it actually means "I'm busy in the middle of a command". Bit 0x02 seems to mean "I'm doing something and I *will* ignore anything you send me".
Bit 0x08 means "The last thing I got was a valid command, so start sending me data".
By testing and waiting for 0x02 to clear before sending or reading I haven't seen any need for retries.
On my unit bit 0x40 is always active. It may not be on others. The emulator calls it a status ready, so it's tested for but that is consolidated in wait_status so it can be commented out if it doesn't work on your machine.
The thing with bit 0x04 is the SMC clears it when it's no longer busy. So the last byte of data sent for a command sees it clear that bit. That explains the timeouts but the command still works. As far as the SMC is concerned it's got all the data and gone off to do its thing, but applesmc was waiting for the bit to be set. When it's in the middle of a command (from the first command byte until the last data byte is received) I've never seen bit 0x04 cleared, so using it as an "ack" works, but as said upward in the thread "probably by accident".
So this code tests for an "idle" SMC before it sends a command. In this context idle just means bit 0x02 isn't set. If the SMC gets into an undefined state it can leave other bits set, but a new write with a valid command will reset the internal state machine and bring it back into line. Bit 0x08 should always be set after it has received a valid command.
I've gone belt and braces by checking the status before and after each command, but with the intention of trying to catch and log anything that goes awry. Like I said above, in 50% of cases I see zero delays required so I thought testing before the delay was a worthwhile win.
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".
If anyone actually knows what they're doing and wants to "correct" me, it'd be appreciated also.
Regards,
Brad
[-- Attachment #2: smc.patch.5 --]
[-- Type: text/plain, Size: 6173 bytes --]
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index a18887990f4a..abb06fbe7bc1 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -42,6 +42,16 @@
#define APPLESMC_MAX_DATA_LENGTH 32
+/* Apple SMC status bits */
+#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.
+#define SMC_STATUS_GOT_COMMAND 0x08 ///< The last input was a command.
+#define SMC_STATUS_UKN_0x16 0x10
+#define SMC_STATUS_KEY_DONE 0x20
+#define SMC_STATUS_READY 0x40 // Ready to work
+#define SMC_STATUS_UKN_0x80 0x80 // error
+
/* wait up to 128 ms for a status change. */
#define APPLESMC_MIN_WAIT 0x0010
#define APPLESMC_RETRY_WAIT 0x0100
@@ -151,65 +161,88 @@ 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;
+ int loops = 0;
+ unsigned long end = jiffies + (APPLESMC_MAX_WAIT * HZ) / USEC_PER_SEC;
+ /* We always check for status ready */
+ val |= SMC_STATUS_READY;
+ mask |= SMC_STATUS_READY;
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);
+ loops += 1;
+ if (loops > 1)
+ pr_warn("wait_status looping %i: 0x%02x, 0x%02x, 0x%02x\n", loops, status, val, mask);
+ }
+ 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 = 0;
+ if (!skip)
+ wstat |= SMC_STATUS_BUSY;
+ 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_READY | SMC_STATUS_GOT_COMMAND | SMC_STATUS_BUSY | SMC_STATUS_IB_CLOSED))
+ 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, an 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 fail 1\n");
+ goto fail; }
+
+ status = inb(APPLESMC_CMD_PORT);
+ if (status != 0x40)
+ pr_warn("At command start, Status was 0x%20x\n", status);
+
+ outb(cmd, APPLESMC_CMD_PORT);
+ if (wait_status(SMC_STATUS_BUSY | SMC_STATUS_GOT_COMMAND,
+ SMC_STATUS_GOT_COMMAND | SMC_STATUS_BUSY | SMC_STATUS_IB_CLOSED |
+ SMC_STATUS_AWAITING_DATA)) {
+ pr_warn("send_command fail 2\n");
+ goto fail; }
+ 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 +250,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 +267,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_GOT_COMMAND | SMC_STATUS_BUSY | SMC_STATUS_IB_CLOSED |
+ SMC_STATUS_AWAITING_DATA)) {
pr_warn("%.4s: read data[%d] fail\n", key, i);
return -EIO;
}
@@ -250,7 +286,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);
}
@@ -269,14 +305,14 @@ static int write_smc(u8 cmd, const char *key, const u8 *buffer, u8 len)
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 == (len-1)))) {
+ pr_warn("%s: write data fail at %i\n", key, i);
return -EIO;
}
}
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [REGRESSION] hwmon: (applesmc) avoid overlong udelay()
2020-11-03 5:56 ` Brad Campbell
@ 2020-11-04 13:20 ` Andreas Kemnade
2020-11-05 2:18 ` Brad Campbell
0 siblings, 1 reply; 3+ messages in thread
From: Andreas Kemnade @ 2020-11-04 13:20 UTC (permalink / raw)
To: Brad Campbell
Cc: Guenter Roeck, Arnd Bergmann, rydberg, Jean Delvare, linux-hwmon,
linux-kernel, hns
On Tue, 3 Nov 2020 16:56:32 +1100
Brad Campbell <brad@fnarfbargle.com> wrote:
> On 3/11/20 10:56 am, Brad Campbell wrote:
>
> >
> > I've examined the code in VirtualSMC and I'm not convinced we were not waiting on the wrong bits.
> >
> > #define SMC_STATUS_AWAITING_DATA BIT0 ///< Ready to read data.
> > #define SMC_STATUS_IB_CLOSED BIT1 /// A write is pending.
> > #define SMC_STATUS_BUSY BIT2 ///< Busy processing a command.
> > #define SMC_STATUS_GOT_COMMAND BIT3 ///< The last input was a command.
> > #define SMC_STATUS_UKN_0x16 BIT4
> > #define SMC_STATUS_KEY_DONE BIT5
> > #define SMC_STATUS_READY BIT6 // Ready to work
> > #define SMC_STATUS_UKN_0x80 BIT7 // error
> >
> > Any chance you could try this patch? It's ugly, hacked together and currently fairly undocumented, but if it works I'll figure out how to clean it up (suggestions welcome).
> > It works on my MacbookPro 11,1.
>
> I had some time so I spent a bit of time refactoring and trying to clarify the magic numbers.
>
> I also did some fuzzing of the SMC and figured out where we can loosen the masks.
> This has some debug code in it to identify if any wait loops exceed 1 loop and if the SMC is reporting anything other than a clear "I'm waiting" prior to each command.
>
> You might see some of these :
> [ 51.316202] applesmc: wait_status looping 2: 0x44, 0x40, 0x4e
> [ 60.002547] applesmc: wait_status looping 2: 0x44, 0x40, 0x4e
> [ 60.130754] applesmc: wait_status looping 2: 0x44, 0x40, 0x4e
>
> I did some heavy tests and found that with the delays at the bottom of the loop about 50% of calls required no delay at all before a read or write and the other 50% require a single delay.
> I can count on one hand the number of times it's exceeded 1 loop, and the max thus far has been 5 loops.
>
That matches my experience. The only delay is at the end of the
command. Critcal seems to be that there is not too much delay in between.
[...]
> 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
Regards,
Andreas
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [REGRESSION] hwmon: (applesmc) avoid overlong udelay()
2020-11-04 13:20 ` Andreas Kemnade
@ 2020-11-05 2:18 ` Brad Campbell
2020-11-05 4:43 ` Guenter Roeck
0 siblings, 1 reply; 3+ messages in thread
From: Brad Campbell @ 2020-11-05 2:18 UTC (permalink / raw)
To: Andreas Kemnade, Guenter Roeck, Jean Delvare
Cc: Arnd Bergmann, rydberg, linux-hwmon, linux-kernel, hns
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.
Regards,
Brad
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [REGRESSION] hwmon: (applesmc) avoid overlong udelay()
2020-11-05 2:18 ` Brad Campbell
@ 2020-11-05 4:43 ` Guenter Roeck
2020-11-05 5:05 ` Brad Campbell
0 siblings, 1 reply; 3+ messages in thread
From: Guenter Roeck @ 2020-11-05 4:43 UTC (permalink / raw)
To: Brad Campbell, Andreas Kemnade, Jean Delvare
Cc: Arnd Bergmann, rydberg, linux-hwmon, linux-kernel, hns
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [REGRESSION] hwmon: (applesmc) avoid overlong udelay()
2020-11-05 4:43 ` Guenter Roeck
@ 2020-11-05 5:05 ` Brad Campbell
2020-11-05 5:26 ` Guenter Roeck
0 siblings, 1 reply; 3+ messages in thread
From: Brad Campbell @ 2020-11-05 5:05 UTC (permalink / raw)
To: Guenter Roeck, Andreas Kemnade, Jean Delvare
Cc: Arnd Bergmann, rydberg, linux-hwmon, linux-kernel, hns
[-- 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;
}
}
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [REGRESSION] hwmon: (applesmc) avoid overlong udelay()
2020-11-05 5:05 ` Brad Campbell
@ 2020-11-05 5:26 ` Guenter Roeck
2020-11-05 5:47 ` [PATCH] applesmc: Re-work SMC comms v1 Brad Campbell
0 siblings, 1 reply; 3+ messages in thread
From: Guenter Roeck @ 2020-11-05 5:26 UTC (permalink / raw)
To: Brad Campbell, Andreas Kemnade, Jean Delvare
Cc: Arnd Bergmann, rydberg, linux-hwmon, linux-kernel, hns
On 11/4/20 9:05 PM, Brad Campbell wrote:
> 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.
>
Can you clean that up and submit as patch ?
Thanks,
Guenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH] applesmc: Re-work SMC comms v1
2020-11-05 5:26 ` Guenter Roeck
@ 2020-11-05 5:47 ` Brad Campbell
2020-11-09 9:27 ` kernel test robot
0 siblings, 1 reply; 3+ messages in thread
From: Brad Campbell @ 2020-11-05 5:47 UTC (permalink / raw)
To: Guenter Roeck, Andreas Kemnade, Jean Delvare
Cc: Arnd Bergmann, rydberg, linux-hwmon, linux-kernel, hns
Commit fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()") introduced
an issue whereby communication with the SMC became unreliable with write
errors :
[ 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.
Reported-by: Andreas Kemnade <andreas@kemnade.info>
Signed-off-by: Brad Campbell <brad@fnarfbargle.com>
---
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;
}
}
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] applesmc: Re-work SMC comms v1
2020-11-05 5:47 ` [PATCH] applesmc: Re-work SMC comms v1 Brad Campbell
@ 2020-11-09 9:27 ` kernel test robot
0 siblings, 0 replies; 3+ messages in thread
From: kernel test robot @ 2020-11-09 9:27 UTC (permalink / raw)
To: Brad Campbell, Guenter Roeck, Andreas Kemnade, Jean Delvare
Cc: kbuild-all, Arnd Bergmann, rydberg, linux-hwmon, linux-kernel, hns
[-- Attachment #1: Type: text/plain, Size: 2339 bytes --]
Hi Brad,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on hwmon/hwmon-next]
[also build test WARNING on v5.10-rc3 next-20201109]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Brad-Campbell/applesmc-Re-work-SMC-comms-v1/20201105-134944
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: i386-randconfig-r022-20201109 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/db5e9737dcc2fec2ff3713bc346904d4b5ac5c0d
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Brad-Campbell/applesmc-Re-work-SMC-comms-v1/20201105-134944
git checkout db5e9737dcc2fec2ff3713bc346904d4b5ac5c0d
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/hwmon/applesmc.c: In function 'send_command':
>> drivers/hwmon/applesmc.c:214:5: warning: variable 'status' set but not used [-Wunused-but-set-variable]
214 | u8 status;
| ^~~~~~
vim +/status +214 drivers/hwmon/applesmc.c
206
207 /*
208 * send_command - Write a command to the SMC. Callers must hold applesmc_lock.
209 * If SMC is in undefined state, any new command write resets the state machine.
210 */
211
212 static int send_command(u8 cmd)
213 {
> 214 u8 status;
215
216 if (wait_status(0,
217 SMC_STATUS_IB_CLOSED)) {
218 pr_warn("send_command SMC was busy\n");
219 goto fail; }
220
221 status = inb(APPLESMC_CMD_PORT);
222
223 outb(cmd, APPLESMC_CMD_PORT);
224 if (!wait_status(SMC_STATUS_BUSY,
225 SMC_STATUS_BUSY))
226 return 0;
227 fail:
228 pr_warn("send_cmd(0x%02x, 0x%04x) fail\n", cmd, APPLESMC_CMD_PORT);
229 return -EIO;
230 }
231
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40374 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-11-09 9:28 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-05 6:32 [PATCH] applesmc: Re-work SMC comms v1 Guenter Roeck
-- strict thread matches above, loose matches on Subject: below --
2020-09-30 8:54 [REGRESSION] hwmon: (applesmc) avoid overlong udelay() Andreas Kemnade
2020-09-30 16:44 ` Guenter Roeck
2020-09-30 20:00 ` Arnd Bergmann
2020-10-01 22:22 ` Andreas Kemnade
2020-10-02 4:07 ` Guenter Roeck
2020-10-06 7:02 ` Andreas Kemnade
2020-11-02 23:56 ` Brad Campbell
2020-11-03 5:56 ` Brad Campbell
2020-11-04 13:20 ` Andreas Kemnade
2020-11-05 2:18 ` Brad Campbell
2020-11-05 4:43 ` Guenter Roeck
2020-11-05 5:05 ` Brad Campbell
2020-11-05 5:26 ` Guenter Roeck
2020-11-05 5:47 ` [PATCH] applesmc: Re-work SMC comms v1 Brad Campbell
2020-11-09 9:27 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).