Re: [PATCH 1/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant

From: Wolfram Sang <wsa@the-dreams.de>
To: Peter Rosin <peda@axentia.se>
Cc: linux-kernel@vger.kernel.org,
	Vadim Pasternak <vadimp@mellanox.com>,
	Michael Shych <michaelsh@mellanox.com>,
	Guenter Roeck <linux@roeck-us.net>,
	Akinobu Mita <akinobu.mita@gmail.com>,
	Jean Delvare <jdelvare@suse.com>,
	linux-i2c@vger.kernel.org
Subject: Re: [PATCH 1/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant
Date: Wed, 27 Jun 2018 09:08:18 +0200	[thread overview]
Message-ID: <20180627070818.4zb3i557qjiicsv7@katana> (raw)
In-Reply-To: <58972586-68FD-4FD6-8B38-E7CA93D2131E@axentia.se>

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

> Because, thinking more about it, the problem with those allocs are not
> related to the locking details; adding another trylock to the mix just
> makes it so much more obvious. I mean, first we would specifically
> handle atomic/irq context with a trylock "documenting" that atomic/irq
> users are welcome to at least try xfers, and then we blattantly break
> the rulez with a GFP_KERNEL alloc...

Yes, thinking more about it, I came to the conclusion that we should not
add trylock to SMBus and keep the requirement to allow sleeping.

True, SMBus is not consistent with I2C then, but actually, I'd prefer
the consistency the other way around: I wish we had a clear statement
that i2c_transfer may sleep. And have a dedicated irqless, non-sleeping
callback for handling the atomic case instead.

I really don't like the commit which introduced the trylock
into i2c_transfer[1]. Its commit message even says: "It is the
reponsability of the caller to ensure that the underlying i2c bus driver
will not sleep either." Which seems broken to me because I can't see how
the caller should do that? And most bus drivers will sleep. But that
commit is upstream for 10 years now, so there are probably users. Which
also are very hard to spot, I am afraid. I wouldn't see a way to convert
them off the top of my head.

[1] cea443a81c9c ("i2c: Support i2c_transfer in atomic contexts")

> Currently, I assume they are only broken when the alloc happens to
> need to do more than is allowed from the given context. Something
> which might or might not be common?

The only regression now would be using smbus_emulated from atomic
context. Which is a bug on the caller side because it cannot know if
smbus_emulated will be used or not. For the non-emulated case, it must
not be atomic anyhow.

So, unless I overlooked something, if we decide to not add trylock to
smbus_xfer, we are all fine?

And I think we should really keep this clean rule of smbus functions
being non-atomic.

D'accord?

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