| Message ID | 20190522000753.13300-2-chris.packham@alliedtelesis.co.nz |
|---|---|
| State | Superseded |
| Delegated to: | Richard Weinberger |
| Headers | show |
| Series | [1/2] mtd: concat: refactor concat_lock/concat_unlock | expand |
On Wed, May 22, 2019 at 2:08 AM Chris Packham <chris.packham@alliedtelesis.co.nz> 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 <chris.packham@alliedtelesis.co.nz> > --- > 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:44 AM, Richard Weinberger wrote: > On Wed, May 22, 2019 at 2:08 AM Chris Packham > <chris.packham@alliedtelesis.co.nz> 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 <chris.packham@alliedtelesis.co.nz> >> --- >> 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 <Chris.Packham@alliedtelesis.co.nz> wrote: > > On 23/05/19 8:44 AM, Richard Weinberger wrote: > > On Wed, May 22, 2019 at 2:08 AM Chris Packham > > <chris.packham@alliedtelesis.co.nz> 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 <chris.packham@alliedtelesis.co.nz> > >> --- > >> 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. :-)
On Wed, May 22, 2019 at 11:26 PM Richard Weinberger <richard.weinberger@gmail.com> wrote: > > On Wed, May 22, 2019 at 11:06 PM Chris Packham > <Chris.Packham@alliedtelesis.co.nz> wrote: > > > > On 23/05/19 8:44 AM, Richard Weinberger wrote: > > > On Wed, May 22, 2019 at 2:08 AM Chris Packham > > > <chris.packham@alliedtelesis.co.nz> 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 <chris.packham@alliedtelesis.co.nz> > > >> --- > > >> 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.
On 23/05/19 9:27 AM, Richard Weinberger wrote: > On Wed, May 22, 2019 at 11:06 PM Chris Packham > <Chris.Packham@alliedtelesis.co.nz> wrote: >> >> On 23/05/19 8:44 AM, Richard Weinberger wrote: >>> On Wed, May 22, 2019 at 2:08 AM Chris Packham >>> <chris.packham@alliedtelesis.co.nz> 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 <chris.packham@alliedtelesis.co.nz> >>>> --- >>>> 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.
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;
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 <chris.packham@alliedtelesis.co.nz> --- drivers/mtd/mtdconcat.c | 6 ++++++ 1 file changed, 6 insertions(+)