From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wolfram Sang Subject: Re: [PATCH 01/12] i2c: remove use of in_atomic() Date: Tue, 16 Apr 2019 12:48:23 +0200 Message-ID: <20190416104823.zabxzdxarzfhdw7m@ninjato> References: <20190403124019.8947-1-wsa+renesas@sang-engineering.com> <20190403124019.8947-2-wsa+renesas@sang-engineering.com> <20190415215546.xr7aftkahcu4ygak@porty> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="va5zqs4dempdj6of" Return-path: Content-Disposition: inline In-Reply-To: <20190415215546.xr7aftkahcu4ygak@porty> Sender: linux-kernel-owner@vger.kernel.org To: Stefan Lengfeld Cc: Wolfram Sang , linux-i2c@vger.kernel.org, linux-renesas-soc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Peter Rosin , linux-omap@vger.kernel.org, linux-tegra@vger.kernel.org, Linus Walleij , Andy Shevchenko List-Id: linux-tegra@vger.kernel.org --va5zqs4dempdj6of Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Stefan, > Tested-by: Stefan Lengfeld Thanks for your comments and testing! I will fix the issues you mentioned and add your tags. > > +/* > > + * We only allow atomic transfers for very late communication, e.g. to= send > > + * the powerdown command to a PMIC. Atomic transfers are a corner case= and not > > + * for generic use! > > + */ > > +static inline bool i2c_in_atomic_xfer_mode(void) > > +{ > > + return system_state > SYSTEM_RUNNING && irqs_disabled(); > > +} >=20 > I agree that this code is a lot better than the previous > 'irqs_disabled() || in_atomic()'. It also makes clear that the atomic > I2C transfers is only meant for late shutdown I2C communcation. >=20 >=20 > After I read the article https://lwn.net/Articles/274695/ again I > nevertheless cannot really say whether the usage of 'irqs_disabled()' is > the theoretical correct approach. The article states explicitly that > only the caller can really decided whether the operation should be > atomic or not. During the discussion with Peter, he stated we need irqs_disabled() because 'system_state > SYSTEM_RUNNING' alone won't do. > Recap from previous discussions: >=20 > But then you have to patch every call site to use either a new function > like 'i2c_transfer_atomic()' or the extend I2C message flags. And mostly > also supported this trough regmap and maybe other translation layers, > which seems unpractical, may breaking existing usages and maybe not > worth the hassle. Yes, I kinda gave up on white-listing late I2C transfers. My hope is that not too many drivers will have an atomic callback, so the WARN will trigger often enough to find late transfers which are inappropriate. Another idea just popping up: Maybe we can improve that even further by first globally disabling atomic transfers. Drivers knowing they need this can then call an I2C core helper to enable them (again globally). Still not perfect as some unwanted late I2C transfers from another driver could slip through, but this should be rare enough. The pro-side is we will detect more unwanted late transfers if support for them is default off. It should be noted that "disabling" means keeping the old behaviour which is: we try the regular transfer but complain about it. Only enabling atomic will make the core quiet. Regards, Wolfram --va5zqs4dempdj6of Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEOZGx6rniZ1Gk92RdFA3kzBSgKbYFAly1svMACgkQFA3kzBSg KbZxBRAApfDnYlIej1zlztDILL+sczBMwOC2hii04cYsWcmCh3D+0h6upc7U3ibE pVykQ6KN5qrvSyu07V1aw2Z/iM2T2rA/MSMKEOAKmnydQLzWGU/VpgKKXWdd6o7J aol3sP+ovgih1qolxcAFw9sW8EfrrkEp8AbcGcfTCALWHfEmcLWyvWcm3kedBZIX br6lJHvLUU4WrKZxhUsuz1VQ/wq9iIcZiG/pEbyBAD/4Yo2Ot2WyiYw4TjScxgvP aGWKC3+GX08/O1PMjnMVTrtGyW2LhrMcQx1w0s3We93KWsSnzRPRQjbNUa0e/uIZ Hes/jSfFV3a8paryTaipp2w2k4u11h3ZbLsRFrN2SXKTv3oJxrh6bcRd2j7dJB4q iwjOQnd5CWRjljZ1xhS2i+RY1TD2ZxspLIS4Z+PAqjE4qTGlioyqkZkH9/aC4Kb2 kbzFu8oOzSVTptTZVslEgCZgjoNB77Z7unFDAiogat71VL1473XT7mdW4sGTT8iy aI60L8EXdYrsde0pcyH23v/iz9e9gV8wk4zo47v48osiPXLw59CmUqr9/8/6jnn/ nAEKyMK5qyQkc3u792gAF9Ex7g1w3QRFBD+6ZIvY3gMeMSoHpUwoysxklQuXWE5e T3OfXcZ3hSYof1AUlO7BfEQ89r/Uil9mtZPrYJ0N6qDTX7eDfY8= =HlAN -----END PGP SIGNATURE----- --va5zqs4dempdj6of-- 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 X-Spam-Level: X-Spam-Status: No, score=-3.0 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS, URIBL_BLOCKED,USER_AGENT_NEOMUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2A36AC10F13 for ; Tue, 16 Apr 2019 10:48:34 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id E8C832075B for ; Tue, 16 Apr 2019 10:48:33 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="PdbskFsj" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E8C832075B Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=the-dreams.de Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender:Content-Type:Cc: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: In-Reply-To:MIME-Version:References:Message-ID:Subject:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=mYs8I0yGKSfSwPalfYEyVNyrHtdYS3am7jic41j8r1M=; b=PdbskFsj/Zp5LpptUbMnm5LnI KwRig83xkUU879MdhubwFEDqMZLt0GANyxyKFnUC3Gdlzr+mYhWg8TaPHUgKir5Y2o5baMRrl1TTA Kc9wrJX2cvY/8Som/AAB6SvTAOL64JDUbNgzOd6/YqQhfvnmQsUVRqp57iiKmb7xNwsP0Qb9p+w+C 3khUEcd3vZm0WqJF3aoMKv4t/GQqtAB3KFaPhfqpwSEtRqbh+0jTnDja2g1CDJFvcsTk2iCmiWf0g P1dxfaSDl0U4BH6rMQcQY3KLt+W4dCo4edUTfX/o7kOsoCeBydSyOGXq7qsim1bG8GuPFXEAoTlnh bM2rbh4gw==; 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 1hGLdc-0008LJ-3A; Tue, 16 Apr 2019 10:48:28 +0000 Received: from sauhun.de ([88.99.104.3] helo=pokefinder.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1hGLdY-0008Kx-9D for linux-arm-kernel@lists.infradead.org; Tue, 16 Apr 2019 10:48:26 +0000 Received: from localhost (p54B332D3.dip0.t-ipconnect.de [84.179.50.211]) by pokefinder.org (Postfix) with ESMTPSA id 888E52C3637; Tue, 16 Apr 2019 12:48:23 +0200 (CEST) Date: Tue, 16 Apr 2019 12:48:23 +0200 From: Wolfram Sang To: Stefan Lengfeld Subject: Re: [PATCH 01/12] i2c: remove use of in_atomic() Message-ID: <20190416104823.zabxzdxarzfhdw7m@ninjato> References: <20190403124019.8947-1-wsa+renesas@sang-engineering.com> <20190403124019.8947-2-wsa+renesas@sang-engineering.com> <20190415215546.xr7aftkahcu4ygak@porty> MIME-Version: 1.0 In-Reply-To: <20190415215546.xr7aftkahcu4ygak@porty> User-Agent: NeoMutt/20170113 (1.7.2) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190416_034824_623474_362614F0 X-CRM114-Status: GOOD ( 14.54 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Andy Shevchenko , Linus Walleij , linux-kernel@vger.kernel.org, linux-renesas-soc@vger.kernel.org, Wolfram Sang , linux-i2c@vger.kernel.org, linux-tegra@vger.kernel.org, linux-omap@vger.kernel.org, Peter Rosin , linux-arm-kernel@lists.infradead.org Content-Type: multipart/mixed; boundary="===============7434660059986634484==" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org --===============7434660059986634484== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="va5zqs4dempdj6of" Content-Disposition: inline --va5zqs4dempdj6of Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Stefan, > Tested-by: Stefan Lengfeld Thanks for your comments and testing! I will fix the issues you mentioned and add your tags. > > +/* > > + * We only allow atomic transfers for very late communication, e.g. to= send > > + * the powerdown command to a PMIC. Atomic transfers are a corner case= and not > > + * for generic use! > > + */ > > +static inline bool i2c_in_atomic_xfer_mode(void) > > +{ > > + return system_state > SYSTEM_RUNNING && irqs_disabled(); > > +} >=20 > I agree that this code is a lot better than the previous > 'irqs_disabled() || in_atomic()'. It also makes clear that the atomic > I2C transfers is only meant for late shutdown I2C communcation. >=20 >=20 > After I read the article https://lwn.net/Articles/274695/ again I > nevertheless cannot really say whether the usage of 'irqs_disabled()' is > the theoretical correct approach. The article states explicitly that > only the caller can really decided whether the operation should be > atomic or not. During the discussion with Peter, he stated we need irqs_disabled() because 'system_state > SYSTEM_RUNNING' alone won't do. > Recap from previous discussions: >=20 > But then you have to patch every call site to use either a new function > like 'i2c_transfer_atomic()' or the extend I2C message flags. And mostly > also supported this trough regmap and maybe other translation layers, > which seems unpractical, may breaking existing usages and maybe not > worth the hassle. Yes, I kinda gave up on white-listing late I2C transfers. My hope is that not too many drivers will have an atomic callback, so the WARN will trigger often enough to find late transfers which are inappropriate. Another idea just popping up: Maybe we can improve that even further by first globally disabling atomic transfers. Drivers knowing they need this can then call an I2C core helper to enable them (again globally). Still not perfect as some unwanted late I2C transfers from another driver could slip through, but this should be rare enough. The pro-side is we will detect more unwanted late transfers if support for them is default off. It should be noted that "disabling" means keeping the old behaviour which is: we try the regular transfer but complain about it. Only enabling atomic will make the core quiet. Regards, Wolfram --va5zqs4dempdj6of Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEOZGx6rniZ1Gk92RdFA3kzBSgKbYFAly1svMACgkQFA3kzBSg KbZxBRAApfDnYlIej1zlztDILL+sczBMwOC2hii04cYsWcmCh3D+0h6upc7U3ibE pVykQ6KN5qrvSyu07V1aw2Z/iM2T2rA/MSMKEOAKmnydQLzWGU/VpgKKXWdd6o7J aol3sP+ovgih1qolxcAFw9sW8EfrrkEp8AbcGcfTCALWHfEmcLWyvWcm3kedBZIX br6lJHvLUU4WrKZxhUsuz1VQ/wq9iIcZiG/pEbyBAD/4Yo2Ot2WyiYw4TjScxgvP aGWKC3+GX08/O1PMjnMVTrtGyW2LhrMcQx1w0s3We93KWsSnzRPRQjbNUa0e/uIZ Hes/jSfFV3a8paryTaipp2w2k4u11h3ZbLsRFrN2SXKTv3oJxrh6bcRd2j7dJB4q iwjOQnd5CWRjljZ1xhS2i+RY1TD2ZxspLIS4Z+PAqjE4qTGlioyqkZkH9/aC4Kb2 kbzFu8oOzSVTptTZVslEgCZgjoNB77Z7unFDAiogat71VL1473XT7mdW4sGTT8iy aI60L8EXdYrsde0pcyH23v/iz9e9gV8wk4zo47v48osiPXLw59CmUqr9/8/6jnn/ nAEKyMK5qyQkc3u792gAF9Ex7g1w3QRFBD+6ZIvY3gMeMSoHpUwoysxklQuXWE5e T3OfXcZ3hSYof1AUlO7BfEQ89r/Uil9mtZPrYJ0N6qDTX7eDfY8= =HlAN -----END PGP SIGNATURE----- --va5zqs4dempdj6of-- --===============7434660059986634484== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --===============7434660059986634484==--