Re: [PATCH] i2c bus acquire / release cleanup, forbid i2c access in hard interrupt context

[christos%astron.com@localhost (Christos Zoulas)]

In article <D6C0A003-FC92-49D3-8F5D-AD895BC61B2E%me.com@localhost>,
Jason Thorpe  <thorpej%me.com@localhost> wrote:
>-=-=-=-=-=-
>
>The i2c subsystem has a mechanism by which drivers for i2c-connected
>devices "acquire" and "release" the i2c bus for their own exclusive
>access while performing a set of operations.  Historically, it has been
>the responsibility of the back-end controller drivers to implement this
>mechanism, which generally speaking, has been implemented as a simple
>mutex.
>
>This has worked out fine, for the most part, but but one of the features
>of the i2c subsystem is that a driver that's using it can request that
>the operation run in a "polled" mode, which has historically meant
>"don't sleep, busy wait for completions, don't use interrupts".
>
>Unfortunately, the way this "polled" mode was implemented in some
>back-end drivers' acquire / release functions was wildly inconsistent,
>and in some cases, just plain broken.  During my audit, I found
>instances where drivers just didn't bother with the mutex at all... some
>would "trylock" ... some would busy-loop around a "trylock".
>
>In sort, it was kind of a hot mess.
>
>In one case (the "ihid" hid-over-i2c driver), a driver used "polled"
>mode in order to perform i2c bus access from a hard interrupt context. 
>When combined with some of the mistakes noted above, you've got recipe
>for deadlocks.
>
>My solution to this is two-fold:
>
>1- Fix the hid-over-i2c driver to not perform i2c access in hard
>interrupt context.  That required some additional capabilities from the
>ACPI interrupt API, which is chronicled here (note there are still some
>messages on that thread that are not on the web archive yet at the time
>I write this email, but they should land soon):
>
>	http://mail-index.netbsd.org/port-i386/2019/08/08/msg003804.html
>	(cross-posted to port-amd64 and port-i386)
>
>2- Centralize the logic for bus acquire / release.  This is now all
>performed in iic_acquire_bus() and iic_release_bus().  The I2C_F_POLL
>case is handled with a "trylock" operation, and EBUSY is returned if
>that fails.  The rationale is that basically NO ONE should be using
>polled mode.  It is used automatically by the i2c subsystem when the
>system is "cold", and honestly, I doubt anyone will be able to convince
>me that any other use of I2C_F_POLL is legitimate.  The acquire /
>release hooks for the back-end drivers remain in case they need to power
>on the controller or something like that, but the hook is now entirely
>optional just for those cases.
>
>Access to i2c bus in hard interrupt context is now forbidden.  Allowing
>it makes synchronization much harder and more expensive, and it's not
>something that can safely be done with all i2c controller back-ends
>(e.g. the type that might share registers with another type of device on
>the system).
>
>The end result -- there is a bunch of redundant (and incorrect) code
>that simply gets deleted.  And every driver has consistent behavior.
>
>Patch attached.  I've *at least* compile-tested these changes just about
>everywhere, and I have done pretty thorough testing of the MI i2c
>portions on a Raspberry Pi (where I also took the opportunity to rewrite
>the driver for the bcm2835 i2c controller to work as an interrupt-driven
>state machine rather than busy-waiting everywhere, which improved system
>responsiveness when transferring lots of data over i2c on a single-cpu
>Pi).  I also found some bugs in the Exynos and Tegra i2c controller
>drivers, but I don't have the hardware to test my fixes for those.
>

LGTM, thanks for doing this.

christos