* [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.