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

Artem Bityutskiy
2012-12-12T15:24:20Z

[-- Attachment #1: Type: text/plain, Size: 1863 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]