| Message ID | 20180605140710.8624-2-joakim.tjernlund@infinera.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] mtd: cfi_cmdset_0002: fix SEGV unlocking multiple chips | expand |
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)
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) > >
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
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 ]---
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)
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(-)