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

From: Andreas Kemnade <andreas@kemnade.info>
To: Brad Campbell <brad@fnarfbargle.com>
Cc: Guenter Roeck <linux@roeck-us.net>, Arnd Bergmann <arnd@arndb.de>,
	rydberg@bitmath.org, Jean Delvare <jdelvare@suse.com>,
	linux-hwmon@vger.kernel.org,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	hns@goldelico.com
Subject: Re: [REGRESSION] hwmon: (applesmc) avoid overlong udelay()
Date: Wed, 4 Nov 2020 14:20:57 +0100	[thread overview]
Message-ID: <20201104142057.62493c12@aktux> (raw)
In-Reply-To: <68467f1b-cea1-47ea-a4d4-8319214b072a@fnarfbargle.com>

On Tue, 3 Nov 2020 16:56:32 +1100
Brad Campbell <brad@fnarfbargle.com> wrote:

> On 3/11/20 10:56 am, Brad Campbell wrote:
> 
> > 
> > I've examined the code in VirtualSMC and I'm not convinced we were not waiting on the wrong bits.
> > 
> > #define SMC_STATUS_AWAITING_DATA  BIT0  ///< Ready to read data.
> > #define SMC_STATUS_IB_CLOSED      BIT1  /// A write is pending.
> > #define SMC_STATUS_BUSY           BIT2  ///< Busy processing a command.
> > #define SMC_STATUS_GOT_COMMAND    BIT3  ///< The last input was a command.
> > #define SMC_STATUS_UKN_0x16       BIT4
> > #define SMC_STATUS_KEY_DONE       BIT5
> > #define SMC_STATUS_READY          BIT6  // Ready to work
> > #define SMC_STATUS_UKN_0x80       BIT7  // error
> > 
> > Any chance you could try this patch? It's ugly, hacked together and currently fairly undocumented, but if it works I'll figure out how to clean it up (suggestions welcome).
> > It works on my MacbookPro 11,1.  
> 
> I had some time so I spent a bit of time refactoring and trying to clarify the magic numbers.
> 
> I also did some fuzzing of the SMC and figured out where we can loosen the masks.
> This has some debug code in it to identify if any wait loops exceed 1 loop and if the SMC is reporting anything other than a clear "I'm waiting" prior to each command.
> 
> You might see some of these :
> [   51.316202] applesmc: wait_status looping 2: 0x44, 0x40, 0x4e
> [   60.002547] applesmc: wait_status looping 2: 0x44, 0x40, 0x4e
> [   60.130754] applesmc: wait_status looping 2: 0x44, 0x40, 0x4e
> 
> I did some heavy tests and found that with the delays at the bottom of the loop about 50% of calls required no delay at all before a read or write and the other 50% require a single delay.
> I can count on one hand the number of times it's exceeded 1 loop, and the max thus far has been 5 loops.
> 
That matches my experience. The only delay is at the end of the
command. Critcal seems to be that there is not too much delay in between.

[...]
> If anyone with a Mac having a conventional SMC and seeing issues on 5.9 could test this it'd be appreciated. I'm not saying this code is "correct", but it "works for me".
> 
Seems to work here.
 dmesg  | grep applesmc

[    1.350782] applesmc: key=561 fan=1 temp=33 index=33 acc=0 lux=2 kbd=1
[    1.350922] applesmc applesmc.768: hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
[   17.748504] applesmc: wait_status looping 2: 0x4a, 0x4c, 0x4f
[  212.008952] applesmc: wait_status looping 2: 0x44, 0x40, 0x4e
[  213.033930] applesmc: wait_status looping 2: 0x44, 0x40, 0x4e
[  213.167908] applesmc: wait_status looping 2: 0x44, 0x40, 0x4e
[  219.087854] applesmc: wait_status looping 2: 0x44, 0x40, 0x4e

Tested it on top of 5.9

Regards,
Andreas