Received: by 2002:a05:6a10:16a7:0:0:0:0 with SMTP id gp39csp2447991pxb; Sun, 8 Nov 2020 00:39:37 -0800 (PST) X-Google-Smtp-Source: ABdhPJxGJN1RXaCrkjSQLLwhmuAUvAwhQfRod8R0MtYHb7v6CUmzY9y4J7szaLpe1liIJYZMa5KJ X-Received: by 2002:a17:906:52c6:: with SMTP id w6mr9466676ejn.199.1604824777715; Sun, 08 Nov 2020 00:39:37 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1604824777; cv=none; d=google.com; s=arc-20160816; b=TcHZeum2KDyg+nAW6Z06n3XaknXtZnMUyfdbyQYNobHTbfCrUYyGS3jMXiM7Iz9d7Q 0iuX70bBqVPmr3DNyAkID/2tGYsopvnnGW18XXICDv6zAeg6W8luGrbPDfzacb80fVEc lIo7jUipu9mIwBRTlCL4sq2CyHPMqvSX+XyuuUWOqoT1YOycAN0iuunHubFJ5poCctpn nZ1xLCSZNYnWM0Oox/J4XJELRbf3QjvCcqhqOemArWEeoXNvCfAf2FILORIOlGrCLo7e nHz5RUIynKI1z0Y/LzRkbPWIIpMA6krx+3TTvNw9ubMLZC7EtzoS2j7cgRWxlJvh5v3m W3AQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:content-language :in-reply-to:mime-version:user-agent:date:message-id:from:references :cc:to:subject:dkim-signature; bh=6qkho/qWzg4BMWgNKrZIpGhwl5Wi0G5G8oSQ4foPpSc=; b=EU+XZ4LBw0QHHgDiqEsSM9rDjA0TQhC67aVPbWRJyDFACZCWZnZLn7E2yqe4Fs5tcm KIeM5rZAg6Yhyce8K/d96sOAckF4xzsjNwH9jZkkmZwIfBqw9Kvb/mwsz2YJgCCNTJ7o gGsNpRvoSppFdOf9Z8VOkbdNzzeQCnPqvpZe++9c4sZCJl1a+ucZaXDzZraYqqr+5Wsa nkk4Hb5RddNnb9i/Iv2hJhETliydcdldW3hdXmFNoVanlYrq54K7bvgMeFH1QYMpQJDV Dg7QYio6+x4LoTaz8c91meoCLQpdKxlGidRNEtop/sDIJbw4TKqs/dEebgtQzd94pW3S JvSw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@bitmath.org header.s=20191106 header.b=LFMWMpUN; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id j6si4656677edr.531.2020.11.08.00.39.05; Sun, 08 Nov 2020 00:39:37 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@bitmath.org header.s=20191106 header.b=LFMWMpUN; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726210AbgKHIfX (ORCPT + 99 others); Sun, 8 Nov 2020 03:35:23 -0500 Received: from mailrelay1-2.pub.mailoutpod1-cph3.one.com ([46.30.212.0]:56186 "EHLO mailrelay1-2.pub.mailoutpod1-cph3.one.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726115AbgKHIfW (ORCPT ); Sun, 8 Nov 2020 03:35:22 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bitmath.org; s=20191106; h=content-transfer-encoding:content-type:in-reply-to:mime-version:date: message-id:from:references:cc:to:subject:from; bh=6qkho/qWzg4BMWgNKrZIpGhwl5Wi0G5G8oSQ4foPpSc=; b=LFMWMpUNldnUPbbAyq31h4DgjYBlve9MmmAFvZWcD3wwVBPVLHkPggJKLMm3wXTvXGcXuzpekK5So 7pJkR6Y1oDTAC14RTNG71irgww7sFHg5c7WbZa33fSdbH2hiMHASwf9tjo3/YAEIVtwfTLZznvdpKJ BizyFwlr0LOa3XcJUa0eRZ/YWfY9klt6osgloe8dShIEsMUHXkXAMg8oJNUYlUFcuvtYE55A15syxK HF3kcS1hSmkQ5AOt89CiCynixn1VeMP8kUZjd8m+C3INMhkeNxDvU2hZqseiERo7GGvz5OlNye9tGG 0PMHZtr9d5nSuTopmOI9gnyuwbSx23g== X-HalOne-Cookie: 84779ecd025a8fde8be3209049157a3622d93a8e X-HalOne-ID: 53c62c29-219d-11eb-9658-d0431ea8a283 Received: from [192.168.19.13] (h-155-4-128-97.na.cust.bahnhof.se [155.4.128.97]) by mailrelay1.pub.mailoutpod1-cph3.one.com (Halon) with ESMTPSA id 53c62c29-219d-11eb-9658-d0431ea8a283; Sun, 08 Nov 2020 08:35:16 +0000 (UTC) Subject: Re: [PATCH v3] applesmc: Re-work SMC comms To: Brad Campbell Cc: Arnd Bergmann , linux-hwmon@vger.kernel.org, "linux-kernel@vger.kernel.org" , hns@goldelico.com, Guenter Roeck , Andreas Kemnade , Jean Delvare References: <20200930105442.3f642f6c@aktux> <20201002002251.28462e64@aktux> <7543ef85-727d-96c3-947e-5b18e9e6c44d@roeck-us.net> <20201006090226.4275c824@kemnade.info> <68467f1b-cea1-47ea-a4d4-8319214b072a@fnarfbargle.com> <20201104142057.62493c12@aktux> <2436afef-99c6-c352-936d-567bf553388c@fnarfbargle.com> <7a085650-2399-08c0-3c4d-6cd1fa28a365@roeck-us.net> <10027199-5d31-93e7-9bd8-7baaebff8b71@roeck-us.net> <70331f82-35a1-50bd-685d-0b06061dd213@fnarfbargle.com> <3c72ccc3-4de1-b5d0-423d-7b8c80991254@fnarfbargle.com> <6d071547-10ee-ca92-ec8b-4b5069d04501@bitmath.org> <8e117844-d62a-bcb1-398d-c59cc0d4b878@fnarfbargle.com> <9109d059-d9cb-7464-edba-3f42aa78ce92@bitmath.org> <5310c0ab-0f80-1f9e-8807-066223edae13@bitmath.org> <57057d07-d3a0-8713-8365-7b12ca222bae@fnarfbargle.com> From: Henrik Rydberg Message-ID: <41909045-9486-78d9-76c2-73b99a901b83@bitmath.org> Date: Sun, 8 Nov 2020 09:35:28 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <57057d07-d3a0-8713-8365-7b12ca222bae@fnarfbargle.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. ;-) Henrik > > 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 > > Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()") > Reported-by: Andreas Kemnade > Tested-by: Andreas Kemnade # MacBookAir6,2 > Acked-by: Arnd Bergmann > Signed-off-by: Brad Campbell > Signed-off-by: Henrik Rydberg > > --- > Changelog : > v1 : Inital attempt > v2 : Address logic and coding style > v3 : Removed some debug hangover. Added tested-by. Modifications for MacBookAir1,1 > > diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c > index a18887990f4a..3e968abb37aa 100644 > --- a/drivers/hwmon/applesmc.c > +++ b/drivers/hwmon/applesmc.c > @@ -32,6 +32,7 @@ > #include > #include > #include > +#include > > /* data port used by Apple SMC */ > #define APPLESMC_DATA_PORT 0x300 > @@ -42,6 +43,11 @@ > > #define APPLESMC_MAX_DATA_LENGTH 32 > > +/* Apple SMC status bits */ > +#define SMC_STATUS_AWAITING_DATA BIT(0) /* SMC has data waiting */ > +#define SMC_STATUS_IB_CLOSED BIT(1) /* Will ignore any input */ > +#define SMC_STATUS_BUSY BIT(2) /* Command in progress */ > + > /* wait up to 128 ms for a status change. */ > #define APPLESMC_MIN_WAIT 0x0010 > #define APPLESMC_RETRY_WAIT 0x0100 > @@ -151,65 +157,73 @@ static unsigned int key_at_index; > static struct workqueue_struct *applesmc_led_wq; > > /* > - * wait_read - Wait for a byte to appear on SMC port. Callers must > - * hold applesmc_lock. > + * Wait for specific status bits with a mask on the SMC > + * Used before and after writes, and before reads > */ > -static int wait_read(void) > + > +static int wait_status(u8 val, u8 mask) > { > unsigned long end = jiffies + (APPLESMC_MAX_WAIT * HZ) / USEC_PER_SEC; > u8 status; > int us; > > + udelay(APPLESMC_MIN_WAIT); > for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) { > - usleep_range(us, us * 16); > status = inb(APPLESMC_CMD_PORT); > - /* read: wait for smc to settle */ > - if (status & 0x01) > + if ((status & mask) == val) > return 0; > /* timeout: give up */ > if (time_after(jiffies, end)) > break; > + usleep_range(us, us * 16); > } > - > - pr_warn("wait_read() fail: 0x%02x\n", status); > return -EIO; > } > > /* > - * send_byte - Write to SMC port, retrying when necessary. Callers > + * send_byte_data - Write to SMC data port. Callers > * must hold applesmc_lock. > + * Parameter skip must be true on the last write of any > + * command or it'll time out. > */ > -static int send_byte(u8 cmd, u16 port) > + > +static int send_byte_data(u8 cmd, u16 port, bool skip) > { > - u8 status; > - int us; > - unsigned long end = jiffies + (APPLESMC_MAX_WAIT * HZ) / USEC_PER_SEC; > + int ret; > > + ret = wait_status(SMC_STATUS_BUSY, SMC_STATUS_BUSY | SMC_STATUS_IB_CLOSED); > + if (ret) > + return ret; > outb(cmd, port); > - for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) { > - usleep_range(us, us * 16); > - status = inb(APPLESMC_CMD_PORT); > - /* write: wait for smc to settle */ > - if (status & 0x02) > - continue; > - /* ready: cmd accepted, return */ > - if (status & 0x04) > - return 0; > - /* timeout: give up */ > - if (time_after(jiffies, end)) > - break; > - /* busy: long wait and resend */ > - udelay(APPLESMC_RETRY_WAIT); > - outb(cmd, port); > - } > + return wait_status(skip ? 0 : SMC_STATUS_BUSY, SMC_STATUS_BUSY); > +} > > - pr_warn("send_byte(0x%02x, 0x%04x) fail: 0x%02x\n", cmd, port, status); > - return -EIO; > +static int send_byte(u8 cmd, u16 port) > +{ > + return send_byte_data(cmd, port, false); > } > > +/* > + * send_command - Write a command to the SMC. Callers must hold applesmc_lock. > + * If SMC is in undefined state, any new command write resets the state machine. > + */ > + > static int send_command(u8 cmd) > { > - return send_byte(cmd, APPLESMC_CMD_PORT); > + int ret; > + int i; > + > + for (i = 0; i < 16; i++) { > + ret = wait_status(0, SMC_STATUS_IB_CLOSED); > + if (ret) > + return ret; > + > + outb(cmd, APPLESMC_CMD_PORT); > + ret = wait_status(SMC_STATUS_BUSY, SMC_STATUS_BUSY); > + if (!ret) > + return ret; > + } > + return -EIO; > } > > static int send_argument(const char *key) > @@ -239,7 +253,8 @@ static int read_smc(u8 cmd, const char *key, u8 *buffer, u8 len) > } > > for (i = 0; i < len; i++) { > - if (wait_read()) { > + if (wait_status(SMC_STATUS_AWAITING_DATA, > + SMC_STATUS_AWAITING_DATA | SMC_STATUS_IB_CLOSED)) { > pr_warn("%.4s: read data[%d] fail\n", key, i); > return -EIO; > } > @@ -250,7 +265,7 @@ static int read_smc(u8 cmd, const char *key, u8 *buffer, u8 len) > for (i = 0; i < 16; i++) { > udelay(APPLESMC_MIN_WAIT); > status = inb(APPLESMC_CMD_PORT); > - if (!(status & 0x01)) > + if (!(status & SMC_STATUS_AWAITING_DATA)) > break; > data = inb(APPLESMC_DATA_PORT); > } > @@ -275,7 +290,7 @@ static int write_smc(u8 cmd, const char *key, const u8 *buffer, u8 len) > } > > for (i = 0; i < len; i++) { > - if (send_byte(buffer[i], APPLESMC_DATA_PORT)) { > + if (send_byte_data(buffer[i], APPLESMC_DATA_PORT, i == len - 1)) { > pr_warn("%s: write data fail\n", key); > return -EIO; > } >