linux-kernel - Re: [PATCH v3] applesmc: Re-work SMC comms

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: <edd79c0f-c562-24af-cd6f-a33d2e662390@fnarfbargle.com>
Date:   Mon, 9 Nov 2020 20:51:32 +1100
From:   Brad Campbell <brad@...rfbargle.com>
To:     Andreas Kemnade <andreas@...nade.info>,
        Henrik Rydberg <rydberg@...math.org>
Cc:     Arnd Bergmann <arnd@...db.de>, linux-hwmon@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        hns@...delico.com, Guenter Roeck <linux@...ck-us.net>,
        Jean Delvare <jdelvare@...e.com>
Subject: Re: [PATCH v3] applesmc: Re-work SMC comms

On 9/11/20 7:44 pm, Andreas Kemnade wrote:
> On Sun, 8 Nov 2020 11:14:29 +0100
> Henrik Rydberg <rydberg@...math.org> 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 <brad@...rfbargle.com>
>> 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 <andreas@...nade.info>
>> Tested-by: Andreas Kemnade <andreas@...nade.info> # MacBookAir6,2
>> Acked-by: Arnd Bergmann <arnd@...db.de>
>> Signed-off-by: Brad Campbell <brad@...rfbargle.com>
>> Signed-off-by: Henrik Rydberg <rydberg@...math.org>
>>
>> ---
>> 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