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.
diff --git a/sys/arch/alpha/pci/tsciic.c b/sys/arch/alpha/pci/tsciic.c
index 37110949d564..29507daa8c95 100644
--- a/sys/arch/alpha/pci/tsciic.c
+++ b/sys/arch/alpha/pci/tsciic.c
@@ -45,8 +45,6 @@ __KERNEL_RCSID(0, "$NetBSD: tsciic.c,v 1.1 2014/02/21
12:23:30 jdc Exp $");
#include
/* I2C glue */
-static int tsciic_acquire_bus(void *, int);
-static void tsciic_release_bus(void *, int);
static int tsciic_send_start(void *, int);
static int tsciic_send_stop(void *, int);
static int tsciic_initiate_xfer(void *, i2c_addr_t, int);
@@ -77,17 +75,13 @@ tsciic_init(device_t self) {
struct tsciic_softc *sc = device_private(self);
struct i2cbus_attach_args iba;
- mutex_init(>sc_buslock, MUTEX_DEFAULT, IPL_NONE);
-
+ iic_tag_init(>sc_i2c);
sc->sc_i2c.ic_cookie = sc;
- sc->sc_i2c.ic_acquire_bus = tsciic_acquire_bus;
- sc->sc_i2c.ic_release_bus = tsciic_release_bus;
sc->sc_i2c.ic_send_start = tsciic_send_start;
sc->sc_i2c.ic_send_stop = tsciic_send_stop;
sc->sc_i2c.ic_initiate_xfer = tsciic_initiate_xfer;
sc->sc_i2c.ic_read_byte = tsciic_read_byte;
sc->sc_i2c.ic_write_byte = tsciic_write_byte;
- sc->sc_i2c.ic_exec = NULL;
memset(, 0, sizeof(iba));
iba.iba_tag = >sc_i2c;
@@ -128,23 +122,6 @@ tsciicbb_read(void *cookie)
}
/* higher level I2C stuff */
-static int
-tsciic_acquire_bus(void *cookie, int flags)
-{
- struct tsciic_softc *sc = cookie;
-
- mutex_enter(>sc_buslock);
- return 0;
-}
-
-static void
