From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 9BC63C4332F for ; Thu, 9 Nov 2023 07:55:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Date:Subject:Cc:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=icy4r2Xk6GR+uoSjYVi7dX30J5ng3JH9kyliswCeTLE=; b=MJlsDW2fHfa5e7 vIAzEv6k4KX6EzHuTifhwtMh5LXWxDfJ2FIVKmKh95EIlzeQ/uApdQxvqJRxJtTsLvqfz0KH3Ptp3 VdpXw4njSOj5utH/7oayIAMDIPbWeZkfZ2hxbewBZ78pcRRHyIWOeOkZyiU2AXquf8Y3V11v6kRY/ fn2qtHZW67IUMhwXWHAKF4oxMmRPEqZFYfVd69Xi4c1w0FmBY9Hw0K2v5Hc2zR0S+9ggPCOuP2xqU R0q9Jj9ApCUJDDwcQ0s8DG8xNeeS/VDxfLPTJRu+Khi005pKav80GX5P1hLbFQvbJJuLVYlak/BSP 6r9wwWkZ9dhOL5tdjXSQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1r0zsN-005Y6G-0u; Thu, 09 Nov 2023 07:54:55 +0000 Received: from mx1.tq-group.com ([93.104.207.81]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1r0zsI-005Y4S-39 for linux-arm-kernel@lists.infradead.org; Thu, 09 Nov 2023 07:54:53 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tq-group.com; i=@tq-group.com; q=dns/txt; s=key1; t=1699516491; x=1731052491; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=dRRDqeVkScDEL1dmC45t7pgui+PHFKft2w9/ONvXEdE=; b=ZDrrCrBNc68krxESArB9bUPMWOgPvGwFsSZ6Isiq3sWbiwcxYlKcT7ZM IWR7emfzAC48FpAZyRZ+kROBWy9IcQqKsaOb7Qef7yExcsHgLoePuPN4p DpAuWCLf9nWmMH2eIJmD0o0vDI+1pbppK5SOMtmRd+0xwnDgpdmjWXCND 62BdsrORQXo8gWWhnOZ+0m54KFwht21xnSC+KBouYv86PGaqUwn4IM1Ln jujaruwpPWjdVVWNkF5Tyc2rA0Wddzz2NtrxNzx94Tm0bWSa9GkcE/yCo o4LeGcaWc8wEtw17YlazSA4AXXGVZfn5I4KorKMDejJubGE67wNOh97gs Q==; X-IronPort-AV: E=Sophos;i="6.03,288,1694728800"; d="scan'208";a="33889327" Received: from vtuxmail01.tq-net.de ([10.115.0.20]) by mx1.tq-group.com with ESMTP; 09 Nov 2023 08:54:48 +0100 Received: from steina-w.localnet (steina-w.tq-net.de [10.123.53.18]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by vtuxmail01.tq-net.de (Postfix) with ESMTPSA id A75FB28007F; Thu, 9 Nov 2023 08:54:48 +0100 (CET) From: Alexander Stein To: Andi Shyti Cc: Dong Aisheng , Shawn Guo , Sascha Hauer , Fabio Estevam , Wolfram Sang , Alexander Sverdlin , Pengutronix Kernel Team , NXP Linux Team , linux-i2c@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v7 1/1] i2c: lpi2c: use clk notifier for rate changes Date: Thu, 09 Nov 2023 08:54:51 +0100 Message-ID: <3597042.R56niFO833@steina-w> Organization: TQ-Systems GmbH In-Reply-To: <20231108233809.u3nvxlttmts6tj2m@zenone.zhora.eu> References: <20231107141201.623482-1-alexander.stein@ew.tq-group.com> <20231108233809.u3nvxlttmts6tj2m@zenone.zhora.eu> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231108_235451_413890_09FE7972 X-CRM114-Status: GOOD ( 41.97 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Andi, thanks for the feedback. Am Donnerstag, 9. November 2023, 00:38:09 CET schrieb Andi Shyti: > Hi Alexander, > = > On Tue, Nov 07, 2023 at 03:12:01PM +0100, Alexander Stein wrote: > > From: Alexander Sverdlin > > = > > Instead of repeatedly calling clk_get_rate for each transfer, register > > a clock notifier to update the cached divider value each time the clock > > rate actually changes. > > There is also a lockdep splat if a I2C based clock provider needs to > > access the i2c bus while in recalc_rate(). "prepare_lock" is about to > > be locked recursively. > > = > > Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver") > = > What's the exact fix here? Is it the lockdep warning? Is it > actually causing a real deadlock? We've seen actual deadlocks on our imx8qxp based hardware using a downstrea= m = kernel, so we have implemented as similar fix [1]. This happened using = tlv320aic32x4 audio codec. The backtrace is similar, a i2c based clock provider is trying t issue an i= 2c = transfer while adding the clock, thus 'prepare_lock' is already locked. Lockdep raises an error both for downstream kernel as well as mainline, bot= h = for tlv320aic32x4 or pcf85063. [1] https://github.com/tq-systems/linux-tqmaxx/commit/ b0339ff83a979f2ea066012b9209ea7c54f2b4e7 > > Signed-off-by: Alexander Sverdlin > > Reviewed-by: Alexander Stein > > Signed-off-by: Alexander Stein > > --- > > Changes in v7: > > * Use dev_err_probe > > * Reworked/Shortened the commit message > > = > > It is not about saving CPU cycles, but to avoid locking the clk subsys= tem > > upon each transfer. > > = > > drivers/i2c/busses/i2c-imx-lpi2c.c | 40 +++++++++++++++++++++++++++++- > > 1 file changed, 39 insertions(+), 1 deletion(-) > > = > > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c > > b/drivers/i2c/busses/i2c-imx-lpi2c.c index 678b30e90492a..3070e605fd8ff > > 100644 > > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c > > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c > > @@ -5,6 +5,7 @@ > > = > > * Copyright 2016 Freescale Semiconductor, Inc. > > */ > > = > > +#include > > = > > #include > > #include > > #include > > = > > @@ -99,6 +100,8 @@ struct lpi2c_imx_struct { > > = > > __u8 *rx_buf; > > __u8 *tx_buf; > > struct completion complete; > > = > > + struct notifier_block clk_change_nb; > > + atomic_t rate_per; > > = > > unsigned int msglen; > > unsigned int delivered; > > unsigned int block_data; > > = > > @@ -197,6 +200,20 @@ static void lpi2c_imx_stop(struct lpi2c_imx_struct > > *lpi2c_imx)> = > > } while (1); > > = > > } > > = > > +static int lpi2c_imx_clk_change_cb(struct notifier_block *nb, > > + unsigned long action, void *data) > > +{ > > + struct clk_notifier_data *ndata =3D data; > > + struct lpi2c_imx_struct *lpi2c_imx =3D container_of(nb, > > + struct = lpi2c_imx_struct, > > + = clk_change_nb); > > + > > + if (action & POST_RATE_CHANGE) > > + atomic_set(&lpi2c_imx->rate_per, ndata->new_rate); > > + > > + return NOTIFY_OK; > > +} > > + > > = > > /* CLKLO =3D I2C_CLK_RATIO * CLKHI, SETHOLD =3D CLKHI, DATAVD =3D CLKH= I/2 */ > > static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx) > > { > > = > > @@ -207,7 +224,7 @@ static int lpi2c_imx_config(struct lpi2c_imx_struct > > *lpi2c_imx)> = > > lpi2c_imx_set_mode(lpi2c_imx); > > = > > - clk_rate =3D clk_get_rate(lpi2c_imx->clks[0].clk); > > + clk_rate =3D atomic_read(&lpi2c_imx->rate_per); > > = > > if (!clk_rate) > > = > > return -EINVAL; > = > Doesn't seem like EINVAL, defined as "Invalid argument", is the > correct number here. As we are failing to read the clock rate, do > you think EIO is better? Well, this is already the current error code. In both the old and new code = I = would consider this error case as 'no clock rate provided' rather than fail= ing = to read. I would reject EIO as there is no IO transfer for retrieving the = clock rate. > > @@ -590,6 +607,27 @@ static int lpi2c_imx_probe(struct platform_device > > *pdev)> = > > if (ret) > > = > > return ret; > > = > > + lpi2c_imx->clk_change_nb.notifier_call =3D lpi2c_imx_clk_change_cb; > > + ret =3D devm_clk_notifier_register(&pdev->dev, lpi2c_imx->clks[0].clk, > > + &lpi2c_imx->clk_change_nb); > > + if (ret) > > + return dev_err_probe(&pdev->dev, ret, > > + "can't register peripheral clock = notifier\n"); > = > can't we fall back to how things were instead of failing the > probe? But I'm not sure it would remove the lockdep warning, > though. I can live with it. I don't see a reason why you would want to continue if = devm_clk_notifier_register() failed. It's either ENOMEM, EINVAL (if you pas= s = NULL for clk or notifier block) or EEXIST if registered twice. So in realit= y = only ENOMEM is possible, but then things went south already. Best regards, Alexander > > + /* > > + * Lock the clock rate to avoid rate change between clk_get_rate() = and > > + * atomic_set() > > + */ > > + ret =3D clk_rate_exclusive_get(lpi2c_imx->clks[0].clk); > > + if (ret) > > + return dev_err_probe(&pdev->dev, ret, > > + "can't lock I2C peripheral clock = rate\n"); > > + > > + atomic_set(&lpi2c_imx->rate_per, clk_get_rate(lpi2c_imx- >clks[0].clk)); > > + clk_rate_exclusive_put(lpi2c_imx->clks[0].clk); > > + if (!atomic_read(&lpi2c_imx->rate_per)) > > + return dev_err_probe(&pdev->dev, -EINVAL, > > + "can't get I2C peripheral clock = rate\n"); > > + > = > Not a bad patch, would be nice if all the above was provided by > the library so that other drivers can use it. > = > Andi > = > > pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT); > > pm_runtime_use_autosuspend(&pdev->dev); > > pm_runtime_get_noresume(&pdev->dev); -- = TQ-Systems GmbH | M=FChlstra=DFe 2, Gut Delling | 82229 Seefeld, Germany Amtsgericht M=FCnchen, HRB 105018 Gesch=E4ftsf=FChrer: Detlef Schneider, R=FCdiger Stahl, Stefan Schneider http://www.tq-group.com/ _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel