Skip to contentSkip to conversation

mtd: lpddr: Fix bad logic bug in print_drs_error

PATCH Superseded Gustavo A. R. Silva 3 participants 14 messages Apr 27, 2020 – Aug 12, 2020 Memory technology devices (MTD)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

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

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 | 33 +++++++++++++++++++++++++++++++++
 include/linux/mtd/pfow.h       | 32 --------------------------------
 2 files changed, 33 insertions(+), 32 deletions(-)
Signature · 6 lines
2.26.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
author
#1 lore raw

PATCH 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]>
---
 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 · 1 line
2.26.0
author
#2 lore raw

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

In reply to Gustavo A. R. Silva

pr_notice is preferred over printk.

Suggested-by Joe Perches <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 include/linux/mtd/pfow.h | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)
include/linux/mtd/pfow.h +11 -11
index c65d7a3be3c6..b1fe2bbfdb04 100644
@@ -127,31 +127,31 @@ 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 "
+		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 "
+		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 "
+		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 "
+		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 "
+		pr_notice("DSR.1: (1) Aborted Erase/Program attempt "
 					"on locked block\n");
 }
 #endif /* __LINUX_MTD_PFOW_H */
Signature · 6 lines
2.26.0


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

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

Quoted 1 line · Gustavo A. R. Silva

pr_notice is preferred over printk.

So is coalescing formats

? diff --git a/include/linux/mtd/pfow.h b/include/linux/mtd/pfow.h []

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

Quoted 10 lines
include/linux/mtd/pfow.h +2 -2
@@ -127,31 +127,31 @@ 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 "
+		pr_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");

etc...

author
#4 lore raw

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

Quoted 10 lines · Joe Perches
Quoted 1 line · Gustavo A. R. Silva

pr_notice is preferred over printk.

So is coalescing formats

? diff --git a/include/linux/mtd/pfow.h b/include/linux/mtd/pfow.h []

Quoted 3 lines
include/linux/mtd/pfow.h +0 -0
@@ -127,31 +127,31 @@ static inline void print_drs_error(unsigned dsr)
 	int prog_status = (dsr & DSR_RPS) >> 8;
Quoted 10 lines

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

Quoted 7 lines
include/linux/mtd/pfow.h +2 -2
 	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 "
+		pr_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");

I didn't want to mess with the rest of format, because some maintainers don't like that. If Miquel is OK with that, I can fix that up, too.

Thanks
--
Gustavo

#5 lore raw

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

Quoted 23 lines · Gustavo A. R. Silva
Quoted 10 lines · Joe Perches
Quoted 1 line · Gustavo A. R. Silva

pr_notice is preferred over printk.

So is coalescing formats

? diff --git a/include/linux/mtd/pfow.h b/include/linux/mtd/pfow.h []

Quoted 3 lines
include/linux/mtd/pfow.h +0 -0
@@ -127,31 +127,31 @@ static inline void print_drs_error(unsigned dsr)
 	int prog_status = (dsr & DSR_RPS) >> 8;
Quoted 10 lines

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

Quoted 7 lines
include/linux/mtd/pfow.h +2 -2
 	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 "
+		pr_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");
Quoted 2 lines

I didn't want to mess with the rest of format, because some maintainers don't like that. If Miquel is OK with that, I can fix that up, too.

He should. Coalescing is part of coding-style.

"never break user-visible strings such as printk messages"

cheers, Joe

maintainer
#6 lore raw

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

Hi Joe,

Joe Perches <[email protected]> wrote on Mon, 27 Apr 2020 12:15:02 -0700:

Quoted 24 lines · Gustavo A. R. Silva
Quoted 23 lines · Joe Perches
Quoted 20 lines · Gustavo A. R. Silva
Quoted 1 line

pr_notice is preferred over printk.

So is coalescing formats

? diff --git a/include/linux/mtd/pfow.h b/include/linux/mtd/pfow.h []

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

Quoted 10 lines
include/linux/mtd/pfow.h +2 -2
@@ -127,31 +127,31 @@ 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 "
+		pr_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");
Quoted 9 lines
Quoted 2 lines

I didn't want to mess with the rest of format, because some maintainers don't like that. If Miquel is OK with that, I can fix that up, too.

He should. Coalescing is part of coding-style.

"never break user-visible strings such as printk messages"

cheers, Joe

I suppose Gustavo meant that there are maintainers in the community (and I am part of it) that do not like when people fix style issues aside their own changes -specifically without writing it in commit logs. But in this situation I think s/printk(KEN_NOTICE/pr_notice(/ needs the second line to be re-aligned with the first one. While touching it, it seems reasonable to also fix this style issue and avoid keeping broken strings.

Thanks,
Miquèl

maintainer
#7 lore raw

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

In reply to Gustavo A. R. Silva

Hi Gustavo,

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

Quoted 22 lines · Joe Perches
Quoted 10 lines · Gustavo A. R. Silva
Quoted 1 line

pr_notice is preferred over printk.

So is coalescing formats

? diff --git a/include/linux/mtd/pfow.h b/include/linux/mtd/pfow.h []

Quoted 3 lines
include/linux/mtd/pfow.h +0 -0
@@ -127,31 +127,31 @@ static inline void print_drs_error(unsigned dsr)
 	int prog_status = (dsr & DSR_RPS) >> 8;
Quoted 10 lines

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

Quoted 7 lines
include/linux/mtd/pfow.h +2 -2
 	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 "
+		pr_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");
Quoted 6 lines

I didn't want to mess with the rest of format, because some maintainers don't like that. If Miquel is OK with that, I can fix that up, too.

Thanks
--
Gustavo

I'm fine with it in this case, just mention it in the commit log, please.

Thanks,
Miquèl

author
#8 lore raw

PATCH 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.

Suggested-by Joe Perches <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/mtd/lpddr/lpddr_cmds.c | 33 +++++++++++++++++++++++++++++++++
 include/linux/mtd/pfow.h       | 32 --------------------------------
 2 files changed, 33 insertions(+), 32 deletions(-)
drivers/mtd/lpddr/lpddr_cmds.c +33 -0
index fb1cbc9a2870..2675032984f9 100644
@@ -94,6 +94,39 @@ struct mtd_info *lpddr_cmdset(struct map_info *map)
 }
 EXPORT_SYMBOL(lpddr_cmdset);
 
+static inline 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 -32
index b1fe2bbfdb04..7b57c36878cc 100644
@@ -122,36 +122,4 @@ static inline void send_pfow_command(struct map_info *map,
 			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 · 6 lines
2.26.0


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

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

Quoted 2 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.

[]

drivers/mtd/lpddr/lpddr_cmds.c

Quoted 1 line
drivers/mtd/lpddr/lpddr_cmds.c +0 -0

[]

drivers/mtd/lpddr/lpddr_cmds.c struct mtd_info *lpddr_cmdset(struct map_info *map) · line 97

Quoted 5 lines
drivers/mtd/lpddr/lpddr_cmds.c +1 -0
@@ -94,6 +94,39 @@ struct mtd_info *lpddr_cmdset(struct map_info *map)
 }
 EXPORT_SYMBOL(lpddr_cmdset);
 
+static inline void print_drs_error(unsigned int dsr)

There's no need for inline as it's used once.

author
#10 lore raw

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

Quoted 6 lines · Joe Perches
Quoted 2 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.

[]

drivers/mtd/lpddr/lpddr_cmds.c

Quoted 1 line
drivers/mtd/lpddr/lpddr_cmds.c +0 -0

[]

Quoted 8 lines

drivers/mtd/lpddr/lpddr_cmds.c struct mtd_info *lpddr_cmdset(struct map_info *map) · line 97

Quoted 5 lines
drivers/mtd/lpddr/lpddr_cmds.c +1 -0
@@ -94,6 +94,39 @@ struct mtd_info *lpddr_cmdset(struct map_info *map)
 }
 EXPORT_SYMBOL(lpddr_cmdset);
 
+static inline void print_drs_error(unsigned int dsr)

There's no need for inline as it's used once.

That's correct. I didn't notice that at first sight.

I'll remove it in V2.

Thanks
--
Gustavo

#11 lore raw
In reply to Gustavo A. R. Silva
Quoted 4 lines · Gustavo A. R. Silva

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.

AFAICT: This series is still not applied to any tree.

Can someone please apply it?

https://lore.kernel.org/linux-mtd/[email protected]/

Quoted 11 lines

Thanks

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 | 33 +++++++++++++++++++++++++++++++++
 include/linux/mtd/pfow.h       | 32 --------------------------------
 2 files changed, 33 insertions(+), 32 deletions(-)
maintainer
#12 lore raw

Hi Joe, Gustavo,

Joe Perches <[email protected]> wrote on Tue, 11 Aug 2020 16:47:36 -0700:

Quoted 9 lines · Gustavo A. R. Silva
Quoted 4 lines

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.

AFAICT: This series is still not applied to any tree.

Can someone please apply it?

I thought a v2 was coming, Gustavo, can you please send an update? IIRC there are strings that need to be "unbroken" and an extra "inline" which needs to be dropped.

Thanks,
Miquèl

author
#13 lore raw

Hi Miquel,

Quoted 10 lines · Miquel Raynal

Hi Joe, Gustavo,

Joe Perches <[email protected]> wrote on Tue, 11 Aug 2020 16:47:36 -0700:

Quoted 6 lines · Gustavo A. R. Silva
Quoted 4 lines

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.

Quoted 8 lines
Quoted 3 lines

AFAICT: This series is still not applied to any tree.

Can someone please apply it?

I thought a v2 was coming, Gustavo, can you please send an update? IIRC there are strings that need to be "unbroken" and an extra "inline" which needs to be dropped.

V2 was sent quite a while ago, and you actually acked it already:
https://lore.kernel.org/lkml/[email protected]/

Thanks
--
Gustavo

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