* [RFC PATCH] mtd: lpddr: Fix bad logic in print_drs_error @ 2020-03-30 19:56 ` Joe Perches 0 siblings, 0 replies; 8+ 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] 8+ messages in thread
* [RFC PATCH] mtd: lpddr: Fix bad logic in print_drs_error @ 2020-03-30 19:56 ` Joe Perches 0 siblings, 0 replies; 8+ 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 */ ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [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 -1 siblings, 0 replies; 8+ 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] 8+ messages in thread
* Re: [RFC PATCH] mtd: lpddr: Fix bad logic in print_drs_error @ 2020-04-27 13:29 ` Miquel Raynal 0 siblings, 0 replies; 8+ messages in thread From: Miquel Raynal @ 2020-04-27 13:29 UTC (permalink / raw) To: Joe Perches Cc: Richard Weinberger, Vignesh Raghavendra, linux-mtd, 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 ^ permalink raw reply [flat|nested] 8+ 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 -1 siblings, 0 replies; 8+ 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] 8+ messages in thread
* Re: [RFC PATCH] mtd: lpddr: Fix bad logic in print_drs_error @ 2020-04-27 17:33 ` Joe Perches 0 siblings, 0 replies; 8+ messages in thread From: Joe Perches @ 2020-04-27 17:33 UTC (permalink / raw) To: Miquel Raynal Cc: Richard Weinberger, Vignesh Raghavendra, linux-mtd, 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 ^ permalink raw reply [flat|nested] 8+ 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 -1 siblings, 0 replies; 8+ 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] 8+ messages in thread
* Re: [RFC PATCH] mtd: lpddr: Fix bad logic in print_drs_error @ 2020-04-27 18:09 ` Gustavo A. R. Silva 0 siblings, 0 replies; 8+ messages in thread From: Gustavo A. R. Silva @ 2020-04-27 18:09 UTC (permalink / raw) To: Joe Perches, Miquel Raynal Cc: Richard Weinberger, Vignesh Raghavendra, linux-mtd, 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-04-27 18:26 UTC | newest] Thread overview: 8+ 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-03-30 19:56 ` Joe Perches 2020-04-27 13:29 ` Miquel Raynal 2020-04-27 13:29 ` Miquel Raynal 2020-04-27 17:33 ` Joe Perches 2020-04-27 17:33 ` Joe Perches 2020-04-27 18:09 ` Gustavo A. R. Silva 2020-04-27 18:09 ` Gustavo A. R. Silva
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.