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

Stefan Roese
2012-12-12T15:44:41Z

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