mtd: lpddr: Fix bad logic bug in print_drs_error

PATCH Applied Gustavo A. R. Silva 2 participants 11 messages Apr 27, 2020 – Sep 7, 2020 Memory technology devices (MTD)

lore

Share & tools

lore

Gustavo A. R. Silva

author

2020-04-27 19:45 UTC #0 lore raw

v2 cover mtd: lpddr: Fix bad logic bug in print_drs_error

Hi,

This series aims to fix a bad logic bug in print_drs_error, which is tagged for -stable. The series also include some formatting fixups.

Thanks

Changes in v2:
- Coalesce formats in Patch 2.
- Remove inline from print_drs_error in Patch 3.
- Update changelog texts according to the last
changes.

Gustavo A. R. Silva (3):
mtd: lpddr: Fix bad logic in print_drs_error
mtd: lpddr: Replace printk with pr_notice
mtd: lpddr: Move function print_drs_error to lpddr_cmds.c

 drivers/mtd/lpddr/lpddr_cmds.c | 28 ++++++++++++++++++++++++++++
 include/linux/mtd/pfow.h       | 33 ---------------------------------
 2 files changed, 28 insertions(+), 33 deletions(-)

Signature · 6 lines

2.26.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Gustavo A. R. Silva

author

2020-04-27 19:46 UTC #1 lore raw

PATCH v2 1/3 mtd: lpddr: Fix bad logic in print_drs_error

Update logic for broken test. Use a more common logging style.

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)

Found by inspection of include files using printk.

Fixes eb3db27507f7 ("[MTD] LPDDR PFOW definition")

Cc [email protected]

Reported-by Joe Perches <[email protected]>

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
Changes in v2:
- None.

 include/linux/mtd/pfow.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/linux/mtd/pfow.h +1 -1

index 122f3439e1af..c65d7a3be3c6 100644
@@ -128,7 +128,7 @@ static inline void print_drs_error(unsigned dsr)
 
 	if (!(dsr & DSR_AVAILABLE))
 		printk(KERN_NOTICE"DSR.15: (0) Device not Available\n");
-	if (prog_status & 0x03)
+	if ((prog_status & 0x03) == 0x03)
 		printk(KERN_NOTICE"DSR.9,8: (11) Attempt to program invalid "
 						"half with 41h command\n");
 	else if (prog_status & 0x02)

Signature · 6 lines

2.26.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Miquel Raynal

maintainer Acked-by on v2 1/3

2020-04-27 20:08 UTC #2 lore raw

Re: [PATCH v2 1/3] mtd: lpddr: Fix bad logic in print_drs_error

Hi Gustavo,

"Gustavo A. R. Silva" <[email protected]> wrote on Mon, 27 Apr 2020 14:50:37 -0500:

Quoted 34 lines

Update logic for broken test. Use a more common logging style.
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)
Found by inspection of include files using printk.
Fixes eb3db27507f7 ("[MTD] LPDDR PFOW definition")
Cc [email protected]
Reported-by Joe Perches <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
Changes in v2:
- None.
 include/linux/mtd/pfow.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/mtd/pfow.h +0 -0
index 122f3439e1af..c65d7a3be3c6 100644

include/linux/mtd/pfow.h static inline void print_drs_error(unsigned dsr) · line 134

Quoted 8 lines

include/linux/mtd/pfow.h +1 -1

@@ -128,7 +128,7 @@ static inline void print_drs_error(unsigned dsr)
 
 	if (!(dsr & DSR_AVAILABLE))
 		printk(KERN_NOTICE"DSR.15: (0) Device not Available\n");
-	if (prog_status & 0x03)
+	if ((prog_status & 0x03) == 0x03)
 		printk(KERN_NOTICE"DSR.9,8: (11) Attempt to program invalid "
 						"half with 41h command\n");
 	else if (prog_status & 0x02)

Acked-by Miquel Raynal <[email protected]>

Thanks,
Miquèl

Gustavo A. R. Silva

author

2020-05-19 16:17 UTC #3 lore raw

Re: [PATCH v2 1/3] mtd: lpddr: Fix bad logic in print_drs_error

Hi all,

Friendly ping: who can take this?

Thanks
--
Gustavo

Quoted 55 lines · Miquel Raynal

Hi Gustavo,
"Gustavo A. R. Silva" <[email protected]> wrote on Mon, 27 Apr 2020 14:50:37 -0500:
Quoted 34 lines
Update logic for broken test. Use a more common logging style.
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)
Found by inspection of include files using printk.
Fixes eb3db27507f7 ("[MTD] LPDDR PFOW definition")
Cc [email protected]
Reported-by Joe Perches <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
Changes in v2:
- None.
 include/linux/mtd/pfow.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/mtd/pfow.h +0 -0
index 122f3439e1af..c65d7a3be3c6 100644
include/linux/mtd/pfow.h static inline void print_drs_error(unsigned dsr) · line 134
Quoted 8 lines
include/linux/mtd/pfow.h +1 -1
@@ -128,7 +128,7 @@ static inline void print_drs_error(unsigned dsr)
 
 	if (!(dsr & DSR_AVAILABLE))
 		printk(KERN_NOTICE"DSR.15: (0) Device not Available\n");
-	if (prog_status & 0x03)
+	if ((prog_status & 0x03) == 0x03)
 		printk(KERN_NOTICE"DSR.9,8: (11) Attempt to program invalid "
 						"half with 41h command\n");
 	else if (prog_status & 0x02)
Acked-by Miquel Raynal <[email protected]>
Thanks,
Miquèl

Miquel Raynal

maintainer Applied

2020-09-07 07:34 UTC #4 lore raw

Re: [PATCH v2 1/3] mtd: lpddr: Fix bad logic in print_drs_error

In reply to Gustavo A. R. Silva

Quoted 16 lines · Gustavo A. R. Silva

Update logic for broken test. Use a more common logging style.
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)

Quoted 7 lines

Found by inspection of include files using printk.
Fixes eb3db27507f7 ("[MTD] LPDDR PFOW definition")
Cc [email protected]
Reported-by Joe Perches <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>
Acked-by Miquel Raynal <[email protected]>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

Gustavo A. R. Silva

author

2020-04-27 19:49 UTC #5 lore raw

PATCH v2 2/3 mtd: lpddr: Replace printk with pr_notice

In reply to Gustavo A. R. Silva

pr_notice is preferred over printk.

Also, coalesce formats as coalescing is part of coding-style:
"never break user-visible strings such as printk messages"

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
Changes in v2:
- Coalesce formats and update changelog text, accordingly.

 include/linux/mtd/pfow.h | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

include/linux/mtd/pfow.h +11 ~~-16~~

index c65d7a3be3c6..cd21c6768065 100644
@@ -127,31 +127,26 @@ 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");
+		pr_notice("DSR.15: (0) Device not Available\n");
 	if ((prog_status & 0x03) == 0x03)
-		printk(KERN_NOTICE"DSR.9,8: (11) Attempt to program invalid "
-						"half with 41h command\n");
+		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 */

Signature · 1 line

2.26.0

Miquel Raynal

maintainer Reviewed-by on v2 2/3

2020-04-27 20:09 UTC #6 lore raw

Re: [PATCH v2 2/3] mtd: lpddr: Replace printk with pr_notice

Hi Gustavo,

"Gustavo A. R. Silva" <[email protected]> wrote on Mon, 27 Apr 2020 14:54:13 -0500:

Quoted 51 lines

pr_notice is preferred over printk.

Also, coalesce formats as coalescing is part of coding-style:
"never break user-visible strings such as printk messages"

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
Changes in v2:
- Coalesce formats and update changelog text, accordingly.

 include/linux/mtd/pfow.h | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

include/linux/mtd/pfow.h +8 ~~-13~~

index c65d7a3be3c6..cd21c6768065 100644
@@ -127,31 +127,26 @@ 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");
+		pr_notice("DSR.15: (0) Device not Available\n");
 	if ((prog_status & 0x03) == 0x03)
-		printk(KERN_NOTICE"DSR.9,8: (11) Attempt to program invalid "
-						"half with 41h command\n");
+		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");

include/linux/mtd/pfow.h static inline void print_drs_error(unsigned dsr) · line 152

Quoted 10 lines

include/linux/mtd/pfow.h +3 -3

+		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 */

Reviewed-by Miquel Raynal <[email protected]>

Thanks,
Miquèl

Miquel Raynal

maintainer Applied

2020-09-07 07:35 UTC #7 lore raw

Re: [PATCH v2 2/3] mtd: lpddr: Replace printk with pr_notice

In reply to Gustavo A. R. Silva

Quoted 8 lines · Gustavo A. R. Silva

pr_notice is preferred over printk.
Also, coalesce formats as coalescing is part of coding-style:
"never break user-visible strings such as printk messages"
Suggested-by Joe Perches <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>
Reviewed-by Miquel Raynal <[email protected]>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

Gustavo A. R. Silva

author

2020-04-27 19:51 UTC #8 lore raw

PATCH v2 3/3 mtd: lpddr: Move function print_drs_error to lpddr_cmds.c

In reply to Gustavo A. R. Silva

Function print_drs_error is only used in drivers/mtd/lpddr/lpddr_cmds.c so, better to move it there.

Also, notice that there's no need for inline as the function is used once. Lastly, fix the following checkpatch warning:

WARNING: Prefer 'unsigned int' to bare use of 'unsigned' +static void print_drs_error(unsigned dsr)

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
Changes in v2:
- Remove inline from print_drs_error and update changelog text, accordingly.

 drivers/mtd/lpddr/lpddr_cmds.c | 28 ++++++++++++++++++++++++++++
 include/linux/mtd/pfow.h       | 28 ----------------------------
 2 files changed, 28 insertions(+), 28 deletions(-)

drivers/mtd/lpddr/lpddr_cmds.c +28 -0

index fb1cbc9a2870..ee063baed136 100644
@@ -94,6 +94,34 @@ struct mtd_info *lpddr_cmdset(struct map_info *map)
 }
 EXPORT_SYMBOL(lpddr_cmdset);
 
+static void print_drs_error(unsigned int dsr)
+{
+	int prog_status = (dsr & DSR_RPS) >> 8;
+
+	if (!(dsr & DSR_AVAILABLE))
+		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)
+		pr_notice("DSR.9,8: (10) Object Mode Program attempt in region with Control Mode data\n");
+	else if (prog_status &  0x01)
+		pr_notice("DSR.9,8: (01) Program attempt in region with Object Mode data\n");
+	if (!(dsr & DSR_READY_STATUS))
+		pr_notice("DSR.7: (0) Device is Busy\n");
+	if (dsr & DSR_ESS)
+		pr_notice("DSR.6: (1) Erase Suspended\n");
+	if (dsr & DSR_ERASE_STATUS)
+		pr_notice("DSR.5: (1) Erase/Blank check error\n");
+	if (dsr & DSR_PROGRAM_STATUS)
+		pr_notice("DSR.4: (1) Program Error\n");
+	if (dsr & DSR_VPPS)
+		pr_notice("DSR.3: (1) Vpp low detect, operation aborted\n");
+	if (dsr & DSR_PSS)
+		pr_notice("DSR.2: (1) Program suspended\n");
+	if (dsr & DSR_DPS)
+		pr_notice("DSR.1: (1) Aborted Erase/Program attempt on locked block\n");
+}
+
 static int wait_for_ready(struct map_info *map, struct flchip *chip,
 		unsigned int chip_op_time)
 {

include/linux/mtd/pfow.h +0 ~~-28~~

index cd21c6768065..b3eae82eb03c 100644
@@ -121,32 +121,4 @@ static inline void send_pfow_command(struct map_info *map,
 	map_write(map, CMD(LPDDR_START_EXECUTION),
 			map->pfow_base + PFOW_COMMAND_EXECUTE);
 }
-
-static inline void print_drs_error(unsigned dsr)
-{
-	int prog_status = (dsr & DSR_RPS) >> 8;
-
-	if (!(dsr & DSR_AVAILABLE))
-		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)
-		pr_notice("DSR.9,8: (10) Object Mode Program attempt in region with Control Mode data\n");
-	else if (prog_status &  0x01)
-		pr_notice("DSR.9,8: (01) Program attempt in region with Object Mode data\n");
-	if (!(dsr & DSR_READY_STATUS))
-		pr_notice("DSR.7: (0) Device is Busy\n");
-	if (dsr & DSR_ESS)
-		pr_notice("DSR.6: (1) Erase Suspended\n");
-	if (dsr & DSR_ERASE_STATUS)
-		pr_notice("DSR.5: (1) Erase/Blank check error\n");
-	if (dsr & DSR_PROGRAM_STATUS)
-		pr_notice("DSR.4: (1) Program Error\n");
-	if (dsr & DSR_VPPS)
-		pr_notice("DSR.3: (1) Vpp low detect, operation aborted\n");
-	if (dsr & DSR_PSS)
-		pr_notice("DSR.2: (1) Program suspended\n");
-	if (dsr & DSR_DPS)
-		pr_notice("DSR.1: (1) Aborted Erase/Program attempt on locked block\n");
-}
 #endif /* __LINUX_MTD_PFOW_H */

Signature · 1 line

2.26.0

Miquel Raynal

maintainer Reviewed-by on v2 3/3

2020-04-27 20:09 UTC #9 lore raw

Re: [PATCH v2 3/3] mtd: lpddr: Move function print_drs_error to lpddr_cmds.c

Hi Gustavo,

"Gustavo A. R. Silva" <[email protected]> wrote on Mon, 27 Apr 2020 14:56:08 -0500:

Quoted 9 lines

Function print_drs_error is only used in drivers/mtd/lpddr/lpddr_cmds.c so, better to move it there.
Also, notice that there's no need for inline as the function is used once. Lastly, fix the following checkpatch warning:
WARNING: Prefer 'unsigned int' to bare use of 'unsigned' +static void print_drs_error(unsigned dsr)

Quoted 7 lines

Suggested-by Joe Perches <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
Changes in v2:
- Remove inline from print_drs_error and update changelog text, accordingly.

Reviewed-by Miquel Raynal <[email protected]>

Thanks,
Miquèl

Miquel Raynal

maintainer Applied

2020-09-07 07:34 UTC #10 lore raw

Re: [PATCH v2 3/3] mtd: lpddr: Move function print_drs_error to lpddr_cmds.c

In reply to Gustavo A. R. Silva

Quoted 6 lines · Gustavo A. R. Silva

Function print_drs_error is only used in drivers/mtd/lpddr/lpddr_cmds.c so, better to move it there.
Also, notice that there's no need for inline as the function is used once. Lastly, fix the following checkpatch warning:

Quoted 6 lines

WARNING: Prefer 'unsigned int' to bare use of 'unsigned' +static void print_drs_error(unsigned dsr)
Suggested-by Joe Perches <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>
Reviewed-by Miquel Raynal <[email protected]>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel