diff mbox series

[v7,5/5] mtd: cfi_cmdset_0002: Change erase one block to enable XIP once

Message ID 20180527232801.1666-6-ikegami@allied-telesis.co.jp
State Superseded
Delegated to: Boris Brezillon
Headers show
Series mtd: cfi_cmdset_0002: Change write and erase functions | expand

Commit Message

IKEGAMI Tokunori May 27, 2018, 11:28 p.m. UTC 
To enable XIP it is executed both normal and error cases.
This call can be moved after the for loop as same with erase chip.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Reviewed-by: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Marek Vasut <marek.vasut@gmail.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
Cc: linux-mtd@lists.infradead.org
Cc: stable@vger.kernel.org
---
 drivers/mtd/chips/cfi_cmdset_0002.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Boris Brezillon May 29, 2018, 6:57 p.m. UTC | #1 
On Mon, 28 May 2018 08:28:01 +0900
Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote:

> To enable XIP it is executed both normal and error cases.
> This call can be moved after the for loop as same with erase chip.
> 
> Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> Reviewed-by: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Cc: Brian Norris <computersforpeace@gmail.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Marek Vasut <marek.vasut@gmail.com>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
> Cc: linux-mtd@lists.infradead.org
> Cc: stable@vger.kernel.org

Is this really a bug fix? Doesn't look like a bug fix to me.

Also, every time you add Cc stable you should try to find the commit
that introduced the bug. Sometime it's not possible because the bug
existed before git was in use, but most of the time you'll find the
offending commit using git blame.

A fixes tag should be formatted like that:

Fixes: <commit-id> ("commit subject")

> ---
>  drivers/mtd/chips/cfi_cmdset_0002.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index 6adda4dc2007..6c681cbf2d37 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -2389,13 +2389,10 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
>  			chip->erase_suspended = 0;
>  		}
>  
> -		if (chip_good(map, adr, map_word_ff(map))) {
> -			xip_enable(map, chip, adr);
> +		if (chip_good(map, adr, map_word_ff(map)))
>  			break;
> -		}
>  
>  		if (time_after(jiffies, timeo)) {
> -			xip_enable(map, chip, adr);
>  			printk(KERN_WARNING "MTD %s(): software timeout\n",
>  			       __func__);
>  			ret = -EIO;
> @@ -2418,6 +2415,7 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
>  	}
>  
>  	chip->state = FL_READY;
> +	xip_enable(map, chip, adr);
>  	DISABLE_VPP(map);
>  	put_chip(map, chip, adr);
>  	mutex_unlock(&chip->mutex);
IKEGAMI Tokunori May 29, 2018, 11:31 p.m. UTC | #2 
Hi Boris-san,

Thanks for your reviewing and advices.

> Is this really a bug fix? Doesn't look like a bug fix to me.

  No as you mentioned it is not a bug fix but just a refactoring to reduce xip_enable() line.

> Also, every time you add Cc stable you should try to find the commit
> that introduced the bug. Sometime it's not possible because the bug
> existed before git was in use, but most of the time you'll find the
> offending commit using git blame.
> 
> A fixes tag should be formatted like that:
> 
> Fixes: <commit-id> ("commit subject")

  Okay I will do that in future.

  This is just FYI.
  I have just confirmed that the xip_enable() line itself was implemented by the commit 02b15e343aeef.
  For this patch it is not a bug fix so I will not add the Fixes line into the commit message.
  But if needed it please let me know that.

Regards,
Ikegami

> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezillon@bootlin.com]
> Sent: Wednesday, May 30, 2018 3:58 AM
> To: IKEGAMI Tokunori
> Cc: PACKHAM Chris; Brian Norris; David Woodhouse; Boris Brezillon; Marek
> Vasut; Richard Weinberger; Cyrille Pitchen;
> linux-mtd@lists.infradead.org; stable@vger.kernel.org
> Subject: Re: [PATCH v7 5/5] mtd: cfi_cmdset_0002: Change erase one block
> to enable XIP once
> 
> On Mon, 28 May 2018 08:28:01 +0900
> Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote:
> 
> > To enable XIP it is executed both normal and error cases.
> > This call can be moved after the for loop as same with erase chip.
> >
> > Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> > Reviewed-by: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > Cc: Brian Norris <computersforpeace@gmail.com>
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Cc: Marek Vasut <marek.vasut@gmail.com>
> > Cc: Richard Weinberger <richard@nod.at>
> > Cc: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
> > Cc: linux-mtd@lists.infradead.org
> > Cc: stable@vger.kernel.org
> 
> Is this really a bug fix? Doesn't look like a bug fix to me.
> 
> Also, every time you add Cc stable you should try to find the commit
> that introduced the bug. Sometime it's not possible because the bug
> existed before git was in use, but most of the time you'll find the
> offending commit using git blame.
> 
> A fixes tag should be formatted like that:
> 
> Fixes: <commit-id> ("commit subject")
> 
> > ---
> >  drivers/mtd/chips/cfi_cmdset_0002.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
> b/drivers/mtd/chips/cfi_cmdset_0002.c
> > index 6adda4dc2007..6c681cbf2d37 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > @@ -2389,13 +2389,10 @@ static int __xipram do_erase_oneblock(struct
> map_info *map, struct flchip *chip,
> >  			chip->erase_suspended = 0;
> >  		}
> >
> > -		if (chip_good(map, adr, map_word_ff(map))) {
> > -			xip_enable(map, chip, adr);
> > +		if (chip_good(map, adr, map_word_ff(map)))
> >  			break;
> > -		}
> >
> >  		if (time_after(jiffies, timeo)) {
> > -			xip_enable(map, chip, adr);
> >  			printk(KERN_WARNING "MTD %s(): software
> timeout\n",
> >  			       __func__);
> >  			ret = -EIO;
> > @@ -2418,6 +2415,7 @@ static int __xipram do_erase_oneblock(struct
> map_info *map, struct flchip *chip,
> >  	}
> >
> >  	chip->state = FL_READY;
> > +	xip_enable(map, chip, adr);
> >  	DISABLE_VPP(map);
> >  	put_chip(map, chip, adr);
> >  	mutex_unlock(&chip->mutex);
Boris Brezillon May 30, 2018, 8:49 a.m. UTC | #3 
Hello,

On Tue, 29 May 2018 23:31:41 +0000
IKEGAMI Tokunori <ikegami@allied-telesis.co.jp> wrote:

> Hi Boris-san,
> 
> Thanks for your reviewing and advices.
> 
> > Is this really a bug fix? Doesn't look like a bug fix to me.  
> 
>   No as you mentioned it is not a bug fix but just a refactoring to reduce xip_enable() line.

Then you should drop the Cc: stable tag.

> 
> > Also, every time you add Cc stable you should try to find the commit
> > that introduced the bug. Sometime it's not possible because the bug
> > existed before git was in use, but most of the time you'll find the
> > offending commit using git blame.
> > 
> > A fixes tag should be formatted like that:
> > 
> > Fixes: <commit-id> ("commit subject")  
> 
>   Okay I will do that in future.
> 
>   This is just FYI.
>   I have just confirmed that the xip_enable() line itself was implemented by the commit 02b15e343aeef.
>   For this patch it is not a bug fix so I will not add the Fixes line into the commit message.

Right.

>   But if needed it please let me know that.

I checked the first patch and it seems it's one of these situation
where the code predates git, so no need to specify a Fixes tag.

Thanks,

Boris
IKEGAMI Tokunori May 30, 2018, 9:39 a.m. UTC | #4 
Hi Boris-san,

I have just sent v8 version patch series and it changed the 5/5 patch only.

> Then you should drop the Cc: stable tag.

  Yes just dropped it from 5/5 patch only by the v8 version patch series.

> >   I have just confirmed that the xip_enable() line itself was implemented
> by the commit 02b15e343aeef.
> >   For this patch it is not a bug fix so I will not add the Fixes line
> into the commit message.
> 
> Right.

  I see.

> >   But if needed it please let me know that.
> 
> I checked the first patch and it seems it's one of these situation
> where the code predates git, so no need to specify a Fixes tag.

  I see so I will not change other 1/5 to 4/5 patches by the v8 version patch series.

Regards,
Ikegami

> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezillon@bootlin.com]
> Sent: Wednesday, May 30, 2018 5:49 PM
> To: IKEGAMI Tokunori
> Cc: PACKHAM Chris; Brian Norris; David Woodhouse; Boris Brezillon; Marek
> Vasut; Richard Weinberger; Cyrille Pitchen;
> linux-mtd@lists.infradead.org; stable@vger.kernel.org
> Subject: Re: [PATCH v7 5/5] mtd: cfi_cmdset_0002: Change erase one block
> to enable XIP once
> 
> Hello,
> 
> On Tue, 29 May 2018 23:31:41 +0000
> IKEGAMI Tokunori <ikegami@allied-telesis.co.jp> wrote:
> 
> > Hi Boris-san,
> >
> > Thanks for your reviewing and advices.
> >
> > > Is this really a bug fix? Doesn't look like a bug fix to me.
> >
> >   No as you mentioned it is not a bug fix but just a refactoring to reduce
> xip_enable() line.
> 
> Then you should drop the Cc: stable tag.
> 
> >
> > > Also, every time you add Cc stable you should try to find the commit
> > > that introduced the bug. Sometime it's not possible because the bug
> > > existed before git was in use, but most of the time you'll find the
> > > offending commit using git blame.
> > >
> > > A fixes tag should be formatted like that:
> > >
> > > Fixes: <commit-id> ("commit subject")
> >
> >   Okay I will do that in future.
> >
> >   This is just FYI.
> >   I have just confirmed that the xip_enable() line itself was implemented
> by the commit 02b15e343aeef.
> >   For this patch it is not a bug fix so I will not add the Fixes line
> into the commit message.
> 
> Right.
> 
> >   But if needed it please let me know that.
> 
> I checked the first patch and it seems it's one of these situation
> where the code predates git, so no need to specify a Fixes tag.
> 
> Thanks,
> 
> Boris
diff mbox series

Patch

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 6adda4dc2007..6c681cbf2d37 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -2389,13 +2389,10 @@  static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
 			chip->erase_suspended = 0;
 		}
 
-		if (chip_good(map, adr, map_word_ff(map))) {
-			xip_enable(map, chip, adr);
+		if (chip_good(map, adr, map_word_ff(map)))
 			break;
-		}
 
 		if (time_after(jiffies, timeo)) {
-			xip_enable(map, chip, adr);
 			printk(KERN_WARNING "MTD %s(): software timeout\n",
 			       __func__);
 			ret = -EIO;
@@ -2418,6 +2415,7 @@  static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
 	}
 
 	chip->state = FL_READY;
+	xip_enable(map, chip, adr);
 	DISABLE_VPP(map);
 	put_chip(map, chip, adr);
 	mutex_unlock(&chip->mutex);