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

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

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.

quoted

+       for (;;) {
+               if (chip_ready(map, adr))
+                       break;
+
+               if (time_after(jiffies, timeo)) {
+                       printk(KERN_ERR "Waiting for chip to be ready timed out.\n");
+                       ret = -EIO;
+                       break;
+               }
+               mutex_unlock(&chip->mutex);
+               cfi_udelay(1);
+               mutex_lock(&chip->mutex);
+       } 

Would you please educate me a bit and explain what is protected by
'chip->mutex' and by 'get_chip()'.

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

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.

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.

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)

So dropping this lock seems to be quite common in this driver.

Thanks,
Stefan