Re: [PATCH] mtd: cfi_cmdset_0002: Support Persistent Protection Bits (PPB) locking

On 12/12/2012 04:25 PM, Artem Bityutskiy wrote:

On Mon, 2012-12-10 at 19:40 +0100, Stefan Roese wrote:

quoted

On 12/10/2012 04:00 PM, Artem Bityutskiy wrote:

quoted

On Fri, 2012-12-07 at 08:22 +0100, Stefan Roese wrote:

quoted

+       /*
+        * Wait for some time as unlocking of all sectors takes quite long
+        */
+       timeo = jiffies + (2 * HZ);     /* 2s max (un)locking */

Please, use msecs_to_jiffies() instead.

Sure, thats better.

Would you please do this instead?

Yes. I was just waiting for some further comments.

quoted

AFAIK, chip->mutex protects the access to the chip itself. So that
sequences are not interrupted.

I have to admit that I haven't looked into get_chip() so far. It seems
to handle a state machine. Normally (idle state) it will just fall
through (FL_READY).

So it looks like the idea is that you first take the mutex, then call
get_chip() which will wait for the chip becoming really ready, and then
you can safely use it.

Thats it.

quoted

quoted

Why you need to drop the mutex here?

Not sure, that might not be necessary. Copy and past from another loop
in the same file.

Probably from 'get_chip()' ?

Yes, most likely.

quoted

quoted

Why is it not an ABBA deadlock to do this:

Task 1: In the loop above, has chip locked, doing
        mutex_lock(&chip->mutex);

Task 2: done mutex_lock(&chip->mutex), now doing
        ret = get_chip(map, chip, adr + chip->start, FL_LOCKING);

I don't see two different locks/mutexes (only A) here. As get_chip()
does no request any real mutex. Please correct me if I'm wrong.

Right, there is indeed no deadlock.

quoted

In many other places UDELAY() is called:

#define UDELAY(map, chip, adr, usec)  \
do {  \
	mutex_unlock(&chip->mutex);  \
	cfi_udelay(usec);  \
	mutex_lock(&chip->mutex);  \
} while (0)

Why not to use this as well then for consistency?

Okay, will do.

I'll send a new patch version today.

Thanks for the review,
Stefan