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

Stefan Roese
2012-12-10T18:42:26Z

On 12/10/2012 04:00 PM, Artem Bityutskiy wrote:
> On Fri, 2012-12-07 at 08:22 +0100, Stefan Roese wrote:
>> + /*
>> + * 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.

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