rt-mutex usage in i2c

rt-mutex usage in i2c

[Sebastian Andrzej Siewior]

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

Re: rt-mutex usage in i2c

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 794 bytes --]

Hi Sebastian,

> - 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.

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


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: rt-mutex usage in i2c

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 956 bytes --]

On Sat, Mar 14, 2015 at 12:27:03PM +0100, Wolfram Sang wrote:
> Hi Sebastian,
> 
> > - 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.
> 
> 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.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: rt-mutex usage in i2c

[Mike Rapoport]

On Sat, Mar 14, 2015 at 1:32 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> On Sat, Mar 14, 2015 at 12:27:03PM +0100, Wolfram Sang wrote:
>> Hi Sebastian,
>>
>> > - 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.

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.


>> 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.
>



-- 
Sincerely yours,
Mike.

Re: rt-mutex usage in i2c

[Sebastian Andrzej Siewior]

On 03/15/2015 08:07 AM, Mike Rapoport wrote:
> On Sat, Mar 14, 2015 at 1:32 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>> On Sat, Mar 14, 2015 at 12:27:03PM +0100, Wolfram Sang wrote:
>>> Hi Sebastian,
>>>
>>>> - 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.
> 
> 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.

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