diff mbox series

[V2,1/1] mmc-utils: fix messy code printed

Message ID 20210208022642.7823-2-wuxy20@gmail.com
State New
Headers show
Series [V2,1/1] mmc-utils: fix messy code printed | expand

Commit Message

Xingyu Wu Feb. 8, 2021, 2:26 a.m. UTC
Some vendors of eMMC use different format to define the
Firmware name. If the Firmware name uses character and if
it exceeds the printable range of ASCII (0x20~0x7e),
mmc-utils will print messy code. This change can fix the
messy code issue, if the firmware name is not printable,
print it out as hexadecimal, this change was verified on
chromium project.

Signed-off-by: Xingyu Wu <wuxy20@gmail.com>
---
 mmc_cmds.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

Comments

Ulf Hansson March 30, 2021, 1 p.m. UTC | #1
+ Avri

On Mon, 8 Feb 2021 at 03:30, Xingyu Wu <wuxy20@gmail.com> wrote:
>

> Some vendors of eMMC use different format to define the

> Firmware name. If the Firmware name uses character and if

> it exceeds the printable range of ASCII (0x20~0x7e),

> mmc-utils will print messy code. This change can fix the

> messy code issue, if the firmware name is not printable,

> print it out as hexadecimal, this change was verified on

> chromium project.

>

> Signed-off-by: Xingyu Wu <wuxy20@gmail.com>


Avri, can you please have a look at this and provide comments or your ack?

Kind regards
Uffe

> ---

>  mmc_cmds.c | 13 +++++++++++--

>  1 file changed, 11 insertions(+), 2 deletions(-)

>

> diff --git a/mmc_cmds.c b/mmc_cmds.c

> index fb37189..d090a24 100644

> --- a/mmc_cmds.c

> +++ b/mmc_cmds.c

> @@ -29,6 +29,7 @@

>  #include <stdint.h>

>  #include <assert.h>

>  #include <linux/fs.h> /* for BLKGETSIZE */

> +#include <ctype.h>

>

>  #include "mmc.h"

>  #include "mmc_cmds.h"

> @@ -1758,8 +1759,16 @@ int do_read_extcsd(int nargs, char **argv)

>         }

>

>         if (ext_csd_rev >= 7) {

> -               printf("eMMC Firmware Version: %s\n",

> -                       (char*)&ext_csd[EXT_CSD_FIRMWARE_VERSION]);

> +               printf("eMMC Firmware Version:");

> +               for (int i = 0; i < 8; i++) {

> +                       char c = ext_csd[EXT_CSD_FIRMWARE_VERSION + i];

> +

> +                       if (isprint(c))

> +                               printf("%c", c);

> +                       else if (c != 0)

> +                               printf("\\x%02x", c);

> +               }

> +               printf("\n");

>                 printf("eMMC Life Time Estimation A [EXT_CSD_DEVICE_LIFE_TIME_EST_TYP_A]: 0x%02x\n",

>                         ext_csd[EXT_CSD_DEVICE_LIFE_TIME_EST_TYP_A]);

>                 printf("eMMC Life Time Estimation B [EXT_CSD_DEVICE_LIFE_TIME_EST_TYP_B]: 0x%02x\n",

> --

> 2.25.1

>
Avri Altman March 30, 2021, 1:56 p.m. UTC | #2
> + Avri

> 

> On Mon, 8 Feb 2021 at 03:30, Xingyu Wu <wuxy20@gmail.com> wrote:

> >

> > Some vendors of eMMC use different format to define the

> > Firmware name. If the Firmware name uses character and if

> > it exceeds the printable range of ASCII (0x20~0x7e),

> > mmc-utils will print messy code. This change can fix the

> > messy code issue, if the firmware name is not printable,

> > print it out as hexadecimal, this change was verified on

> > chromium project.

> >

> > Signed-off-by: Xingyu Wu <wuxy20@gmail.com>

> 

> Avri, can you please have a look at this and provide comments or your ack?

> 

> Kind regards

> Uffe

> 

> > ---

> >  mmc_cmds.c | 13 +++++++++++--

> >  1 file changed, 11 insertions(+), 2 deletions(-)

> >

> > diff --git a/mmc_cmds.c b/mmc_cmds.c

> > index fb37189..d090a24 100644

> > --- a/mmc_cmds.c

> > +++ b/mmc_cmds.c

> > @@ -29,6 +29,7 @@

> >  #include <stdint.h>

> >  #include <assert.h>

> >  #include <linux/fs.h> /* for BLKGETSIZE */

> > +#include <ctype.h>

> >

> >  #include "mmc.h"

> >  #include "mmc_cmds.h"

> > @@ -1758,8 +1759,16 @@ int do_read_extcsd(int nargs, char **argv)

> >         }

> >

> >         if (ext_csd_rev >= 7) {

int i;
please don't mix declaration and code

> > -               printf("eMMC Firmware Version: %s\n",

> > -                       (char*)&ext_csd[EXT_CSD_FIRMWARE_VERSION]);

> > +               printf("eMMC Firmware Version:");

Maybe add a comment that will explain why 8 bytes: /* FIRMWARE_VERSION [261-254] */

> > +               for (int i = 0; i < 8; i++) {

> > +                       char c = ext_csd[EXT_CSD_FIRMWARE_VERSION + i];

> > +

> > +                       if (isprint(c))

> > +                               printf("%c", c);

> > +                       else if (c != 0)

> > +                               printf("\\x%02x", c);

else 
	break;
also why do you need the \\x?  %02x should work

Thanks,
Avri
> > +               }

> > +               printf("\n");

> >                 printf("eMMC Life Time Estimation A

> [EXT_CSD_DEVICE_LIFE_TIME_EST_TYP_A]: 0x%02x\n",

> >                         ext_csd[EXT_CSD_DEVICE_LIFE_TIME_EST_TYP_A]);

> >                 printf("eMMC Life Time Estimation B

> [EXT_CSD_DEVICE_LIFE_TIME_EST_TYP_B]: 0x%02x\n",

> > --

> > 2.25.1

> >
diff mbox series

Patch

diff --git a/mmc_cmds.c b/mmc_cmds.c
index fb37189..d090a24 100644
--- a/mmc_cmds.c
+++ b/mmc_cmds.c
@@ -29,6 +29,7 @@ 
 #include <stdint.h>
 #include <assert.h>
 #include <linux/fs.h> /* for BLKGETSIZE */
+#include <ctype.h>
 
 #include "mmc.h"
 #include "mmc_cmds.h"
@@ -1758,8 +1759,16 @@  int do_read_extcsd(int nargs, char **argv)
 	}
 
 	if (ext_csd_rev >= 7) {
-		printf("eMMC Firmware Version: %s\n",
-			(char*)&ext_csd[EXT_CSD_FIRMWARE_VERSION]);
+		printf("eMMC Firmware Version:");
+		for (int i = 0; i < 8; i++) {
+			char c = ext_csd[EXT_CSD_FIRMWARE_VERSION + i];
+
+			if (isprint(c))
+				printf("%c", c);
+			else if (c != 0)
+				printf("\\x%02x", c);
+		}
+		printf("\n");
 		printf("eMMC Life Time Estimation A [EXT_CSD_DEVICE_LIFE_TIME_EST_TYP_A]: 0x%02x\n",
 			ext_csd[EXT_CSD_DEVICE_LIFE_TIME_EST_TYP_A]);
 		printf("eMMC Life Time Estimation B [EXT_CSD_DEVICE_LIFE_TIME_EST_TYP_B]: 0x%02x\n",