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

Message ID 20180606101330.11071-4-joakim.tjernlund@infinera.com
State New
Delegated to: Boris Brezillon
Headers show
Series
  • [v2,1/4] mtd: cfi_cmdset_0002: Use right chip in do_ppb_xxlock()
Related show

Commit Message

Joakim Tjernlund June 6, 2018, 10:13 a.m.
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(+)

Comments

Boris Brezillon June 20, 2018, 9:25 a.m. | #1
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++;
>
Joakim Tjernlund June 20, 2018, 11:10 a.m. | #2
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++;
> > 
> 
>
Boris Brezillon June 20, 2018, 12:19 p.m. | #3
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.
Joakim Tjernlund June 20, 2018, 3:07 p.m. | #4
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

Patch

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++;