Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Chris Packham <Chris.Packham@alliedtelesis.co.nz>
To:     Richard Weinberger <richard.weinberger@gmail.com>
CC:     David Woodhouse <dwmw2@infradead.org>,
        Brian Norris <computersforpeace@gmail.com>,
        Marek Vasut <marek.vasut@gmail.com>,
        "Miquel Raynal" <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        "linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] mtd: concat: implement _is_locked mtd operation
Thread-Topic: [PATCH 2/2] mtd: concat: implement _is_locked mtd operation
Thread-Index: AQHVEDJo0iBq93PH/EeJquPgUtfeew==
Date:   Wed, 22 May 2019 22:16:44 +0000
Message-ID: <82de06c4122b4b958b5c840aa2166dd8@svr-chch-ex1.atlnz.lc>
References: <20190522000753.13300-1-chris.packham@alliedtelesis.co.nz>
 <20190522000753.13300-2-chris.packham@alliedtelesis.co.nz>
 <CAFLxGvy2c9KV1CyoFaD76jvThfPiotqfoeNchqjGcDp+uHie7Q@mail.gmail.com>
 <0c59bcd6c866429cb9727f787b7f61ce@svr-chch-ex1.atlnz.lc>
 <CAFLxGvwRnBtscaJDQ4qYGpQt87+amKYb4vBJvtt-3BmsOorL_g@mail.gmail.com>
Accept-Language: en-NZ, en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 23/05/19 9:27 AM, Richard Weinberger wrote:=0A=
> On Wed, May 22, 2019 at 11:06 PM Chris Packham=0A=
> <Chris.Packham@alliedtelesis.co.nz> wrote:=0A=
>>=0A=
>> On 23/05/19 8:44 AM, Richard Weinberger wrote:=0A=
>>> On Wed, May 22, 2019 at 2:08 AM Chris Packham=0A=
>>> <chris.packham@alliedtelesis.co.nz> wrote:=0A=
>>>>=0A=
>>>> Add an implementation of the _is_locked operation for concatenated mtd=
=0A=
>>>> devices. As with concat_lock/concat_unlock this can simply use the=0A=
>>>> common helper and pass mtd_is_locked as the operation.=0A=
>>>>=0A=
>>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>=0A=
>>>> ---=0A=
>>>>    drivers/mtd/mtdconcat.c | 6 ++++++=0A=
>>>>    1 file changed, 6 insertions(+)=0A=
>>>>=0A=
>>>> diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c=0A=
>>>> index 9514cd2db63c..0e919f3423af 100644=0A=
>>>> --- a/drivers/mtd/mtdconcat.c=0A=
>>>> +++ b/drivers/mtd/mtdconcat.c=0A=
>>>> @@ -496,6 +496,11 @@ static int concat_unlock(struct mtd_info *mtd, lo=
ff_t ofs, uint64_t len)=0A=
>>>>           return __concat_xxlock(mtd, ofs, len, mtd_unlock);=0A=
>>>>    }=0A=
>>>>=0A=
>>>> +static int concat_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_=
t len)=0A=
>>>> +{=0A=
>>>> +       return __concat_xxlock(mtd, ofs, len, mtd_is_locked);=0A=
>>>> +}=0A=
>>>=0A=
>>> Hmm, here you start abusing your own new API. :(=0A=
>>=0A=
>> Abusing because xxlock is a poor choice of name? I initially had a third=
=0A=
>> copy of the logic from lock/unlock which is what lead me to do the=0A=
>> cleanup first. mtd_lock(), mtd_unlock() and mtd_is_locked() all work the=
=0A=
>> same way namely given an offset and a length either lock, unlock or=0A=
>> return the status of the len/erasesz blocks at ofs.=0A=
> =0A=
> Well, for unlock/lock it is just a loop which applies an operation to=0A=
> a given range on all submtds.=0A=
> But as soon an operation returns non-zero, the loop stops and returns=0A=
> that error.=0A=
> This makes sense for unlock/lock.=0A=
> =0A=
> Now you abuse this as "apply a random mtd operation to a given range".=0A=
> So, giving it a proper name is the first step. Step two is figuring=0A=
> for what kind=0A=
> of mtd operations it makes sense and is correct.=0A=
=0A=
Ah now I understand you concern. I guess the question is what is the =0A=
right thing for MEMISLOCKED to return when consecutive blocks differ in =0A=
lock status.=0A=
=0A=
>>>=0A=
>>> Did you verify that the unlock/lock-functions deal correctly with all=
=0A=
>>> semantics from mtd_is_locked?=0A=
>>> i.e. mtd_is_locked() with len =3D 0 returns 1 for spi-nor.=0A=
>>>=0A=
>>=0A=
>> I believe so. I've only got access to a parallel NOR flash system that=
=0A=
>> uses concatenation and that seems sane  (is mtdconcat able to work with=
=0A=
>> spi memories?). The concat_is_locked() should just reflect what the=0A=
>> underlying mtd device driver returns.=0A=
> =0A=
> mtdconcat *should* work with any mtd. But I never used it much, I see=0A=
> it more as legacy=0A=
> code.=0A=
> =0A=
> What happens if one submtd is locked and another not?=0A=
> Does concat_is_locked() return something sane then?=0A=
> I'd expect it to return true if at least one submtd is locked and 0=0A=
> of no submtd is locked.=0A=
> =0A=
> If the loop and return code handling in __concat_xxlock() can take care o=
f that,=0A=
> awesome. Then all you need is giving it a better name. :-)=0A=
=0A=
As implemented right now the loop will stop at the first locked block. =0A=
So if the range starts in a unlocked block and spans into a locked one =0A=
the return value will be 1.=0A=
=0A=
Is that correct? Well do_ppb_xxlock and  do_getlockstatus_oneblock seem =0A=
to only care about the first block (they both ignore len)? So they'd =0A=
return 0 in the case of unlocked,locked.=0A=
=0A=
stm_is_locked_sr does about the len and will return 0 if len falls =0A=
outside the locked region or if ofs starts before the locked region.=0A=
=0A=
So here's a quick breakdown=0A=
=0A=
                  ppb_is_locked intelext_is_locked stm_is_locked concat=0A=
unlocked,unlocked            0                  0             0      0=0A=
locked,locked                1                  1             1      1=0A=
locked,unlocked              1                  1             0      1=0A=
unlocked,locked              0                  0             0      1=0A=
=0A=
I'll try and make concat_is_locked consistent with the two cfi =0A=
implementations.=0A=
=0A=
Thanks for your feedback on this. I think the v2 series should look a =0A=
lot better as a result.=0A=
=0A=
=0A=