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
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
On Wed, Sep 30, 2020 at 6:44 PM Guenter Roeck <[email protected]> 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
On Wed, 30 Sep 2020 22:00:09 +0200
Arnd Bergmann <[email protected]> wrote:
> On Wed, Sep 30, 2020 at 6:44 PM Guenter Roeck <[email protected]> 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
On 10/1/20 3:22 PM, Andreas Kemnade wrote:
> On Wed, 30 Sep 2020 22:00:09 +0200
> Arnd Bergmann <[email protected]> wrote:
>
>> On Wed, Sep 30, 2020 at 6:44 PM Guenter Roeck <[email protected]> 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
On Thu, 1 Oct 2020 21:07:51 -0700
Guenter Roeck <[email protected]> wrote:
> On 10/1/20 3:22 PM, Andreas Kemnade wrote:
> > On Wed, 30 Sep 2020 22:00:09 +0200
> > Arnd Bergmann <[email protected]> wrote:
> >
> >> On Wed, Sep 30, 2020 at 6:44 PM Guenter Roeck <[email protected]> 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
On 6/10/20 6:02 pm, Andreas Kemnade wrote:
> On Thu, 1 Oct 2020 21:07:51 -0700
> Guenter Roeck <[email protected]> wrote:
>
>> On 10/1/20 3:22 PM, Andreas Kemnade wrote:
>>> On Wed, 30 Sep 2020 22:00:09 +0200
>>> Arnd Bergmann <[email protected]> wrote:
>>>
>>>> On Wed, Sep 30, 2020 at 6:44 PM Guenter Roeck <[email protected]> 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
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
On Tue, 3 Nov 2020 16:56:32 +1100
Brad Campbell <[email protected]> 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
On 5/11/20 12:20 am, Andreas Kemnade wrote:
> On Tue, 3 Nov 2020 16:56:32 +1100
> Brad Campbell <[email protected]> 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
On 5/11/20 1:18 pm, Brad Campbell wrote:
> 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.
>
I managed to find an iMac 12,2 to test with. Had to remove the check for bit 0x40 (that machine sets bit 0x10). Updated patch with debugging code removed attached for comment.
Regards,
Brad
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 <[email protected]> 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
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 <[email protected]> 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
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 <[email protected]> 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
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 <[email protected]>
Signed-off-by: Brad Campbell <[email protected]>
---
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 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 <[email protected]>
Add
Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
> Signed-off-by: Brad Campbell <[email protected]>
>
> ---
> 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;
> }
> }
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 <[email protected]>
Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
Signed-off-by: Brad Campbell <[email protected]>
---
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;
}
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 <[email protected]>
> Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
> Signed-off-by: Brad Campbell <[email protected]>
>
> ---
> 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 <[email protected]> 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 <[email protected]>
> Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
> Signed-off-by: Brad Campbell <[email protected]>
>
Still works here:
Tested-by: Andreas Kemnade <[email protected]> # MacBookAir6,2
On Thu, 5 Nov 2020 08:56:04 +0100
Henrik Rydberg <[email protected]> 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 Thu, Nov 5, 2020 at 6:05 AM Brad Campbell <[email protected]> wrote:
> 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.
For the patch:
Acked-by: Arnd Bergmann <[email protected]>
I'm glad you figured out something that works. This all looks reasonable and
it makes much more sense than the original version that I tried to clean up
just based on the code comments but without having access to hardware or
documentation.
Arnd
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 <[email protected]>
> Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
> Signed-off-by: Brad Campbell <[email protected]>
>
> ---
> 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 <[email protected]>
>> Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
>> Signed-off-by: Brad Campbell <[email protected]>
>>
>> ---
>> 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 <[email protected]>
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 <[email protected]>
Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
Signed-off-by: Brad Campbell <[email protected]>
Signed-off-by: Henrik Rydberg <[email protected]>
---
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)
--
2.29.2
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;
}
G'day Henrik,
I noticed you'd also loosened up the requirement for SMC_STATUS_BUSY in read_smc(). I assume
that causes problems on the early Macbook. This is revised on the one sent earlier.
If you could test this on your Air1,1 it'd be appreciated.
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.
Tested on : MacbookAir6,2 MacBookPro11,1 iMac12,2
Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
Reported-by: Andreas Kemnade <[email protected]>
Tested-by: Andreas Kemnade <[email protected]> # MacBookAir6,2
Acked-by: Arnd Bergmann <[email protected]>
Signed-off-by: Brad Campbell <[email protected]>
Signed-off-by: Henrik Rydberg <[email protected]>
---
Changelog :
v1 : Inital attempt
v2 : Address logic and coding style
v3 : Removed some debug hangover. Added tested-by. Modifications for MacBookAir1,1
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index a18887990f4a..3e968abb37aa 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,8 @@ 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_AWAITING_DATA | SMC_STATUS_IB_CLOSED)) {
pr_warn("%.4s: read data[%d] fail\n", key, i);
return -EIO;
}
@@ -250,7 +265,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 +290,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
Hi Brad,
On 2020-11-08 02:00, Brad Campbell wrote:
> G'day Henrik,
>
> I noticed you'd also loosened up the requirement for SMC_STATUS_BUSY in read_smc(). I assume
> that causes problems on the early Macbook. This is revised on the one sent earlier.
> If you could test this on your Air1,1 it'd be appreciated.
No, I managed to screw up the patch; you can see that I carefully added
the same treatment for the read argument, being unsure if the BUSY state
would remain during the AVAILABLE data phase. I can check that again,
but unfortunately the patch in this email shows the same problem.
I think it may be worthwhile to rethink the behavior of wait_status()
here. If one machine shows no change after a certain status bit change,
then perhaps the others share that behavior, and we are waiting in vain.
Just imagine how many years of cpu that is, combined. ;-)
Henrik
>
> 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.
>
> Tested on : MacbookAir6,2 MacBookPro11,1 iMac12,2
>
> Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
> Reported-by: Andreas Kemnade <[email protected]>
> Tested-by: Andreas Kemnade <[email protected]> # MacBookAir6,2
> Acked-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Brad Campbell <[email protected]>
> Signed-off-by: Henrik Rydberg <[email protected]>
>
> ---
> Changelog :
> v1 : Inital attempt
> v2 : Address logic and coding style
> v3 : Removed some debug hangover. Added tested-by. Modifications for MacBookAir1,1
>
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index a18887990f4a..3e968abb37aa 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,8 @@ 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_AWAITING_DATA | SMC_STATUS_IB_CLOSED)) {
> pr_warn("%.4s: read data[%d] fail\n", key, i);
> return -EIO;
> }
> @@ -250,7 +265,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 +290,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 Sun, Nov 08, 2020 at 09:35:28AM +0100, Henrik Rydberg wrote:
> Hi Brad,
>
> On 2020-11-08 02:00, Brad Campbell wrote:
> > G'day Henrik,
> >
> > I noticed you'd also loosened up the requirement for SMC_STATUS_BUSY in read_smc(). I assume
> > that causes problems on the early Macbook. This is revised on the one sent earlier.
> > If you could test this on your Air1,1 it'd be appreciated.
>
> No, I managed to screw up the patch; you can see that I carefully added the
> same treatment for the read argument, being unsure if the BUSY state would
> remain during the AVAILABLE data phase. I can check that again, but
> unfortunately the patch in this email shows the same problem.
>
> I think it may be worthwhile to rethink the behavior of wait_status() here.
> If one machine shows no change after a certain status bit change, then
> perhaps the others share that behavior, and we are waiting in vain. Just
> imagine how many years of cpu that is, combined. ;-)
Here is a modification along that line.
Compared to your latest version, this one has wait_status() return the
actual status on success. Instead of waiting for BUSY, it waits for
the other status bits, and checks BUSY afterwards. So as not to wait
unneccesarily, the udelay() is placed together with the single
outb(). The return value of send_byte_data() is augmented with
-EAGAIN, which is then used in send_command() to create the resend
loop.
I reach 41 reads per second on the MBA1,1 with this version, which is
getting close to the performance prior to the problems.
From b4405457f4ba07cff7b7e4f48c47668bee176a25 Mon Sep 17 00:00:00 2001
From: Brad Campbell <[email protected]>
Date: Sun, 8 Nov 2020 12:00:03 +1100
Subject: [PATCH] hwmon: (applesmc) Re-work SMC comms
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.
Tested on : MacbookAir6,2 MacBookPro11,1 iMac12,2, MacBookAir1,1,
MacBookAir3,1
Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
Reported-by: Andreas Kemnade <[email protected]>
Tested-by: Andreas Kemnade <[email protected]> # MacBookAir6,2
Acked-by: Arnd Bergmann <[email protected]>
Signed-off-by: Brad Campbell <[email protected]>
Signed-off-by: Henrik Rydberg <[email protected]>
---
Changelog :
v1 : Inital attempt
v2 : Address logic and coding style
v3 : Removed some debug hangover. Added tested-by. Modifications for MacBookAir1,1
v4 : Do not expect busy state to appear without other state changes
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index a18887990f4a..ea7c66d5788e 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,78 @@ 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
+ * On success, returns the full status
+ * On failure, returns a negative error
*/
-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)
- return 0;
+ if ((status & mask) == val)
+ return status;
/* 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 status;
+ status = wait_status(0, SMC_STATUS_IB_CLOSED);
+ if (status < 0)
+ return status;
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);
- }
+ udelay(APPLESMC_MIN_WAIT);
+ status = wait_status(0, SMC_STATUS_IB_CLOSED);
+ if (status < 0)
+ return status;
+ if (skip || (status & SMC_STATUS_BUSY))
+ return 0;
+ return -EAGAIN;
+}
- 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 = send_byte(cmd, APPLESMC_CMD_PORT);
+ if (!ret)
+ return ret;
+ if (ret != -EAGAIN)
+ break;
+ usleep_range(APPLESMC_MIN_WAIT, APPLESMC_MIN_WAIT * 16);
+ }
+ return -EIO;
}
static int send_argument(const char *key)
@@ -239,7 +258,8 @@ 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_AWAITING_DATA | SMC_STATUS_IB_CLOSED) < 0) {
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);
}
@@ -275,7 +295,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;
}
--
2.29.2
On 8/11/20 9:14 pm, Henrik Rydberg wrote:
> On Sun, Nov 08, 2020 at 09:35:28AM +0100, Henrik Rydberg wrote:
>> Hi Brad,
>>
>> On 2020-11-08 02:00, Brad Campbell wrote:
>>> G'day Henrik,
>>>
>>> I noticed you'd also loosened up the requirement for SMC_STATUS_BUSY in read_smc(). I assume
>>> that causes problems on the early Macbook. This is revised on the one sent earlier.
>>> If you could test this on your Air1,1 it'd be appreciated.
>>
>> No, I managed to screw up the patch; you can see that I carefully added the
>> same treatment for the read argument, being unsure if the BUSY state would
>> remain during the AVAILABLE data phase. I can check that again, but
>> unfortunately the patch in this email shows the same problem.
>>
>> I think it may be worthwhile to rethink the behavior of wait_status() here.
>> If one machine shows no change after a certain status bit change, then
>> perhaps the others share that behavior, and we are waiting in vain. Just
>> imagine how many years of cpu that is, combined. ;-)
>
> Here is a modification along that line.
>
> Compared to your latest version, this one has wait_status() return the
> actual status on success. Instead of waiting for BUSY, it waits for
> the other status bits, and checks BUSY afterwards. So as not to wait
> unneccesarily, the udelay() is placed together with the single
> outb(). The return value of send_byte_data() is augmented with
> -EAGAIN, which is then used in send_command() to create the resend
> loop.
>
> I reach 41 reads per second on the MBA1,1 with this version, which is
> getting close to the performance prior to the problems.
G'day Henrik,
I like this one. It's slower on my laptop (40 rps vs 50 on the MacbookPro11,1) and the same 17 rps on the iMac 12,2 but it's as reliable
and if it works for both of yours then I think it's a winner. I can't really diagnose the iMac properly as I'm 2,800KM away and have
nobody to reboot it if I kill it. 5.7.2 gives 20 rps, so 17 is ok for me.
Andreas, could I ask you to test this one?
Odd my original version worked on your Air3,1 and the other 3 machines without retry.
I wonder how many commands require retries, how many retires are actually required, and what we are going wrong on the Air1,1 that requires
one or more retries.
I just feels like a brute force approach because there's something we're missing.
> From b4405457f4ba07cff7b7e4f48c47668bee176a25 Mon Sep 17 00:00:00 2001
> From: Brad Campbell <[email protected]>
> Date: Sun, 8 Nov 2020 12:00:03 +1100
> Subject: [PATCH] hwmon: (applesmc) Re-work SMC comms
>
> 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.
>
> Tested on : MacbookAir6,2 MacBookPro11,1 iMac12,2, MacBookAir1,1,
> MacBookAir3,1
>
> Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
> Reported-by: Andreas Kemnade <[email protected]>
> Tested-by: Andreas Kemnade <[email protected]> # MacBookAir6,2
> Acked-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Brad Campbell <[email protected]>
> Signed-off-by: Henrik Rydberg <[email protected]>
>
> ---
> Changelog :
> v1 : Inital attempt
> v2 : Address logic and coding style
> v3 : Removed some debug hangover. Added tested-by. Modifications for MacBookAir1,1
> v4 : Do not expect busy state to appear without other state changes
>
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index a18887990f4a..ea7c66d5788e 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,78 @@ 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
> + * On success, returns the full status
> + * On failure, returns a negative error
> */
> -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)
> - return 0;
> + if ((status & mask) == val)
> + return status;
> /* 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 status;
>
> + status = wait_status(0, SMC_STATUS_IB_CLOSED);
> + if (status < 0)
> + return status;
> 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);
> - }
> + udelay(APPLESMC_MIN_WAIT);
> + status = wait_status(0, SMC_STATUS_IB_CLOSED);
> + if (status < 0)
> + return status;
> + if (skip || (status & SMC_STATUS_BUSY))
> + return 0;
> + return -EAGAIN;
> +}
>
> - 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 = send_byte(cmd, APPLESMC_CMD_PORT);
> + if (!ret)
> + return ret;
> + if (ret != -EAGAIN)
> + break;
> + usleep_range(APPLESMC_MIN_WAIT, APPLESMC_MIN_WAIT * 16);
> + }
> + return -EIO;
> }
>
> static int send_argument(const char *key)
> @@ -239,7 +258,8 @@ 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_AWAITING_DATA | SMC_STATUS_IB_CLOSED) < 0) {
> 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);
> }
> @@ -275,7 +295,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 2020-11-08 12:57, Brad Campbell wrote:
> On 8/11/20 9:14 pm, Henrik Rydberg wrote:
>> On Sun, Nov 08, 2020 at 09:35:28AM +0100, Henrik Rydberg wrote:
>>> Hi Brad,
>>>
>>> On 2020-11-08 02:00, Brad Campbell wrote:
>>>> G'day Henrik,
>>>>
>>>> I noticed you'd also loosened up the requirement for SMC_STATUS_BUSY in read_smc(). I assume
>>>> that causes problems on the early Macbook. This is revised on the one sent earlier.
>>>> If you could test this on your Air1,1 it'd be appreciated.
>>>
>>> No, I managed to screw up the patch; you can see that I carefully added the
>>> same treatment for the read argument, being unsure if the BUSY state would
>>> remain during the AVAILABLE data phase. I can check that again, but
>>> unfortunately the patch in this email shows the same problem.
>>>
>>> I think it may be worthwhile to rethink the behavior of wait_status() here.
>>> If one machine shows no change after a certain status bit change, then
>>> perhaps the others share that behavior, and we are waiting in vain. Just
>>> imagine how many years of cpu that is, combined. ;-)
>>
>> Here is a modification along that line.
>>
>> Compared to your latest version, this one has wait_status() return the
>> actual status on success. Instead of waiting for BUSY, it waits for
>> the other status bits, and checks BUSY afterwards. So as not to wait
>> unneccesarily, the udelay() is placed together with the single
>> outb(). The return value of send_byte_data() is augmented with
>> -EAGAIN, which is then used in send_command() to create the resend
>> loop.
>>
>> I reach 41 reads per second on the MBA1,1 with this version, which is
>> getting close to the performance prior to the problems.
>
> G'day Henrik,
>
> I like this one. It's slower on my laptop (40 rps vs 50 on the MacbookPro11,1) and the same 17 rps on the iMac 12,2 but it's as reliable
> and if it works for both of yours then I think it's a winner. I can't really diagnose the iMac properly as I'm 2,800KM away and have
> nobody to reboot it if I kill it. 5.7.2 gives 20 rps, so 17 is ok for me.
>
> Andreas, could I ask you to test this one?
>
> Odd my original version worked on your Air3,1 and the other 3 machines without retry.
> I wonder how many commands require retries, how many retires are actually required, and what we are going wrong on the Air1,1 that requires
> one or more retries.
>
> I just feels like a brute force approach because there's something we're missing.
I would think you are right. There should be a way to follow the status
changes in realtime, so one can determine handshake and processing from
that information. At least, with this change, we are making the blunt
instrument a little smaller.
Cheers,
Henrik
On 11/8/20 2:14 AM, Henrik Rydberg wrote:
> On Sun, Nov 08, 2020 at 09:35:28AM +0100, Henrik Rydberg wrote:
>> Hi Brad,
>>
>> On 2020-11-08 02:00, Brad Campbell wrote:
>>> G'day Henrik,
>>>
>>> I noticed you'd also loosened up the requirement for SMC_STATUS_BUSY in read_smc(). I assume
>>> that causes problems on the early Macbook. This is revised on the one sent earlier.
>>> If you could test this on your Air1,1 it'd be appreciated.
>>
>> No, I managed to screw up the patch; you can see that I carefully added the
>> same treatment for the read argument, being unsure if the BUSY state would
>> remain during the AVAILABLE data phase. I can check that again, but
>> unfortunately the patch in this email shows the same problem.
>>
>> I think it may be worthwhile to rethink the behavior of wait_status() here.
>> If one machine shows no change after a certain status bit change, then
>> perhaps the others share that behavior, and we are waiting in vain. Just
>> imagine how many years of cpu that is, combined. ;-)
>
> Here is a modification along that line.
>
Please resend this patch as stand-alone v4 patch. If sent like it was sent here,
it doesn't make it into patchwork, and is thus not only difficult to apply but
may get lost, and it is all but impossible to find and apply all tags.
Also, prior to Henrik's Signed=off-by: there should be a one-line explanation
of the changes made.
Thanks,
Guenter
> Compared to your latest version, this one has wait_status() return the
> actual status on success. Instead of waiting for BUSY, it waits for
> the other status bits, and checks BUSY afterwards. So as not to wait
> unneccesarily, the udelay() is placed together with the single
> outb(). The return value of send_byte_data() is augmented with
> -EAGAIN, which is then used in send_command() to create the resend
> loop.
>
> I reach 41 reads per second on the MBA1,1 with this version, which is
> getting close to the performance prior to the problems.
>
>>From b4405457f4ba07cff7b7e4f48c47668bee176a25 Mon Sep 17 00:00:00 2001
> From: Brad Campbell <[email protected]>
> Date: Sun, 8 Nov 2020 12:00:03 +1100
> Subject: [PATCH] hwmon: (applesmc) Re-work SMC comms
>
> 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.
>
> Tested on : MacbookAir6,2 MacBookPro11,1 iMac12,2, MacBookAir1,1,
> MacBookAir3,1
>
> Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
> Reported-by: Andreas Kemnade <[email protected]>
> Tested-by: Andreas Kemnade <[email protected]> # MacBookAir6,2
> Acked-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Brad Campbell <[email protected]>
> Signed-off-by: Henrik Rydberg <[email protected]>
>
> ---
> Changelog :
> v1 : Inital attempt
> v2 : Address logic and coding style
> v3 : Removed some debug hangover. Added tested-by. Modifications for MacBookAir1,1
> v4 : Do not expect busy state to appear without other state changes
>
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index a18887990f4a..ea7c66d5788e 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,78 @@ 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
> + * On success, returns the full status
> + * On failure, returns a negative error
> */
> -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)
> - return 0;
> + if ((status & mask) == val)
> + return status;
> /* 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 status;
>
> + status = wait_status(0, SMC_STATUS_IB_CLOSED);
> + if (status < 0)
> + return status;
> 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);
> - }
> + udelay(APPLESMC_MIN_WAIT);
> + status = wait_status(0, SMC_STATUS_IB_CLOSED);
> + if (status < 0)
> + return status;
> + if (skip || (status & SMC_STATUS_BUSY))
> + return 0;
> + return -EAGAIN;
> +}
>
> - 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 = send_byte(cmd, APPLESMC_CMD_PORT);
> + if (!ret)
> + return ret;
> + if (ret != -EAGAIN)
> + break;
> + usleep_range(APPLESMC_MIN_WAIT, APPLESMC_MIN_WAIT * 16);
> + }
> + return -EIO;
> }
>
> static int send_argument(const char *key)
> @@ -239,7 +258,8 @@ 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_AWAITING_DATA | SMC_STATUS_IB_CLOSED) < 0) {
> 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);
> }
> @@ -275,7 +295,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 9/11/20 3:06 am, Guenter Roeck wrote:
> On 11/8/20 2:14 AM, Henrik Rydberg wrote:
>> On Sun, Nov 08, 2020 at 09:35:28AM +0100, Henrik Rydberg wrote:
>>> Hi Brad,
>>>
>>> On 2020-11-08 02:00, Brad Campbell wrote:
>>>> G'day Henrik,
>>>>
>>>> I noticed you'd also loosened up the requirement for SMC_STATUS_BUSY in read_smc(). I assume
>>>> that causes problems on the early Macbook. This is revised on the one sent earlier.
>>>> If you could test this on your Air1,1 it'd be appreciated.
>>>
>>> No, I managed to screw up the patch; you can see that I carefully added the
>>> same treatment for the read argument, being unsure if the BUSY state would
>>> remain during the AVAILABLE data phase. I can check that again, but
>>> unfortunately the patch in this email shows the same problem.
>>>
>>> I think it may be worthwhile to rethink the behavior of wait_status() here.
>>> If one machine shows no change after a certain status bit change, then
>>> perhaps the others share that behavior, and we are waiting in vain. Just
>>> imagine how many years of cpu that is, combined. ;-)
>>
>> Here is a modification along that line.
>>
>
> Please resend this patch as stand-alone v4 patch. If sent like it was sent here,
> it doesn't make it into patchwork, and is thus not only difficult to apply but
> may get lost, and it is all but impossible to find and apply all tags.
> Also, prior to Henrik's Signed=off-by: there should be a one-line explanation
> of the changes made.
G'day Guenter,
Yes, I'll do that. I still have some more testing to do before it's pushed forwards.
Regards,
Brad
>
>> Compared to your latest version, this one has wait_status() return the
>> actual status on success. Instead of waiting for BUSY, it waits for
>> the other status bits, and checks BUSY afterwards. So as not to wait
>> unneccesarily, the udelay() is placed together with the single
>> outb(). The return value of send_byte_data() is augmented with
>> -EAGAIN, which is then used in send_command() to create the resend
>> loop.
>>
>> I reach 41 reads per second on the MBA1,1 with this version, which is
>> getting close to the performance prior to the problems.
>>
>> >From b4405457f4ba07cff7b7e4f48c47668bee176a25 Mon Sep 17 00:00:00 2001
>> From: Brad Campbell <[email protected]>
>> Date: Sun, 8 Nov 2020 12:00:03 +1100
>> Subject: [PATCH] hwmon: (applesmc) Re-work SMC comms
>>
>> 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.
>>
>> Tested on : MacbookAir6,2 MacBookPro11,1 iMac12,2, MacBookAir1,1,
>> MacBookAir3,1
>>
>> Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
>> Reported-by: Andreas Kemnade <[email protected]>
>> Tested-by: Andreas Kemnade <[email protected]> # MacBookAir6,2
>> Acked-by: Arnd Bergmann <[email protected]>
>> Signed-off-by: Brad Campbell <[email protected]>
>> Signed-off-by: Henrik Rydberg <[email protected]>
>>
>> ---
>> Changelog :
>> v1 : Inital attempt
>> v2 : Address logic and coding style
>> v3 : Removed some debug hangover. Added tested-by. Modifications for MacBookAir1,1
>> v4 : Do not expect busy state to appear without other state changes
>>
>> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
>> index a18887990f4a..ea7c66d5788e 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,78 @@ 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
>> + * On success, returns the full status
>> + * On failure, returns a negative error
>> */
>> -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)
>> - return 0;
>> + if ((status & mask) == val)
>> + return status;
>> /* 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 status;
>>
>> + status = wait_status(0, SMC_STATUS_IB_CLOSED);
>> + if (status < 0)
>> + return status;
>> 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);
>> - }
>> + udelay(APPLESMC_MIN_WAIT);
>> + status = wait_status(0, SMC_STATUS_IB_CLOSED);
>> + if (status < 0)
>> + return status;
>> + if (skip || (status & SMC_STATUS_BUSY))
>> + return 0;
>> + return -EAGAIN;
>> +}
>>
>> - 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 = send_byte(cmd, APPLESMC_CMD_PORT);
>> + if (!ret)
>> + return ret;
>> + if (ret != -EAGAIN)
>> + break;
>> + usleep_range(APPLESMC_MIN_WAIT, APPLESMC_MIN_WAIT * 16);
>> + }
>> + return -EIO;
>> }
>>
>> static int send_argument(const char *key)
>> @@ -239,7 +258,8 @@ 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_AWAITING_DATA | SMC_STATUS_IB_CLOSED) < 0) {
>> 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);
>> }
>> @@ -275,7 +295,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 Sun, 8 Nov 2020 11:14:29 +0100
Henrik Rydberg <[email protected]> wrote:
> On Sun, Nov 08, 2020 at 09:35:28AM +0100, Henrik Rydberg wrote:
> > Hi Brad,
> >
> > On 2020-11-08 02:00, Brad Campbell wrote:
> > > G'day Henrik,
> > >
> > > I noticed you'd also loosened up the requirement for SMC_STATUS_BUSY in read_smc(). I assume
> > > that causes problems on the early Macbook. This is revised on the one sent earlier.
> > > If you could test this on your Air1,1 it'd be appreciated.
> >
> > No, I managed to screw up the patch; you can see that I carefully added the
> > same treatment for the read argument, being unsure if the BUSY state would
> > remain during the AVAILABLE data phase. I can check that again, but
> > unfortunately the patch in this email shows the same problem.
> >
> > I think it may be worthwhile to rethink the behavior of wait_status() here.
> > If one machine shows no change after a certain status bit change, then
> > perhaps the others share that behavior, and we are waiting in vain. Just
> > imagine how many years of cpu that is, combined. ;-)
>
> Here is a modification along that line.
>
> Compared to your latest version, this one has wait_status() return the
> actual status on success. Instead of waiting for BUSY, it waits for
> the other status bits, and checks BUSY afterwards. So as not to wait
> unneccesarily, the udelay() is placed together with the single
> outb(). The return value of send_byte_data() is augmented with
> -EAGAIN, which is then used in send_command() to create the resend
> loop.
>
> I reach 41 reads per second on the MBA1,1 with this version, which is
> getting close to the performance prior to the problems.
>
> From b4405457f4ba07cff7b7e4f48c47668bee176a25 Mon Sep 17 00:00:00 2001
> From: Brad Campbell <[email protected]>
> Date: Sun, 8 Nov 2020 12:00:03 +1100
> Subject: [PATCH] hwmon: (applesmc) Re-work SMC comms
>
> 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.
>
> Tested on : MacbookAir6,2 MacBookPro11,1 iMac12,2, MacBookAir1,1,
> MacBookAir3,1
>
> Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
> Reported-by: Andreas Kemnade <[email protected]>
> Tested-by: Andreas Kemnade <[email protected]> # MacBookAir6,2
> Acked-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Brad Campbell <[email protected]>
> Signed-off-by: Henrik Rydberg <[email protected]>
>
> ---
> Changelog :
> v1 : Inital attempt
> v2 : Address logic and coding style
> v3 : Removed some debug hangover. Added tested-by. Modifications for MacBookAir1,1
> v4 : Do not expect busy state to appear without other state changes
>
still works here (MacBookAir6,2)
Regards,
Andreas
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 <[email protected]>
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/[email protected]
On 9/11/20 7:44 pm, Andreas Kemnade wrote:
> On Sun, 8 Nov 2020 11:14:29 +0100
> Henrik Rydberg <[email protected]> wrote:
>
>> On Sun, Nov 08, 2020 at 09:35:28AM +0100, Henrik Rydberg wrote:
>>> Hi Brad,
>>>
>>> On 2020-11-08 02:00, Brad Campbell wrote:
>>>> G'day Henrik,
>>>>
>>>> I noticed you'd also loosened up the requirement for SMC_STATUS_BUSY in read_smc(). I assume
>>>> that causes problems on the early Macbook. This is revised on the one sent earlier.
>>>> If you could test this on your Air1,1 it'd be appreciated.
>>>
>>> No, I managed to screw up the patch; you can see that I carefully added the
>>> same treatment for the read argument, being unsure if the BUSY state would
>>> remain during the AVAILABLE data phase. I can check that again, but
>>> unfortunately the patch in this email shows the same problem.
>>>
>>> I think it may be worthwhile to rethink the behavior of wait_status() here.
>>> If one machine shows no change after a certain status bit change, then
>>> perhaps the others share that behavior, and we are waiting in vain. Just
>>> imagine how many years of cpu that is, combined. ;-)
>>
>> Here is a modification along that line.
>>
>> Compared to your latest version, this one has wait_status() return the
>> actual status on success. Instead of waiting for BUSY, it waits for
>> the other status bits, and checks BUSY afterwards. So as not to wait
>> unneccesarily, the udelay() is placed together with the single
>> outb(). The return value of send_byte_data() is augmented with
>> -EAGAIN, which is then used in send_command() to create the resend
>> loop.
>>
>> I reach 41 reads per second on the MBA1,1 with this version, which is
>> getting close to the performance prior to the problems.
>>
>> From b4405457f4ba07cff7b7e4f48c47668bee176a25 Mon Sep 17 00:00:00 2001
>> From: Brad Campbell <[email protected]>
>> Date: Sun, 8 Nov 2020 12:00:03 +1100
>> Subject: [PATCH] hwmon: (applesmc) Re-work SMC comms
>>
>> 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.
>>
>> Tested on : MacbookAir6,2 MacBookPro11,1 iMac12,2, MacBookAir1,1,
>> MacBookAir3,1
>>
>> Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
>> Reported-by: Andreas Kemnade <[email protected]>
>> Tested-by: Andreas Kemnade <[email protected]> # MacBookAir6,2
>> Acked-by: Arnd Bergmann <[email protected]>
>> Signed-off-by: Brad Campbell <[email protected]>
>> Signed-off-by: Henrik Rydberg <[email protected]>
>>
>> ---
>> Changelog :
>> v1 : Inital attempt
>> v2 : Address logic and coding style
>> v3 : Removed some debug hangover. Added tested-by. Modifications for MacBookAir1,1
>> v4 : Do not expect busy state to appear without other state changes
>>
>
> still works here (MacBookAir6,2)
Much appreciated Andreas.
Regards,
Brad
On 8/11/20 11:04 pm, Henrik Rydberg wrote:
> On 2020-11-08 12:57, Brad Campbell wrote:
>> On 8/11/20 9:14 pm, Henrik Rydberg wrote:
>>> On Sun, Nov 08, 2020 at 09:35:28AM +0100, Henrik Rydberg wrote:
>>>> Hi Brad,
>>>>
>>>> On 2020-11-08 02:00, Brad Campbell wrote:
>>>>> G'day Henrik,
>>>>>
>>>>> I noticed you'd also loosened up the requirement for SMC_STATUS_BUSY in read_smc(). I assume
>>>>> that causes problems on the early Macbook. This is revised on the one sent earlier.
>>>>> If you could test this on your Air1,1 it'd be appreciated.
>>>>
>>>> No, I managed to screw up the patch; you can see that I carefully added the
>>>> same treatment for the read argument, being unsure if the BUSY state would
>>>> remain during the AVAILABLE data phase. I can check that again, but
>>>> unfortunately the patch in this email shows the same problem.
>>>>
>>>> I think it may be worthwhile to rethink the behavior of wait_status() here.
>>>> If one machine shows no change after a certain status bit change, then
>>>> perhaps the others share that behavior, and we are waiting in vain. Just
>>>> imagine how many years of cpu that is, combined. ;-)
>>>
>>> Here is a modification along that line.
>>>
>>> Compared to your latest version, this one has wait_status() return the
>>> actual status on success. Instead of waiting for BUSY, it waits for
>>> the other status bits, and checks BUSY afterwards. So as not to wait
>>> unneccesarily, the udelay() is placed together with the single
>>> outb(). The return value of send_byte_data() is augmented with
>>> -EAGAIN, which is then used in send_command() to create the resend
>>> loop.
>>>
>>> I reach 41 reads per second on the MBA1,1 with this version, which is
>>> getting close to the performance prior to the problems.
>>
>> G'day Henrik,
>>
>> I like this one. It's slower on my laptop (40 rps vs 50 on the MacbookPro11,1) and the same 17 rps on the iMac 12,2 but it's as reliable
>> and if it works for both of yours then I think it's a winner. I can't really diagnose the iMac properly as I'm 2,800KM away and have
>> nobody to reboot it if I kill it. 5.7.2 gives 20 rps, so 17 is ok for me.
>>
>> Andreas, could I ask you to test this one?
>>
>> Odd my original version worked on your Air3,1 and the other 3 machines without retry.
>> I wonder how many commands require retries, how many retires are actually required, and what we are going wrong on the Air1,1 that requires
>> one or more retries.
>>
>> I just feels like a brute force approach because there's something we're missing.
>
> I would think you are right. There should be a way to follow the status changes in realtime, so one can determine handshake and processing from that information. At least, with this change, we are making the blunt instrument a little smaller.
G'day Henrik,
Out of morbid curiosity I grabbed an older MacOS AppleSMC.kext (10.7) and ran it through the disassembler.
Every read/write to the SMC starts the same way with a check to make sure the SMC is in a sane state. If it's not, a read command is sent to try and kick it back into line :
Wait for 0x04 to clear. This is 1,000,000 iterations of "read status, check if 0x04 is set, delay 10uS".
If it clears, move on. If it doesn't, try and send a read command (just the command 0x10) and wait for the busy flag to clear again with the same loop.
So in theory if the SMC was locked up, it'd be into the weeds for 20 seconds before it pushed the error out.
So, lets say we've waited long enough and the busy flag dropped :
Each command write is :
Wait for 0x02 to clear. This is 1,000,000 iterations of "read status, check if 0x02 is set, delay 10uS".
Send command
Each data byte write is :
Wait for 0x02 to clear. This is 1,000,000 iterations of "read status, check if 0x02 is set, delay 10uS".
Immediate and single status read, check 0x04. If not set, abort.
Send data byte
Each data byte read is :
read staus, wait for 0x01 and 0x04 to be set. delay 10uS and repeat. Abort if fail.
Each timeout is 1,000,000 loops with a 10uS delay.
So aside from the startup set which occurs on *every* read or write set, status checks happen before a command or data write, and not at all after.
Under no circumstances are writes of any kind re-tried, but these timeouts are up to 10 seconds!
That would indicate that the requirement for retries on the early Mac means we're not waiting long enough somewhere. Not that I'm suggesting we do another re-work, but when I get back in front of my iMac which does 17 transactions per second with this driver, I might re-work it similar to the Apple driver and see what happens.
Oh, and it looks like the 0x40 flag that is on mine is the "I have an interrupt pending" flag, and the result should be able to be read from 0x31F. I'll play with that when I get time. That probably explains why IRQ9 screams until the kernel gags it on this machine as it's not being given any love.
Regards,
Brad
Hi Brad,
> Out of morbid curiosity I grabbed an older MacOS AppleSMC.kext (10.7) and ran it through the disassembler.
>
> Every read/write to the SMC starts the same way with a check to make sure the SMC is in a sane state. If it's not, a read command is sent to try and kick it back into line :
> Wait for 0x04 to clear. This is 1,000,000 iterations of "read status, check if 0x04 is set, delay 10uS".
> If it clears, move on. If it doesn't, try and send a read command (just the command 0x10) and wait for the busy flag to clear again with the same loop.
>
> So in theory if the SMC was locked up, it'd be into the weeds for 20 seconds before it pushed the error out.
>
> So, lets say we've waited long enough and the busy flag dropped :
>
> Each command write is :
> Wait for 0x02 to clear. This is 1,000,000 iterations of "read status, check if 0x02 is set, delay 10uS".
> Send command
>
> Each data byte write is :
> Wait for 0x02 to clear. This is 1,000,000 iterations of "read status, check if 0x02 is set, delay 10uS".
> Immediate and single status read, check 0x04. If not set, abort.
> Send data byte
>
> Each data byte read is :
> read staus, wait for 0x01 and 0x04 to be set. delay 10uS and repeat. Abort if fail.
>
> Each timeout is 1,000,000 loops with a 10uS delay.
>
> So aside from the startup set which occurs on *every* read or write set, status checks happen before a command or data write, and not at all after.
> Under no circumstances are writes of any kind re-tried, but these timeouts are up to 10 seconds!
Great findings here. But from this, it would seem we are doing almost
the right thing already, no? The essential difference seems to be that
where the kext does a read to wake up the SMC, while we retry the first
command until it works. If would of course be very interesting to know
if that makes a difference.
> That would indicate that the requirement for retries on the early Mac means we're not waiting long enough somewhere. Not that I'm suggesting we do another re-work, but when I get back in front of my iMac which does 17 transactions per second with this driver, I might re-work it similar to the Apple driver and see what happens.
>
> Oh, and it looks like the 0x40 flag that is on mine is the "I have an interrupt pending" flag, and the result should be able to be read from 0x31F. I'll play with that when I get time. That probably explains why IRQ9 screams until the kernel gags it on this machine as it's not being given any love.
Sounds good, getting interrupts working would have been nice.
Henrik
On 10/11/20 4:08 am, Henrik Rydberg wrote:
> Hi Brad,
>
>> Out of morbid curiosity I grabbed an older MacOS AppleSMC.kext (10.7) and ran it through the disassembler.
>>
>> Every read/write to the SMC starts the same way with a check to make sure the SMC is in a sane state. If it's not, a read command is sent to try and kick it back into line :
>> Wait for 0x04 to clear. This is 1,000,000 iterations of "read status, check if 0x04 is set, delay 10uS".
>> If it clears, move on. If it doesn't, try and send a read command (just the command 0x10) and wait for the busy flag to clear again with the same loop.
>>
>> So in theory if the SMC was locked up, it'd be into the weeds for 20 seconds before it pushed the error out.
>>
>> So, lets say we've waited long enough and the busy flag dropped :
>>
>> Each command write is :
>> Wait for 0x02 to clear. This is 1,000,000 iterations of "read status, check if 0x02 is set, delay 10uS".
>> Send command
>>
>> Each data byte write is :
>> Wait for 0x02 to clear. This is 1,000,000 iterations of "read status, check if 0x02 is set, delay 10uS".
>> Immediate and single status read, check 0x04. If not set, abort.
>> Send data byte
>>
>> Each data byte read is :
>> read staus, wait for 0x01 and 0x04 to be set. delay 10uS and repeat. Abort if fail.
>>
>> Each timeout is 1,000,000 loops with a 10uS delay.
>>
>> So aside from the startup set which occurs on *every* read or write set, status checks happen before a command or data write, and not at all after.
>> Under no circumstances are writes of any kind re-tried, but these timeouts are up to 10 seconds!
>
> Great findings here. But from this, it would seem we are doing almost the right thing already, no? The essential difference seems to be that where the kext does a read to wake up the SMC, while we retry the first command until it works. If would of course be very interesting to know if that makes a difference.
It does make a significant difference here. It doesn't use the read to wake up the SMC as such. It appears to use the read to get the SMC in sync with the driver. It only performs the extra read if the busy line is still active when it shouldn't be and provided the driver plays by the rules it only seems to do it once on init and only if the SMC thinks it's mid command (so has been left in an undefined state).
Re-working the driver to use the logic described my MacbookPro11,1 goes from 40 reads/sec to 125 reads/sec. My iMac12,2 goes from 17 reads/sec to 30.
I have one issue to understand before I post a patch.
If the SMC is in an inconsistent state (as in busy persistently high) then the driver issues a read command and waits for busy to drop (and it does, and bit 0x08 goes high on my laptop but nothing checks that). That is to a point expected based on the poking I did early on in this process.
On the other hand, when we perform a read or a write, the driver issues a read or write command and the following commands to send the key rely on the busy bit being set.
Now, in practice this works, and I've sent spurious commands to get things out of sync and after a long wait it syncs back up. I just want to try and understand the state machine inside the SMC a bit better before posting another patch.
>> That would indicate that the requirement for retries on the early Mac means we're not waiting long enough somewhere. Not that I'm suggesting we do another re-work, but when I get back in front of my iMac which does 17 transactions per second with this driver, I might re-work it similar to the Apple driver and see what happens.
>>
>> Oh, and it looks like the 0x40 flag that is on mine is the "I have an interrupt pending" flag, and the result should be able to be read from 0x31F. I'll play with that when I get time. That probably explains why IRQ9 screams until the kernel gags it on this machine as it's not being given any love.
>
> Sounds good, getting interrupts working would have been nice.
I've put it on my list of things to look at. There's a lot of magic constants in the interrupt handler.
Regards,
Brad
On 9/11/20 3:06 am, Guenter Roeck wrote:
> On 11/8/20 2:14 AM, Henrik Rydberg wrote:
>> On Sun, Nov 08, 2020 at 09:35:28AM +0100, Henrik Rydberg wrote:
>>> Hi Brad,
>>>
>>> On 2020-11-08 02:00, Brad Campbell wrote:
>>>> G'day Henrik,
>>>>
>>>> I noticed you'd also loosened up the requirement for SMC_STATUS_BUSY in read_smc(). I assume
>>>> that causes problems on the early Macbook. This is revised on the one sent earlier.
>>>> If you could test this on your Air1,1 it'd be appreciated.
>>>
>>> No, I managed to screw up the patch; you can see that I carefully added the
>>> same treatment for the read argument, being unsure if the BUSY state would
>>> remain during the AVAILABLE data phase. I can check that again, but
>>> unfortunately the patch in this email shows the same problem.
>>>
>>> I think it may be worthwhile to rethink the behavior of wait_status() here.
>>> If one machine shows no change after a certain status bit change, then
>>> perhaps the others share that behavior, and we are waiting in vain. Just
>>> imagine how many years of cpu that is, combined. ;-)
>>
>> Here is a modification along that line.
>>
>
> Please resend this patch as stand-alone v4 patch. If sent like it was sent here,
> it doesn't make it into patchwork, and is thus not only difficult to apply but
> may get lost, and it is all but impossible to find and apply all tags.
> Also, prior to Henrik's Signed=off-by: there should be a one-line explanation
> of the changes made.
>
> Thanks,
> Guenter
>
>> Compared to your latest version, this one has wait_status() return the
>> actual status on success. Instead of waiting for BUSY, it waits for
>> the other status bits, and checks BUSY afterwards. So as not to wait
>> unneccesarily, the udelay() is placed together with the single
>> outb(). The return value of send_byte_data() is augmented with
>> -EAGAIN, which is then used in send_command() to create the resend
>> loop.
>>
>> I reach 41 reads per second on the MBA1,1 with this version, which is
>> getting close to the performance prior to the problems.
>>
Can I get an opinion on this wait statement please?
The apple driver uses a loop with a million (1,000,000) retries spaced with a 10uS delay.
In my testing on 2 machines, we don't busy wait more than about 2 loops.
Replacing a small udelay with the usleep_range kills performance.
With the following (do 10 fast checks before we start sleeping) I nearly triple the performance
of the driver on my laptop, and double it on my iMac. This is on an otherwise unmodified version of
Henriks v4 submission.
Yes, given the timeouts I know it's a ridiculous loop condition.
static int wait_status(u8 val, u8 mask)
{
unsigned long end = jiffies + (APPLESMC_MAX_WAIT * HZ) / USEC_PER_SEC;
u8 status;
int i;
for (i=1; i < 1000000; i++) {
status = inb(APPLESMC_CMD_PORT);
if ((status & mask) == val)
return status;
/* timeout: give up */
if (time_after(jiffies, end))
break;
if (i < 10)
udelay(10);
else
usleep_range(APPLESMC_MIN_WAIT, APPLESMC_MIN_WAIT * 16);
}
return -EIO;
}
Regards,
Brad
On Tue, Nov 10, 2020 at 01:04:04PM +1100, Brad Campbell wrote:
> On 9/11/20 3:06 am, Guenter Roeck wrote:
> > On 11/8/20 2:14 AM, Henrik Rydberg wrote:
> >> On Sun, Nov 08, 2020 at 09:35:28AM +0100, Henrik Rydberg wrote:
> >>> Hi Brad,
> >>>
> >>> On 2020-11-08 02:00, Brad Campbell wrote:
> >>>> G'day Henrik,
> >>>>
> >>>> I noticed you'd also loosened up the requirement for SMC_STATUS_BUSY in read_smc(). I assume
> >>>> that causes problems on the early Macbook. This is revised on the one sent earlier.
> >>>> If you could test this on your Air1,1 it'd be appreciated.
> >>>
> >>> No, I managed to screw up the patch; you can see that I carefully added the
> >>> same treatment for the read argument, being unsure if the BUSY state would
> >>> remain during the AVAILABLE data phase. I can check that again, but
> >>> unfortunately the patch in this email shows the same problem.
> >>>
> >>> I think it may be worthwhile to rethink the behavior of wait_status() here.
> >>> If one machine shows no change after a certain status bit change, then
> >>> perhaps the others share that behavior, and we are waiting in vain. Just
> >>> imagine how many years of cpu that is, combined. ;-)
> >>
> >> Here is a modification along that line.
> >>
> >
> > Please resend this patch as stand-alone v4 patch. If sent like it was sent here,
> > it doesn't make it into patchwork, and is thus not only difficult to apply but
> > may get lost, and it is all but impossible to find and apply all tags.
> > Also, prior to Henrik's Signed=off-by: there should be a one-line explanation
> > of the changes made.
> >
> > Thanks,
> > Guenter
> >
> >> Compared to your latest version, this one has wait_status() return the
> >> actual status on success. Instead of waiting for BUSY, it waits for
> >> the other status bits, and checks BUSY afterwards. So as not to wait
> >> unneccesarily, the udelay() is placed together with the single
> >> outb(). The return value of send_byte_data() is augmented with
> >> -EAGAIN, which is then used in send_command() to create the resend
> >> loop.
> >>
> >> I reach 41 reads per second on the MBA1,1 with this version, which is
> >> getting close to the performance prior to the problems.
> >>
>
> Can I get an opinion on this wait statement please?
>
> The apple driver uses a loop with a million (1,000,000) retries spaced with a 10uS delay.
>
> In my testing on 2 machines, we don't busy wait more than about 2 loops.
> Replacing a small udelay with the usleep_range kills performance.
> With the following (do 10 fast checks before we start sleeping) I nearly triple the performance
> of the driver on my laptop, and double it on my iMac. This is on an otherwise unmodified version of
> Henriks v4 submission.
>
> Yes, given the timeouts I know it's a ridiculous loop condition.
>
> static int wait_status(u8 val, u8 mask)
> {
> unsigned long end = jiffies + (APPLESMC_MAX_WAIT * HZ) / USEC_PER_SEC;
> u8 status;
> int i;
>
> for (i=1; i < 1000000; i++) {
The minimum wait time is 10 us, or 16uS after the first 10
attempts. 1000000 * 10 = 10 seconds. I mean, it would make
some sense to limit the loop to APPLESMC_MAX_WAIT /
APPLESMC_MIN_WAIT iterations, but why 1,000,000 ?
> status = inb(APPLESMC_CMD_PORT);
> if ((status & mask) == val)
> return status;
> /* timeout: give up */
> if (time_after(jiffies, end))
> break;
> if (i < 10)
> udelay(10);
> else
> usleep_range(APPLESMC_MIN_WAIT, APPLESMC_MIN_WAIT * 16);
The original code had the exponential wait time increase.
I don't really see the point of changing that. I'd suggest
to keep the exponential increase but change the code to
something like
if (us < APPLESMC_MIN_WAIT * 4)
udelay(us)
else
usleep_range(us, us * 16);
Effectively that means the first wait would be 16 uS,
followed by 32 uS, followed by increasingly larger sleep
times. I don't know the relevance of APPLESMC_MIN_WAIT
being set to 16, but if you'd want to start with smaller
wait times you could reduce it to 8. If you are concerned
about excessively large sleep times you could reduce
the span from us..us*16 to, say, us..us*4 or us..us*2.
Thanks,
Guenter
> }
> return -EIO;
> }
>
> Regards,
> Brad
On 10/11/20 3:55 pm, Guenter Roeck wrote:
> On Tue, Nov 10, 2020 at 01:04:04PM +1100, Brad Campbell wrote:
>> On 9/11/20 3:06 am, Guenter Roeck wrote:
>>> On 11/8/20 2:14 AM, Henrik Rydberg wrote:
>>>> On Sun, Nov 08, 2020 at 09:35:28AM +0100, Henrik Rydberg wrote:
>>>>> Hi Brad,
>>>>>
>>>>> On 2020-11-08 02:00, Brad Campbell wrote:
>>>>>> G'day Henrik,
>>>>>>
>>>>>> I noticed you'd also loosened up the requirement for SMC_STATUS_BUSY in read_smc(). I assume
>>>>>> that causes problems on the early Macbook. This is revised on the one sent earlier.
>>>>>> If you could test this on your Air1,1 it'd be appreciated.
>>>>>
>>>>> No, I managed to screw up the patch; you can see that I carefully added the
>>>>> same treatment for the read argument, being unsure if the BUSY state would
>>>>> remain during the AVAILABLE data phase. I can check that again, but
>>>>> unfortunately the patch in this email shows the same problem.
>>>>>
>>>>> I think it may be worthwhile to rethink the behavior of wait_status() here.
>>>>> If one machine shows no change after a certain status bit change, then
>>>>> perhaps the others share that behavior, and we are waiting in vain. Just
>>>>> imagine how many years of cpu that is, combined. ;-)
>>>>
>>>> Here is a modification along that line.
>>>>
>>>
>>> Please resend this patch as stand-alone v4 patch. If sent like it was sent here,
>>> it doesn't make it into patchwork, and is thus not only difficult to apply but
>>> may get lost, and it is all but impossible to find and apply all tags.
>>> Also, prior to Henrik's Signed=off-by: there should be a one-line explanation
>>> of the changes made.
>>>
>>> Thanks,
>>> Guenter
>>>
>>>> Compared to your latest version, this one has wait_status() return the
>>>> actual status on success. Instead of waiting for BUSY, it waits for
>>>> the other status bits, and checks BUSY afterwards. So as not to wait
>>>> unneccesarily, the udelay() is placed together with the single
>>>> outb(). The return value of send_byte_data() is augmented with
>>>> -EAGAIN, which is then used in send_command() to create the resend
>>>> loop.
>>>>
>>>> I reach 41 reads per second on the MBA1,1 with this version, which is
>>>> getting close to the performance prior to the problems.
>>>>
>>
>> Can I get an opinion on this wait statement please?
>>
>> The apple driver uses a loop with a million (1,000,000) retries spaced with a 10uS delay.
>>
>> In my testing on 2 machines, we don't busy wait more than about 2 loops.
>> Replacing a small udelay with the usleep_range kills performance.
>> With the following (do 10 fast checks before we start sleeping) I nearly triple the performance
>> of the driver on my laptop, and double it on my iMac. This is on an otherwise unmodified version of
>> Henriks v4 submission.
>>
>> Yes, given the timeouts I know it's a ridiculous loop condition.
>>
>> static int wait_status(u8 val, u8 mask)
>> {
>> unsigned long end = jiffies + (APPLESMC_MAX_WAIT * HZ) / USEC_PER_SEC;
>> u8 status;
>> int i;
>>
>> for (i=1; i < 1000000; i++) {
>
> The minimum wait time is 10 us, or 16uS after the first 10
> attempts. 1000000 * 10 = 10 seconds. I mean, it would make
> some sense to limit the loop to APPLESMC_MAX_WAIT /
> APPLESMC_MIN_WAIT iterations, but why 1,000,000 ?
I had to pick a big number and it was easy to punch in 6 zeros as that is what is in
the OSX driver. In this instance it's more a proof of concept rather than sane example
because it'll either abort on timeout or return the correct status anyway.
Could also have been a while (true) {} but then I'd need to manually increment i.
>> status = inb(APPLESMC_CMD_PORT);
>> if ((status & mask) == val)
>> return status;
>> /* timeout: give up */
>> if (time_after(jiffies, end))
>> break;
>> if (i < 10)
>> udelay(10);
>> else
>> usleep_range(APPLESMC_MIN_WAIT, APPLESMC_MIN_WAIT * 16);
>
> The original code had the exponential wait time increase.
> I don't really see the point of changing that. I'd suggest
> to keep the exponential increase but change the code to
> something like
> if (us < APPLESMC_MIN_WAIT * 4)
> udelay(us)
> else
> usleep_range(us, us * 16);
The main reason I dropped the exponential was best case on this laptop the modified code with exponential
wait as described above increase increases the performance from ~40 -> 62 reads/sec, whereas the version
I posted with the first 10 loops at 10uS goes from ~40 -> 100 reads/sec.
About 95% of waits never get past a few of iterations of that loop (so ~30-60uS). With a
modified exponential starting at 8uS a 30uS requirement ends up at 56uS. If it needed 60us with
the original we end up waiting 120uS.
If it needs longer than 120uS it's rare enough that a bigger sleep isn't going to cause much
of a performance hit.
Getting completely pathological and busy waiting with a fixed 10uS delay like the OSX driver
does give about 125 reads/sec but I was trying to find a balance and 10 loops seemed to hit that mark.
> Effectively that means the first wait would be 16 uS,
> followed by 32 uS, followed by increasingly larger sleep
> times. I don't know the relevance of APPLESMC_MIN_WAIT
> being set to 16, but if you'd want to start with smaller
> wait times you could reduce it to 8. If you are concerned
> about excessively large sleep times you could reduce
> the span from us..us*16 to, say, us..us*4 or us..us*2.
I was tossing up here between the overhead required to manage a tighter usleep_range
vs some extra udelays.
It's not exactly a performance sensitive driver, but I thought there might be a balance to be
struck between performance and simplicity. The exponential delay always struck me as odd.
If the preference is to leave it as is or modify as suggested I'm ok with that too.
Appreciate the input.
Regards,
Brad
On Tue, Nov 10, 2020 at 04:40:23PM +1100, Brad Campbell wrote:
> On 10/11/20 3:55 pm, Guenter Roeck wrote:
> > On Tue, Nov 10, 2020 at 01:04:04PM +1100, Brad Campbell wrote:
> >> On 9/11/20 3:06 am, Guenter Roeck wrote:
> >>> On 11/8/20 2:14 AM, Henrik Rydberg wrote:
> >>>> On Sun, Nov 08, 2020 at 09:35:28AM +0100, Henrik Rydberg wrote:
> >>>>> Hi Brad,
> >>>>>
> >>>>> On 2020-11-08 02:00, Brad Campbell wrote:
> >>>>>> G'day Henrik,
> >>>>>>
> >>>>>> I noticed you'd also loosened up the requirement for SMC_STATUS_BUSY in read_smc(). I assume
> >>>>>> that causes problems on the early Macbook. This is revised on the one sent earlier.
> >>>>>> If you could test this on your Air1,1 it'd be appreciated.
> >>>>>
> >>>>> No, I managed to screw up the patch; you can see that I carefully added the
> >>>>> same treatment for the read argument, being unsure if the BUSY state would
> >>>>> remain during the AVAILABLE data phase. I can check that again, but
> >>>>> unfortunately the patch in this email shows the same problem.
> >>>>>
> >>>>> I think it may be worthwhile to rethink the behavior of wait_status() here.
> >>>>> If one machine shows no change after a certain status bit change, then
> >>>>> perhaps the others share that behavior, and we are waiting in vain. Just
> >>>>> imagine how many years of cpu that is, combined. ;-)
> >>>>
> >>>> Here is a modification along that line.
> >>>>
> >>>
> >>> Please resend this patch as stand-alone v4 patch. If sent like it was sent here,
> >>> it doesn't make it into patchwork, and is thus not only difficult to apply but
> >>> may get lost, and it is all but impossible to find and apply all tags.
> >>> Also, prior to Henrik's Signed=off-by: there should be a one-line explanation
> >>> of the changes made.
> >>>
> >>> Thanks,
> >>> Guenter
> >>>
> >>>> Compared to your latest version, this one has wait_status() return the
> >>>> actual status on success. Instead of waiting for BUSY, it waits for
> >>>> the other status bits, and checks BUSY afterwards. So as not to wait
> >>>> unneccesarily, the udelay() is placed together with the single
> >>>> outb(). The return value of send_byte_data() is augmented with
> >>>> -EAGAIN, which is then used in send_command() to create the resend
> >>>> loop.
> >>>>
> >>>> I reach 41 reads per second on the MBA1,1 with this version, which is
> >>>> getting close to the performance prior to the problems.
> >>>>
> >>
> >> Can I get an opinion on this wait statement please?
> >>
> >> The apple driver uses a loop with a million (1,000,000) retries spaced with a 10uS delay.
> >>
> >> In my testing on 2 machines, we don't busy wait more than about 2 loops.
> >> Replacing a small udelay with the usleep_range kills performance.
> >> With the following (do 10 fast checks before we start sleeping) I nearly triple the performance
> >> of the driver on my laptop, and double it on my iMac. This is on an otherwise unmodified version of
> >> Henriks v4 submission.
> >>
> >> Yes, given the timeouts I know it's a ridiculous loop condition.
> >>
> >> static int wait_status(u8 val, u8 mask)
> >> {
> >> unsigned long end = jiffies + (APPLESMC_MAX_WAIT * HZ) / USEC_PER_SEC;
> >> u8 status;
> >> int i;
> >>
> >> for (i=1; i < 1000000; i++) {
> >
> > The minimum wait time is 10 us, or 16uS after the first 10
> > attempts. 1000000 * 10 = 10 seconds. I mean, it would make
> > some sense to limit the loop to APPLESMC_MAX_WAIT /
> > APPLESMC_MIN_WAIT iterations, but why 1,000,000 ?
>
> I had to pick a big number and it was easy to punch in 6 zeros as that is what is in
> the OSX driver. In this instance it's more a proof of concept rather than sane example
> because it'll either abort on timeout or return the correct status anyway.
> Could also have been a while (true) {} but then I'd need to manually increment i.
>
> >> status = inb(APPLESMC_CMD_PORT);
> >> if ((status & mask) == val)
> >> return status;
> >> /* timeout: give up */
> >> if (time_after(jiffies, end))
> >> break;
> >> if (i < 10)
> >> udelay(10);
> >> else
> >> usleep_range(APPLESMC_MIN_WAIT, APPLESMC_MIN_WAIT * 16);
> >
> > The original code had the exponential wait time increase.
> > I don't really see the point of changing that. I'd suggest
> > to keep the exponential increase but change the code to
> > something like
> > if (us < APPLESMC_MIN_WAIT * 4)
> > udelay(us)
> > else
> > usleep_range(us, us * 16);
>
> The main reason I dropped the exponential was best case on this laptop the modified code with exponential
> wait as described above increase increases the performance from ~40 -> 62 reads/sec, whereas the version
> I posted with the first 10 loops at 10uS goes from ~40 -> 100 reads/sec.
>
> About 95% of waits never get past a few of iterations of that loop (so ~30-60uS). With a
> modified exponential starting at 8uS a 30uS requirement ends up at 56uS. If it needed 60us with
> the original we end up waiting 120uS.
>
> If it needs longer than 120uS it's rare enough that a bigger sleep isn't going to cause much
> of a performance hit.
>
> Getting completely pathological and busy waiting with a fixed 10uS delay like the OSX driver
> does give about 125 reads/sec but I was trying to find a balance and 10 loops seemed to hit that mark.
>
> > Effectively that means the first wait would be 16 uS,
> > followed by 32 uS, followed by increasingly larger sleep
> > times. I don't know the relevance of APPLESMC_MIN_WAIT
> > being set to 16, but if you'd want to start with smaller
> > wait times you could reduce it to 8. If you are concerned
> > about excessively large sleep times you could reduce
> > the span from us..us*16 to, say, us..us*4 or us..us*2.
>
> I was tossing up here between the overhead required to manage a tighter usleep_range
> vs some extra udelays.
>
> It's not exactly a performance sensitive driver, but I thought there might be a balance to be
> struck between performance and simplicity. The exponential delay always struck me as odd.
>
> If the preference is to leave it as is or modify as suggested I'm ok with that too.
> Appreciate the input.
Ok, not worth arguing about.
Guenter
>
> Regards,
> Brad
G'day All,
Versions 1-3 of this patch were various attempts to try and simplify/clarify the communication to the SMC in order to remove the timing sensitivity which was exposed by Commit fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()"). As with the original author(s), we were limited in not having any documentation and relying on a "poke it and see what happens".
Whilst version 3 ticked all the boxes from a performance and reliability standpoint it still wasn't clear why it worked and why retries were required when sending a command to the SMC. I dug into the OSX driver to try and seek some clarity around that and found a very simple "state corrector" for want of a better word, whereby any interaction with the SMC was preceded by a simple 3 step process (found in smc_sane()) which ensured the SMC was in the right state to accept a transaction (or ultimately reported as broken). My testing has shown this routine is generally only required once on driver load to sync up the state machine, however it's purpose is to pull the SMC back into line if its internal state machine gets out of sync. This was likely happening with the command retry loop in the earlier versions, however due to the way the status bits were being checked it was unclear.
The other thing that was clear is outside of the "state corrector", all checking of SMC status bits happened before a read/write, not after. By re-working the comms to follow this protocol I believe we've simplified the code, made the actual operation clearer and removed *all* timing dependencies. This has also allowed a simplification of the timing in wait_status and a reduction in the waits incurred.
Henrik further simplified wait_status by leaving the minimum wait in usleep_range as 8uS. Testing on multiple machines has borne this out resulting in less loops/sleeps and no apparent impact in the performance of the driver (and in fact an increase on my iMac12,2 with a _very_ slow SMC). It also allowed for the removal of the jiffy based timeout as worst case it's going to sleep for a couple of seconds. The OSX driver puts a 10s timeout on every wait.
So, after much testing I'll submit v4 for comment and further testing.
Regards,
Brad
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.
Tested on : MacbookAir6,2 MacBookPro11,1 iMac12,2, MacBookAir1,1,
MacBookAir3,1
Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
Reported-by: Andreas Kemnade <[email protected]>
Tested-by: Andreas Kemnade <[email protected]> # MacBookAir6,2
Acked-by: Arnd Bergmann <[email protected]>
Signed-off-by: Brad Campbell <[email protected]>
Signed-off-by: Henrik Rydberg <[email protected]>
---
Changelog :
v1 : Initial attempt
v2 : Address logic and coding style
v3 : Removed some debug hangover. Added tested-by. Modifications for MacBookAir1,1
v4 : Re-factored logic based on Apple driver. Simplified wait_status loop
Index: linux-stable/drivers/hwmon/applesmc.c
===================================================================
--- linux-stable.orig/drivers/hwmon/applesmc.c
+++ linux-stable/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,10 +43,14 @@
#define APPLESMC_MAX_DATA_LENGTH 32
-/* wait up to 128 ms for a status change. */
-#define APPLESMC_MIN_WAIT 0x0010
-#define APPLESMC_RETRY_WAIT 0x0100
-#define APPLESMC_MAX_WAIT 0x20000
+/* Apple SMC status bits */
+#define SMC_STATUS_AWAITING_DATA BIT(0) /* SMC has data waiting to be read */
+#define SMC_STATUS_IB_CLOSED BIT(1) /* Will ignore any input */
+#define SMC_STATUS_BUSY BIT(2) /* Command in progress */
+
+/* Exponential delay boundaries */
+#define APPLESMC_MIN_WAIT 0x0008
+#define APPLESMC_MAX_WAIT 0x100000
#define APPLESMC_READ_CMD 0x10
#define APPLESMC_WRITE_CMD 0x11
@@ -151,65 +156,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 all transactions
*/
-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);
+ for (us = APPLESMC_MIN_WAIT; us <= APPLESMC_MAX_WAIT; us <<= 1) {
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(APPLESMC_MIN_WAIT, us);
}
-
- pr_warn("wait_read() fail: 0x%02x\n", status);
return -EIO;
}
-/*
- * send_byte - Write to SMC port, retrying when necessary. Callers
- * must hold applesmc_lock.
- */
+/* send_byte - Write to SMC data port. 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;
+ int status;
+ status = wait_status(0, SMC_STATUS_IB_CLOSED);
+ if (status)
+ return status;
+ status = wait_status(SMC_STATUS_BUSY, SMC_STATUS_BUSY);
+ if (status)
+ return status;
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);
- return -EIO;
+ return 0;
}
+/* send_command - Write a command to the SMC. Callers must hold applesmc_lock. */
+
static int send_command(u8 cmd)
{
- return send_byte(cmd, APPLESMC_CMD_PORT);
+ int ret;
+
+ ret = wait_status(0, SMC_STATUS_IB_CLOSED);
+ if (ret)
+ return ret;
+ outb(cmd, APPLESMC_CMD_PORT);
+ return 0;
+}
+
+/* Based on logic from the Apple driver. This is issued before any interaction
+ * If busy is stuck high, issue a read command to reset the SMC state
+ * machine. If busy is stuck high after the command then the SMC is
+ * jammed.
+ */
+
+static int smc_sane(void)
+{
+ int ret;
+
+ ret = wait_status(0, SMC_STATUS_BUSY);
+ if (!ret)
+ return ret;
+ ret = send_command(APPLESMC_READ_CMD);
+ if (ret)
+ return ret;
+ ret = wait_status(0, SMC_STATUS_BUSY);
+ if (!ret)
+ return ret;
+ return -EIO;
}
static int send_argument(const char *key)
@@ -226,6 +239,11 @@ static int read_smc(u8 cmd, const char *
{
u8 status, data = 0;
int i;
+ int ret;
+
+ ret = smc_sane();
+ if (ret)
+ return ret;
if (send_command(cmd) || send_argument(key)) {
pr_warn("%.4s: read arg fail\n", key);
@@ -239,7 +257,8 @@ static int read_smc(u8 cmd, const char *
}
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) < 0) {
pr_warn("%.4s: read data[%d] fail\n", key, i);
return -EIO;
}
@@ -250,19 +269,24 @@ static int read_smc(u8 cmd, const char *
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);
}
if (i)
pr_warn("flushed %d bytes, last value is: %d\n", i, data);
- return 0;
+ return wait_status(0, SMC_STATUS_BUSY);
}
static int write_smc(u8 cmd, const char *key, const u8 *buffer, u8 len)
{
int i;
+ int ret;
+
+ ret = smc_sane();
+ if (ret)
+ return ret;
if (send_command(cmd) || send_argument(key)) {
pr_warn("%s: write arg fail\n", key);
@@ -281,7 +305,7 @@ static int write_smc(u8 cmd, const char
}
}
- return 0;
+ return wait_status(0, SMC_STATUS_BUSY);
}
static int read_register_count(unsigned int *count)
A few small cleanups on top of the comms changes for applesmc.c :
send_byte() is always called with APPLESMC_CMD_PORT, so simplify.
Remove redundant check from smc_sane().
Consolidate writing length with other setup parameters.
Consolidate read and write error messages to a single statement each.
Length and error consolidation suggested by Henrik Rydberg <[email protected]>
Signed-off-by: Brad Campbell <[email protected]>
Index: linux-stable/drivers/hwmon/applesmc.c
===================================================================
--- linux-stable.orig/drivers/hwmon/applesmc.c
+++ linux-stable/drivers/hwmon/applesmc.c
@@ -176,7 +176,7 @@ static int wait_status(u8 val, u8 mask)
/* send_byte - Write to SMC data port. Callers must hold applesmc_lock. */
-static int send_byte(u8 cmd, u16 port)
+static int send_byte(u8 cmd)
{
int status;
@@ -186,7 +186,7 @@ static int send_byte(u8 cmd, u16 port)
status = wait_status(SMC_STATUS_BUSY, SMC_STATUS_BUSY);
if (status)
return status;
- outb(cmd, port);
+ outb(cmd, APPLESMC_DATA_PORT);
return 0;
}
@@ -219,10 +219,7 @@ static int smc_sane(void)
ret = send_command(APPLESMC_READ_CMD);
if (ret)
return ret;
- ret = wait_status(0, SMC_STATUS_BUSY);
- if (!ret)
- return ret;
- return -EIO;
+ return wait_status(0, SMC_STATUS_BUSY);
}
static int send_argument(const char *key)
@@ -230,7 +227,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(key[i]))
return -EIO;
return 0;
}
@@ -245,23 +242,14 @@ static int read_smc(u8 cmd, const char *
if (ret)
return ret;
- 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_byte(len))
+ goto err;
for (i = 0; i < len; i++) {
if (wait_status(SMC_STATUS_AWAITING_DATA | SMC_STATUS_BUSY,
- SMC_STATUS_AWAITING_DATA | SMC_STATUS_BUSY) < 0) {
- pr_warn("%.4s: read data[%d] fail\n", key, i);
- return -EIO;
- }
+ SMC_STATUS_AWAITING_DATA | SMC_STATUS_BUSY))
+ goto err;
+
buffer[i] = inb(APPLESMC_DATA_PORT);
}
@@ -277,6 +265,9 @@ static int read_smc(u8 cmd, const char *
pr_warn("flushed %d bytes, last value is: %d\n", i, data);
return wait_status(0, SMC_STATUS_BUSY);
+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)
@@ -288,24 +279,17 @@ static int write_smc(u8 cmd, const char
if (ret)
return ret;
- 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_byte(len))
+ 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(buffer[i]))
+ goto err;
return wait_status(0, SMC_STATUS_BUSY);
+err:
+ pr_warn("write cmd fail: %x %.4s %d\n", cmd, key, len);
+ return -EIO;
}
static int read_register_count(unsigned int *count)
On 11/10/20 7:38 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.
>
> Tested on : MacbookAir6,2 MacBookPro11,1 iMac12,2, MacBookAir1,1,
> MacBookAir3,1
>
> Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
> Reported-by: Andreas Kemnade <[email protected]>
> Tested-by: Andreas Kemnade <[email protected]> # MacBookAir6,2
> Acked-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Brad Campbell <[email protected]>
> Signed-off-by: Henrik Rydberg <[email protected]>
>
> ---
> Changelog :
> v1 : Initial attempt
> v2 : Address logic and coding style
> v3 : Removed some debug hangover. Added tested-by. Modifications for MacBookAir1,1
> v4 : Re-factored logic based on Apple driver. Simplified wait_status loop
> Index: linux-stable/drivers/hwmon/applesmc.c
> ===================================================================
> --- linux-stable.orig/drivers/hwmon/applesmc.c
> +++ linux-stable/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,10 +43,14 @@
>
> #define APPLESMC_MAX_DATA_LENGTH 32
>
> -/* wait up to 128 ms for a status change. */
> -#define APPLESMC_MIN_WAIT 0x0010
> -#define APPLESMC_RETRY_WAIT 0x0100
> -#define APPLESMC_MAX_WAIT 0x20000
> +/* Apple SMC status bits */
> +#define SMC_STATUS_AWAITING_DATA BIT(0) /* SMC has data waiting to be read */
> +#define SMC_STATUS_IB_CLOSED BIT(1) /* Will ignore any input */
> +#define SMC_STATUS_BUSY BIT(2) /* Command in progress */
> +
> +/* Exponential delay boundaries */
> +#define APPLESMC_MIN_WAIT 0x0008
> +#define APPLESMC_MAX_WAIT 0x100000
This is a substantial increase in wait time which should be documented.
0x20000 was explained (it translated to 128 ms), but this isn't,
and no reason is provided why it was increased to one second.
Is there any evidence that this is needed ? The only "benefit" I
can see is that a stuck SMC will now hang everything 8 times longer.
There really should be some evidence suggesting that the longer
timeout is really needed, better than "the apple driver does it".
The timeout was increased to 128 ms back in 2012, according to
the commit because timeouts were observed on MacBookPro6,1.
I would expect something similar here. In other words, describe
the circumstances of observed timeouts and the affected system(s).
>
> #define APPLESMC_READ_CMD 0x10
> #define APPLESMC_WRITE_CMD 0x11
> @@ -151,65 +156,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 all transactions
> */
> -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);
> + for (us = APPLESMC_MIN_WAIT; us <= APPLESMC_MAX_WAIT; us <<= 1) {
> 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(APPLESMC_MIN_WAIT, us);
Fighting with indentation again... and does that range really make sense ?
Seems to me it makes the whole loop quite unpredictable in its behavior.
> }
> -
> - pr_warn("wait_read() fail: 0x%02x\n", status);
> return -EIO;
> }
>
> -/*
> - * send_byte - Write to SMC port, retrying when necessary. Callers
> - * must hold applesmc_lock.
> - */
> +/* send_byte - Write to SMC data port. 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;
> + int status;
>
> + status = wait_status(0, SMC_STATUS_IB_CLOSED);
> + if (status)
> + return status;
> + status = wait_status(SMC_STATUS_BUSY, SMC_STATUS_BUSY);
> + if (status)
> + return status;
I think this warrants an explanation/comment.
> 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);
> - return -EIO;
> + return 0;
> }
>
> +/* send_command - Write a command to the SMC. Callers must hold applesmc_lock. */
> +
> static int send_command(u8 cmd)
> {
> - return send_byte(cmd, APPLESMC_CMD_PORT);
> + int ret;
> +
> + ret = wait_status(0, SMC_STATUS_IB_CLOSED);
> + if (ret)
> + return ret;
> + outb(cmd, APPLESMC_CMD_PORT);
> + return 0;
> +}
> +
> +/* Based on logic from the Apple driver. This is issued before any interaction
> + * If busy is stuck high, issue a read command to reset the SMC state
> + * machine. If busy is stuck high after the command then the SMC is
> + * jammed.
> + */
This is not the networking subsystem. Please use standard multi-line comments.
> +
> +static int smc_sane(void)
> +{
> + int ret;
> +
> + ret = wait_status(0, SMC_STATUS_BUSY);
> + if (!ret)
> + return ret;
> + ret = send_command(APPLESMC_READ_CMD);
> + if (ret)
> + return ret;
> + ret = wait_status(0, SMC_STATUS_BUSY);
> + if (!ret)
> + return ret;
> + return -EIO;
return wait_status(0, SMC_STATUS_BUSY);
> }
>
> static int send_argument(const char *key)
> @@ -226,6 +239,11 @@ static int read_smc(u8 cmd, const char *
> {
> u8 status, data = 0;
> int i;
> + int ret;
> +
> + ret = smc_sane();
> + if (ret)
> + return ret;
>
> if (send_command(cmd) || send_argument(key)) {
> pr_warn("%.4s: read arg fail\n", key);
> @@ -239,7 +257,8 @@ static int read_smc(u8 cmd, const char *
> }
>
> 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) < 0) {
> pr_warn("%.4s: read data[%d] fail\n", key, i);
> return -EIO;
> }
> @@ -250,19 +269,24 @@ static int read_smc(u8 cmd, const char *
> 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);
> }
> if (i)
> pr_warn("flushed %d bytes, last value is: %d\n", i, data);
>
> - return 0;
> + return wait_status(0, SMC_STATUS_BUSY);
> }
>
> static int write_smc(u8 cmd, const char *key, const u8 *buffer, u8 len)
> {
> int i;
> + int ret;
> +
> + ret = smc_sane();
> + if (ret)
> + return ret;
>
> if (send_command(cmd) || send_argument(key)) {
> pr_warn("%s: write arg fail\n", key);
> @@ -281,7 +305,7 @@ static int write_smc(u8 cmd, const char
> }
> }
>
> - return 0;
> + return wait_status(0, SMC_STATUS_BUSY);
> }
>
> static int read_register_count(unsigned int *count)
>
On 11/11/20 4:56 pm, Guenter Roeck wrote:
> On 11/10/20 7:38 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.
>>
>> Tested on : MacbookAir6,2 MacBookPro11,1 iMac12,2, MacBookAir1,1,
>> MacBookAir3,1
>>
>> Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
>> Reported-by: Andreas Kemnade <[email protected]>
>> Tested-by: Andreas Kemnade <[email protected]> # MacBookAir6,2
>> Acked-by: Arnd Bergmann <[email protected]>
>> Signed-off-by: Brad Campbell <[email protected]>
>> Signed-off-by: Henrik Rydberg <[email protected]>
>>
>> ---
>> Changelog :
>> v1 : Initial attempt
>> v2 : Address logic and coding style
>> v3 : Removed some debug hangover. Added tested-by. Modifications for MacBookAir1,1
>> v4 : Re-factored logic based on Apple driver. Simplified wait_status loop
>> Index: linux-stable/drivers/hwmon/applesmc.c
>> ===================================================================
>> --- linux-stable.orig/drivers/hwmon/applesmc.c
>> +++ linux-stable/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,10 +43,14 @@
>>
>> #define APPLESMC_MAX_DATA_LENGTH 32
>>
>> -/* wait up to 128 ms for a status change. */
>> -#define APPLESMC_MIN_WAIT 0x0010
>> -#define APPLESMC_RETRY_WAIT 0x0100
>> -#define APPLESMC_MAX_WAIT 0x20000
>> +/* Apple SMC status bits */
>> +#define SMC_STATUS_AWAITING_DATA BIT(0) /* SMC has data waiting to be read */
>> +#define SMC_STATUS_IB_CLOSED BIT(1) /* Will ignore any input */
>> +#define SMC_STATUS_BUSY BIT(2) /* Command in progress */
>> +
>> +/* Exponential delay boundaries */
>> +#define APPLESMC_MIN_WAIT 0x0008
>> +#define APPLESMC_MAX_WAIT 0x100000
>
> This is a substantial increase in wait time which should be documented.
> 0x20000 was explained (it translated to 128 ms), but this isn't,
> and no reason is provided why it was increased to one second.
> Is there any evidence that this is needed ? The only "benefit" I
> can see is that a stuck SMC will now hang everything 8 times longer.
>
> There really should be some evidence suggesting that the longer
> timeout is really needed, better than "the apple driver does it".
> The timeout was increased to 128 ms back in 2012, according to
> the commit because timeouts were observed on MacBookPro6,1.
> I would expect something similar here. In other words, describe
> the circumstances of observed timeouts and the affected system(s).
>
G'day Guenter,
The wait timer turns out to be the most contentious part of the whole patch.
That particular algorithm was put forward off list, and in testing it was as
fast as {while true ; do stuff; udelay(10)}. The reason for the larger max value
isn't actually for timing purposes. It was to allow a minimum of 16 times around the hedge.
I've probably had 10 chops at this timeout trying to balance performance with a sane
algorithm.
How does this look? This performs pretty well.
/* Minimum sleep time is 8uS */
#define APPLESMC_MIN_WAIT 0x0008
/*
* Wait for specific status bits with a mask on the SMC.
* Used before all transactions.
* This does 10 fast loops of 8us then exponentially backs off for a
* minimum total wait of 262ms. Depending on usleep_range this could
* run out past 500ms.
*/
static int wait_status(u8 val, u8 mask)
{
u8 status;
int us;
int i;
us=APPLESMC_MIN_WAIT;
for (i = 0; i < 24 ; i++) {
status = inb(APPLESMC_CMD_PORT);
if ((status & mask) == val)
return 0;
usleep_range(us, us * 2);
if (i > 9)
us <<= 1;
}
return -EIO;
}
Regards,
Brad
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. Logic changes based on inspection of the Apple SMC kext.
Tested on : MacbookAir6,2 MacBookPro11,1 iMac12,2, MacBookAir1,1,
MacBookAir3,1
Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
Reported-by: Andreas Kemnade <[email protected]>
Tested-by: Andreas Kemnade <[email protected]> # MacBookAir6,2
Acked-by: Arnd Bergmann <[email protected]>
Signed-off-by: Brad Campbell <[email protected]>
Signed-off-by: Henrik Rydberg <[email protected]>
---
Changelog :
v1 : Initial attempt
v2 : Address logic and coding style
v3 : Removed some debug hangover. Added tested-by. Modifications for MacBookAir1,1
v4 : Re-factored logic based on Apple driver. Simplified wait_status loop
v5 : Re-wrote status loop. Simplified busy check in send_byte(). Fixed formatting
Index: linux-stable/drivers/hwmon/applesmc.c
===================================================================
--- linux-stable.orig/drivers/hwmon/applesmc.c
+++ linux-stable/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,10 +43,13 @@
#define APPLESMC_MAX_DATA_LENGTH 32
-/* wait up to 128 ms for a status change. */
-#define APPLESMC_MIN_WAIT 0x0010
-#define APPLESMC_RETRY_WAIT 0x0100
-#define APPLESMC_MAX_WAIT 0x20000
+/* Apple SMC status bits */
+#define SMC_STATUS_AWAITING_DATA BIT(0) /* SMC has data waiting to be read */
+#define SMC_STATUS_IB_CLOSED BIT(1) /* Will ignore any input */
+#define SMC_STATUS_BUSY BIT(2) /* Command in progress */
+
+/* Initial wait is 8us */
+#define APPLESMC_MIN_WAIT 0x0008
#define APPLESMC_READ_CMD 0x10
#define APPLESMC_WRITE_CMD 0x11
@@ -151,65 +155,84 @@ 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 all transactions.
+ * This does 10 fast loops of 8us then exponentially backs off for a
+ * minimum total wait of 262ms. Depending on usleep_range this could
+ * run out past 500ms.
*/
-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 i;
- for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
- usleep_range(us, us * 16);
+ us = APPLESMC_MIN_WAIT;
+ for (i = 0; i < 24 ; i++) {
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 * 2);
+ if (i > 9)
+ us <<= 1;
}
-
- pr_warn("wait_read() fail: 0x%02x\n", status);
return -EIO;
}
-/*
- * send_byte - Write to SMC port, retrying when necessary. Callers
- * must hold applesmc_lock.
- */
+/* send_byte - Write to SMC data port. 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;
+ int status;
- 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);
- }
+ status = wait_status(0, SMC_STATUS_IB_CLOSED);
+ if (status)
+ return status;
+ /*
+ * This needs to be a separate read looking for bit 0x04
+ * after bit 0x02 falls. If consolidated with the wait above
+ * this extra read may not happen if status returns both
+ * simultaneously and this would appear to be required.
+ */
+ status = inb(APPLESMC_CMD_PORT) & SMC_STATUS_BUSY;
+ if (!status)
+ return -EIO;
- pr_warn("send_byte(0x%02x, 0x%04x) fail: 0x%02x\n", cmd, port, status);
- return -EIO;
+ outb(cmd, port);
+ return 0;
}
+/* send_command - Write a command to the SMC. Callers must hold applesmc_lock. */
+
static int send_command(u8 cmd)
{
- return send_byte(cmd, APPLESMC_CMD_PORT);
+ int ret;
+
+ ret = wait_status(0, SMC_STATUS_IB_CLOSED);
+ if (ret)
+ return ret;
+ outb(cmd, APPLESMC_CMD_PORT);
+ return 0;
+}
+
+/*
+ * Based on logic from the Apple driver. This is issued before any interaction
+ * If busy is stuck high, issue a read command to reset the SMC state machine.
+ * If busy is stuck high after the command then the SMC is jammed.
+ */
+
+static int smc_sane(void)
+{
+ int ret;
+
+ ret = wait_status(0, SMC_STATUS_BUSY);
+ if (!ret)
+ return ret;
+ ret = send_command(APPLESMC_READ_CMD);
+ if (ret)
+ return ret;
+ return wait_status(0, SMC_STATUS_BUSY);
}
static int send_argument(const char *key)
@@ -226,6 +249,11 @@ static int read_smc(u8 cmd, const char *
{
u8 status, data = 0;
int i;
+ int ret;
+
+ ret = smc_sane();
+ if (ret)
+ return ret;
if (send_command(cmd) || send_argument(key)) {
pr_warn("%.4s: read arg fail\n", key);
@@ -239,7 +267,8 @@ static int read_smc(u8 cmd, const char *
}
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) < 0) {
pr_warn("%.4s: read data[%d] fail\n", key, i);
return -EIO;
}
@@ -250,19 +279,24 @@ static int read_smc(u8 cmd, const char *
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);
}
if (i)
pr_warn("flushed %d bytes, last value is: %d\n", i, data);
- return 0;
+ return wait_status(0, SMC_STATUS_BUSY);
}
static int write_smc(u8 cmd, const char *key, const u8 *buffer, u8 len)
{
int i;
+ int ret;
+
+ ret = smc_sane();
+ if (ret)
+ return ret;
if (send_command(cmd) || send_argument(key)) {
pr_warn("%s: write arg fail\n", key);
@@ -281,7 +315,7 @@ static int write_smc(u8 cmd, const char
}
}
- return 0;
+ return wait_status(0, SMC_STATUS_BUSY);
}
static int read_register_count(unsigned int *count)
On 2020-11-11 14:06, 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. Logic changes based on inspection of the Apple SMC kext.
>
> Tested on : MacbookAir6,2 MacBookPro11,1 iMac12,2, MacBookAir1,1,
> MacBookAir3,1
>
> Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
> Reported-by: Andreas Kemnade <[email protected]>
> Tested-by: Andreas Kemnade <[email protected]> # MacBookAir6,2
> Acked-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Brad Campbell <[email protected]>
> Signed-off-by: Henrik Rydberg <[email protected]>
>
> ---
> Changelog :
> v1 : Initial attempt
> v2 : Address logic and coding style
> v3 : Removed some debug hangover. Added tested-by. Modifications for MacBookAir1,1
> v4 : Re-factored logic based on Apple driver. Simplified wait_status loop
> v5 : Re-wrote status loop. Simplified busy check in send_byte(). Fixed formatting
Hi Brad,
This version is still working fine on the MBA1,1, at 50 reads per second.
Thanks,
Henrik
On 12/11/20 7:05 am, Henrik Rydberg wrote:
> On 2020-11-11 14:06, 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. Logic changes based on inspection of the Apple SMC kext.
>>
>> Tested on : MacbookAir6,2 MacBookPro11,1 iMac12,2, MacBookAir1,1,
>> MacBookAir3,1
>>
>> Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
>> Reported-by: Andreas Kemnade <[email protected]>
>> Tested-by: Andreas Kemnade <[email protected]> # MacBookAir6,2
>> Acked-by: Arnd Bergmann <[email protected]>
>> Signed-off-by: Brad Campbell <[email protected]>
>> Signed-off-by: Henrik Rydberg <[email protected]>
>>
>> ---
>> Changelog :
>> v1 : Initial attempt
>> v2 : Address logic and coding style
>> v3 : Removed some debug hangover. Added tested-by. Modifications for MacBookAir1,1
>> v4 : Re-factored logic based on Apple driver. Simplified wait_status loop
>> v5 : Re-wrote status loop. Simplified busy check in send_byte(). Fixed formatting
>
> Hi Brad,
>
> This version is still working fine on the MBA1,1, at 50 reads per second.
>
Cheers Henrik,
I did 5.6 million reads overnight and had 3 failures.
I suspect it's this :
status = inb(APPLESMC_CMD_PORT) & SMC_STATUS_BUSY;
if (!status)
return -EIO;
When I used wait_status() previously I didn't see any read errors.
With hindsight I probably shouldn't have made that simplification.
I'll do some more testing and probably submit a v6 with just this change.
I think it's just about right provided that wait_status loop gets across the
line.
Regards,
Brad
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.
Tested on : MacbookAir6,2 MacBookPro11,1 iMac12,2, MacBookAir1,1,
MacBookAir3,1
Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
Reported-by: Andreas Kemnade <[email protected]>
Tested-by: Andreas Kemnade <[email protected]> # MacBookAir6,2
Acked-by: Arnd Bergmann <[email protected]>
Signed-off-by: Brad Campbell <[email protected]>
Signed-off-by: Henrik Rydberg <[email protected]>
---
Changelog :
v1 : Initial attempt
v2 : Address logic and coding style based on comments received
v3 : Removed some debug hangover. Added tested-by. Modifications for MacBookAir1,1
- Significant rework of wait logic by Henrik Rydberg <[email protected]> to
make it function at all on the MacBookAir1,1.
v4 : Re-factored logic based on Apple driver. Simplified wait_status loop
- Re-factored the logic again, this time based on the Apple driver. This
functioned without error on all tested Macs. wait_status() contributed
by Henrik Rydberg <[email protected]>
v5 : Re-wrote status loop. Simplified busy check in send_byte()
- Re-wrote wait_status() based on feedback from Guenter Roeck <[email protected]>
- Added additional comments to explain multiple tests in send_byte()
- Addressed repeated indentation issues
v6 : Reverted simplification of busy check in send_byte()
- The logic simplification in v5 send_byte() caused a few read failures in
stress testing on a fast SMC (3 in 5.6million). Reverting that change passed
5 million reads with 0 errors.
Index: linux-stable/drivers/hwmon/applesmc.c
===================================================================
--- linux-stable.orig/drivers/hwmon/applesmc.c
+++ linux-stable/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,10 +43,13 @@
#define APPLESMC_MAX_DATA_LENGTH 32
-/* wait up to 128 ms for a status change. */
-#define APPLESMC_MIN_WAIT 0x0010
-#define APPLESMC_RETRY_WAIT 0x0100
-#define APPLESMC_MAX_WAIT 0x20000
+/* Apple SMC status bits */
+#define SMC_STATUS_AWAITING_DATA BIT(0) /* SMC has data waiting to be read */
+#define SMC_STATUS_IB_CLOSED BIT(1) /* Will ignore any input */
+#define SMC_STATUS_BUSY BIT(2) /* Command in progress */
+
+/* Initial wait is 8us */
+#define APPLESMC_MIN_WAIT 0x0008
#define APPLESMC_READ_CMD 0x10
#define APPLESMC_WRITE_CMD 0x11
@@ -151,65 +155,84 @@ 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 all transactions.
+ * This does 10 fast loops of 8us then exponentially backs off for a
+ * minimum total wait of 262ms. Depending on usleep_range this could
+ * run out past 500ms.
*/
-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 i;
- for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
- usleep_range(us, us * 16);
+ us = APPLESMC_MIN_WAIT;
+ for (i = 0; i < 24 ; i++) {
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 * 2);
+ if (i > 9)
+ us <<= 1;
}
-
- pr_warn("wait_read() fail: 0x%02x\n", status);
return -EIO;
}
-/*
- * send_byte - Write to SMC port, retrying when necessary. Callers
- * must hold applesmc_lock.
- */
+/* send_byte - Write to SMC data port. 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;
+ int status;
- 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);
- }
+ status = wait_status(0, SMC_STATUS_IB_CLOSED);
+ if (status)
+ return status;
+ /*
+ * This needs to be a separate read looking for bit 0x04
+ * after bit 0x02 falls. If consolidated with the wait above
+ * this extra read may not happen if status returns both
+ * simultaneously and this would appear to be required.
+ */
+ status = wait_status(SMC_STATUS_BUSY, SMC_STATUS_BUSY);
+ if (status)
+ return status;
- pr_warn("send_byte(0x%02x, 0x%04x) fail: 0x%02x\n", cmd, port, status);
- return -EIO;
+ outb(cmd, port);
+ return 0;
}
+/* send_command - Write a command to the SMC. Callers must hold applesmc_lock. */
+
static int send_command(u8 cmd)
{
- return send_byte(cmd, APPLESMC_CMD_PORT);
+ int ret;
+
+ ret = wait_status(0, SMC_STATUS_IB_CLOSED);
+ if (ret)
+ return ret;
+ outb(cmd, APPLESMC_CMD_PORT);
+ return 0;
+}
+
+/*
+ * Based on logic from the Apple driver. This is issued before any interaction
+ * If busy is stuck high, issue a read command to reset the SMC state machine.
+ * If busy is stuck high after the command then the SMC is jammed.
+ */
+
+static int smc_sane(void)
+{
+ int ret;
+
+ ret = wait_status(0, SMC_STATUS_BUSY);
+ if (!ret)
+ return ret;
+ ret = send_command(APPLESMC_READ_CMD);
+ if (ret)
+ return ret;
+ return wait_status(0, SMC_STATUS_BUSY);
}
static int send_argument(const char *key)
@@ -226,6 +249,11 @@ static int read_smc(u8 cmd, const char *
{
u8 status, data = 0;
int i;
+ int ret;
+
+ ret = smc_sane();
+ if (ret)
+ return ret;
if (send_command(cmd) || send_argument(key)) {
pr_warn("%.4s: read arg fail\n", key);
@@ -239,7 +267,8 @@ static int read_smc(u8 cmd, const char *
}
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)) {
pr_warn("%.4s: read data[%d] fail\n", key, i);
return -EIO;
}
@@ -250,19 +279,24 @@ static int read_smc(u8 cmd, const char *
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);
}
if (i)
pr_warn("flushed %d bytes, last value is: %d\n", i, data);
- return 0;
+ return wait_status(0, SMC_STATUS_BUSY);
}
static int write_smc(u8 cmd, const char *key, const u8 *buffer, u8 len)
{
int i;
+ int ret;
+
+ ret = smc_sane();
+ if (ret)
+ return ret;
if (send_command(cmd) || send_argument(key)) {
pr_warn("%s: write arg fail\n", key);
@@ -281,7 +315,7 @@ static int write_smc(u8 cmd, const char
}
}
- return 0;
+ return wait_status(0, SMC_STATUS_BUSY);
}
static int read_register_count(unsigned int *count)
On 11/11/20 7:08 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.
>
> Tested on : MacbookAir6,2 MacBookPro11,1 iMac12,2, MacBookAir1,1,
> MacBookAir3,1
>
> Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
> Reported-by: Andreas Kemnade <[email protected]>
> Tested-by: Andreas Kemnade <[email protected]> # MacBookAir6,2
> Acked-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Brad Campbell <[email protected]>
> Signed-off-by: Henrik Rydberg <[email protected]>
Applied.
Guenter
>
> ---
> Changelog :
> v1 : Initial attempt
> v2 : Address logic and coding style based on comments received
> v3 : Removed some debug hangover. Added tested-by. Modifications for MacBookAir1,1
> - Significant rework of wait logic by Henrik Rydberg <[email protected]> to
> make it function at all on the MacBookAir1,1.
>
> v4 : Re-factored logic based on Apple driver. Simplified wait_status loop
> - Re-factored the logic again, this time based on the Apple driver. This
> functioned without error on all tested Macs. wait_status() contributed
> by Henrik Rydberg <[email protected]>
>
> v5 : Re-wrote status loop. Simplified busy check in send_byte()
> - Re-wrote wait_status() based on feedback from Guenter Roeck <[email protected]>
> - Added additional comments to explain multiple tests in send_byte()
> - Addressed repeated indentation issues
>
> v6 : Reverted simplification of busy check in send_byte()
> - The logic simplification in v5 send_byte() caused a few read failures in
> stress testing on a fast SMC (3 in 5.6million). Reverting that change passed
> 5 million reads with 0 errors.
>
> Index: linux-stable/drivers/hwmon/applesmc.c
> ===================================================================
> --- linux-stable.orig/drivers/hwmon/applesmc.c
> +++ linux-stable/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,10 +43,13 @@
>
> #define APPLESMC_MAX_DATA_LENGTH 32
>
> -/* wait up to 128 ms for a status change. */
> -#define APPLESMC_MIN_WAIT 0x0010
> -#define APPLESMC_RETRY_WAIT 0x0100
> -#define APPLESMC_MAX_WAIT 0x20000
> +/* Apple SMC status bits */
> +#define SMC_STATUS_AWAITING_DATA BIT(0) /* SMC has data waiting to be read */
> +#define SMC_STATUS_IB_CLOSED BIT(1) /* Will ignore any input */
> +#define SMC_STATUS_BUSY BIT(2) /* Command in progress */
> +
> +/* Initial wait is 8us */
> +#define APPLESMC_MIN_WAIT 0x0008
>
> #define APPLESMC_READ_CMD 0x10
> #define APPLESMC_WRITE_CMD 0x11
> @@ -151,65 +155,84 @@ 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 all transactions.
> + * This does 10 fast loops of 8us then exponentially backs off for a
> + * minimum total wait of 262ms. Depending on usleep_range this could
> + * run out past 500ms.
> */
> -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 i;
>
> - for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
> - usleep_range(us, us * 16);
> + us = APPLESMC_MIN_WAIT;
> + for (i = 0; i < 24 ; i++) {
> 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 * 2);
> + if (i > 9)
> + us <<= 1;
> }
> -
> - pr_warn("wait_read() fail: 0x%02x\n", status);
> return -EIO;
> }
>
> -/*
> - * send_byte - Write to SMC port, retrying when necessary. Callers
> - * must hold applesmc_lock.
> - */
> +/* send_byte - Write to SMC data port. 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;
> + int status;
>
> - 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);
> - }
> + status = wait_status(0, SMC_STATUS_IB_CLOSED);
> + if (status)
> + return status;
> + /*
> + * This needs to be a separate read looking for bit 0x04
> + * after bit 0x02 falls. If consolidated with the wait above
> + * this extra read may not happen if status returns both
> + * simultaneously and this would appear to be required.
> + */
> + status = wait_status(SMC_STATUS_BUSY, SMC_STATUS_BUSY);
> + if (status)
> + return status;
>
> - pr_warn("send_byte(0x%02x, 0x%04x) fail: 0x%02x\n", cmd, port, status);
> - return -EIO;
> + outb(cmd, port);
> + return 0;
> }
>
> +/* send_command - Write a command to the SMC. Callers must hold applesmc_lock. */
> +
> static int send_command(u8 cmd)
> {
> - return send_byte(cmd, APPLESMC_CMD_PORT);
> + int ret;
> +
> + ret = wait_status(0, SMC_STATUS_IB_CLOSED);
> + if (ret)
> + return ret;
> + outb(cmd, APPLESMC_CMD_PORT);
> + return 0;
> +}
> +
> +/*
> + * Based on logic from the Apple driver. This is issued before any interaction
> + * If busy is stuck high, issue a read command to reset the SMC state machine.
> + * If busy is stuck high after the command then the SMC is jammed.
> + */
> +
> +static int smc_sane(void)
> +{
> + int ret;
> +
> + ret = wait_status(0, SMC_STATUS_BUSY);
> + if (!ret)
> + return ret;
> + ret = send_command(APPLESMC_READ_CMD);
> + if (ret)
> + return ret;
> + return wait_status(0, SMC_STATUS_BUSY);
> }
>
> static int send_argument(const char *key)
> @@ -226,6 +249,11 @@ static int read_smc(u8 cmd, const char *
> {
> u8 status, data = 0;
> int i;
> + int ret;
> +
> + ret = smc_sane();
> + if (ret)
> + return ret;
>
> if (send_command(cmd) || send_argument(key)) {
> pr_warn("%.4s: read arg fail\n", key);
> @@ -239,7 +267,8 @@ static int read_smc(u8 cmd, const char *
> }
>
> 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)) {
> pr_warn("%.4s: read data[%d] fail\n", key, i);
> return -EIO;
> }
> @@ -250,19 +279,24 @@ static int read_smc(u8 cmd, const char *
> 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);
> }
> if (i)
> pr_warn("flushed %d bytes, last value is: %d\n", i, data);
>
> - return 0;
> + return wait_status(0, SMC_STATUS_BUSY);
> }
>
> static int write_smc(u8 cmd, const char *key, const u8 *buffer, u8 len)
> {
> int i;
> + int ret;
> +
> + ret = smc_sane();
> + if (ret)
> + return ret;
>
> if (send_command(cmd) || send_argument(key)) {
> pr_warn("%s: write arg fail\n", key);
> @@ -281,7 +315,7 @@ static int write_smc(u8 cmd, const char
> }
> }
>
> - return 0;
> + return wait_status(0, SMC_STATUS_BUSY);
> }
>
> static int read_register_count(unsigned int *count)
>