Re: [PATCH 1/4] drivers/base: add managed token devres interfaces

[Tejun Heo <tj@kernel.org>]

Hello, Shuah.

On Mon, May 05, 2014 at 01:16:46PM -0600, Shuah Khan wrote:
> You are right that there is a need for an owner field to indicate who
> has the token. Since the path is very long, I didn't want to use just
> the mutex and keep it tied up for long periods of time. That is the
> reason why I added in_use field that marks it in-use or free. I hold
> the mutex just to change the token status. This is what you are seeing
> on the the following path:

Can you tell me the difference between the following two?

my_trylock1() {
	if (!mutex_trylock(my_lock->lock))
		return -EBUSY;
	was_busy = my_lock->busy;
	my_lock->busy = true;
	mutex_unlock(my_lock->lock);
	return was_busy ? -EBUSY : 0;
}

my_trylock2() {
	mutex_lock();
	was_busy = my_lock->busy;
	my_lock->busy = true;
	mutex_unlock(my_lock->lock);
	return was_busy ? -EBUSY : 0;
}

Now, because the only operation you support is trylock and unlock,
neither will malfunction (as contention on the inner lock can only
happen iff there's another lock holder).  That said, the code doesn't
make any sense.

Here's the problem.  I really don't feel comfortable acking the
submitted code which implements a locking primitive when the primary
author who would probably be the primary caretaker of the code for the
time being doesn't really seem to understand basics of
synchronization.

I'm sure that this could just be from lack of experience but at least
for now I really think this should at least be gated through someone
else who's more knowledgeable and I defintely don't think I'm setting
the bar too high here.

As such, please consider the patches nacked and try to find someone
who can shepherd the code.  Mauro, can you help out here?

Thanks.

-- 
tejun