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

From: Andreas Kemnade <andreas@kemnade.info>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Guenter Roeck <linux@roeck-us.net>,
	rydberg@bitmath.org, Jean Delvare <jdelvare@suse.com>,
	linux-hwmon@vger.kernel.org,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [REGRESSION] hwmon: (applesmc) avoid overlong udelay()
Date: Fri, 2 Oct 2020 00:22:51 +0200	[thread overview]
Message-ID: <20201002002251.28462e64@aktux> (raw)
In-Reply-To: <CAK8P3a2CbhJT+B-F+cnX+uiJep9oiLM28n045-ATaVaU41u2hw@mail.gmail.com>

On Wed, 30 Sep 2020 22:00:09 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> On Wed, Sep 30, 2020 at 6:44 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On Wed, Sep 30, 2020 at 10:54:42AM +0200, Andreas Kemnade wrote:  
> > > Hi,
> > >
> > > after the $subject patch I get lots of errors like this:  
> >
> > For reference, this refers to commit fff2d0f701e6 ("hwmon: (applesmc)
> > avoid overlong udelay()").
> >  
> > > [  120.378614] applesmc: send_byte(0x00, 0x0300) fail: 0x40
> > > [  120.378621] applesmc: LKSB: write data fail
> > > [  120.512782] applesmc: send_byte(0x00, 0x0300) fail: 0x40
> > > [  120.512787] applesmc: LKSB: write data fail
> > >
> > > CPU sticks at low speed and no fan is turning on.
> > > Reverting this patch on top of 5.9-rc6 solves this problem.
> > >
> > > Some information from dmidecode:
> > >
> > > Base Board Information
> > >         Manufacturer: Apple Inc.
> > >         Product Name: Mac-7DF21CB3ED6977E5
> > >         Version: MacBookAir6,2
> > >
> > > Handle 0x0020, DMI type 11, 5 bytes OEM Strings         String 1: Apple ROM Version.  Model:       …,
> > > Handle 0x0020, DMI type 11, 5 bytes
> > > OEM Strings
> > >         String 1: Apple ROM Version.  Model:        MBA61.  EFI Version:  122.0.0
> > >         String 2: .0.0.  Built by:     root@saumon.  Date:         Wed Jun 10 18:
> > >         String 3: 10:36 PDT 2020.  Revision:     122 (B&I).  ROM Version:  F000_B
> > >         String 4: 00.  Build Type:   Official Build, Release.  Compiler:     Appl
> > >         String 5: e clang version 3.0 (tags/Apple/clang-211.10.1) (based on LLVM
> > >         String 6: 3.0svn).
> > >
> > > Writing to things in /sys/devices/platform/applesmc.768 gives also the
> > > said errors.
> > > But writing 1 to fan1_maunal and 5000 to fan1_output turns the fan on
> > > despite error messages.
> > >  
> > Not really sure what to do here. I could revert the patch, but then we'd gain
> > clang compile failures. Arnd, any idea ?  
> 
> It seems that either I made a mistake in the conversion and it sleeps for
> less time than before, or my assumption was wrong that converting a delay to
> a sleep is safe here.
> 
> The error message indicates that the write fails, not the read, so that
> is what I'd look at first. Right away I can see that the maximum time to
> retry is only half of what it used to be, as we used to wait for
> 0x10, 0x20, 0x40, 0x80, ..., 0x20000 microseconds for a total of
> 0x3fff0 microseconds (262ms), while my patch went with the 131ms
> total delay based on the comment saying "/* wait up to 128 ms for a
> status change. */".
> 
Yes, that is also what I read from the code. I just thought there must
be something simple, which just needs a short look from another pair of
eyes.

> Since there is sleeping wait, I see no reason the timeout couldn't
> be extended a lot, e.g. to a second, as in
> 
> #define APPLESMC_MAX_WAIT 0x100000
> 
> If that doesn't work, I'd try using mdelay() in place of
> usleep_range(), such as
> 
>            mdelay(DIV_ROUND_UP(us, USEC_PER_MSEC)));
> 
> This adds back a really nasty latency, but it should avoid the
> compile-time problem.
> 
> Andreas, can you try those two things? (one at a time,
> not both)

Ok, I tried. None of them works. I rechecked my work and created real
git commits out of them and CONFIG_LOCALVERSION_AUTO is also set so
the usual stupid things are rules out.
In detail:
On top of 5.9-rc6 + *reverted* patch:

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index fd99c9df8a00..2a9bd7f2b71b 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -45,7 +45,7 @@
 /* wait up to 128 ms for a status change. */
 #define APPLESMC_MIN_WAIT	0x0010
 #define APPLESMC_RETRY_WAIT	0x0100
-#define APPLESMC_MAX_WAIT	0x20000
+#define APPLESMC_MAX_WAIT	0x8000
 
 #define APPLESMC_READ_CMD	0x10
 #define APPLESMC_WRITE_CMD	0x11
-- 
2.20.1

-> no trouble found, but I have not tested very long, just some
sysfs writes.

On top of 5.9-rc6:
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index a18887990f4a..3b0108b75a24 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -161,7 +161,7 @@ static int wait_read(void)
 	int us;
 
 	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
-		usleep_range(us, us * 16);
+		mdelay(DIV_ROUND_UP(us, USEC_PER_MSEC));
 		status = inb(APPLESMC_CMD_PORT);
 		/* read: wait for smc to settle */
 		if (status & 0x01)
@@ -187,7 +187,7 @@ static int send_byte(u8 cmd, u16 port)
 
 	outb(cmd, port);
 	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
-		usleep_range(us, us * 16);
+		mdelay(DIV_ROUND_UP(us, USEC_PER_MSEC));
 		status = inb(APPLESMC_CMD_PORT);
 		/* write: wait for smc to settle */
 		if (status & 0x02)
-- 
2.20.1
-> write errors:

[    2.472801] applesmc: key=561 fan=1 temp=33 index=33 acc=0 lux=2 kbd=1
[    2.472961] applesmc applesmc.768: hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
[   18.535659] applesmc: send_byte(0x00, 0x0300) fail: 0x40
[   18.538171] applesmc: LKSB: write data fail
[   45.260307] applesmc: send_byte(0x01, 0x0300) fail: 0x40
[   45.260324] applesmc: FS! : write data fail
[   47.870135] applesmc: send_byte(0x20, 0x0300) fail: 0x40
[   47.870193] applesmc: F0Tg: write data fail

On top of 5.9-rc6:
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index a18887990f4a..f67a25651d03 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -45,7 +45,7 @@
 /* wait up to 128 ms for a status change. */
 #define APPLESMC_MIN_WAIT	0x0010
 #define APPLESMC_RETRY_WAIT	0x0100
-#define APPLESMC_MAX_WAIT	0x20000
+#define APPLESMC_MAX_WAIT	0x100000
 
 #define APPLESMC_READ_CMD	0x10
 #define APPLESMC_WRITE_CMD	0x11
-- 
2.20.1

-> write errors:

[    1.428726] applesmc: key=561 fan=1 temp=33 index=33 acc=0 lux=2 kbd=1
[    1.428869] applesmc applesmc.768: hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
[   19.672561] applesmc: send_byte(0x00, 0x0300) fail: 0x40
[   19.674641] applesmc: LKSB: write data fail
[   34.266216] applesmc: send_byte(0x01, 0x0300) fail: 0x40
[   34.266277] applesmc: FS! : write data fail
[   37.357023] applesmc: send_byte(0x20, 0x0300) fail: 0x40
[   37.357082] applesmc: F0Tg: write data fail

Accessing things in sysfs took longer, so the increase seems to be in effect.
Conclusion:

head->scratch();
So something requires really exact timings.

Regards,
Andreas
