[PATCH 0/3] cmd: tlv_eeprom: global variables and error cleanup
This patch-set removes some uses of global variables, and improves error reporting for the "read" command. It is intended to help switching to a split tlv library eventually, but general enough to apply independently.
Josua Mayer (3): cmd: tlv_eeprom: remove use of global variable has_been_read cmd: tlv_eeprom: handle -ENODEV error from read_eeprom function cmd: tlv_eeprom: enable 'dev' subcommand before 'read'
cmd/tlv_eeprom.c | 61 +++++++++++++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 22 deletions(-)
has_been_read is only used as an optimization for do_tlv_eeprom. Explicitly use and set inside this function, thus making read_eeprom stateless.
Signed-off-by: Josua Mayer josua@solid-run.com Cc: Stefan Roese sr@denx.de Cc: Baruch Siach baruch@tkos.co.il Cc: Heinrich Schuchardt xypron.glpk@gmx.de --- cmd/tlv_eeprom.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index 8049bf9843c..d36401e9138 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -42,8 +42,6 @@ static int set_date(char *buf, const char *string); static int set_bytes(char *buf, const char *string, int *converted_accum); static void show_tlv_devices(int current_dev);
-/* Set to 1 if we've read EEPROM into memory */ -static int has_been_read; /* The EERPOM contents after being read into memory */ static u8 eeprom[TLV_INFO_MAX_LEN];
@@ -130,9 +128,6 @@ static int read_eeprom(int devnum, u8 *eeprom) struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_tlv = to_entry(&eeprom[HDR_SIZE]);
- if (has_been_read) - return 0; - /* Read the header */ ret = read_tlv_eeprom((void *)eeprom_hdr, 0, HDR_SIZE, devnum); /* If the header was successfully read, read the TLVs */ @@ -149,10 +144,8 @@ static int read_eeprom(int devnum, u8 *eeprom) update_crc(eeprom); }
- has_been_read = 1; - #ifdef DEBUG - show_eeprom(eeprom); + show_eeprom(devnum, eeprom); #endif
return ret; @@ -432,10 +425,16 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) char cmd; struct tlvinfo_header *eeprom_hdr = to_header(eeprom); static unsigned int current_dev; + /* Set to 1 if we've read EEPROM into memory */ + static int has_been_read; + int ret;
// If no arguments, read the EERPOM and display its contents if (argc == 1) { - read_eeprom(current_dev, eeprom); + if (!has_been_read) { + if (read_eeprom(current_dev, eeprom) == 0) + has_been_read = 1; + } show_eeprom(current_dev, eeprom); return 0; } @@ -447,8 +446,10 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) // Read the EEPROM contents if (cmd == 'r') { has_been_read = 0; - if (!read_eeprom(current_dev, eeprom)) + if (read_eeprom(current_dev, eeprom) == 0) { printf("EEPROM data loaded from device to memory.\n"); + has_been_read = 1; + } return 0; }
On 4/29/23 11:15, Josua Mayer wrote:
has_been_read is only used as an optimization for do_tlv_eeprom. Explicitly use and set inside this function, thus making read_eeprom stateless.
Signed-off-by: Josua Mayer josua@solid-run.com Cc: Stefan Roese sr@denx.de Cc: Baruch Siach baruch@tkos.co.il Cc: Heinrich Schuchardt xypron.glpk@gmx.de
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
cmd/tlv_eeprom.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index 8049bf9843c..d36401e9138 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -42,8 +42,6 @@ static int set_date(char *buf, const char *string); static int set_bytes(char *buf, const char *string, int *converted_accum); static void show_tlv_devices(int current_dev);
-/* Set to 1 if we've read EEPROM into memory */ -static int has_been_read; /* The EERPOM contents after being read into memory */ static u8 eeprom[TLV_INFO_MAX_LEN];
@@ -130,9 +128,6 @@ static int read_eeprom(int devnum, u8 *eeprom) struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_tlv = to_entry(&eeprom[HDR_SIZE]);
- if (has_been_read)
return 0;- /* Read the header */ ret = read_tlv_eeprom((void *)eeprom_hdr, 0, HDR_SIZE, devnum); /* If the header was successfully read, read the TLVs */
@@ -149,10 +144,8 @@ static int read_eeprom(int devnum, u8 *eeprom) update_crc(eeprom); }
- has_been_read = 1;
- #ifdef DEBUG
- show_eeprom(eeprom);
show_eeprom(devnum, eeprom); #endif
return ret;
@@ -432,10 +425,16 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) char cmd; struct tlvinfo_header *eeprom_hdr = to_header(eeprom); static unsigned int current_dev;
/* Set to 1 if we've read EEPROM into memory */
static int has_been_read;
int ret;
// If no arguments, read the EERPOM and display its contents if (argc == 1) {
read_eeprom(current_dev, eeprom);
if (!has_been_read) {if (read_eeprom(current_dev, eeprom) == 0)has_been_read = 1;}@@ -447,8 +446,10 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) // Read the EEPROM contents if (cmd == 'r') { has_been_read = 0;
if (!read_eeprom(current_dev, eeprom))
if (read_eeprom(current_dev, eeprom) == 0) { printf("EEPROM data loaded from device to memory.\n");has_been_read = 1;}
Viele Grüße, Stefan Roese
Hi Josua,
On 4/29/23 11:15, Josua Mayer wrote:
has_been_read is only used as an optimization for do_tlv_eeprom. Explicitly use and set inside this function, thus making read_eeprom stateless.
Signed-off-by: Josua Mayer josua@solid-run.com Cc: Stefan Roese sr@denx.de Cc: Baruch Siach baruch@tkos.co.il Cc: Heinrich Schuchardt xypron.glpk@gmx.de
This patchset does not apply to master. Could you please check, rebase and send again?
Thanks, Stefan
cmd/tlv_eeprom.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index 8049bf9843c..d36401e9138 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -42,8 +42,6 @@ static int set_date(char *buf, const char *string); static int set_bytes(char *buf, const char *string, int *converted_accum); static void show_tlv_devices(int current_dev);
-/* Set to 1 if we've read EEPROM into memory */ -static int has_been_read; /* The EERPOM contents after being read into memory */ static u8 eeprom[TLV_INFO_MAX_LEN];
@@ -130,9 +128,6 @@ static int read_eeprom(int devnum, u8 *eeprom) struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_tlv = to_entry(&eeprom[HDR_SIZE]);
- if (has_been_read)
return 0;- /* Read the header */ ret = read_tlv_eeprom((void *)eeprom_hdr, 0, HDR_SIZE, devnum); /* If the header was successfully read, read the TLVs */
@@ -149,10 +144,8 @@ static int read_eeprom(int devnum, u8 *eeprom) update_crc(eeprom); }
- has_been_read = 1;
- #ifdef DEBUG
- show_eeprom(eeprom);
show_eeprom(devnum, eeprom); #endif
return ret;
@@ -432,10 +425,16 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) char cmd; struct tlvinfo_header *eeprom_hdr = to_header(eeprom); static unsigned int current_dev;
/* Set to 1 if we've read EEPROM into memory */
static int has_been_read;
int ret;
// If no arguments, read the EERPOM and display its contents if (argc == 1) {
read_eeprom(current_dev, eeprom);
if (!has_been_read) {if (read_eeprom(current_dev, eeprom) == 0)has_been_read = 1;}@@ -447,8 +446,10 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) // Read the EEPROM contents if (cmd == 'r') { has_been_read = 0;
if (!read_eeprom(current_dev, eeprom))
if (read_eeprom(current_dev, eeprom) == 0) { printf("EEPROM data loaded from device to memory.\n");has_been_read = 1;}
Viele Grüße, Stefan Roese
Hi Stefan,
Thanks for reviewing. I just rebased on https://source.denx.de/u-boot/u-boot.git / master, and will send new version soon.
- Josua Mayer
Am 03.05.23 um 09:55 schrieb Stefan Roese:
Hi Josua,
On 4/29/23 11:15, Josua Mayer wrote:
has_been_read is only used as an optimization for do_tlv_eeprom. Explicitly use and set inside this function, thus making read_eeprom stateless.
Signed-off-by: Josua Mayer josua@solid-run.com Cc: Stefan Roese sr@denx.de Cc: Baruch Siach baruch@tkos.co.il Cc: Heinrich Schuchardt xypron.glpk@gmx.de
This patchset does not apply to master. Could you please check, rebase and send again?
Thanks, Stefan
cmd/tlv_eeprom.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index 8049bf9843c..d36401e9138 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -42,8 +42,6 @@ static int set_date(char *buf, const char *string); static int set_bytes(char *buf, const char *string, int *converted_accum); static void show_tlv_devices(int current_dev); -/* Set to 1 if we've read EEPROM into memory */ -static int has_been_read; /* The EERPOM contents after being read into memory */ static u8 eeprom[TLV_INFO_MAX_LEN]; @@ -130,9 +128,6 @@ static int read_eeprom(int devnum, u8 *eeprom) struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_tlv = to_entry(&eeprom[HDR_SIZE]); - if (has_been_read) - return 0;
/* Read the header */ ret = read_tlv_eeprom((void *)eeprom_hdr, 0, HDR_SIZE, devnum); /* If the header was successfully read, read the TLVs */ @@ -149,10 +144,8 @@ static int read_eeprom(int devnum, u8 *eeprom) update_crc(eeprom); } - has_been_read = 1;
#ifdef DEBUG - show_eeprom(eeprom); + show_eeprom(devnum, eeprom); #endif return ret; @@ -432,10 +425,16 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) char cmd; struct tlvinfo_header *eeprom_hdr = to_header(eeprom); static unsigned int current_dev; + /* Set to 1 if we've read EEPROM into memory */ + static int has_been_read; + int ret; // If no arguments, read the EERPOM and display its contents if (argc == 1) { - read_eeprom(current_dev, eeprom); + if (!has_been_read) { + if (read_eeprom(current_dev, eeprom) == 0) + has_been_read = 1; + } show_eeprom(current_dev, eeprom); return 0; } @@ -447,8 +446,10 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) // Read the EEPROM contents if (cmd == 'r') { has_been_read = 0; - if (!read_eeprom(current_dev, eeprom)) + if (read_eeprom(current_dev, eeprom) == 0) { printf("EEPROM data loaded from device to memory.\n"); + has_been_read = 1; + } return 0; }
Viele Grüße, Stefan Roese
When tlv eeprom does not exist, return error code instead of quietly making up tlv structure in memory.
Signed-off-by: Josua Mayer josua@solid-run.com Cc: Stefan Roese sr@denx.de Cc: Baruch Siach baruch@tkos.co.il Cc: Heinrich Schuchardt xypron.glpk@gmx.de --- cmd/tlv_eeprom.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index d36401e9138..636c1fe32ef 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -134,6 +134,8 @@ static int read_eeprom(int devnum, u8 *eeprom) if (ret == 0 && is_valid_tlvinfo_header(eeprom_hdr)) ret = read_tlv_eeprom((void *)eeprom_tlv, HDR_SIZE, be16_to_cpu(eeprom_hdr->totallen), devnum); + else if (ret == -ENODEV) + return ret;
// If the contents are invalid, start over with default contents if (!is_valid_tlvinfo_header(eeprom_hdr) || @@ -432,8 +434,13 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) // If no arguments, read the EERPOM and display its contents if (argc == 1) { if (!has_been_read) { - if (read_eeprom(current_dev, eeprom) == 0) - has_been_read = 1; + ret = read_eeprom(current_dev, eeprom); + if (ret) { + printf("Failed to read EEPROM data from device.\n"); + return 0; + } + + has_been_read = 1; } show_eeprom(current_dev, eeprom); return 0; @@ -446,11 +453,14 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) // Read the EEPROM contents if (cmd == 'r') { has_been_read = 0; - if (read_eeprom(current_dev, eeprom) == 0) { - printf("EEPROM data loaded from device to memory.\n"); - has_been_read = 1; + ret = read_eeprom(current_dev, eeprom); + if (ret) { + printf("Failed to read EEPROM data from device.\n"); + return 0; } - return 0; + + printf("EEPROM data loaded from device to memory.\n"); + has_been_read = 1; }
// Subsequent commands require that the EEPROM has already been read.
On 4/29/23 11:15, Josua Mayer wrote:
When tlv eeprom does not exist, return error code instead of quietly making up tlv structure in memory.
Signed-off-by: Josua Mayer josua@solid-run.com Cc: Stefan Roese sr@denx.de Cc: Baruch Siach baruch@tkos.co.il Cc: Heinrich Schuchardt xypron.glpk@gmx.de
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
cmd/tlv_eeprom.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index d36401e9138..636c1fe32ef 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -134,6 +134,8 @@ static int read_eeprom(int devnum, u8 *eeprom) if (ret == 0 && is_valid_tlvinfo_header(eeprom_hdr)) ret = read_tlv_eeprom((void *)eeprom_tlv, HDR_SIZE, be16_to_cpu(eeprom_hdr->totallen), devnum);
else if (ret == -ENODEV)
return ret;// If the contents are invalid, start over with default contents if (!is_valid_tlvinfo_header(eeprom_hdr) ||
@@ -432,8 +434,13 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) // If no arguments, read the EERPOM and display its contents if (argc == 1) { if (!has_been_read) {
if (read_eeprom(current_dev, eeprom) == 0)has_been_read = 1;
ret = read_eeprom(current_dev, eeprom);if (ret) {printf("Failed to read EEPROM data from device.\n");return 0;}has_been_read = 1;@@ -446,11 +453,14 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) // Read the EEPROM contents if (cmd == 'r') { has_been_read = 0;
if (read_eeprom(current_dev, eeprom) == 0) {printf("EEPROM data loaded from device to memory.\n");has_been_read = 1;
ret = read_eeprom(current_dev, eeprom);if (ret) {printf("Failed to read EEPROM data from device.\n");return 0;
return 0;
printf("EEPROM data loaded from device to memory.\n");has_been_read = 1;}
// Subsequent commands require that the EEPROM has already been read.
Viele Grüße, Stefan Roese
Move the handler for "tlv_eeprom dev X" command to the beginning of do_tlv_eeprom, to allow using it before issuing a "read" command for currently selected eeprom.
Also remove the check if eeprom exists, since that can only work after the first execution of read_eeprom triggered device lookup. Instead accept values up to the defined array size (MAX_TLV_DEVICES).
Signed-off-by: Josua Mayer josua@solid-run.com Cc: Stefan Roese sr@denx.de Cc: Baruch Siach baruch@tkos.co.il Cc: Heinrich Schuchardt xypron.glpk@gmx.de --- cmd/tlv_eeprom.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index 636c1fe32ef..79796394c5c 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -450,6 +450,22 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) // "reset" will both be treated as "read". cmd = argv[1][0];
+ // select device + if (cmd == 'd') { + /* 'dev' command */ + unsigned int devnum; + + devnum = simple_strtoul(argv[2], NULL, 0); + if (devnum >= MAX_TLV_DEVICES) { + printf("Invalid device number\n"); + return 0; + } + current_dev = devnum; + has_been_read = 0; + + return 0; + } + // Read the EEPROM contents if (cmd == 'r') { has_been_read = 0; @@ -508,16 +524,6 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) tlvinfo_delete_tlv(eeprom, tcode); if (argc == 4) tlvinfo_add_tlv(eeprom, tcode, argv[3]); - } else if (cmd == 'd') { /* 'dev' command */ - unsigned int devnum; - - devnum = simple_strtoul(argv[2], NULL, 0); - if (devnum > MAX_TLV_DEVICES || !tlv_devices[devnum]) { - printf("Invalid device number\n"); - return 0; - } - current_dev = devnum; - has_been_read = 0; } else { return CMD_RET_USAGE; }
On 4/29/23 11:15, Josua Mayer wrote:
Move the handler for "tlv_eeprom dev X" command to the beginning of do_tlv_eeprom, to allow using it before issuing a "read" command for currently selected eeprom.
Also remove the check if eeprom exists, since that can only work after the first execution of read_eeprom triggered device lookup. Instead accept values up to the defined array size (MAX_TLV_DEVICES).
Signed-off-by: Josua Mayer josua@solid-run.com Cc: Stefan Roese sr@denx.de Cc: Baruch Siach baruch@tkos.co.il Cc: Heinrich Schuchardt xypron.glpk@gmx.de
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
cmd/tlv_eeprom.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index 636c1fe32ef..79796394c5c 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -450,6 +450,22 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) // "reset" will both be treated as "read". cmd = argv[1][0];
- // select device
- if (cmd == 'd') {
/* 'dev' command */unsigned int devnum;devnum = simple_strtoul(argv[2], NULL, 0);if (devnum >= MAX_TLV_DEVICES) {printf("Invalid device number\n");return 0;}current_dev = devnum;has_been_read = 0;return 0;- }
- // Read the EEPROM contents if (cmd == 'r') { has_been_read = 0;
@@ -508,16 +524,6 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) tlvinfo_delete_tlv(eeprom, tcode); if (argc == 4) tlvinfo_add_tlv(eeprom, tcode, argv[3]);
- } else if (cmd == 'd') { /* 'dev' command */
unsigned int devnum;devnum = simple_strtoul(argv[2], NULL, 0);if (devnum > MAX_TLV_DEVICES || !tlv_devices[devnum]) {printf("Invalid device number\n");return 0;}current_dev = devnum;has_been_read = 0;
Viele Grüße, Stefan Roese
participants (2)
-
Josua Mayer -
Stefan Roese