Received: by 2002:a25:4158:0:0:0:0:0 with SMTP id o85csp2577683yba; Mon, 15 Apr 2019 14:56:49 -0700 (PDT) X-Google-Smtp-Source: APXvYqzfj+iY9yuP4NGZeRuwmox0AA5ApK49fFNBGSdKioz8cN373mmPpd4FAytqDp9iS0cZ4bJ1 X-Received: by 2002:a62:69c2:: with SMTP id e185mr77740099pfc.119.1555365409354; Mon, 15 Apr 2019 14:56:49 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1555365409; cv=none; d=google.com; s=arc-20160816; b=R9Yo8hgQoBb++BcDSyXM1ZXYP9nuSy/8Rc27Q7k3siB2wDWkCbvEzoniNQN3OaedrT 0GAtv2srRK/kDEQUSO9ebVyZJsSKUIz1x8h2eraGE7oMMD8HkkTsnFW8tRQ7aFIEJa4Z 1Hr7oxnIsEGc6tK4gQI4Kuj+HiaKWlztqjyoFxqI1CrZoBsgI/U0QY/ij6hqmkCU0a9B R9Z1eBqCBmMGztJ4oHpHToYx9PhH/U6VedxJgsMfMDt/tqdGjeme8KUvOMsDPTlgcuVV wAqOS24IFza34jmi3e/ofJeLzodB19S0dojmXtq4DqFrXQMBSuvnAxKGB/mBYF8heuRs rlaA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date; bh=1bwZkZ1dT0k9LSwngTpVBJj1h4lFccMoTpp7daBXlsc=; b=IXSEVVBjrbSdfxZSH8UVKi7G798FRqTZ7qsKbrgKazZk3N5rftHvfljqiamZmY6v4Z /NoZqgHLrZQubKPyJy6tXmttII8sBm/r1zlWEFWRdOlXuUhI19iYwWXKj8RPQ0DO4+qi pWcI2/76SXwjqPNPXZ4et2ZknEk6PMyW83uDkSgWZzgpvq6PgOEIiep2ok/2v8b9drD1 ZMWBZblqjdO/FfMCUedf3hiFpxKeUm0e5CxwtBMcICpZLfOs/P59hncMdEwH7KHDcjBZ p98DqftOeeZ44oFz6X0DR7AVOoFjoHyQreOym5WA+5rDRgoXAQAQdeDLYYZcLT6MkfVb lu4Q== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 32si42551120pgv.387.2019.04.15.14.56.31; Mon, 15 Apr 2019 14:56:49 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727895AbfDOVz4 (ORCPT + 99 others); Mon, 15 Apr 2019 17:55:56 -0400 Received: from stcim.de ([78.46.90.227]:43648 "EHLO stcim.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726609AbfDOVzz (ORCPT ); Mon, 15 Apr 2019 17:55:55 -0400 Received: from xdsl-89-0-59-235.nc.de ([89.0.59.235] helo=porty) by stcim with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1hG9Zq-0000zf-IS; Mon, 15 Apr 2019 23:55:46 +0200 Date: Mon, 15 Apr 2019 23:55:46 +0200 From: Stefan Lengfeld To: Wolfram Sang Cc: 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 Subject: Re: [PATCH 01/12] i2c: remove use of in_atomic() Message-ID: <20190415215546.xr7aftkahcu4ygak@porty> References: <20190403124019.8947-1-wsa+renesas@sang-engineering.com> <20190403124019.8947-2-wsa+renesas@sang-engineering.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20190403124019.8947-2-wsa+renesas@sang-engineering.com> X-PGP-Key: https://stefanchrist.eu/personal/Stefan_Lengfeld_0xE44A23B289092311.asc User-Agent: NeoMutt/20180716 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Wolfram, On Wed, Apr 03, 2019 at 02:40:08PM +0200, Wolfram Sang wrote: > Commit cea443a81c9c ("i2c: Support i2c_transfer in atomic contexts") > added in_atomic() to the I2C core. However, the use of in_atomic() > outside of core kernel code is discouraged and was already[1] when this > code was added in early 2008. The above commit was a preparation for > commit b7a3670131c7 ("i2c-pxa: Add polling transfer"). Its commit > message says explicitly it was added "for cases where I2C transactions > have to occur at times interrup[t]s are disabled". So, the intention was > 'disabled interrupts'. This matches the use cases for atomic I2C > transfers I have seen so far: very late communication (mostly to a PMIC) > to powerdown or reboot the system. For those cases, interrupts are > disabled then. It doesn't seem that in_atomic() adds value. > > After a discussion with Peter Zijlstra[2], we came up with a better set > of conditionals to match the use case. > > The I2C core will soon gain an extra callback into bus drivers > especially for atomic transfers to make them more generic. The code > deciding which transfer to use (atomic/non-atomic) should mimic the > behaviour which locking to use (trylock/lock). This is why we add a > helper for it. > > [1] https://lwn.net/Articles/274695/ > [2] http://patchwork.ozlabs.org/patch/1067437/ > > Signed-off-by: Wolfram Sang Tested-by: Stefan Lengfeld snipped > diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h > index 37576f50fe20..9d8526415b26 100644 > --- a/drivers/i2c/i2c-core.h > +++ b/drivers/i2c/i2c-core.h > @@ -29,6 +29,16 @@ extern int __i2c_first_dynamic_bus_num; > > int i2c_check_7bit_addr_validity_strict(unsigned short addr); > > +/* > + * 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(); > +} 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. 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. Recap from previous discussions: 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. For me this patch seems good and I don't know better. Kind regards, Stefan