From patchwork Tue Apr 2 22:44:44 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Doug Anderson X-Patchwork-Id: 10882407 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 37B4B15AC for ; Tue, 2 Apr 2019 22:45:25 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0D3FE28958 for ; Tue, 2 Apr 2019 22:45:25 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id EE76E28965; Tue, 2 Apr 2019 22:45:24 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.0 required=2.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 1BD1328958 for ; Tue, 2 Apr 2019 22:45:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:Message-Id:Date:Subject:To :From:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-Reply-To:References: List-Owner; bh=FnFc1HPDzbyWRWQEt076S7ranA6TqamD6B7jjYY/g2o=; b=kCRZfCS+rC9KSb HTh9RooKus7Uvyi8v8CM6E9v8hQ2TmGHaWqlAJe3n9u/b017v2zvyrYbNNCWGXhZROOxAtI5deSRy tqv/vsp80HbNqEzE9J7dV79syeMaRQ/JAieWy25cgaGUSq8rOZ2xaRtSqqXEiGAX1NCejid3qQU5l BFpSddZ55MMEoHf41PCFIEa23qlLXS65AFyYTmqW/PF29FSGqAawTHeh7K1TUB0voywR54wBCyUMO fXT+U7SWFFY3r7Gnhqcwzbnk64HLqk7hLzopCxOkjzupG+Uf0fyeB+HBxCXPfqnFt7RrFCZpIDLWB TgzJq42/VQdCwIhkE9DA==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1hBS9g-00048t-1H; Tue, 02 Apr 2019 22:45:20 +0000 Received: from mail-pg1-x542.google.com ([2607:f8b0:4864:20::542]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1hBS9c-00048C-VB for linux-rockchip@lists.infradead.org; Tue, 02 Apr 2019 22:45:18 +0000 Received: by mail-pg1-x542.google.com with SMTP id v12so7278827pgq.1 for ; Tue, 02 Apr 2019 15:45:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=sfP6zObWAKgqAMuUmRaUQ8eIO1kmM7n1FGiJrLQFKhI=; b=JfnH+Wf/Jfiv3izQV6cl4/B+jff7FxI4EsdFMSszeHEGkQsAsQMR78jy+LwMcIcop2 NhG9MSutxhIZpDLAr3SYEdOZzg+KTX7KgEc6SzG6hAyDs0du7jLslwkMyA8gj78A7/v9 oV5LL0gpJIwmHG/nTN4X+ubsHYlV2hD/F3pbs= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=sfP6zObWAKgqAMuUmRaUQ8eIO1kmM7n1FGiJrLQFKhI=; b=rGvX1Pb7aAhyV5fR9ZBIDMj7IuHX/IjB47XD5TN0qxpnSokBTGnkTmbcw7G48VS8f8 /tbKm8xP7X+v9O+3rdg3MdwOz/BgP9U69/Nu8lwneoWJ2QoBIX/t+Mxjqh5LfaWRbplF fsyWHYrpbLLXuL5j5PYAnzfklv8OgDlF+Ic8vXI6P93/PDFh+e2vjQp0FTjpr7GU+n8I 6dVlFNOd33yj6p8Ruz+kArDBKcSRXL3rH7PjiLpmmeorsE+dBSqSspCGMh3tIwtC+ktF Um125OOelNAHw7x7X6zO5/KCJfG0vLlp54HGOMjQSYUCSHOlo289xQATzmdJSCibTATi L8Aw== X-Gm-Message-State: APjAAAU3EHafu8onwrQ9sE9iptTPE+bgpijWO3owvJX1W3Uar+H5mbmo XrMKx56a8JCU46vnLZinQv/3OA== X-Google-Smtp-Source: APXvYqxSpKiHXJG7YJs/+Qmz19hl8YjdDlZPO3I3rJ3Hm6HtSVhb+iDPtyLcMFyhsk//XgCxZ8wgxQ== X-Received: by 2002:a65:51c3:: with SMTP id i3mr68281999pgq.45.1554245113255; Tue, 02 Apr 2019 15:45:13 -0700 (PDT) Received: from tictac2.mtv.corp.google.com ([2620:15c:202:1:24fa:e766:52c9:e3b2]) by smtp.gmail.com with ESMTPSA id c3sm21518351pfo.2.2019.04.02.15.45.11 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 02 Apr 2019 15:45:12 -0700 (PDT) From: Douglas Anderson To: Benson Leung , Enric Balletbo i Serra Subject: [PATCH] platform/chrome: cros_ec_spi: Transfer messages at high priority Date: Tue, 2 Apr 2019 15:44:44 -0700 Message-Id: <20190402224445.64823-1-dianders@chromium.org> X-Mailer: git-send-email 2.21.0.392.gf8f6787159e-goog MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190402_154517_040234_D31A9275 X-CRM114-Status: GOOD ( 20.13 ) X-BeenThere: linux-rockchip@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: Upstream kernel work for Rockchip platforms List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: rspangler@chromium.org, amstan@chromium.org, sjg@chromium.org, briannorris@chromium.org, Douglas Anderson , linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org, broonie@kernel.org, ryandcase@chromium.org, groeck@chromium.org, mka@chromium.org, heiko@sntech.de Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+patchwork-linux-rockchip=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP The software running on the Chrome OS Embedded Controller (cros_ec) handles SPI transfers in a bit of a wonky way. Specifically if the EC sees too long of a delay in a SPI transfer it will give up and the transfer will be counted as failed. Unfortunately the timeout is fairly short, though the actual number may be different for different EC codebases. We can end up tripping the timeout pretty easily if we happen to preempt the task running the SPI transfer and don't get back to it for a little while. Historically this hasn't been a _huge_ deal because: 1. On old devices Chrome OS used to run PREEMPT_VOLUNTARY. That meant we were pretty unlikely to take a big break from the transfer. 2. On recent devices we had faster / more processors. 3. Recent devices didn't use "cros-ec-spi-pre-delay". Using that delay makes us more likely to trip this use case. 4. For whatever reasons (I didn't dig) old kernels seem to be less likely to trip this. 5. For the most part it's kinda OK if a few transfers to the EC fail. Mostly we're just polling the battery or doing some other task where we'll try again. Even with the above things, this issue has reared its ugly head periodically. We could solve this in a nice way by adding reliable retries to the EC protocol [1] or by re-designing the code in the EC codebase to allow it to wait longer, but that code doesn't ever seem to get changed. ...and even if it did, it wouldn't help old devices. It's now time to finally take a crack at making this a little better. This patch isn't guaranteed to make every cros_ec SPI transfer perfect, but it should improve things by a few orders of magnitude. Specifically you can try this on a rk3288-veyron Chromebook (which is slower and also _does_ need "cros-ec-spi-pre-delay"): md5sum /dev/zero & md5sum /dev/zero & md5sum /dev/zero & md5sum /dev/zero & while true; do cat /sys/class/power_supply/sbs-20-000b/charge_now > /dev/null; done ...before this patch you'll see boatloads of errors. After this patch I don't see any in the testing I did. The way this patch works is by effectively boosting the priority of the cros_ec transfers. As far as I know there is no simple way to just boost the priority of the current process temporarily so the way we accomplish this is by creating a "WQ_HIGHPRI" workqueue and doing the transfers there. NOTE: this patch relies on the fact that the SPI framework attempts to push the messages out on the calling context (which is the one that is boosted to high priority). As I understand from earlier (long ago) discussions with Mark Brown this should be a fine assumption. Even if it isn't true sometimes this patch will still not make things worse. [1] https://crbug.com/678675 Signed-off-by: Douglas Anderson --- drivers/platform/chrome/cros_ec_spi.c | 107 ++++++++++++++++++++++++-- 1 file changed, 101 insertions(+), 6 deletions(-) diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c index ffc38f9d4829..101f2deb7d3c 100644 --- a/drivers/platform/chrome/cros_ec_spi.c +++ b/drivers/platform/chrome/cros_ec_spi.c @@ -67,12 +67,30 @@ * is sent when we want to turn on CS at the start of a transaction. * @end_of_msg_delay: used to set the delay_usecs on the spi_transfer that * is sent when we want to turn off CS at the end of a transaction. + * @high_pri_wq: We'll do our transfers here to reduce preemption problems. */ struct cros_ec_spi { struct spi_device *spi; s64 last_transfer_ns; unsigned int start_of_msg_delay; unsigned int end_of_msg_delay; + struct workqueue_struct *high_pri_wq; +}; + +/** + * struct cros_ec_xfer_work_params - params for our high priority workers + * + * @work: The work_struct needed to queue work + * @ec_dev: ChromeOS EC device + * @ec_msg: Message to transfer + * @ret: The return value of the function + */ + +struct cros_ec_xfer_work_params { + struct work_struct work; + struct cros_ec_device *ec_dev; + struct cros_ec_command *ec_msg; + int ret; }; static void debug_packet(struct device *dev, const char *name, u8 *ptr, @@ -350,13 +368,13 @@ static int cros_ec_spi_receive_response(struct cros_ec_device *ec_dev, } /** - * cros_ec_pkt_xfer_spi - Transfer a packet over SPI and receive the reply + * do_cros_ec_pkt_xfer_spi - Transfer a packet over SPI and receive the reply * * @ec_dev: ChromeOS EC device * @ec_msg: Message to transfer */ -static int cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev, - struct cros_ec_command *ec_msg) +static int do_cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev, + struct cros_ec_command *ec_msg) { struct ec_host_response *response; struct cros_ec_spi *ec_spi = ec_dev->priv; @@ -493,13 +511,13 @@ static int cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev, } /** - * cros_ec_cmd_xfer_spi - Transfer a message over SPI and receive the reply + * do_cros_ec_cmd_xfer_spi - Transfer a message over SPI and receive the reply * * @ec_dev: ChromeOS EC device * @ec_msg: Message to transfer */ -static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev, - struct cros_ec_command *ec_msg) +static int do_cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev, + struct cros_ec_command *ec_msg) { struct cros_ec_spi *ec_spi = ec_dev->priv; struct spi_transfer trans; @@ -611,6 +629,54 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev, return ret; } +static void cros_ec_pkt_xfer_spi_work(struct work_struct *work) +{ + struct cros_ec_xfer_work_params *params; + + params = container_of(work, struct cros_ec_xfer_work_params, work); + params->ret = do_cros_ec_pkt_xfer_spi(params->ec_dev, params->ec_msg); +} + +static int cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev, + struct cros_ec_command *ec_msg) +{ + struct cros_ec_spi *ec_spi = ec_dev->priv; + struct cros_ec_xfer_work_params params; + + INIT_WORK(¶ms.work, cros_ec_pkt_xfer_spi_work); + params.ec_dev = ec_dev; + params.ec_msg = ec_msg; + + queue_work(ec_spi->high_pri_wq, ¶ms.work); + flush_workqueue(ec_spi->high_pri_wq); + + return params.ret; +} + +static void cros_ec_cmd_xfer_spi_work(struct work_struct *work) +{ + struct cros_ec_xfer_work_params *params; + + params = container_of(work, struct cros_ec_xfer_work_params, work); + params->ret = do_cros_ec_cmd_xfer_spi(params->ec_dev, params->ec_msg); +} + +static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev, + struct cros_ec_command *ec_msg) +{ + struct cros_ec_spi *ec_spi = ec_dev->priv; + struct cros_ec_xfer_work_params params; + + INIT_WORK(¶ms.work, cros_ec_cmd_xfer_spi_work); + params.ec_dev = ec_dev; + params.ec_msg = ec_msg; + + queue_work(ec_spi->high_pri_wq, ¶ms.work); + flush_workqueue(ec_spi->high_pri_wq); + + return params.ret; +} + static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev) { struct device_node *np = dev->of_node; @@ -626,6 +692,31 @@ static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev) ec_spi->end_of_msg_delay = val; } +static void cros_ec_spi_workqueue_release(struct device *dev, void *res) +{ + destroy_workqueue(*(struct workqueue_struct **)res); +} + +static struct workqueue_struct *cros_ec_spi_workqueue_alloc(struct device *dev) +{ + struct workqueue_struct **ptr; + + ptr = devres_alloc(cros_ec_spi_workqueue_release, sizeof(*ptr), + GFP_KERNEL); + if (!ptr) + return NULL; + + *ptr = alloc_workqueue("cros_ec_spi", WQ_HIGHPRI, 1); + if (*ptr == NULL) { + devres_free(ptr); + return NULL; + } + + devres_add(dev, ptr); + + return *ptr; +} + static int cros_ec_spi_probe(struct spi_device *spi) { struct device *dev = &spi->dev; @@ -664,6 +755,10 @@ static int cros_ec_spi_probe(struct spi_device *spi) ec_spi->last_transfer_ns = ktime_get_ns(); + ec_spi->high_pri_wq = cros_ec_spi_workqueue_alloc(dev); + if (!ec_spi->high_pri_wq) + return -ENOMEM; + err = cros_ec_register(ec_dev); if (err) { dev_err(dev, "cannot register EC\n");