* [RFC PATCH] mtd: lpddr: Fix bad logic in print_drs_error @ 2020-03-30 19:56 Joe Perches 2020-04-27 13:29 ` Miquel Raynal 0 siblings, 1 reply; 4+ messages in thread From: Joe Perches @ 2020-03-30 19:56 UTC (permalink / raw) To: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra Cc: linux-mtd, linux-kernel Update logic for broken test. Use a more common logging style. Miscellanea: o Coalesce formats Signed-off-by: Joe Perches <joe@perches.com> --- Found by inspection of include files using printk. It appears the logic in this function is broken for the consecutive tests of if (prog_status & 0x3) ... else if (prog_status & 0x2) ... else (prog_status & 0x1) ... Likely the first test should be if ((prog_status & 0x3) == 0x3) And this function is only used in drivers/mtd/lpddr/lpddr_cmds.c perhaps it should be moved there. include/linux/mtd/pfow.h | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/include/linux/mtd/pfow.h b/include/linux/mtd/pfow.h index 122f343..1c08e75 100644 --- a/include/linux/mtd/pfow.h +++ b/include/linux/mtd/pfow.h @@ -127,31 +127,28 @@ static inline void print_drs_error(unsigned dsr) int prog_status = (dsr & DSR_RPS) >> 8; if (!(dsr & DSR_AVAILABLE)) - printk(KERN_NOTICE"DSR.15: (0) Device not Available\n"); - if (prog_status & 0x03) - printk(KERN_NOTICE"DSR.9,8: (11) Attempt to program invalid " - "half with 41h command\n"); + pr_notice("DSR.15: (0) Device not Available\n"); + + if ((prog_status & 0x03) == 0x03) + pr_notice("DSR.9,8: (11) Attempt to program invalid half with 41h command\n"); else if (prog_status & 0x02) - printk(KERN_NOTICE"DSR.9,8: (10) Object Mode Program attempt " - "in region with Control Mode data\n"); + pr_notice("DSR.9,8: (10) Object Mode Program attempt in region with Control Mode data\n"); else if (prog_status & 0x01) - printk(KERN_NOTICE"DSR.9,8: (01) Program attempt in region " - "with Object Mode data\n"); + pr_notice("DSR.9,8: (01) Program attempt in region with Object Mode data\n"); + if (!(dsr & DSR_READY_STATUS)) - printk(KERN_NOTICE"DSR.7: (0) Device is Busy\n"); + pr_notice("DSR.7: (0) Device is Busy\n"); if (dsr & DSR_ESS) - printk(KERN_NOTICE"DSR.6: (1) Erase Suspended\n"); + pr_notice("DSR.6: (1) Erase Suspended\n"); if (dsr & DSR_ERASE_STATUS) - printk(KERN_NOTICE"DSR.5: (1) Erase/Blank check error\n"); + pr_notice("DSR.5: (1) Erase/Blank check error\n"); if (dsr & DSR_PROGRAM_STATUS) - printk(KERN_NOTICE"DSR.4: (1) Program Error\n"); + pr_notice("DSR.4: (1) Program Error\n"); if (dsr & DSR_VPPS) - printk(KERN_NOTICE"DSR.3: (1) Vpp low detect, operation " - "aborted\n"); + pr_notice("DSR.3: (1) Vpp low detect, operation aborted\n"); if (dsr & DSR_PSS) - printk(KERN_NOTICE"DSR.2: (1) Program suspended\n"); + pr_notice("DSR.2: (1) Program suspended\n"); if (dsr & DSR_DPS) - printk(KERN_NOTICE"DSR.1: (1) Aborted Erase/Program attempt " - "on locked block\n"); + pr_notice("DSR.1: (1) Aborted Erase/Program attempt on locked block\n"); } #endif /* __LINUX_MTD_PFOW_H */ ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] mtd: lpddr: Fix bad logic in print_drs_error 2020-03-30 19:56 [RFC PATCH] mtd: lpddr: Fix bad logic in print_drs_error Joe Perches @ 2020-04-27 13:29 ` Miquel Raynal 2020-04-27 17:33 ` Joe Perches 0 siblings, 1 reply; 4+ messages in thread From: Miquel Raynal @ 2020-04-27 13:29 UTC (permalink / raw) To: Joe Perches Cc: Richard Weinberger, linux-mtd, Vignesh Raghavendra, linux-kernel Hi Joe, Joe Perches <joe@perches.com> wrote on Mon, 30 Mar 2020 12:56:59 -0700: > Update logic for broken test. > Use a more common logging style. > > Miscellanea: > > o Coalesce formats > > Signed-off-by: Joe Perches <joe@perches.com> > --- > > Found by inspection of include files using printk. > > It appears the logic in this function is broken for the > consecutive tests of > > if (prog_status & 0x3) > ... > else if (prog_status & 0x2) > ... > else (prog_status & 0x1) > ... > > Likely the first test should be > > if ((prog_status & 0x3) == 0x3) I had a hard time understanding the patch just with the commit log, I think the above text is as important. In fact, would you mind doing the printk->pr_notice in a first patch, and fix the wrong condition in a separate change? > > And this function is only used in drivers/mtd/lpddr/lpddr_cmds.c > perhaps it should be moved there. Agreed, do not hesitate to move the function as suggested in a third patch. > > include/linux/mtd/pfow.h | 31 ++++++++++++++----------------- > 1 file changed, 14 insertions(+), 17 deletions(-) > > diff --git a/include/linux/mtd/pfow.h b/include/linux/mtd/pfow.h > index 122f343..1c08e75 100644 > --- a/include/linux/mtd/pfow.h > +++ b/include/linux/mtd/pfow.h > @@ -127,31 +127,28 @@ static inline void print_drs_error(unsigned dsr) > int prog_status = (dsr & DSR_RPS) >> 8; > > if (!(dsr & DSR_AVAILABLE)) > - printk(KERN_NOTICE"DSR.15: (0) Device not Available\n"); > - if (prog_status & 0x03) > - printk(KERN_NOTICE"DSR.9,8: (11) Attempt to program invalid " > - "half with 41h command\n"); > + pr_notice("DSR.15: (0) Device not Available\n"); > + > + if ((prog_status & 0x03) == 0x03) > + pr_notice("DSR.9,8: (11) Attempt to program invalid half with 41h command\n"); > else if (prog_status & 0x02) > - printk(KERN_NOTICE"DSR.9,8: (10) Object Mode Program attempt " > - "in region with Control Mode data\n"); > + pr_notice("DSR.9,8: (10) Object Mode Program attempt in region with Control Mode data\n"); > else if (prog_status & 0x01) > - printk(KERN_NOTICE"DSR.9,8: (01) Program attempt in region " > - "with Object Mode data\n"); > + pr_notice("DSR.9,8: (01) Program attempt in region with Object Mode data\n"); > + > if (!(dsr & DSR_READY_STATUS)) > - printk(KERN_NOTICE"DSR.7: (0) Device is Busy\n"); > + pr_notice("DSR.7: (0) Device is Busy\n"); > if (dsr & DSR_ESS) > - printk(KERN_NOTICE"DSR.6: (1) Erase Suspended\n"); > + pr_notice("DSR.6: (1) Erase Suspended\n"); > if (dsr & DSR_ERASE_STATUS) > - printk(KERN_NOTICE"DSR.5: (1) Erase/Blank check error\n"); > + pr_notice("DSR.5: (1) Erase/Blank check error\n"); > if (dsr & DSR_PROGRAM_STATUS) > - printk(KERN_NOTICE"DSR.4: (1) Program Error\n"); > + pr_notice("DSR.4: (1) Program Error\n"); > if (dsr & DSR_VPPS) > - printk(KERN_NOTICE"DSR.3: (1) Vpp low detect, operation " > - "aborted\n"); > + pr_notice("DSR.3: (1) Vpp low detect, operation aborted\n"); > if (dsr & DSR_PSS) > - printk(KERN_NOTICE"DSR.2: (1) Program suspended\n"); > + pr_notice("DSR.2: (1) Program suspended\n"); > if (dsr & DSR_DPS) > - printk(KERN_NOTICE"DSR.1: (1) Aborted Erase/Program attempt " > - "on locked block\n"); > + pr_notice("DSR.1: (1) Aborted Erase/Program attempt on locked block\n"); > } > #endif /* __LINUX_MTD_PFOW_H */ > > Thanks, Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] mtd: lpddr: Fix bad logic in print_drs_error 2020-04-27 13:29 ` Miquel Raynal @ 2020-04-27 17:33 ` Joe Perches 2020-04-27 18:09 ` Gustavo A. R. Silva 0 siblings, 1 reply; 4+ messages in thread From: Joe Perches @ 2020-04-27 17:33 UTC (permalink / raw) To: Miquel Raynal Cc: Richard Weinberger, linux-mtd, Vignesh Raghavendra, linux-kernel On Mon, 2020-04-27 at 15:29 +0200, Miquel Raynal wrote: > Hi Joe, Hello Miquel. > Joe Perches <joe@perches.com> wrote on Mon, 30 Mar 2020 12:56:59 -0700: > > > Update logic for broken test. > > Use a more common logging style. > > > > Miscellanea: > > > > o Coalesce formats > > > > Signed-off-by: Joe Perches <joe@perches.com> > > --- > > > > Found by inspection of include files using printk. > > > > It appears the logic in this function is broken for the > > consecutive tests of > > > > if (prog_status & 0x3) > > ... > > else if (prog_status & 0x2) > > ... > > else (prog_status & 0x1) > > ... > > > > Likely the first test should be > > > > if ((prog_status & 0x3) == 0x3) > > I had a hard time understanding the patch just with the commit log, I > think the above text is as important. > > In fact, would you mind doing the printk->pr_notice in a first patch, > and fix the wrong condition in a separate change? > > > And this function is only used in drivers/mtd/lpddr/lpddr_cmds.c > > perhaps it should be moved there. > > Agreed, do not hesitate to move the function as suggested in a third > patch. You are the maintainer here no? I think you (or perhaps the author Alexey Korolev but he hasn't been active in a decade) should be doing all this. I just identified the logic defect. > > include/linux/mtd/pfow.h | 31 ++++++++++++++----------------- > > 1 file changed, 14 insertions(+), 17 deletions(-) > > > > diff --git a/include/linux/mtd/pfow.h b/include/linux/mtd/pfow.h > > index 122f343..1c08e75 100644 > > --- a/include/linux/mtd/pfow.h > > +++ b/include/linux/mtd/pfow.h > > @@ -127,31 +127,28 @@ static inline void print_drs_error(unsigned dsr) > > int prog_status = (dsr & DSR_RPS) >> 8; > > > > if (!(dsr & DSR_AVAILABLE)) > > - printk(KERN_NOTICE"DSR.15: (0) Device not Available\n"); > > - if (prog_status & 0x03) > > - printk(KERN_NOTICE"DSR.9,8: (11) Attempt to program invalid " > > - "half with 41h command\n"); > > + pr_notice("DSR.15: (0) Device not Available\n"); > > + > > + if ((prog_status & 0x03) == 0x03) > > + pr_notice("DSR.9,8: (11) Attempt to program invalid half with 41h command\n"); > > else if (prog_status & 0x02) > > - printk(KERN_NOTICE"DSR.9,8: (10) Object Mode Program attempt " > > - "in region with Control Mode data\n"); > > + pr_notice("DSR.9,8: (10) Object Mode Program attempt in region with Control Mode data\n"); > > else if (prog_status & 0x01) > > - printk(KERN_NOTICE"DSR.9,8: (01) Program attempt in region " > > - "with Object Mode data\n"); > > + pr_notice("DSR.9,8: (01) Program attempt in region with Object Mode data\n"); > > + > > if (!(dsr & DSR_READY_STATUS)) > > - printk(KERN_NOTICE"DSR.7: (0) Device is Busy\n"); > > + pr_notice("DSR.7: (0) Device is Busy\n"); > > if (dsr & DSR_ESS) > > - printk(KERN_NOTICE"DSR.6: (1) Erase Suspended\n"); > > + pr_notice("DSR.6: (1) Erase Suspended\n"); > > if (dsr & DSR_ERASE_STATUS) > > - printk(KERN_NOTICE"DSR.5: (1) Erase/Blank check error\n"); > > + pr_notice("DSR.5: (1) Erase/Blank check error\n"); > > if (dsr & DSR_PROGRAM_STATUS) > > - printk(KERN_NOTICE"DSR.4: (1) Program Error\n"); > > + pr_notice("DSR.4: (1) Program Error\n"); > > if (dsr & DSR_VPPS) > > - printk(KERN_NOTICE"DSR.3: (1) Vpp low detect, operation " > > - "aborted\n"); > > + pr_notice("DSR.3: (1) Vpp low detect, operation aborted\n"); > > if (dsr & DSR_PSS) > > - printk(KERN_NOTICE"DSR.2: (1) Program suspended\n"); > > + pr_notice("DSR.2: (1) Program suspended\n"); > > if (dsr & DSR_DPS) > > - printk(KERN_NOTICE"DSR.1: (1) Aborted Erase/Program attempt " > > - "on locked block\n"); > > + pr_notice("DSR.1: (1) Aborted Erase/Program attempt on locked block\n"); > > } > > #endif /* __LINUX_MTD_PFOW_H */ > > > > > > Thanks, > Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] mtd: lpddr: Fix bad logic in print_drs_error 2020-04-27 17:33 ` Joe Perches @ 2020-04-27 18:09 ` Gustavo A. R. Silva 0 siblings, 0 replies; 4+ messages in thread From: Gustavo A. R. Silva @ 2020-04-27 18:09 UTC (permalink / raw) To: Joe Perches, Miquel Raynal Cc: Richard Weinberger, linux-mtd, Vignesh Raghavendra, linux-kernel Hi, It seems that this fix should be tagged for -stable. I can create a three-patch series for this (as Miquel suggested), starting with the patch that fix the wrong condition, so it can be ported to -stable, separately. I'll include you Reported-by, Joe. Thanks -- Gustavo On 4/27/20 12:33, Joe Perches wrote: > On Mon, 2020-04-27 at 15:29 +0200, Miquel Raynal wrote: >> Hi Joe, > > Hello Miquel. > >> Joe Perches <joe@perches.com> wrote on Mon, 30 Mar 2020 12:56:59 -0700: >> >>> Update logic for broken test. >>> Use a more common logging style. >>> >>> Miscellanea: >>> >>> o Coalesce formats >>> >>> Signed-off-by: Joe Perches <joe@perches.com> >>> --- >>> >>> Found by inspection of include files using printk. >>> >>> It appears the logic in this function is broken for the >>> consecutive tests of >>> >>> if (prog_status & 0x3) >>> ... >>> else if (prog_status & 0x2) >>> ... >>> else (prog_status & 0x1) >>> ... >>> >>> Likely the first test should be >>> >>> if ((prog_status & 0x3) == 0x3) >> >> I had a hard time understanding the patch just with the commit log, I >> think the above text is as important. >> >> In fact, would you mind doing the printk->pr_notice in a first patch, >> and fix the wrong condition in a separate change? >> >>> And this function is only used in drivers/mtd/lpddr/lpddr_cmds.c >>> perhaps it should be moved there. >> >> Agreed, do not hesitate to move the function as suggested in a third >> patch. > > You are the maintainer here no? > > I think you (or perhaps the author Alexey Korolev but he hasn't > been active in a decade) should be doing all this. > > I just identified the logic defect. > >>> include/linux/mtd/pfow.h | 31 ++++++++++++++----------------- >>> 1 file changed, 14 insertions(+), 17 deletions(-) >>> >>> diff --git a/include/linux/mtd/pfow.h b/include/linux/mtd/pfow.h >>> index 122f343..1c08e75 100644 >>> --- a/include/linux/mtd/pfow.h >>> +++ b/include/linux/mtd/pfow.h >>> @@ -127,31 +127,28 @@ static inline void print_drs_error(unsigned dsr) >>> int prog_status = (dsr & DSR_RPS) >> 8; >>> >>> if (!(dsr & DSR_AVAILABLE)) >>> - printk(KERN_NOTICE"DSR.15: (0) Device not Available\n"); >>> - if (prog_status & 0x03) >>> - printk(KERN_NOTICE"DSR.9,8: (11) Attempt to program invalid " >>> - "half with 41h command\n"); >>> + pr_notice("DSR.15: (0) Device not Available\n"); >>> + >>> + if ((prog_status & 0x03) == 0x03) >>> + pr_notice("DSR.9,8: (11) Attempt to program invalid half with 41h command\n"); >>> else if (prog_status & 0x02) >>> - printk(KERN_NOTICE"DSR.9,8: (10) Object Mode Program attempt " >>> - "in region with Control Mode data\n"); >>> + pr_notice("DSR.9,8: (10) Object Mode Program attempt in region with Control Mode data\n"); >>> else if (prog_status & 0x01) >>> - printk(KERN_NOTICE"DSR.9,8: (01) Program attempt in region " >>> - "with Object Mode data\n"); >>> + pr_notice("DSR.9,8: (01) Program attempt in region with Object Mode data\n"); >>> + >>> if (!(dsr & DSR_READY_STATUS)) >>> - printk(KERN_NOTICE"DSR.7: (0) Device is Busy\n"); >>> + pr_notice("DSR.7: (0) Device is Busy\n"); >>> if (dsr & DSR_ESS) >>> - printk(KERN_NOTICE"DSR.6: (1) Erase Suspended\n"); >>> + pr_notice("DSR.6: (1) Erase Suspended\n"); >>> if (dsr & DSR_ERASE_STATUS) >>> - printk(KERN_NOTICE"DSR.5: (1) Erase/Blank check error\n"); >>> + pr_notice("DSR.5: (1) Erase/Blank check error\n"); >>> if (dsr & DSR_PROGRAM_STATUS) >>> - printk(KERN_NOTICE"DSR.4: (1) Program Error\n"); >>> + pr_notice("DSR.4: (1) Program Error\n"); >>> if (dsr & DSR_VPPS) >>> - printk(KERN_NOTICE"DSR.3: (1) Vpp low detect, operation " >>> - "aborted\n"); >>> + pr_notice("DSR.3: (1) Vpp low detect, operation aborted\n"); >>> if (dsr & DSR_PSS) >>> - printk(KERN_NOTICE"DSR.2: (1) Program suspended\n"); >>> + pr_notice("DSR.2: (1) Program suspended\n"); >>> if (dsr & DSR_DPS) >>> - printk(KERN_NOTICE"DSR.1: (1) Aborted Erase/Program attempt " >>> - "on locked block\n"); >>> + pr_notice("DSR.1: (1) Aborted Erase/Program attempt on locked block\n"); >>> } >>> #endif /* __LINUX_MTD_PFOW_H */ >>> >>> >> >> Thanks, >> Miquèl > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-04-27 18:05 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-30 19:56 [RFC PATCH] mtd: lpddr: Fix bad logic in print_drs_error Joe Perches 2020-04-27 13:29 ` Miquel Raynal 2020-04-27 17:33 ` Joe Perches 2020-04-27 18:09 ` Gustavo A. R. Silva
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).