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

[Artem Bityutskiy]

On Mon, 2012-12-10 at 19:40 +0100, Stefan Roese wrote:
> 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.

Would you please do this instead?

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

> 
> > 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()' ?

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

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

-- 
Best Regards,
Artem Bityutskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-mtd/attachments/20121212/8585b665/attachment.sig>