[PATCH 0/3] mtd: lpddr: Fix bad logic bug in print_drs_error

[PATCH 0/3] mtd: lpddr: Fix bad logic bug in print_drs_error

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

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(-)

-- 
2.26.0

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

[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)

Found by inspection of include files using printk.

Fixes: eb3db27507f7 ("[MTD] LPDDR PFOW definition")
Cc: stable@vger.kernel.org
Reported-by: Joe Perches <joe@perches.com>
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 include/linux/mtd/pfow.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mtd/pfow.h b/include/linux/mtd/pfow.h
index 122f3439e1af..c65d7a3be3c6 100644
--- a/include/linux/mtd/pfow.h
+++ b/include/linux/mtd/pfow.h
@@ -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)
-- 
2.26.0

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

[Gustavo A. R. Silva]

pr_notice is preferred over printk.

Suggested-by: Joe Perches <joe@perches.com>
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 include/linux/mtd/pfow.h | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/include/linux/mtd/pfow.h b/include/linux/mtd/pfow.h
index c65d7a3be3c6..b1fe2bbfdb04 100644
--- a/include/linux/mtd/pfow.h
+++ b/include/linux/mtd/pfow.h
@@ -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 */
-- 
2.26.0

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

[Joe Perches]

On Mon, 2020-04-27 at 14:03 -0500, Gustavo A. R. Silva wrote:
> pr_notice is preferred over printk.

So is coalescing formats

? diff --git a/include/linux/mtd/pfow.h b/include/linux/mtd/pfow.h
[]
> @@ -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...

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

[Gustavo A. R. Silva]

On 4/27/20 14:01, Joe Perches wrote:
> On Mon, 2020-04-27 at 14:03 -0500, Gustavo A. R. Silva wrote:
>> pr_notice is preferred over printk.
> 
> So is coalescing formats
> 
> ? diff --git a/include/linux/mtd/pfow.h b/include/linux/mtd/pfow.h
> []
>> @@ -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");
> 

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

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

[Joe Perches]

On Mon, 2020-04-27 at 14:10 -0500, Gustavo A. R. Silva wrote:
> 
> On 4/27/20 14:01, Joe Perches wrote:
> > On Mon, 2020-04-27 at 14:03 -0500, Gustavo A. R. Silva wrote:
> > > pr_notice is preferred over printk.
> > 
> > So is coalescing formats
> > 
> > ? diff --git a/include/linux/mtd/pfow.h b/include/linux/mtd/pfow.h
> > []
> > > @@ -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");
> > 
> 
> 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

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

[Miquel Raynal]

Hi Joe,

Joe Perches <joe@perches.com> wrote on Mon, 27 Apr 2020 12:15:02 -0700:

> On Mon, 2020-04-27 at 14:10 -0500, Gustavo A. R. Silva wrote:
> > 
> > On 4/27/20 14:01, Joe Perches wrote:  
> > > On Mon, 2020-04-27 at 14:03 -0500, Gustavo A. R. Silva wrote:  
> > > > pr_notice is preferred over printk.  
> > > 
> > > So is coalescing formats
> > > 
> > > ? diff --git a/include/linux/mtd/pfow.h b/include/linux/mtd/pfow.h
> > > []  
> > > > @@ -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");
> > >   
> > 
> > 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

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

[Miquel Raynal]

Hi Gustavo,

"Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote on Mon, 27 Apr
2020 14:10:36 -0500:

> On 4/27/20 14:01, Joe Perches wrote:
> > On Mon, 2020-04-27 at 14:03 -0500, Gustavo A. R. Silva wrote:  
> >> pr_notice is preferred over printk.  
> > 
> > So is coalescing formats
> > 
> > ? diff --git a/include/linux/mtd/pfow.h b/include/linux/mtd/pfow.h
> > []  
> >> @@ -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");
> >   
> 
> 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

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

[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 <joe@perches.com>
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/mtd/lpddr/lpddr_cmds.c | 33 +++++++++++++++++++++++++++++++++
 include/linux/mtd/pfow.h       | 32 --------------------------------
 2 files changed, 33 insertions(+), 32 deletions(-)

diff --git a/drivers/mtd/lpddr/lpddr_cmds.c b/drivers/mtd/lpddr/lpddr_cmds.c
index fb1cbc9a2870..2675032984f9 100644
--- a/drivers/mtd/lpddr/lpddr_cmds.c
+++ b/drivers/mtd/lpddr/lpddr_cmds.c
@@ -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)
 {
diff --git a/include/linux/mtd/pfow.h b/include/linux/mtd/pfow.h
index b1fe2bbfdb04..7b57c36878cc 100644
--- a/include/linux/mtd/pfow.h
+++ b/include/linux/mtd/pfow.h
@@ -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 */
-- 
2.26.0

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

[Joe Perches]

On Mon, 2020-04-27 at 14:04 -0500, Gustavo A. R. Silva wrote:
> Function print_drs_error is only used in drivers/mtd/lpddr/lpddr_cmds.c
> so, better to move it there.
[]
> diff --git a/drivers/mtd/lpddr/lpddr_cmds.c b/drivers/mtd/lpddr/lpddr_cmds.c
[]
> @@ -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.

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

[Gustavo A. R. Silva]

On 4/27/20 14:03, Joe Perches wrote:
> On Mon, 2020-04-27 at 14:04 -0500, Gustavo A. R. Silva wrote:
>> Function print_drs_error is only used in drivers/mtd/lpddr/lpddr_cmds.c
>> so, better to move it there.
> []
>> diff --git a/drivers/mtd/lpddr/lpddr_cmds.c b/drivers/mtd/lpddr/lpddr_cmds.c
> []
>> @@ -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

Re: [PATCH 0/3] mtd: lpddr: Fix bad logic bug in print_drs_error

[Joe Perches]

On Mon, 2020-04-27 at 14:00 -0500, Gustavo A. R. Silva wrote:
> 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/cover.1588016644.git.gustavo@embeddedor.com/

> 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(-)
> 

Re: [PATCH 0/3] mtd: lpddr: Fix bad logic bug in print_drs_error

[Miquel Raynal]

Hi Joe, Gustavo,

Joe Perches <joe@perches.com> wrote on Tue, 11 Aug 2020 16:47:36 -0700:

> On Mon, 2020-04-27 at 14:00 -0500, Gustavo A. R. Silva wrote:
> > 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

Re: [PATCH 0/3] mtd: lpddr: Fix bad logic bug in print_drs_error

[Gustavo A. R. Silva]

Hi Miquel,

On 8/12/20 02:23, Miquel Raynal wrote:
> Hi Joe, Gustavo,
> 
> Joe Perches <joe@perches.com> wrote on Tue, 11 Aug 2020 16:47:36 -0700:
> 
>> On Mon, 2020-04-27 at 14:00 -0500, Gustavo A. R. Silva wrote:
>>> 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.
> 

V2 was sent quite a while ago, and you actually acked it already:
https://lore.kernel.org/lkml/cover.1588016644.git.gustavo@embeddedor.com/

Thanks
--
Gustavo