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

[PATCH 1/2] 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).

In addition, do_ppb_xxlock() failed 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: 1648eaaa1575e
Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
---
 drivers/mtd/chips/cfi_cmdset_0002.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 5e526aec66f0..c74c53b886be 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -2535,7 +2535,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;
 };
 
@@ -2553,8 +2553,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;
@@ -2572,8 +2573,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
@@ -2611,7 +2612,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;
@@ -2670,7 +2671,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);
@@ -2686,7 +2687,6 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs,
 		if (adr >> cfi->chipshift) {
 			adr = 0;
 			chipnum++;
-
 			if (chipnum >= cfi->numchips)
 				break;
 		}
@@ -2714,7 +2714,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 2/2] mtd: cfi_cmdset_0002: Avoid point less unlocking/locking

[Joakim Tjernlund]

cfi_ppb_unlock() walks all flash chips when unlocking sectors.
This is a waste when crossing chip boundaris unaffected
by the unlock operation.

Fixes: 1648eaaa1575e
Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
---
 drivers/mtd/chips/cfi_cmdset_0002.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index c74c53b886be..ee8b70e54298 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -2669,7 +2669,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(
@@ -2685,6 +2685,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++;
 			if (chipnum >= cfi->numchips)
-- 
2.13.6

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

[Boris Brezillon]

On Tue,  5 Jun 2018 16:07:09 +0200
Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote:

> 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).
> 
> In addition, do_ppb_xxlock() failed 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: 1648eaaa1575e

Fixes: 1648eaaa1575 ("mtd: cfi_cmdset_0002: Support Persistent Protection Bits (PPB) locking")

And you probably want to add

Cc: stable@vger.kernel.org

> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> ---
>  drivers/mtd/chips/cfi_cmdset_0002.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index 5e526aec66f0..c74c53b886be 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -2535,7 +2535,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;
>  };
>  
> @@ -2553,8 +2553,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;
> @@ -2572,8 +2573,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
> @@ -2611,7 +2612,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;
> @@ -2670,7 +2671,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);
> @@ -2686,7 +2687,6 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs,
>  		if (adr >> cfi->chipshift) {
>  			adr = 0;
>  			chipnum++;
> -
>  			if (chipnum >= cfi->numchips)
>  				break;
>  		}
> @@ -2714,7 +2714,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 2/2] mtd: cfi_cmdset_0002: Avoid point less unlocking/locking

[Boris Brezillon]

On Tue,  5 Jun 2018 16:07:10 +0200
Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote:

> cfi_ppb_unlock() walks all flash chips when unlocking sectors.
> This is a waste when crossing chip boundaris unaffected

				      ^ boundaries.

> by the unlock operation.

AFAICT there are 2 unrelated changes in this commit:
1/ Fix the boundary check which is broken if the unlock operation
   crosses a chip boundary (adr is reset to 0 when that happens).
2/ Avoid unlocking chips that are outside the requested unlock range.

#1 is a fix, #2 is not. You should split that in 2 different commits,
and tag #1 with Fixes and Cc-stable.

> 
> Fixes: 1648eaaa1575e
> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> ---
>  drivers/mtd/chips/cfi_cmdset_0002.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index c74c53b886be..ee8b70e54298 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -2669,7 +2669,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(
> @@ -2685,6 +2685,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++;
>  			if (chipnum >= cfi->numchips)

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

[Boris Brezillon]

Hello Joakim,

On Tue,  5 Jun 2018 16:07:09 +0200
Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote:

> 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).
> 
> In addition, do_ppb_xxlock() failed 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.

I guess you checked that all functions that are passed adr expect an
absolute address and not an offset relative to the chip.

Also, you seem to fix 2 different problems, can you please split that in
2 commits?

Thanks,

Boris

> 
> Fixes: 1648eaaa1575e
> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> ---
>  drivers/mtd/chips/cfi_cmdset_0002.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index 5e526aec66f0..c74c53b886be 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -2535,7 +2535,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;
>  };
>  
> @@ -2553,8 +2553,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;
> @@ -2572,8 +2573,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
> @@ -2611,7 +2612,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;
> @@ -2670,7 +2671,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);
> @@ -2686,7 +2687,6 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs,
>  		if (adr >> cfi->chipshift) {
>  			adr = 0;
>  			chipnum++;
> -

Please drop this change from the commit.

>  			if (chipnum >= cfi->numchips)
>  				break;
>  		}
> @@ -2714,7 +2714,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);
>  	}
>  

Thanks,

Boris

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

[Joakim Tjernlund]

On Tue, 2018-06-05 at 17:26 +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.
> 
> 
> Hello Joakim,
> 
> On Tue,  5 Jun 2018 16:07:09 +0200
> Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote:
> 
> > 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).
> > 
> > In addition, do_ppb_xxlock() failed 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.
> 
> I guess you checked that all functions that are passed adr expect an
> absolute address and not an offset relative to the chip.

Yes :)

> 
> Also, you seem to fix 2 different problems, can you please split that in
> 2 commits?

No, they might seem different but my unlock problem needs both or I see a SEGV.

> 
> Thanks,
> 
> Boris
> 
> > 
> > Fixes: 1648eaaa1575e
> > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > ---
> >  drivers/mtd/chips/cfi_cmdset_0002.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> > index 5e526aec66f0..c74c53b886be 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > @@ -2535,7 +2535,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;
> >  };
> > 
> > @@ -2553,8 +2553,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;
> > @@ -2572,8 +2573,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
> > @@ -2611,7 +2612,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;
> > @@ -2670,7 +2671,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);
> > @@ -2686,7 +2687,6 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs,
> >               if (adr >> cfi->chipshift) {
> >                       adr = 0;
> >                       chipnum++;
> > -
> 
> Please drop this change from the commit.
> 
> >                       if (chipnum >= cfi->numchips)
> >                               break;
> >               }
> > @@ -2714,7 +2714,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);
> >       }
> > 
> 
> Thanks,
> 
> Boris

Re: [PATCH 2/2] mtd: cfi_cmdset_0002: Avoid point less unlocking/locking

[Joakim Tjernlund]

On Tue, 2018-06-05 at 17: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 Tue,  5 Jun 2018 16:07:10 +0200
> Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote:
> 
> > cfi_ppb_unlock() walks all flash chips when unlocking sectors.
> > This is a waste when crossing chip boundaris unaffected
> 
>                                       ^ boundaries.
> 
> > by the unlock operation.
> 
> AFAICT there are 2 unrelated changes in this commit:
> 1/ Fix the boundary check which is broken if the unlock operation
>    crosses a chip boundary (adr is reset to 0 when that happens).

not broken, the driver will just relock the same sector needlessly.
I did not notice anything breaking when that happened.
These two changes optimizes the unlock operation.
Still need two commits?

> 2/ Avoid unlocking chips that are outside the requested unlock range.
> 
> #1 is a fix, #2 is not. You should split that in 2 different commits,
> and tag #1 with Fixes and Cc-stable.
> 
> > 
> > Fixes: 1648eaaa1575e
> > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > ---
> >  drivers/mtd/chips/cfi_cmdset_0002.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> > index c74c53b886be..ee8b70e54298 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > @@ -2669,7 +2669,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(
> > @@ -2685,6 +2685,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++;
> >                       if (chipnum >= cfi->numchips)
> 
> 

[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 2/2] mtd: cfi_cmdset_0002: Avoid point less unlocking/locking

[Joakim Tjernlund]

On Tue, 2018-06-05 at 18:57 +0200, Joakim Tjernlund wrote:
> On Tue, 2018-06-05 at 17: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 Tue,  5 Jun 2018 16:07:10 +0200
> > Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote:
> > 
> > > cfi_ppb_unlock() walks all flash chips when unlocking sectors.
> > > This is a waste when crossing chip boundaris unaffected
> > 
> >                                       ^ boundaries.
> > 
> > > by the unlock operation.
> > 
> > AFAICT there are 2 unrelated changes in this commit:
> > 1/ Fix the boundary check which is broken if the unlock operation
> >    crosses a chip boundary (adr is reset to 0 when that happens).
> 
> not broken, the driver will just relock the same sector needlessly.
> I did not notice anything breaking when that happened.
> These two changes optimizes the unlock operation.
> Still need two commits?

I just assumed you did and reset both patches split up into 4.

 Jocke

Re: [PATCH 2/2] mtd: cfi_cmdset_0002: Avoid point less unlocking/locking

[Joakim Tjernlund]

On Wed, 2018-06-06 at 12:15 +0200, Joakim Tjernlund wrote:
> On Tue, 2018-06-05 at 18:57 +0200, Joakim Tjernlund wrote:
> > On Tue, 2018-06-05 at 17: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 Tue,  5 Jun 2018 16:07:10 +0200
> > > Joakim Tjernlund <joakim.tjernlund@infinera.com> wrote:
> > > 
> > > > cfi_ppb_unlock() walks all flash chips when unlocking sectors.
> > > > This is a waste when crossing chip boundaris unaffected
> > > 
> > >                                       ^ boundaries.
> > > 
> > > > by the unlock operation.
> > > 
> > > AFAICT there are 2 unrelated changes in this commit:
> > > 1/ Fix the boundary check which is broken if the unlock operation
> > >    crosses a chip boundary (adr is reset to 0 when that happens).
> > 
> > not broken, the driver will just relock the same sector needlessly.
> > I did not notice anything breaking when that happened.
> > These two changes optimizes the unlock operation.
> > Still need two commits?
> 
> I just assumed you did and reset both patches split up into 4.
> 
>  Jocke

Did this get lost? It is a somewhat nasty bug when it hits, you get(and then kernel freezes):

[   38.462607] Unable to handle kernel paging request for data at address 0xd91a0000
[   38.470082] Faulting instruction address: 0xc01b6fec
[   38.475040] Oops: Kernel access of bad area, sig: 11 [#1]
[   38.480418] TMCUTU
[   38.482426] Modules linked in:
[   38.485493] CPU: 0 PID: 437 Comm: fw_setenv Not tainted 4.1.43+ #38
[   38.491748] task: cec0e070 ti: cf8c8000 task.ti: cf8c8000
[   38.497133] NIP: c01b6fec LR: c01b0b04 CTR: c01b6fe4
[   38.502087] REGS: cf8c9c20 TRAP: 0300   Not tainted  (4.1.43+)
[   38.507899] MSR: 00009032 <EE,ME,IR,DR,RI>  CR: 22022428  XER: 20000000
[   38.514539] DAR: d91a0000 DSISR: 22000000 
GPR00: c01b07ec cf8c9cd0 cec0e070 cf89a21c cf8c9cd8 080a0000 0000000f 00000001 
GPR08: c01b6fe4 d1100000 000000a0 04000000 22022422 1001b704 000003ff 00000002 
GPR16: 00000000 04000000 00000002 cec43090 00000000 00020000 00000000 00020000 
GPR24: 00000000 cf8c9cd8 cf93f288 00000001 cf93f200 040a0000 cf89a21c cf93f2a4 
[   38.546903] NIP [c01b6fec] simple_map_write+0x8/0x14
[   38.551882] LR [c01b0b04] do_ppb_xxlock+0x360/0x468
[   38.556743] Call Trace:
[   38.559202] [cf8c9cd0] [c01b07ec] do_ppb_xxlock+0x48/0x468 (unreliable)
[   38.565817] [cf8c9d00] [c01b0e74] cfi_ppb_unlock+0x268/0x360
[   38.571474] [cf8c9d90] [c01a87c4] mtdchar_ioctl+0x750/0x112c
[   38.577131] [cf8c9eb0] [c01a91dc] mtdchar_unlocked_ioctl+0x3c/0x60
[   38.583304] [cf8c9ed0] [c00c3948] do_vfs_ioctl+0x3a4/0x658
[   38.588784] [cf8c9f20] [c00c3c3c] SyS_ioctl+0x40/0x74
[   38.593847] [cf8c9f40] [c000e564] ret_from_syscall+0x0/0x38
[   38.599415] --- interrupt: c01 at 0xff502f8
[   38.599415]     LR = 0xffeca78
[   38.606616] Instruction dump:
[   38.609579] 813c0004 7fe3fb78 913f000c 39200000 913f0008 4bffff2c 8124000c 7d292a2e 
[   38.617343] 91230000 4e800020 a1440002 8123000c <7d492b2e> 7c0004ac 4e800020 81230018 
[   38.625288] ---[ end trace d5c466b3bd564140 ]---

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;