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 C9A86C4332F for ; Tue, 14 Nov 2023 12:27:46 +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=7bZ6DNK/8102C33S7YyQpM832szXUbkItWmzcbP0XBI=; b=aqOihQYw+o1XCp GHIY4j119Pal19x5PORgViW7DQnJ6szlGDNPgV+Ed9NBBJtyOsag2eUA+47BZOIZAgB7Dy8Sqd0yz EzkgS9s3N1z998y61S3faF8QlCXZTs64EMDqs3tYecEz5R2a2iO+uvPLdzJv3AGwe1q1MHIzSGn+M BFemAqZ2b2zl8aMYsWIdaeSjnLnjpktN7Scog55m/uNdZOQjlSDLH96tOwj2k1nN35ssPai8tGylX 4CJdc05W8bE2lXwV4T+/RGht9VTjVKqe5rg5nlEVIMCc0VG/zw5E3KItuP0QPfZ2cepgRdVPXPQkl Qbu4L6pchAY+Ib8JpEGA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1r2sVi-00FtU3-2K; Tue, 14 Nov 2023 12:27:18 +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 1r2sVe-00FtT0-1C for linux-arm-kernel@lists.infradead.org; Tue, 14 Nov 2023 12:27:16 +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=1699964835; x=1731500835; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=JZctFU5m/dXUIFR7HKC2+eJt3NNeiLinqCxMe3AvGR0=; b=QcVkG0DE5bqba+cZr1urOJmkKoBrB4Ndk8Q40pNN/uDtCF6l7Vp9/wYm ltissHgOhfy4rJwYPup+LDuFvChm7x8gkd437vhJblW7Qx9vf4pMk2CnG p1BGr8K+zLHoCQ29AAdjdKW78MWyr6ZDdWRLuoOnY8a29m1xEVGUH4AHH 8B2aC+wvq4fLm9kwMQ7coszSygabp86QL248prCyATZz3SUX7W0vfyAIm EVT4teG0LCIY6YWrQ+BuD1wZm+A4VH59fj01VL7nVISVlhyykbM/06LnU qF/V15abmDcw893W5Bkbkir0cfwzoqSwvc9M0mmbG2vWcS7YicuZMrq25 Q==; X-IronPort-AV: E=Sophos;i="6.03,302,1694728800"; d="scan'208";a="33970392" Received: from vtuxmail01.tq-net.de ([10.115.0.20]) by mx1.tq-group.com with ESMTP; 14 Nov 2023 13:27:09 +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 D602828007F; Tue, 14 Nov 2023 13:27:07 +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: Tue, 14 Nov 2023 13:27:11 +0100 Message-ID: <2912069.e9J7NaK4W3@steina-w> Organization: TQ-Systems GmbH In-Reply-To: <20231110122720.cyxtnpj5k6bip3ok@zenone.zhora.eu> References: <20231107141201.623482-1-alexander.stein@ew.tq-group.com> <20231109091046.4hrvxr7g5imfrykq@zenone.zhora.eu> <20231110122720.cyxtnpj5k6bip3ok@zenone.zhora.eu> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231114_042714_808370_130488AF X-CRM114-Status: GOOD ( 40.38 ) 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, Am Freitag, 10. November 2023, 13:27:20 CET schrieb Andi Shyti: > Alexander, > = > if you... > = > On Thu, Nov 09, 2023 at 10:10:46AM +0100, Andi Shyti wrote: > > On Thu, Nov 09, 2023 at 08:54:51AM +0100, Alexander Stein wrote: > > > Am Donnerstag, 9. November 2023, 00:38:09 CET schrieb Andi Shyti: > > > > On Tue, Nov 07, 2023 at 03:12:01PM +0100, Alexander Stein wrote: > > > > > 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 drive= r") > > > > = > > > > 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 > > > downstream 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 i2c transfer while adding the clock, thus 'prepare_lock' is already > > > locked. Lockdep raises an error both for downstream kernel as well as > > > mainline, both for tlv320aic32x4 or pcf85063. > > = > > yes, if the clock is using the same controller then it's likely > > that you might end up in a deadlock. This is why I like this > > patch and I believe this shouild go in the library at some point. > > = > > But the deadlock is not mentioned in the commit log and lockdep > > doesn't always mean deadlock. > = > ... improve the commit message, reporting the real deadlock case > instead of a lockdep warning and... I've improved the commit message about an actual deadlock. > [...] > = > > > > > @@ -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 failing to read. I would reject EIO as there is no IO > > > transfer for retrieving the clock rate. > > = > > It's definitely not EINVAL as we are failing not because of > > invalid arguments. I thought of EIO because at some point the > > clock rate has been retrieved (or set, thus i/o) and "rate_per" > > updated accordingly. But I agree that's not the perfect value > > either. > > = > > I couldn't think of a better error value, though. > = > ... find a more appropriate error number, I will ack this patch. Thinking about this again, I think EINVAL is an appropriate error code. The parent clock frequency is also an input for the i2c transfer. So if, fo= r = whatever reason, that clock frequency is 0, it is an invalid value (argumen= t). I've checked other drivers what they do if that clock is 0. Unfortunately m= ost = don't consider this case at all. But some do, so e.g. i2c_lpc2k_probe() or = dc_i2c_init_hw() both return EINVAL if the clk or a calculated divider is 0. > If the deadlock you mentioned is a warning from the lockdep, then > please remove the "Fixes:" tag. It's not just a lockdep warning, the deadlock actually happened. Best regards, Alexander > Andi -- = 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