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

[Stefan Roese <sr-ynQEQJNshbs@public.gmane.org>]

On 12/12/2012 04:25 PM, Artem Bityutskiy wrote:
> 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?

Yes. I was just waiting for some further comments.

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

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

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

Okay, will do.

I'll send a new patch version today.

Thanks for the review,
Stefan