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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <68467f1b-cea1-47ea-a4d4-8319214b072a@fnarfbargle.com>
Date:   Tue, 3 Nov 2020 16:56:32 +1100
From:   Brad Campbell <brad@...rfbargle.com>
To:     Andreas Kemnade <andreas@...nade.info>,
        Guenter Roeck <linux@...ck-us.net>
Cc:     Arnd Bergmann <arnd@...db.de>, rydberg@...math.org,
        Jean Delvare <jdelvare@...e.com>, linux-hwmon@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        hns@...delico.com
Subject: Re: [REGRESSION] hwmon: (applesmc) avoid overlong udelay()

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

View attachment "smc.patch.5" of type "text/plain" (6173 bytes)