On 2018-06-14 17:41, Wolfram Sang wrote:
>
>> So, maybe the easier thing to do is change i2c_lock_adapter to only
>> lock the segment, and then have the callers beneath drivers/i2c/
>> (plus the above mlx90614 driver) that really want to lock the root
>> adapter instead of the segment adapter call a new function named
>> i2c_lock_root (or something like that). Admittedly, that will be
>> a few more trivial changes, but all but one will be under the I2C
>> umbrella and thus require less interaction.
>>
>> Wolfram, what do you think?
>
> It sounds tempting, yet I am concerned about regressions. From that
> point of view, it is safer to introduce i2c_lock_segment() and convert the
> users which would benefit from that. How many drivers would be affected?
Right, there is also the aspect that changing a function like this
might surprise people. Maybe i2c_lock_adapter should be killed and
all callers changed to one of i2c_lock_segment and i2c_lock_root?
It's not that much churn...
Current callers (I didn't hunt the very latest sources, nor -next)
of i2c_lock_adapter are:
drivers/i2c/i2c-core-slave.c
Locks the adapter during (un)registration of the slave. Should
keep locking the root adapter.
drivers/i2c/busses/i2c-brcmstb.c
Locks the adapter during suspend/resume. Should keep locking
the root adapter.
drivers/i2c/busses/i2c-davinci.c
Locks the adapter if/when the CPU frequency changes. Should
keep locking the root adapter.
drivers/i2c/busses/i2c-gpio.c
Locks the adapter while twiddling the lines from debugfs. Should
keep locking the root adapter.
drivers/i2c/busses/i2c-s3c2410.c
Locks the adapter if/when the CPU frequency changes.Should keep
locking the root adapter.
drivers/i2c/busses/i2c-sprd.c
Locks the adapter during suspend/resume. Should keep locking
the root adapter.
drivers/i2c/busses/i2c-tegra.c
Locks the adapter during suspend/resume. Should keep locking
the root adapter.
drivers/i2c/muxes/i2c-mux-pca9541.c
Locks the adapter during probe to make I2C transfers. Should
only need to lock the segment.
drivers/iio/temperature/mlx90614.c
Mentioned previously up-thread, suspend/resume apparently does
nasty things with the bus. Should probably keep locking the root
adapter.
drivers/char/tpm/tpm_i2c_infineon.c
Does a bunch of __i2c_transfer calls inside the locked regions
(and some sleeping). Should only need to lock the segment.
drivers/input/touchscreen/rohm_bu21023.c
Does a couple of __i2c_transfer calls inside the locked region.
Should only need to lock the segment.
drivers/media/dvb-frontends/af9013.c
Does a one or two __i2c_transfer calls inside the locked regions.
Should only need to lock the segment.
drivers/media/dvb-frontends/drxk_hard.c
This one is a bit hairy. It does all sorts of things inside the
locked region. And it is a bit hard to say if the root adapter
really needs to be locked.
drivers/media/dvb-frontends/rtl2830.c
Does regmap accesses inside the locked regions, with the regmap
ops overridden to use __i2c_transfer. Should only need to lock
the segment.
drivers/media/dvb-frontends/tda1004x.c
Does a bunch of __i2c_transfer calls inside the locked region.
Should only need to lock the segment.
drivers/media/tuners/tda18271-common.c
Seems to opens a gate in conjunction with locking the adapter.
So, I'm a bit uncertain what will happen on the other side of
that gate if there is any stray thing happening on the I2C
bus.
drivers/mfd/88pm860x-i2c.c
Does a bunch of adap->algo->master_xfer calls inside the locked
regions. Should only need to lock the segment (and should probably
be using __i2c_transfer).
So, drxk_hard.c and tda18271-common.c are a bit uncertain. However, since
these drivers do make calls to __i2c_transfer from inside their locked
regions, both drivers will deadlock if the chips sit downstream from a
mux-locked I2C mux. And if they don't, locking the segment and the
adapter are equivalent. So, the risk of regressions are nil AFAICT. Famous
last words...
Cheers,
Peter
