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

From: Brad Campbell <brad@fnarfbargle.com>
To: Guenter Roeck <linux@roeck-us.net>, Henrik Rydberg <rydberg@bitmath.org>
Cc: Arnd Bergmann <arnd@arndb.de>,
	linux-hwmon@vger.kernel.org,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	hns@goldelico.com, Andreas Kemnade <andreas@kemnade.info>,
	Jean Delvare <jdelvare@suse.com>
Subject: Re: [PATCH v3] applesmc: Re-work SMC comms
Date: Tue, 10 Nov 2020 13:04:04 +1100	[thread overview]
Message-ID: <daa34876-55b6-1a85-2532-976934d2fbb1@fnarfbargle.com> (raw)
In-Reply-To: <db65e8ba-80fd-2142-56f2-8f625defb160@roeck-us.net>

On 9/11/20 3:06 am, Guenter Roeck wrote:
> On 11/8/20 2:14 AM, Henrik Rydberg 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.
>>
> 
> Please resend this patch as stand-alone v4 patch. If sent like it was sent here,
> it doesn't make it into patchwork, and is thus not only difficult to apply but
> may get lost, and it is all but impossible to find and apply all tags.
> Also, prior to Henrik's Signed=off-by: there should be a one-line explanation
> of the changes made.
> 
> Thanks,
> Guenter
> 
>> 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.
>>

Can I get an opinion on this wait statement please?

The apple driver uses a loop with a million (1,000,000) retries spaced with a 10uS delay.

In my testing on 2 machines, we don't busy wait more than about 2 loops.
Replacing a small udelay with the usleep_range kills performance.
With the following (do 10 fast checks before we start sleeping) I nearly triple the performance
of the driver on my laptop, and double it on my iMac. This is on an otherwise unmodified version of
Henriks v4 submission.

Yes, given the timeouts I know it's a ridiculous loop condition.

static int wait_status(u8 val, u8 mask)
{
	unsigned long end = jiffies + (APPLESMC_MAX_WAIT * HZ) / USEC_PER_SEC;
	u8 status;
	int i;

	for (i=1; i < 1000000; i++) {
		status = inb(APPLESMC_CMD_PORT);
		if ((status & mask) == val)
			return status;
		/* timeout: give up */
		if (time_after(jiffies, end))
			break;
		if (i < 10)
			udelay(10);
		else
			usleep_range(APPLESMC_MIN_WAIT, APPLESMC_MIN_WAIT * 16);
	}
	return -EIO;
}

Regards,
Brad