rt-mutex usage in i2c
Subsystems (2)
I have two questions:
- em28xx_i2c_xfer() in drivers/media/usb/em28xx/em28xx-i2c.c takes a rt_mutex lock. This struct i2c_algorithm's ->master_xfer callback. Why does it take an extra lock since it looks to me that it is protected by struct i2c_adapter's bus_lock already.
- i2c_transfer() has this piece:
2091 if (in_atomic() || irqs_disabled()) {
2092 ret = i2c_trylock_adapter(adap);
is this irqs_disabled() is what bothers me and should not be there. pxa does a spin_lock_irq() which would enable interrupts on return / too early.
mxs has a wait_for_completion() which needs irqs enabled _and_ makes in_atomic() problematic, too. I have't checked other drivers but the commit, that introduced it, does not explain why it is required. So _should_ this be invoked from an interrupt handler (for instance), then it would record the wrong process as the lock owner. This isn't problematic unless on SMP the owner gets boosted because a process with a higher priority needs the lock.
Sebastian
Hi Sebastian,
Quoted 10 lines
-
i2c_transfer()has this piece:
2091 if (in_atomic()||irqs_disabled()) {
2092 ret =i2c_trylock_adapter(adap);is this
irqs_disabled()is what bothers me and should not be there. pxa does aspin_lock_irq()which would enable interrupts on return / too early.
mxs has await_for_completion()which needs irqs enabled _and_ makesin_atomic()problematic, too. I have't checked other drivers but the commit, that introduced it, does not explain why it is required.
I haven't really looked into it, but a quick search gave me this thread explaining the intention of the code in question:
http://lists.lm-sensors.org/pipermail/i2c/2007-November/002268.html
Regards,
Wolfram
Quoted 13 lines · Wolfram Sang
Hi Sebastian,
Quoted 10 lines
-
i2c_transfer()has this piece:
2091 if (in_atomic()||irqs_disabled()) {
2092 ret =i2c_trylock_adapter(adap);is this
irqs_disabled()is what bothers me and should not be there. pxa does aspin_lock_irq()which would enable interrupts on return / too early.
mxs has await_for_completion()which needs irqs enabled _and_ makesin_atomic()problematic, too. I have't checked other drivers but the commit, that introduced it, does not explain why it is required.
Quoted 9 lines
I haven't really looked into it, but a quick search gave me this thread explaining the intention of the code in question:
http://lists.lm-sensors.org/pipermail/i2c/2007-November/002268.html
Regards,
Wolfram
And adding a recent mail address from Mike to cc.
Quoted 7 lines · Wolfram Sang
Quoted 6 lines · Wolfram Sang
Hi Sebastian,
Quoted 4 lines
-
i2c_transfer()has this piece:
2091 if (in_atomic()||irqs_disabled()) {
2092 ret =i2c_trylock_adapter(adap);
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
is this
irqs_disabled()is what bothers me and should not be there. pxa does aspin_lock_irq()which would enable interrupts on return / too early.
mxs has await_for_completion()which needs irqs enabled _and_ makesin_atomic()problematic, too. I have't checked other drivers but the commit, that introduced it, does not explain why it is required.
That was some time ago, but as far as I remember, PIO in i2c_pxa was required to enable communication with PMIC in interrupt context.
Quoted 12 lines
Quoted 9 lines
I haven't really looked into it, but a quick search gave me this thread explaining the intention of the code in question:
http://lists.lm-sensors.org/pipermail/i2c/2007-November/002268.html
Regards,
Wolfram
And adding a recent mail address from Mike to cc.
Signature · 2 lines
Sincerely yours, Mike.
A message that is not in the archive (sent off-list or not yet received): CABpLfoiQg1smiebL0=nWX4Sp1H+XD9VViUqGk13gRcfdAwkFoA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 15 lines · Mike Rapoport
Quoted 7 lines · Wolfram Sang
Quoted 6 lines · Wolfram Sang
Hi Sebastian,
Quoted 4 lines
-
i2c_transfer()has this piece:
2091 if (in_atomic()||irqs_disabled()) {
2092 ret =i2c_trylock_adapter(adap);Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
is this
irqs_disabled()is what bothers me and should not be there. pxa does aspin_lock_irq()which would enable interrupts on return / too early.
mxs has await_for_completion()which needs irqs enabled _and_ makesin_atomic()problematic, too. I have't checked other drivers but the commit, that introduced it, does not explain why it is required.
Quoted 2 lines
That was some time ago, but as far as I remember, PIO in
i2c_pxawas required to enable communication with PMIC in interrupt context.
Let me add one thing I forgot: the locking is using raw locks which are not irq safe. It usually works. But. If the wait_lock is hold during the unlock's slow path (that means there is no owner but the owner field is not yet NULL) and the interrupt handler gets here with a try_lock attempt then and it will spin forever on the wait_lock.
I will try to lookup the threads later…
Sebastian