Message ID | 20230125160142.586358-1-Leif.Middelschulte@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:eb09:0:0:0:0:0 with SMTP id s9csp355173wrn; Wed, 25 Jan 2023 08:10:34 -0800 (PST) X-Google-Smtp-Source: AMrXdXsbrdtWPsYDuyyZJycfLUfHS7xjci9IVBvPDTO2Bmzb2Iu2Z62H8b/54fIngQtDw26vbRFz X-Received: by 2002:aa7:cd69:0:b0:499:bffb:7e58 with SMTP id ca9-20020aa7cd69000000b00499bffb7e58mr47480720edb.20.1674663034476; Wed, 25 Jan 2023 08:10:34 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1674663034; cv=none; d=google.com; s=arc-20160816; b=ivKiSDDxYbMV7wBU28vZ4WmsMr1TNE+TlNVHHN2bRD67dKyoRh7Q+ypPUlsDKsAk+G BDJgSEw8wleH9f61MoKvDA4SegF6n1Wdju9QjA+4fylZ+7KitT6WJiy/HTOONWX8MmmA SqDEEA4wWSCAXqjqTVijkJQd6yFbeeokaDup9E0pwW8ld17oVzKiVCc2hNoezCeMgA/M DJ6r3rAZFElgSb6vJBeFotCABJwNOYTPaX6EoRNQLZpKF80Lk10RVT8Q/Bd77Apjs+6v UZCJ/VYqrrJzyjUO7B0xUkbeoitL5zqJgq0k/6cueYZ8fdALEjRHdq6q1vSmk3cJhI9R QW2g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=s0udQBNiyTVHtXU/kII3EPH6dSydFl+W8baA8J5nZ+U=; b=Ccb2YdK+0hthCG1UBNAAYI+b95X8NbhKqJiQqtB7oXSTlyH5m9uFZl4ANSlGED/Dah PWaKrRFMpM+//4+t7fvWTc0lTV3igLyYL9c1oXR+gBy2Iut/aTWFgVKssyPDfY+yYO6+ dsQZnccGj6Mwg5zAThau132dWLBVq1Xy3XyzVcoSLMdXq36ddvSJcOftYYeCWduEOLXu Zj0sIOF5xaY6r77Xu3GvN9qVDLoyv8oeFdJothqfAR1iRpj1RC7xvoxg1uj607c/8d+G MoqqdN3U8N3hnBHgwnnYwdOl4Ux3JsEE+yw8hBgOiKyilxUmNRc/547M6SHChQJ7EYoL CEOA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=KS8Dhefv; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id f24-20020a05640214d800b00461bde34a12si6419270edx.627.2023.01.25.08.10.10; Wed, 25 Jan 2023 08:10:34 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=KS8Dhefv; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235822AbjAYQCC (ORCPT <rfc822;rust.linux@gmail.com> + 99 others); Wed, 25 Jan 2023 11:02:02 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41548 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235769AbjAYQB7 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 25 Jan 2023 11:01:59 -0500 Received: from mail-ej1-x630.google.com (mail-ej1-x630.google.com [IPv6:2a00:1450:4864:20::630]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3541A4B740; Wed, 25 Jan 2023 08:01:58 -0800 (PST) Received: by mail-ej1-x630.google.com with SMTP id vw16so48875307ejc.12; Wed, 25 Jan 2023 08:01:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=s0udQBNiyTVHtXU/kII3EPH6dSydFl+W8baA8J5nZ+U=; b=KS8DhefvPt+fMOJY0QiaaCruv0yftCwSRrQqF02HoppSGB5KwRt3vC1MqbTPpXHVmE zapEgcqOgea9mLIjGvp/b2VN+ffjPo+2GRlhOCiZBM6CtPxnPO1kDUdx9kQik6qpbnzB V2pt/0wmRKG5pu63J8SBTJGtox0sUy+rQqIBTzT8jA4zs3A+u2af/9ZTXtT8DSImJE8e HDHL2RWgTie4DotqDNxoaJcFxoornznK7LGIjrR5JE7wybuBY0q19EDQtFVxLY5x1Pyo rciGxVHQ9eUVNNYl9iUgCMbIuEUh53c85A18HxrvQSdi1FAQJwO73i7wq8LaiqoFdyzA ccvA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=s0udQBNiyTVHtXU/kII3EPH6dSydFl+W8baA8J5nZ+U=; b=W7+NG8vTBotkN8FxQcFR1M2kdFi1igKpvnJFSqekQgTTX3E2fmAc6ymMXq7YQXz1cz GQo4bhkxKAD3gugVKy4UqxL9YBqOgu42nry0uaJKdcaDgJL1SK0YHfIBLGpX22n1sjSA Z5E1hAbF3cLRSFi+P7uf6cW9xTuJ+GATG0Z8AvPCoMNTs8MUzGZX47uYe8eiXmbTUSGa kvWGN1w2iHfY+E81gH2JTxL0T/Ca5Iy+0SroclB0XXZglEYTxc6C6nO1bse9TTqnEJS+ 3d9DoeoG15oVtzgTU6usNsQV7x7nAKkh4z5VWSWWh8QLdP/Mgi7eF4guOpS9SLStVbI9 +Jbw== X-Gm-Message-State: AFqh2kodBQa34ZhUfl8w1r3K3EFidWEHV0I8wPs2Jn6icLGEH+a2u+7k 5eFsyvX3oJ63E8saFcgPzuo= X-Received: by 2002:a17:907:a80b:b0:86f:220e:94af with SMTP id vo11-20020a170907a80b00b0086f220e94afmr46778569ejc.56.1674662516648; Wed, 25 Jan 2023 08:01:56 -0800 (PST) Received: from DE008-GL-00034.UMK.KLS.zentral ([62.157.169.226]) by smtp.gmail.com with ESMTPSA id lr26-20020a170906fb9a00b008566b807d8asm2543662ejb.73.2023.01.25.08.01.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 25 Jan 2023 08:01:55 -0800 (PST) From: Leif Middelschulte <leif.middelschulte@gmail.com> X-Google-Original-From: Leif Middelschulte <Leif.Middelschulte@gmail.com> To: Thierry Reding <thierry.reding@gmail.com>, =?utf-8?q?Uwe_Kleine-K=C3=B6n?= =?utf-8?q?ig?= <u.kleine-koenig@pengutronix.de>, Shawn Guo <shawnguo@kernel.org>, Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix Kernel Team <kernel@pengutronix.de>, Fabio Estevam <festevam@gmail.com>, NXP Linux Team <linux-imx@nxp.com> Cc: Leif Middelschulte <Leif.Middelschulte@klsmartin.com>, linux-pwm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: [PATCH] pwm: imx27: fix race condition .apply,.get_state Date: Wed, 25 Jan 2023 17:01:42 +0100 Message-Id: <20230125160142.586358-1-Leif.Middelschulte@gmail.com> X-Mailer: git-send-email 2.39.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1756011466064562045?= X-GMAIL-MSGID: =?utf-8?q?1756011466064562045?= |
Series |
pwm: imx27: fix race condition .apply,.get_state
|
|
Commit Message
Leif Middelschulte
Jan. 25, 2023, 4:01 p.m. UTC
From: Leif Middelschulte <Leif.Middelschulte@klsmartin.com> A race condition might occur, ultimately leading to switching off the PWM that is supposed to be turned on. The condition is more likely, if `CONFIG_PWM_DEBUG` is set and the PWM has been enabled before Linux is booted. After writing some value to the register linked to the duty cycle (`MX3_PWMSAR`), the related debug function (`core.c:pwm_apply_state_debug`) reads back (`.get_state`) a wrong value (`0`) as the configured duty cycle. This value is stored as part of a temporary state variable that it subsequently reapplies to the PWM for testing purposes. Which, effectively, turns off the PWM. This patch postpones the return of the `.apply` function until either the written value can be read back as expected or the operation times out. Signed-off-by: Leif Middelschulte <Leif.Middelschulte@klsmartin.com> --- drivers/pwm/pwm-imx27.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
Comments
Hello Leif, first of all thanks for the patch. On Wed, Jan 25, 2023 at 05:01:42PM +0100, Leif Middelschulte wrote: > From: Leif Middelschulte <Leif.Middelschulte@klsmartin.com> > > A race condition might occur, ultimately leading to switching off the > PWM that is supposed to be turned on. > The condition is more likely, if `CONFIG_PWM_DEBUG` is set and the PWM > has been enabled before Linux is booted. As I understand it there is no problem if PWM_DEBUG is off, isn't it? > After writing some value to the register linked to the duty cycle > (`MX3_PWMSAR`), the related debug function > (`core.c:pwm_apply_state_debug`) reads back (`.get_state`) > a wrong value (`0`) as the configured duty cycle. This value is stored > as part of a temporary state variable that it subsequently reapplies > to the PWM for testing purposes. Which, effectively, turns off the PWM. I thought the thing is: Reading PWMSAR yields the duty_cycle that is currently in use. Now if .apply() is called with a new value for PWMSAR and immediately after that .get_state() reads out PWMSAR the previous period (with the previous duty_cycle) probably isn't completed yet and so the old value is read. In this case it wouldn't always be 0 which is read. (Hmm, but with the conversion we had about this issue, my theory sounds wrong?!) Maybe instead of waiting in .apply() (which hurts active consumers), only wait in .get_state() until MX3_PWMSR_FIFOAV drops to zero? Apart from that, the markdown(?) style you use is unusual for kernel commit logs and comments. I'd write: With CONFIG_PWM_DEBUG=y after writing a value to the PWMSAR register in .apply(), the register is read in .get_state(). Unless a period completed in the meantime, this read yields the previously used duty cycle configuration. As the PWM_DEBUG code applies the read out configuration for testing purposes this effectively undoes the intended effect by rewriting the previous hardware state. Best regards Uwe
Hello Uwe, > Am 25.01.2023 um 17:43 schrieb Uwe Kleine-König <u.kleine-koenig@pengutronix.de>: > > Hello Leif, > > first of all thanks for the patch. Thank you for supporting the effort. > > On Wed, Jan 25, 2023 at 05:01:42PM +0100, Leif Middelschulte wrote: >> From: Leif Middelschulte <Leif.Middelschulte@klsmartin.com> >> >> A race condition might occur, ultimately leading to switching off the >> PWM that is supposed to be turned on. >> The condition is more likely, if `CONFIG_PWM_DEBUG` is set and the PWM >> has been enabled before Linux is booted. > > As I understand it there is no problem if PWM_DEBUG is off, isn't it? There is „no problem“ as in: It’s not obvious at the moment/it slumbers. That’s true. > >> After writing some value to the register linked to the duty cycle >> (`MX3_PWMSAR`), the related debug function >> (`core.c:pwm_apply_state_debug`) reads back (`.get_state`) >> a wrong value (`0`) as the configured duty cycle. This value is stored >> as part of a temporary state variable that it subsequently reapplies >> to the PWM for testing purposes. Which, effectively, turns off the PWM. > > I thought the thing is: Reading PWMSAR yields the duty_cycle that is > currently in use. Now if .apply() is called with a new value for PWMSAR > and immediately after that .get_state() reads out PWMSAR the previous > period (with the previous duty_cycle) probably isn't completed yet and > so the old value is read. > > In this case it wouldn't always be 0 which is read. (Hmm, but with the > conversion we had about this issue, my theory sounds wrong?!) This is correct. The value will not always be 0, but the current and/or future value of the sample FIFO. The problem is that it can be quiet hard to tell exactly which value will be read back, as the PWM features a FIFO. The read back value depends on the timing between register read and write. > > Maybe instead of waiting in .apply() (which hurts active consumers), > only wait in .get_state() until MX3_PWMSR_FIFOAV drops to zero? This is what I’ve implemented in v2 of this patch. > > Apart from that, the markdown(?) style you use is unusual for kernel > commit logs and comments. I'd write: > > With CONFIG_PWM_DEBUG=y after writing a value to the PWMSAR > register in .apply(), the register is read in .get_state(). > Unless a period completed in the meantime, this read yields the > previously used duty cycle configuration. As the PWM_DEBUG code > applies the read out configuration for testing purposes this > effectively undoes the intended effect by rewriting the previous > hardware state. Thank you for pointing this out. I have adopted the suggested description In v2 of this patch. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ | Thanks again, Leif
diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c index 29a3089c534c..9b473fe10cb9 100644 --- a/drivers/pwm/pwm-imx27.c +++ b/drivers/pwm/pwm-imx27.c @@ -15,6 +15,7 @@ #include <linux/delay.h> #include <linux/err.h> #include <linux/io.h> +#include <linux/iopoll.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/of.h> @@ -223,7 +224,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, unsigned long long c; unsigned long long clkrate; int ret; - u32 cr; + u32 cr, val; pwm_get_state(pwm, &cstate); @@ -290,6 +291,18 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, if (!state->enabled) pwm_imx27_clk_disable_unprepare(imx); + /* + * According to imx pwm RM the value can be: + * - written at any time + * - only be read, if the pwm is enabled + * Yet it returns a wrong value (i.e. within `pwm_imx27_get_state`) if it is subsequently read (while enabled). + * Apparently it takes the value some cycles to propagate. + * Wait a bit to make sure the right value can be read by other functions, before returning. + */ + ret = readl_relaxed_poll_timeout(imx->mmio_base + MX3_PWMSAR, val, val == duty_cycles, 20000, 300000); + if (ret) + return ret; + return 0; }