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

From: Arnd Bergmann <arnd@arndb.de>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Andreas Kemnade <andreas@kemnade.info>,
	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: Wed, 30 Sep 2020 22:00:09 +0200	[thread overview]
Message-ID: <CAK8P3a2CbhJT+B-F+cnX+uiJep9oiLM28n045-ATaVaU41u2hw@mail.gmail.com> (raw)
In-Reply-To: <20200930164446.GB219887@roeck-us.net>

On Wed, Sep 30, 2020 at 6:44 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Wed, Sep 30, 2020 at 10:54:42AM +0200, Andreas Kemnade wrote:
> > Hi,
> >
> > after the $subject patch I get lots of errors like this:
>
> For reference, this refers to commit fff2d0f701e6 ("hwmon: (applesmc)
> avoid overlong udelay()").
>
> > [  120.378614] applesmc: send_byte(0x00, 0x0300) fail: 0x40
> > [  120.378621] applesmc: LKSB: write data fail
> > [  120.512782] applesmc: send_byte(0x00, 0x0300) fail: 0x40
> > [  120.512787] applesmc: LKSB: write data fail
> >
> > CPU sticks at low speed and no fan is turning on.
> > Reverting this patch on top of 5.9-rc6 solves this problem.
> >
> > Some information from dmidecode:
> >
> > Base Board Information
> >         Manufacturer: Apple Inc.
> >         Product Name: Mac-7DF21CB3ED6977E5
> >         Version: MacBookAir6,2
> >
> > Handle 0x0020, DMI type 11, 5 bytes OEM Strings         String 1: Apple ROM Version.  Model:       …,
> > Handle 0x0020, DMI type 11, 5 bytes
> > OEM Strings
> >         String 1: Apple ROM Version.  Model:        MBA61.  EFI Version:  122.0.0
> >         String 2: .0.0.  Built by:     root@saumon.  Date:         Wed Jun 10 18:
> >         String 3: 10:36 PDT 2020.  Revision:     122 (B&I).  ROM Version:  F000_B
> >         String 4: 00.  Build Type:   Official Build, Release.  Compiler:     Appl
> >         String 5: e clang version 3.0 (tags/Apple/clang-211.10.1) (based on LLVM
> >         String 6: 3.0svn).
> >
> > Writing to things in /sys/devices/platform/applesmc.768 gives also the
> > said errors.
> > But writing 1 to fan1_maunal and 5000 to fan1_output turns the fan on
> > despite error messages.
> >
> Not really sure what to do here. I could revert the patch, but then we'd gain
> clang compile failures. Arnd, any idea ?

It seems that either I made a mistake in the conversion and it sleeps for
less time than before, or my assumption was wrong that converting a delay to
a sleep is safe here.

The error message indicates that the write fails, not the read, so that
is what I'd look at first. Right away I can see that the maximum time to
retry is only half of what it used to be, as we used to wait for
0x10, 0x20, 0x40, 0x80, ..., 0x20000 microseconds for a total of
0x3fff0 microseconds (262ms), while my patch went with the 131ms
total delay based on the comment saying "/* wait up to 128 ms for a
status change. */".

Since there is sleeping wait, I see no reason the timeout couldn't
be extended a lot, e.g. to a second, as in

#define APPLESMC_MAX_WAIT 0x100000

If that doesn't work, I'd try using mdelay() in place of
usleep_range(), such as

           mdelay(DIV_ROUND_UP(us, USEC_PER_MSEC)));

This adds back a really nasty latency, but it should avoid the
compile-time problem.

Andreas, can you try those two things? (one at a time,
not both)

       Arnd