concat_lock() and concat_unlock() only differed in terms of the mtd_xx
operation they called. Refactor them to use a common helper function and
pass mtd_lock or mtd_unlock as an argument.
Signed-off-by: Chris Packham <[email protected]>
---
drivers/mtd/mtdconcat.c | 41 +++++++++--------------------------------
1 file changed, 9 insertions(+), 32 deletions(-)
diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
index cbc5925e6440..9514cd2db63c 100644
--- a/drivers/mtd/mtdconcat.c
+++ b/drivers/mtd/mtdconcat.c
@@ -451,7 +451,8 @@ static int concat_erase(struct mtd_info *mtd, struct erase_info *instr)
return err;
}
-static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
+static int __concat_xxlock(struct mtd_info *mtd, loff_t ofs, uint64_t len,
+ int (*mtd_op)(struct mtd_info *mtd, loff_t ofs, uint64_t len))
{
struct mtd_concat *concat = CONCAT(mtd);
int i, err = -EINVAL;
@@ -470,7 +471,7 @@ static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
else
size = len;
- err = mtd_lock(subdev, ofs, size);
+ err = mtd_op(subdev, ofs, size);
if (err)
break;
@@ -485,38 +486,14 @@ static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
return err;
}
-static int concat_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
+static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
{
- struct mtd_concat *concat = CONCAT(mtd);
- int i, err = 0;
-
- for (i = 0; i < concat->num_subdev; i++) {
- struct mtd_info *subdev = concat->subdev[i];
- uint64_t size;
-
- if (ofs >= subdev->size) {
- size = 0;
- ofs -= subdev->size;
- continue;
- }
- if (ofs + len > subdev->size)
- size = subdev->size - ofs;
- else
- size = len;
-
- err = mtd_unlock(subdev, ofs, size);
- if (err)
- break;
-
- len -= size;
- if (len == 0)
- break;
-
- err = -EINVAL;
- ofs = 0;
- }
+ return __concat_xxlock(mtd, ofs, len, mtd_lock);
+}
- return err;
+static int concat_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
+{
+ return __concat_xxlock(mtd, ofs, len, mtd_unlock);
}
static void concat_sync(struct mtd_info *mtd)
--
2.21.0
Add an implementation of the _is_locked operation for concatenated mtd
devices. As with concat_lock/concat_unlock this can simply use the
common helper and pass mtd_is_locked as the operation.
Signed-off-by: Chris Packham <[email protected]>
---
drivers/mtd/mtdconcat.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
index 9514cd2db63c..0e919f3423af 100644
--- a/drivers/mtd/mtdconcat.c
+++ b/drivers/mtd/mtdconcat.c
@@ -496,6 +496,11 @@ static int concat_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
return __concat_xxlock(mtd, ofs, len, mtd_unlock);
}
+static int concat_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
+{
+ return __concat_xxlock(mtd, ofs, len, mtd_is_locked);
+}
+
static void concat_sync(struct mtd_info *mtd)
{
struct mtd_concat *concat = CONCAT(mtd);
@@ -695,6 +700,7 @@ struct mtd_info *mtd_concat_create(struct mtd_info *subdev[], /* subdevices to c
concat->mtd._sync = concat_sync;
concat->mtd._lock = concat_lock;
concat->mtd._unlock = concat_unlock;
+ concat->mtd._is_locked = concat_is_locked;
concat->mtd._suspend = concat_suspend;
concat->mtd._resume = concat_resume;
--
2.21.0
On Wed, May 22, 2019 at 2:08 AM Chris Packham
<[email protected]> wrote:
>
> concat_lock() and concat_unlock() only differed in terms of the mtd_xx
> operation they called. Refactor them to use a common helper function and
> pass mtd_lock or mtd_unlock as an argument.
>
> Signed-off-by: Chris Packham <[email protected]>
> ---
> drivers/mtd/mtdconcat.c | 41 +++++++++--------------------------------
> 1 file changed, 9 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
> index cbc5925e6440..9514cd2db63c 100644
> --- a/drivers/mtd/mtdconcat.c
> +++ b/drivers/mtd/mtdconcat.c
> @@ -451,7 +451,8 @@ static int concat_erase(struct mtd_info *mtd, struct erase_info *instr)
> return err;
> }
>
> -static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> +static int __concat_xxlock(struct mtd_info *mtd, loff_t ofs, uint64_t len,
> + int (*mtd_op)(struct mtd_info *mtd, loff_t ofs, uint64_t len))
> {
> struct mtd_concat *concat = CONCAT(mtd);
> int i, err = -EINVAL;
> @@ -470,7 +471,7 @@ static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> else
> size = len;
>
> - err = mtd_lock(subdev, ofs, size);
> + err = mtd_op(subdev, ofs, size);
> if (err)
> break;
>
> @@ -485,38 +486,14 @@ static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> return err;
> }
>
> -static int concat_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> +static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> {
> - struct mtd_concat *concat = CONCAT(mtd);
> - int i, err = 0;
> -
> - for (i = 0; i < concat->num_subdev; i++) {
> - struct mtd_info *subdev = concat->subdev[i];
> - uint64_t size;
> -
> - if (ofs >= subdev->size) {
> - size = 0;
> - ofs -= subdev->size;
> - continue;
> - }
> - if (ofs + len > subdev->size)
> - size = subdev->size - ofs;
> - else
> - size = len;
> -
> - err = mtd_unlock(subdev, ofs, size);
> - if (err)
> - break;
> -
> - len -= size;
> - if (len == 0)
> - break;
> -
> - err = -EINVAL;
> - ofs = 0;
> - }
> + return __concat_xxlock(mtd, ofs, len, mtd_lock);
> +}
>
> - return err;
> +static int concat_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> +{
> + return __concat_xxlock(mtd, ofs, len, mtd_unlock);
> }
>
> static void concat_sync(struct mtd_info *mtd)
Not sure if it passing a function pointer is worth it. bool is_lock would
also do it. But this is a matter of taste, I guess. :)
Reviewed-by: Richard Weinberger <[email protected]>
--
Thanks,
//richard
On Wed, May 22, 2019 at 2:08 AM Chris Packham
<[email protected]> wrote:
>
> Add an implementation of the _is_locked operation for concatenated mtd
> devices. As with concat_lock/concat_unlock this can simply use the
> common helper and pass mtd_is_locked as the operation.
>
> Signed-off-by: Chris Packham <[email protected]>
> ---
> drivers/mtd/mtdconcat.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
> index 9514cd2db63c..0e919f3423af 100644
> --- a/drivers/mtd/mtdconcat.c
> +++ b/drivers/mtd/mtdconcat.c
> @@ -496,6 +496,11 @@ static int concat_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> return __concat_xxlock(mtd, ofs, len, mtd_unlock);
> }
>
> +static int concat_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> +{
> + return __concat_xxlock(mtd, ofs, len, mtd_is_locked);
> +}
Hmm, here you start abusing your own new API. :(
Did you verify that the unlock/lock-functions deal correctly with all
semantics from mtd_is_locked?
i.e. mtd_is_locked() with len = 0 returns 1 for spi-nor.
On 23/05/19 8:30 AM, Richard Weinberger wrote:
> On Wed, May 22, 2019 at 2:08 AM Chris Packham
> <[email protected]> wrote:
>>
>> concat_lock() and concat_unlock() only differed in terms of the mtd_xx
>> operation they called. Refactor them to use a common helper function and
>> pass mtd_lock or mtd_unlock as an argument.
>>
>> Signed-off-by: Chris Packham <[email protected]>
>> ---
>> drivers/mtd/mtdconcat.c | 41 +++++++++--------------------------------
>> 1 file changed, 9 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
>> index cbc5925e6440..9514cd2db63c 100644
>> --- a/drivers/mtd/mtdconcat.c
>> +++ b/drivers/mtd/mtdconcat.c
>> @@ -451,7 +451,8 @@ static int concat_erase(struct mtd_info *mtd, struct erase_info *instr)
>> return err;
>> }
>>
>> -static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>> +static int __concat_xxlock(struct mtd_info *mtd, loff_t ofs, uint64_t len,
>> + int (*mtd_op)(struct mtd_info *mtd, loff_t ofs, uint64_t len))
>> {
>> struct mtd_concat *concat = CONCAT(mtd);
>> int i, err = -EINVAL;
>> @@ -470,7 +471,7 @@ static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>> else
>> size = len;
>>
>> - err = mtd_lock(subdev, ofs, size);
>> + err = mtd_op(subdev, ofs, size);
>> if (err)
>> break;
>>
>> @@ -485,38 +486,14 @@ static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>> return err;
>> }
>>
>> -static int concat_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>> +static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>> {
>> - struct mtd_concat *concat = CONCAT(mtd);
>> - int i, err = 0;
>> -
>> - for (i = 0; i < concat->num_subdev; i++) {
>> - struct mtd_info *subdev = concat->subdev[i];
>> - uint64_t size;
>> -
>> - if (ofs >= subdev->size) {
>> - size = 0;
>> - ofs -= subdev->size;
>> - continue;
>> - }
>> - if (ofs + len > subdev->size)
>> - size = subdev->size - ofs;
>> - else
>> - size = len;
>> -
>> - err = mtd_unlock(subdev, ofs, size);
>> - if (err)
>> - break;
>> -
>> - len -= size;
>> - if (len == 0)
>> - break;
>> -
>> - err = -EINVAL;
>> - ofs = 0;
>> - }
>> + return __concat_xxlock(mtd, ofs, len, mtd_lock);
>> +}
>>
>> - return err;
>> +static int concat_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>> +{
>> + return __concat_xxlock(mtd, ofs, len, mtd_unlock);
>> }
>>
>> static void concat_sync(struct mtd_info *mtd)
>
> Not sure if it passing a function pointer is worth it. bool is_lock would
> also do it. But this is a matter of taste, I guess. :)
I briefly considered that. But since mtd_lock(), mtd_unlock() and
mtd_is_locked() all take the same arguments I figured it'd benefit from
some type checking. A bool wouldn't work (assuming I can convince you
about 2/2) but an enum mtd_op or int flags would do the trick if you
want me to change it.
>
> Reviewed-by: Richard Weinberger <[email protected]>
>
On 23/05/19 8:44 AM, Richard Weinberger wrote:
> On Wed, May 22, 2019 at 2:08 AM Chris Packham
> <[email protected]> wrote:
>>
>> Add an implementation of the _is_locked operation for concatenated mtd
>> devices. As with concat_lock/concat_unlock this can simply use the
>> common helper and pass mtd_is_locked as the operation.
>>
>> Signed-off-by: Chris Packham <[email protected]>
>> ---
>> drivers/mtd/mtdconcat.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
>> index 9514cd2db63c..0e919f3423af 100644
>> --- a/drivers/mtd/mtdconcat.c
>> +++ b/drivers/mtd/mtdconcat.c
>> @@ -496,6 +496,11 @@ static int concat_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>> return __concat_xxlock(mtd, ofs, len, mtd_unlock);
>> }
>>
>> +static int concat_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>> +{
>> + return __concat_xxlock(mtd, ofs, len, mtd_is_locked);
>> +}
>
> Hmm, here you start abusing your own new API. :(
Abusing because xxlock is a poor choice of name? I initially had a third
copy of the logic from lock/unlock which is what lead me to do the
cleanup first. mtd_lock(), mtd_unlock() and mtd_is_locked() all work the
same way namely given an offset and a length either lock, unlock or
return the status of the len/erasesz blocks at ofs.
>
> Did you verify that the unlock/lock-functions deal correctly with all
> semantics from mtd_is_locked?
> i.e. mtd_is_locked() with len = 0 returns 1 for spi-nor.
>
I believe so. I've only got access to a parallel NOR flash system that
uses concatenation and that seems sane (is mtdconcat able to work with
spi memories?). The concat_is_locked() should just reflect what the
underlying mtd device driver returns.
On Wed, May 22, 2019 at 11:06 PM Chris Packham
<[email protected]> wrote:
>
> On 23/05/19 8:44 AM, Richard Weinberger wrote:
> > On Wed, May 22, 2019 at 2:08 AM Chris Packham
> > <[email protected]> wrote:
> >>
> >> Add an implementation of the _is_locked operation for concatenated mtd
> >> devices. As with concat_lock/concat_unlock this can simply use the
> >> common helper and pass mtd_is_locked as the operation.
> >>
> >> Signed-off-by: Chris Packham <[email protected]>
> >> ---
> >> drivers/mtd/mtdconcat.c | 6 ++++++
> >> 1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
> >> index 9514cd2db63c..0e919f3423af 100644
> >> --- a/drivers/mtd/mtdconcat.c
> >> +++ b/drivers/mtd/mtdconcat.c
> >> @@ -496,6 +496,11 @@ static int concat_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> >> return __concat_xxlock(mtd, ofs, len, mtd_unlock);
> >> }
> >>
> >> +static int concat_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> >> +{
> >> + return __concat_xxlock(mtd, ofs, len, mtd_is_locked);
> >> +}
> >
> > Hmm, here you start abusing your own new API. :(
>
> Abusing because xxlock is a poor choice of name? I initially had a third
> copy of the logic from lock/unlock which is what lead me to do the
> cleanup first. mtd_lock(), mtd_unlock() and mtd_is_locked() all work the
> same way namely given an offset and a length either lock, unlock or
> return the status of the len/erasesz blocks at ofs.
Well, for unlock/lock it is just a loop which applies an operation to
a given range on all submtds.
But as soon an operation returns non-zero, the loop stops and returns
that error.
This makes sense for unlock/lock.
Now you abuse this as "apply a random mtd operation to a given range".
So, giving it a proper name is the first step. Step two is figuring
for what kind
of mtd operations it makes sense and is correct.
> >
> > Did you verify that the unlock/lock-functions deal correctly with all
> > semantics from mtd_is_locked?
> > i.e. mtd_is_locked() with len = 0 returns 1 for spi-nor.
> >
>
> I believe so. I've only got access to a parallel NOR flash system that
> uses concatenation and that seems sane (is mtdconcat able to work with
> spi memories?). The concat_is_locked() should just reflect what the
> underlying mtd device driver returns.
mtdconcat *should* work with any mtd. But I never used it much, I see
it more as legacy
code.
What happens if one submtd is locked and another not?
Does concat_is_locked() return something sane then?
I'd expect it to return true if at least one submtd is locked and 0
of no submtd is locked.
If the loop and return code handling in __concat_xxlock() can take care of that,
awesome. Then all you need is giving it a better name. :-)
--
Thanks,
//richard
On Wed, May 22, 2019 at 11:26 PM Richard Weinberger
<[email protected]> wrote:
>
> On Wed, May 22, 2019 at 11:06 PM Chris Packham
> <[email protected]> wrote:
> >
> > On 23/05/19 8:44 AM, Richard Weinberger wrote:
> > > On Wed, May 22, 2019 at 2:08 AM Chris Packham
> > > <[email protected]> wrote:
> > >>
> > >> Add an implementation of the _is_locked operation for concatenated mtd
> > >> devices. As with concat_lock/concat_unlock this can simply use the
> > >> common helper and pass mtd_is_locked as the operation.
> > >>
> > >> Signed-off-by: Chris Packham <[email protected]>
> > >> ---
> > >> drivers/mtd/mtdconcat.c | 6 ++++++
> > >> 1 file changed, 6 insertions(+)
> > >>
> > >> diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
> > >> index 9514cd2db63c..0e919f3423af 100644
> > >> --- a/drivers/mtd/mtdconcat.c
> > >> +++ b/drivers/mtd/mtdconcat.c
> > >> @@ -496,6 +496,11 @@ static int concat_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> > >> return __concat_xxlock(mtd, ofs, len, mtd_unlock);
> > >> }
> > >>
> > >> +static int concat_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> > >> +{
> > >> + return __concat_xxlock(mtd, ofs, len, mtd_is_locked);
> > >> +}
> > >
> > > Hmm, here you start abusing your own new API. :(
> >
> > Abusing because xxlock is a poor choice of name? I initially had a third
> > copy of the logic from lock/unlock which is what lead me to do the
> > cleanup first. mtd_lock(), mtd_unlock() and mtd_is_locked() all work the
> > same way namely given an offset and a length either lock, unlock or
> > return the status of the len/erasesz blocks at ofs.
>
> Well, for unlock/lock it is just a loop which applies an operation to
> a given range on all submtds.
> But as soon an operation returns non-zero, the loop stops and returns
> that error.
> This makes sense for unlock/lock.
>
> Now you abuse this as "apply a random mtd operation to a given range".
> So, giving it a proper name is the first step. Step two is figuring
> for what kind
> of mtd operations it makes sense and is correct.
>
> > >
> > > Did you verify that the unlock/lock-functions deal correctly with all
> > > semantics from mtd_is_locked?
> > > i.e. mtd_is_locked() with len = 0 returns 1 for spi-nor.
> > >
> >
> > I believe so. I've only got access to a parallel NOR flash system that
> > uses concatenation and that seems sane (is mtdconcat able to work with
> > spi memories?). The concat_is_locked() should just reflect what the
> > underlying mtd device driver returns.
>
> mtdconcat *should* work with any mtd. But I never used it much, I see
> it more as legacy
> code.
>
> What happens if one submtd is locked and another not?
> Does concat_is_locked() return something sane then?
> I'd expect it to return true if at least one submtd is locked and 0
> of no submtd is locked.
BTW: Meant overlapping requests. If it targets always only one submtd,
it is easy.
--
Thanks,
//richard
On 23/05/19 9:27 AM, Richard Weinberger wrote:
> On Wed, May 22, 2019 at 11:06 PM Chris Packham
> <[email protected]> wrote:
>>
>> On 23/05/19 8:44 AM, Richard Weinberger wrote:
>>> On Wed, May 22, 2019 at 2:08 AM Chris Packham
>>> <[email protected]> wrote:
>>>>
>>>> Add an implementation of the _is_locked operation for concatenated mtd
>>>> devices. As with concat_lock/concat_unlock this can simply use the
>>>> common helper and pass mtd_is_locked as the operation.
>>>>
>>>> Signed-off-by: Chris Packham <[email protected]>
>>>> ---
>>>> drivers/mtd/mtdconcat.c | 6 ++++++
>>>> 1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
>>>> index 9514cd2db63c..0e919f3423af 100644
>>>> --- a/drivers/mtd/mtdconcat.c
>>>> +++ b/drivers/mtd/mtdconcat.c
>>>> @@ -496,6 +496,11 @@ static int concat_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>>>> return __concat_xxlock(mtd, ofs, len, mtd_unlock);
>>>> }
>>>>
>>>> +static int concat_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>>>> +{
>>>> + return __concat_xxlock(mtd, ofs, len, mtd_is_locked);
>>>> +}
>>>
>>> Hmm, here you start abusing your own new API. :(
>>
>> Abusing because xxlock is a poor choice of name? I initially had a third
>> copy of the logic from lock/unlock which is what lead me to do the
>> cleanup first. mtd_lock(), mtd_unlock() and mtd_is_locked() all work the
>> same way namely given an offset and a length either lock, unlock or
>> return the status of the len/erasesz blocks at ofs.
>
> Well, for unlock/lock it is just a loop which applies an operation to
> a given range on all submtds.
> But as soon an operation returns non-zero, the loop stops and returns
> that error.
> This makes sense for unlock/lock.
>
> Now you abuse this as "apply a random mtd operation to a given range".
> So, giving it a proper name is the first step. Step two is figuring
> for what kind
> of mtd operations it makes sense and is correct.
Ah now I understand you concern. I guess the question is what is the
right thing for MEMISLOCKED to return when consecutive blocks differ in
lock status.
>>>
>>> Did you verify that the unlock/lock-functions deal correctly with all
>>> semantics from mtd_is_locked?
>>> i.e. mtd_is_locked() with len = 0 returns 1 for spi-nor.
>>>
>>
>> I believe so. I've only got access to a parallel NOR flash system that
>> uses concatenation and that seems sane (is mtdconcat able to work with
>> spi memories?). The concat_is_locked() should just reflect what the
>> underlying mtd device driver returns.
>
> mtdconcat *should* work with any mtd. But I never used it much, I see
> it more as legacy
> code.
>
> What happens if one submtd is locked and another not?
> Does concat_is_locked() return something sane then?
> I'd expect it to return true if at least one submtd is locked and 0
> of no submtd is locked.
>
> If the loop and return code handling in __concat_xxlock() can take care of that,
> awesome. Then all you need is giving it a better name. :-)
As implemented right now the loop will stop at the first locked block.
So if the range starts in a unlocked block and spans into a locked one
the return value will be 1.
Is that correct? Well do_ppb_xxlock and do_getlockstatus_oneblock seem
to only care about the first block (they both ignore len)? So they'd
return 0 in the case of unlocked,locked.
stm_is_locked_sr does about the len and will return 0 if len falls
outside the locked region or if ofs starts before the locked region.
So here's a quick breakdown
ppb_is_locked intelext_is_locked stm_is_locked concat
unlocked,unlocked 0 0 0 0
locked,locked 1 1 1 1
locked,unlocked 1 1 0 1
unlocked,locked 0 0 0 1
I'll try and make concat_is_locked consistent with the two cfi
implementations.
Thanks for your feedback on this. I think the v2 series should look a
lot better as a result.