Re: [PATCH 01/12] i2c: remove use of in_atomic()

From: Wolfram Sang <wsa@the-dreams.de>
To: Stefan Lengfeld <contact@stefanchrist.eu>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>,
	linux-i2c@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Peter Rosin <peda@axentia.se>,
	linux-omap@vger.kernel.org, linux-tegra@vger.kernel.org,
	Linus Walleij <linus.walleij@linaro.org>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Subject: Re: [PATCH 01/12] i2c: remove use of in_atomic()
Date: Tue, 16 Apr 2019 12:48:23 +0200	[thread overview]
Message-ID: <20190416104823.zabxzdxarzfhdw7m@ninjato> (raw)
In-Reply-To: <20190415215546.xr7aftkahcu4ygak@porty>

[-- Attachment #1: Type: text/plain, Size: 2384 bytes --]

Hi Stefan,

> Tested-by: Stefan Lengfeld <contact@stefanchrist.eu>

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();
> > +}
> 
> 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.

During the discussion with Peter, he stated we need irqs_disabled()
because 'system_state > SYSTEM_RUNNING' alone won't do.

> 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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]