| Message ID | 20180606101330.11071-4-joakim.tjernlund@infinera.com |
|---|---|
| State | Accepted |
| Delegated to: | Boris Brezillon |
| Headers | show |
| Series | [v2,1/4] mtd: cfi_cmdset_0002: Use right chip in do_ppb_xxlock() | expand |
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++; >
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++; > > > >
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.
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
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++;
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(+)