In article <[email protected]>, Jason Thorpe <[email protected]> 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
