[i2c] [v4l-dvb-maintainer] Re: [PATCH] I2C && DVB: fix recursive locking lockped warning in i2c_transfer()

[Jiri Kosina]

On Sun, 15 Oct 2006, Jean Delvare wrote:

> > Lockdep is tracking the classes of locks (not the individual instances) to 
> > be able to find possibility of wrong locking as soon as mathematically 
> > possible, that there might be cyclic dependency between locks. Here, the 
> > class of the lock is "adap->bus_lock". This class has cyclic dependency 
> > (because of recursion), so lockdep yields the warning I have posted in my 
> > original mail.
> There is no cycle here. One instance of i2c_adapter is relying on
> another instance of i2c_adapter, and that's it. You (or lockdep)
> decided that the lock of the first i2c_adapter was somehow related to
> the lock of the second i2c_adapter, while they are not.
> Considering all i2c_adapter.bus_lock as a "class of locks" doesn't make
> any sense to me. There is no relation between them in the general case.

Hi Jean,

(I have added Ingo (lockdep author) to CC).

Well, it's basically the design of lockdep validator - please see 
Documentation/lockdep.txt for details (the principe of lock classes is 
described at the very beginning). The fact that there is no relation 
between two instances of the same class has to be explicitly told to 
lockdep, othwerwise it won't be possible to maintain a proof of 
locking correctness.

> > As the locking is fine in this case (we have nesting semantics of locks 
> > here, and every time the lock order in the source is correct), we have to 
> > annotate this fact for lockdep, so that we get rid of the warning. This is 
> > done by my patch, which belongs to the cathegory "shut up lockdep patches" 
> > and shoudl be applied (or any variant of it), so that people will not be 
> > re-investigating this particular warning, to check whether it is valid or 
> > not.
> This doesn't fly, sorry. Your "annotations" are adding one member to
> one of the core structure of the i2c subsystem, and some code in the
> specific device drivers. This means increased memory usage and slower
> code (not by much, granted, but still), to solve a problem which
> doesn't exist in the first place. This isn't my definition of an
> annotation.

I can see your point. On the other hand, if we don't teach lockdep that 
this locking is correct, the warning will be there forever and people will 
probably be sending reports again and again.

To the question of adding member to the core structure of i2c subsystem - 
I agree that this might be unneccessary overhead, and that was the main 
purpose of sending this mail (and prefixing the question by "RFC") - if 
anyone could see any better way. We could also put some #ifdefs in the 
code to inject the locking "anotations" only in cases the kernel is 
compiled with lockdep, but I don't think I like it.

> I don't know who is behind this theory of "classes of locks", but I
> fail to see how it can work at all. You can't let your validation system
> guess which locks are related and which locks are not. And I can't see

By definition all locks of the same class are related. In special cases of 
lock nesting, it's necessary to use mutex_lock_nested(), to express the 
nesting semantics of locks. Otherwise lockdep can't maintain the proof of 
locking correctness, as it sees possibility of deadlock within a lock 
class. See Documentation/lockdep.txt

> any reason why per-instance locks of a given structure type would be
> more related to each other than two random locks, so why decide they
> constitute a class?

Two 'identical' locks are in the same class (in this case adap->bus_lock, 
no matter ow many instances are there). Lockdep then maintains graphs of 
relations between lock classes, to detect problem as soon as it is 
mathematically possible. If cycle is detected, it violates the proof of 
correctness. If the cycle is OK, lockdep has to be taught of this special 
nesting semantics. That's it.

> So I'm sorry but I can't accept your patch. Your lockdep thingie is
> somehow making wrong assumptions, and this is what needs to be fixed
> here.

Lockdep is correct, the only thing is to decide how to annotate the 
recursive locking in a non-violent way to the DVB/I2C code, which was the 
reason I was sending the original question.

Thanks a lot,

-- 
Jiri Kosina