[PATCH v2 1/4] mtd: cfi_cmdset_0002: Use right chip in do_ppb_xxlock()

[PATCH v2 1/4] mtd: cfi_cmdset_0002: Use right chip in do_ppb_xxlock()

[Joakim Tjernlund]

do_ppb_xxlock() fails to add chip->start when
quering for lock status(and chip_ready test),
which caused false status reports.
Fix by adding adr += chip->start and adjust call sites accordingly.

Fixes: 1648eaaa1575 ("mtd: cfi_cmdset_0002: Support Persistent Protection Bits (PPB) locking")
Cc: stable@vger.kernel.org
Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
---

 v2 - Spilt into several patches
 
 drivers/mtd/chips/cfi_cmdset_0002.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 53a976a8e614..8648b1adccd5 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -2554,8 +2554,9 @@ static int __maybe_unused do_ppb_xxlock(struct map_info *map,
 	unsigned long timeo;
 	int ret;
 
+	adr += chip->start;
 	mutex_lock(&chip->mutex);
-	ret = get_chip(map, chip, adr + chip->start, FL_LOCKING);
+	ret = get_chip(map, chip, adr, FL_LOCKING);
 	if (ret) {
 		mutex_unlock(&chip->mutex);
 		return ret;
@@ -2573,8 +2574,8 @@ static int __maybe_unused do_ppb_xxlock(struct map_info *map,
 
 	if (thunk == DO_XXLOCK_ONEBLOCK_LOCK) {
 		chip->state = FL_LOCKING;
-		map_write(map, CMD(0xA0), chip->start + adr);
-		map_write(map, CMD(0x00), chip->start + adr);
+		map_write(map, CMD(0xA0), adr);
+		map_write(map, CMD(0x00), adr);
 	} else if (thunk == DO_XXLOCK_ONEBLOCK_UNLOCK) {
 		/*
 		 * Unlocking of one specific sector is not supported, so we
@@ -2612,7 +2613,7 @@ static int __maybe_unused do_ppb_xxlock(struct map_info *map,
 	map_write(map, CMD(0x00), chip->start);
 
 	chip->state = FL_READY;
-	put_chip(map, chip, adr + chip->start);
+	put_chip(map, chip, adr);
 	mutex_unlock(&chip->mutex);
 
 	return ret;
-- 
2.13.6

[PATCH v2 2/4] mtd: cfi_cmdset_0002: fix SEGV unlocking multiple chips

[Joakim Tjernlund]

cfi_ppb_unlock() tries to relock all sectors that was locked before
unlocking the whole chip.
This locking used the chip start address + the FULL offset from the
first flash chip, thereby forming an illegal address. Correct by using
the chip offset(adr).

Fixes: 1648eaaa1575 ("mtd: cfi_cmdset_0002: Support Persistent Protection Bits (PPB) locking")
Cc: stable@vger.kernel.org
Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
---

 v2 - Spilt into several patches
 
 drivers/mtd/chips/cfi_cmdset_0002.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 8648b1adccd5..cb85cccc48c1 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -2536,7 +2536,7 @@ static int cfi_atmel_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 
 struct ppb_lock {
 	struct flchip *chip;
-	loff_t offset;
+	unsigned long adr;
 	int locked;
 };
 
@@ -2672,7 +2672,7 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs,
 		 */
 		if ((adr < ofs) || (adr >= (ofs + len))) {
 			sect[sectors].chip = &cfi->chips[chipnum];
-			sect[sectors].offset = offset;
+			sect[sectors].adr = adr;
 			sect[sectors].locked = do_ppb_xxlock(
 				map, &cfi->chips[chipnum], adr, 0,
 				DO_XXLOCK_ONEBLOCK_GETLOCK);
@@ -2716,7 +2716,7 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs,
 	 */
 	for (i = 0; i < sectors; i++) {
 		if (sect[i].locked)
-			do_ppb_xxlock(map, sect[i].chip, sect[i].offset, 0,
+			do_ppb_xxlock(map, sect[i].chip, sect[i].adr, 0,
 				      DO_XXLOCK_ONEBLOCK_LOCK);
 	}
 
-- 
2.13.6

[PATCH v2 3/4] mtd: cfi_cmdset_0002: Avoid point less unlocking/locking

[Joakim Tjernlund]

cfi_ppb_unlock() walks all flash chips when unlocking sectors.
testing lock status on each chip which causes relocking of already
locked sectors. Test against offset to aviod this aliasing.

Fixes: 1648eaaa1575 ("mtd: cfi_cmdset_0002: Support Persistent Protection Bits (PPB) locking")
Cc: stable@vger.kernel.org
Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
---

 v2 - Spilt into several patches


 drivers/mtd/chips/cfi_cmdset_0002.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index cb85cccc48c1..b6273ce83de7 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -2670,7 +2670,7 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs,
 		 * sectors shall be unlocked, so lets keep their locking
 		 * status at "unlocked" (locked=0) for the final re-locking.
 		 */
-		if ((adr < ofs) || (adr >= (ofs + len))) {
+		if ((offset < ofs) || (offset >= (ofs + len))) {
 			sect[sectors].chip = &cfi->chips[chipnum];
 			sect[sectors].adr = adr;
 			sect[sectors].locked = do_ppb_xxlock(
-- 
2.13.6

[PATCH v2 4/4] mtd: cfi_cmdset_0002: Avoid walking all chips when unlocking.

[Joakim Tjernlund]

cfi_ppb_unlock() walks all flash chips when unlocking sectors,
avoid walking chips unaffected by the unlock operation.

Fixes: 1648eaaa1575 ("mtd: cfi_cmdset_0002: Support Persistent Protection Bits (PPB) locking")
Cc: stable@vger.kernel.org
Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
---
 v2 - Spilt into several patches

 drivers/mtd/chips/cfi_cmdset_0002.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index b6273ce83de7..62cd4ee280b3 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -2686,6 +2686,8 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs,
 			i++;
 
 		if (adr >> cfi->chipshift) {
+			if (offset >= (ofs + len))
+				break;
 			adr = 0;
 			chipnum++;
 
-- 
2.13.6

Re: [PATCH v2 1/4] mtd: cfi_cmdset_0002: Use right chip in do_ppb_xxlock()

[Boris Brezillon]

On Wed,  6 Jun 2018 12:13:27 +0200
Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote:

> do_ppb_xxlock() fails to add chip->start when
> quering for lock status(and chip_ready test),

  ^ querying?

> which caused false status reports.

The 3 above lines are wrapped at less than 50 chars, is this normal?

> Fix by adding adr += chip->start and adjust call sites accordingly.
> 
> Fixes: 1648eaaa1575 ("mtd: cfi_cmdset_0002: Support Persistent Protection Bits (PPB) locking")
> Cc: stable@vger.kernel.org
> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> ---
> 
>  v2 - Spilt into several patches
>  
>  drivers/mtd/chips/cfi_cmdset_0002.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index 53a976a8e614..8648b1adccd5 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -2554,8 +2554,9 @@ static int __maybe_unused do_ppb_xxlock(struct map_info *map,
>  	unsigned long timeo;
>  	int ret;
>  
> +	adr += chip->start;
>  	mutex_lock(&chip->mutex);
> -	ret = get_chip(map, chip, adr + chip->start, FL_LOCKING);
> +	ret = get_chip(map, chip, adr, FL_LOCKING);
>  	if (ret) {
>  		mutex_unlock(&chip->mutex);
>  		return ret;
> @@ -2573,8 +2574,8 @@ static int __maybe_unused do_ppb_xxlock(struct map_info *map,
>  
>  	if (thunk == DO_XXLOCK_ONEBLOCK_LOCK) {
>  		chip->state = FL_LOCKING;
> -		map_write(map, CMD(0xA0), chip->start + adr);
> -		map_write(map, CMD(0x00), chip->start + adr);
> +		map_write(map, CMD(0xA0), adr);
> +		map_write(map, CMD(0x00), adr);
>  	} else if (thunk == DO_XXLOCK_ONEBLOCK_UNLOCK) {
>  		/*
>  		 * Unlocking of one specific sector is not supported, so we
> @@ -2612,7 +2613,7 @@ static int __maybe_unused do_ppb_xxlock(struct map_info *map,
>  	map_write(map, CMD(0x00), chip->start);
>  
>  	chip->state = FL_READY;
> -	put_chip(map, chip, adr + chip->start);
> +	put_chip(map, chip, adr);
>  	mutex_unlock(&chip->mutex);
>  
>  	return ret;

Re: [PATCH v2 2/4] mtd: cfi_cmdset_0002: fix SEGV unlocking multiple chips

[Boris Brezillon]

On Wed,  6 Jun 2018 12:13:28 +0200
Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote:

> cfi_ppb_unlock() tries to relock all sectors that was locked before

						    ^ were

> unlocking the whole chip.
> This locking used the chip start address + the FULL offset from the
> first flash chip, thereby forming an illegal address. Correct by using

							Fix/Correct that by... ?

> the chip offset(adr).
> 
> Fixes: 1648eaaa1575 ("mtd: cfi_cmdset_0002: Support Persistent Protection Bits (PPB) locking")
> Cc: stable@vger.kernel.org
> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> ---
> 
>  v2 - Spilt into several patches
>  
>  drivers/mtd/chips/cfi_cmdset_0002.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index 8648b1adccd5..cb85cccc48c1 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -2536,7 +2536,7 @@ static int cfi_atmel_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  
>  struct ppb_lock {
>  	struct flchip *chip;
> -	loff_t offset;
> +	unsigned long adr;
>  	int locked;
>  };
>  
> @@ -2672,7 +2672,7 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs,
>  		 */
>  		if ((adr < ofs) || (adr >= (ofs + len))) {
>  			sect[sectors].chip = &cfi->chips[chipnum];
> -			sect[sectors].offset = offset;
> +			sect[sectors].adr = adr;
>  			sect[sectors].locked = do_ppb_xxlock(
>  				map, &cfi->chips[chipnum], adr, 0,
>  				DO_XXLOCK_ONEBLOCK_GETLOCK);
> @@ -2716,7 +2716,7 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs,
>  	 */
>  	for (i = 0; i < sectors; i++) {
>  		if (sect[i].locked)
> -			do_ppb_xxlock(map, sect[i].chip, sect[i].offset, 0,
> +			do_ppb_xxlock(map, sect[i].chip, sect[i].adr, 0,
>  				      DO_XXLOCK_ONEBLOCK_LOCK);
>  	}
>  

Re: [PATCH v2 3/4] mtd: cfi_cmdset_0002: Avoid point less unlocking/locking

[Boris Brezillon]

On Wed,  6 Jun 2018 12:13:29 +0200
Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote:

> cfi_ppb_unlock() walks all flash chips when unlocking sectors.
> testing lock status on each chip which causes relocking of already
> locked sectors. Test against offset to aviod this aliasing.

					 ^ avoid

As I said before, I think the current code is doing worse than just
relocking already locked sectors. As soon as you cross a chip boundary,
addr is set back to 0, and the (addr < offs || adr >= (ofs + len)) might
be true while it shouldn't be (absolute offset still in the unlock
range), which means you'll lock sectors that the caller expect to be
unlocked.

> 
> Fixes: 1648eaaa1575 ("mtd: cfi_cmdset_0002: Support Persistent Protection Bits (PPB) locking")
> Cc: stable@vger.kernel.org
> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> ---
> 
>  v2 - Spilt into several patches
> 
> 
>  drivers/mtd/chips/cfi_cmdset_0002.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index cb85cccc48c1..b6273ce83de7 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -2670,7 +2670,7 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs,
>  		 * sectors shall be unlocked, so lets keep their locking
>  		 * status at "unlocked" (locked=0) for the final re-locking.
>  		 */
> -		if ((adr < ofs) || (adr >= (ofs + len))) {
> +		if ((offset < ofs) || (offset >= (ofs + len))) {
>  			sect[sectors].chip = &cfi->chips[chipnum];
>  			sect[sectors].adr = adr;
>  			sect[sectors].locked = do_ppb_xxlock(

Re: [PATCH v2 4/4] mtd: cfi_cmdset_0002: Avoid walking all chips when unlocking.

[Boris Brezillon]

On Wed,  6 Jun 2018 12:13:30 +0200
Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote:

> cfi_ppb_unlock() walks all flash chips when unlocking sectors,
> avoid walking chips unaffected by the unlock operation.
> 
> Fixes: 1648eaaa1575 ("mtd: cfi_cmdset_0002: Support Persistent Protection Bits (PPB) locking")
> Cc: stable@vger.kernel.org

That's clearly not a fix, just an optimization. You should drop the
Fixes and Cc-stable tags.

> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> ---
>  v2 - Spilt into several patches
> 
>  drivers/mtd/chips/cfi_cmdset_0002.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index b6273ce83de7..62cd4ee280b3 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -2686,6 +2686,8 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs,
>  			i++;
>  
>  		if (adr >> cfi->chipshift) {
> +			if (offset >= (ofs + len))
> +				break;
>  			adr = 0;
>  			chipnum++;
>  

Re: [PATCH v2 1/4] mtd: cfi_cmdset_0002: Use right chip in do_ppb_xxlock()

[Joakim Tjernlund]

On Wed, 2018-06-20 at 11:03 +0200, Boris Brezillon wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On Wed,  6 Jun 2018 12:13:27 +0200
> Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote:
> 
> > do_ppb_xxlock() fails to add chip->start when
> > quering for lock status(and chip_ready test),
> 
>   ^ querying?
> 
> > which caused false status reports.
> 
> The 3 above lines are wrapped at less than 50 chars, is this normal?

I actually hit return every now and then when I type, sometimes the
lines might become a bit short, but yes, this is normal for me.

> 
> > Fix by adding adr += chip->start and adjust call sites accordingly.
> > 
> > Fixes: 1648eaaa1575 ("mtd: cfi_cmdset_0002: Support Persistent Protection Bits (PPB) locking")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > ---
> > 
> >  v2 - Spilt into several patches
> > 
> >  drivers/mtd/chips/cfi_cmdset_0002.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> > index 53a976a8e614..8648b1adccd5 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > @@ -2554,8 +2554,9 @@ static int __maybe_unused do_ppb_xxlock(struct map_info *map,
> >       unsigned long timeo;
> >       int ret;
> > 
> > +     adr += chip->start;
> >       mutex_lock(&chip->mutex);
> > -     ret = get_chip(map, chip, adr + chip->start, FL_LOCKING);
> > +     ret = get_chip(map, chip, adr, FL_LOCKING);
> >       if (ret) {
> >               mutex_unlock(&chip->mutex);
> >               return ret;
> > @@ -2573,8 +2574,8 @@ static int __maybe_unused do_ppb_xxlock(struct map_info *map,
> > 
> >       if (thunk == DO_XXLOCK_ONEBLOCK_LOCK) {
> >               chip->state = FL_LOCKING;
> > -             map_write(map, CMD(0xA0), chip->start + adr);
> > -             map_write(map, CMD(0x00), chip->start + adr);
> > +             map_write(map, CMD(0xA0), adr);
> > +             map_write(map, CMD(0x00), adr);
> >       } else if (thunk == DO_XXLOCK_ONEBLOCK_UNLOCK) {
> >               /*
> >                * Unlocking of one specific sector is not supported, so we
> > @@ -2612,7 +2613,7 @@ static int __maybe_unused do_ppb_xxlock(struct map_info *map,
> >       map_write(map, CMD(0x00), chip->start);
> > 
> >       chip->state = FL_READY;
> > -     put_chip(map, chip, adr + chip->start);
> > +     put_chip(map, chip, adr);
> >       mutex_unlock(&chip->mutex);
> > 
> >       return ret;
> 
> 

Re: [PATCH v2 3/4] mtd: cfi_cmdset_0002: Avoid point less unlocking/locking

[Joakim Tjernlund]

On Wed, 2018-06-20 at 11:14 +0200, Boris Brezillon wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On Wed,  6 Jun 2018 12:13:29 +0200
> Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote:
> 
> > cfi_ppb_unlock() walks all flash chips when unlocking sectors.
> > testing lock status on each chip which causes relocking of already
> > locked sectors. Test against offset to aviod this aliasing.
> 
>                                          ^ avoid
> 
> As I said before, I think the current code is doing worse than just
> relocking already locked sectors. As soon as you cross a chip boundary,
> addr is set back to 0, and the (addr < offs || adr >= (ofs + len)) might
> be true while it shouldn't be (absolute offset still in the unlock
> range), which means you'll lock sectors that the caller expect to be
> unlocked.

I don't see how, the code asks for its current lock status and will reapply
those that are locked again.

> 
> > 
> > Fixes: 1648eaaa1575 ("mtd: cfi_cmdset_0002: Support Persistent Protection Bits (PPB) locking")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > ---
> > 
> >  v2 - Spilt into several patches
> > 
> > 
> >  drivers/mtd/chips/cfi_cmdset_0002.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> > index cb85cccc48c1..b6273ce83de7 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > @@ -2670,7 +2670,7 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs,
> >                * sectors shall be unlocked, so lets keep their locking
> >                * status at "unlocked" (locked=0) for the final re-locking.
> >                */
> > -             if ((adr < ofs) || (adr >= (ofs + len))) {
> > +             if ((offset < ofs) || (offset >= (ofs + len))) {
> >                       sect[sectors].chip = &cfi->chips[chipnum];
> >                       sect[sectors].adr = adr;
> >                       sect[sectors].locked = do_ppb_xxlock(
> 
> 

Re: [PATCH v2 4/4] mtd: cfi_cmdset_0002: Avoid walking all chips when unlocking.

[Joakim Tjernlund]

On Wed, 2018-06-20 at 11:25 +0200, Boris Brezillon wrote:
> 
> 
> On Wed,  6 Jun 2018 12:13:30 +0200
> Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote:
> 
> > cfi_ppb_unlock() walks all flash chips when unlocking sectors,
> > avoid walking chips unaffected by the unlock operation.
> > 
> > Fixes: 1648eaaa1575 ("mtd: cfi_cmdset_0002: Support Persistent Protection Bits (PPB) locking")
> > Cc: stable@vger.kernel.org
> 
> That's clearly not a fix, just an optimization. You should drop the
> Fixes and Cc-stable tags.

It sure IS! The code never intended to do this and it is just bad luck that nothing bad
happened and I sure don't want to walk all 4 chips we have, stealing CPU and keeping the
flash busy just because I am using stable.

Given I have moved on now and we disagree, I will not reword and resubmit any
time soon. Feel free to do needed edits though.

 Jocke

> 
> > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > ---
> >  v2 - Spilt into several patches
> > 
> >  drivers/mtd/chips/cfi_cmdset_0002.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> > index b6273ce83de7..62cd4ee280b3 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > @@ -2686,6 +2686,8 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs,
> >                       i++;
> > 
> >               if (adr >> cfi->chipshift) {
> > +                     if (offset >= (ofs + len))
> > +                             break;
> >                       adr = 0;
> >                       chipnum++;
> > 
> 
> 

Re: [PATCH v2 3/4] mtd: cfi_cmdset_0002: Avoid point less unlocking/locking

[Boris Brezillon]

On Wed, 20 Jun 2018 11:10:23 +0000
Joakim Tjernlund <Joakim.Tjernlund@infinera.com> wrote:

> On Wed, 2018-06-20 at 11:14 +0200, Boris Brezillon wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > 
> > 
> > On Wed,  6 Jun 2018 12:13:29 +0200
> > Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote:
> >   
> > > cfi_ppb_unlock() walks all flash chips when unlocking sectors.
> > > testing lock status on each chip which causes relocking of already
> > > locked sectors. Test against offset to aviod this aliasing.  
> > 
> >                                          ^ avoid
> > 
> > As I said before, I think the current code is doing worse than just
> > relocking already locked sectors. As soon as you cross a chip boundary,
> > addr is set back to 0, and the (addr < offs || adr >= (ofs + len)) might
> > be true while it shouldn't be (absolute offset still in the unlock
> > range), which means you'll lock sectors that the caller expect to be
> > unlocked.  
> 
> I don't see how, the code asks for its current lock status and will reapply
> those that are locked again.

After reading the commit message a second time I think I
misunderstood it. I thought you were implying that the re-locking
operation was harmless and the only reason for fixing the test was to
avoid useless lock operations, but that's not what you wrote.

Sorry for the noise.

Re: [PATCH v2 4/4] mtd: cfi_cmdset_0002: Avoid walking all chips when unlocking.

[Boris Brezillon]

On Wed, 20 Jun 2018 11:10:49 +0000
Joakim Tjernlund <Joakim.Tjernlund@infinera.com> wrote:

> On Wed, 2018-06-20 at 11:25 +0200, Boris Brezillon wrote:
> > 
> > 
> > On Wed,  6 Jun 2018 12:13:30 +0200
> > Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote:
> >   
> > > cfi_ppb_unlock() walks all flash chips when unlocking sectors,
> > > avoid walking chips unaffected by the unlock operation.
> > > 
> > > Fixes: 1648eaaa1575 ("mtd: cfi_cmdset_0002: Support Persistent Protection Bits (PPB) locking")
> > > Cc: stable@vger.kernel.org  
> > 
> > That's clearly not a fix, just an optimization. You should drop the
> > Fixes and Cc-stable tags.  
> 
> It sure IS! The code never intended to do this and it is just bad luck that nothing bad
> happened and I sure don't want to walk all 4 chips we have, stealing CPU and keeping the
> flash busy just because I am using stable.

Except it's like that from the beginning, so that's not a regression
you're fixing nor it is a real bug preventing you from using the driver
on your platform. I'm not making the rules of what is appropriate to be
backported and what is not, but I've been told several times that only
patches fixing bugs or perf regressions are supposed to be backported,
and that's not the case here.

> 
> Given I have moved on now and we disagree, I will not reword and resubmit any
> time soon. Feel free to do needed edits though.

I'm sorry, maybe you don't like it but that's the process. I understand
that it's not pleasant to have to send a new version of patches that
you thought were good enough to go upstream, but it's like that. If I
don't apply this rule to you, why should it apply to others.

Re: [PATCH v2 4/4] mtd: cfi_cmdset_0002: Avoid walking all chips when unlocking.

[Joakim Tjernlund]

On Wed, 2018-06-20 at 14:19 +0200, Boris Brezillon wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On Wed, 20 Jun 2018 11:10:49 +0000
> Joakim Tjernlund <Joakim.Tjernlund@infinera.com> wrote:
> 
> > On Wed, 2018-06-20 at 11:25 +0200, Boris Brezillon wrote:
> > > 
> > > 
> > > On Wed,  6 Jun 2018 12:13:30 +0200
> > > Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote:
> > > 
> > > > cfi_ppb_unlock() walks all flash chips when unlocking sectors,
> > > > avoid walking chips unaffected by the unlock operation.
> > > > 
> > > > Fixes: 1648eaaa1575 ("mtd: cfi_cmdset_0002: Support Persistent Protection Bits (PPB) locking")
> > > > Cc: stable@vger.kernel.org
> > > 
> > > That's clearly not a fix, just an optimization. You should drop the
> > > Fixes and Cc-stable tags.
> > 
> > It sure IS! The code never intended to do this and it is just bad luck that nothing bad
> > happened and I sure don't want to walk all 4 chips we have, stealing CPU and keeping the
> > flash busy just because I am using stable.
> 
> Except it's like that from the beginning, so that's not a regression
> you're fixing nor it is a real bug preventing you from using the driver
> on your platform. I'm not making the rules of what is appropriate to be
> backported and what is not, but I've been told several times that only
> patches fixing bugs or perf regressions are supposed to be backported,
> and that's not the case here.

I think you are oversimplifying things, look at 
 https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?h=linux-4.14.y&id=058dd233b5593a1a5fae4b8df6cb44cbcdccb537
it does not actually fix anything, yet it is in stable. 
> 
> > 
> > Given I have moved on now and we disagree, I will not reword and resubmit any
> > time soon. Feel free to do needed edits though.
> 
> I'm sorry, maybe you don't like it but that's the process. I understand
> that it's not pleasant to have to send a new version of patches that
> you thought were good enough to go upstream, but it's like that. If I
> don't apply this rule to you, why should it apply to others.

Come on, you are nitpicking late and want me to do changes I don't agree with.
I don't have to do what you ask and I am tired of this debate.
Once again, choose yourself. If this last patch bothers you, just drop that patch then.

 Jocke

Re: [PATCH v2 1/4] mtd: cfi_cmdset_0002: Use right chip in do_ppb_xxlock()

[Boris Brezillon]

On Wed,  6 Jun 2018 12:13:27 +0200
Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote:

> do_ppb_xxlock() fails to add chip->start when
> quering for lock status(and chip_ready test),
> which caused false status reports.
> Fix by adding adr += chip->start and adjust call sites accordingly.
> 
> Fixes: 1648eaaa1575 ("mtd: cfi_cmdset_0002: Support Persistent Protection Bits (PPB) locking")
> Cc: stable@vger.kernel.org
> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>

Applied all patches of this series.

> ---
> 
>  v2 - Spilt into several patches
>  
>  drivers/mtd/chips/cfi_cmdset_0002.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index 53a976a8e614..8648b1adccd5 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -2554,8 +2554,9 @@ static int __maybe_unused do_ppb_xxlock(struct map_info *map,
>  	unsigned long timeo;
>  	int ret;
>  
> +	adr += chip->start;
>  	mutex_lock(&chip->mutex);
> -	ret = get_chip(map, chip, adr + chip->start, FL_LOCKING);
> +	ret = get_chip(map, chip, adr, FL_LOCKING);
>  	if (ret) {
>  		mutex_unlock(&chip->mutex);
>  		return ret;
> @@ -2573,8 +2574,8 @@ static int __maybe_unused do_ppb_xxlock(struct map_info *map,
>  
>  	if (thunk == DO_XXLOCK_ONEBLOCK_LOCK) {
>  		chip->state = FL_LOCKING;
> -		map_write(map, CMD(0xA0), chip->start + adr);
> -		map_write(map, CMD(0x00), chip->start + adr);
> +		map_write(map, CMD(0xA0), adr);
> +		map_write(map, CMD(0x00), adr);
>  	} else if (thunk == DO_XXLOCK_ONEBLOCK_UNLOCK) {
>  		/*
>  		 * Unlocking of one specific sector is not supported, so we
> @@ -2612,7 +2613,7 @@ static int __maybe_unused do_ppb_xxlock(struct map_info *map,
>  	map_write(map, CMD(0x00), chip->start);
>  
>  	chip->state = FL_READY;
> -	put_chip(map, chip, adr + chip->start);
> +	put_chip(map, chip, adr);
>  	mutex_unlock(&chip->mutex);
>  
>  	return ret;