ti: common: board_detect: basic fixes for blank eeprom

PATCH New Nishanth Menon 3 participants 13 messages Oct 11, 2016 – Nov 2, 2016

Share & tools

Nishanth Menon

author

2016-10-11 17:39 UTC #0 lore raw

cover ti: common: board_detect: basic fixes for blank eeprom

In some cases where eeprom has not been programmed, Brad reported issues of crash at strcmp against NULL, this series was triggered thanks to issue in manufacturing flow where the mandatory eeprom programming was missed.

Nishanth Menon (3):
ti: common: board_detect: Replace hardcoded value with macro
ti: common: board_detect: Setup initial default value for config as
well
ti: common: board_detect: Return a valid empty string for
un-initialized eeprom

 board/ti/common/board_detect.c | 16 ++++++----------
 board/ti/common/board_detect.h |  6 +++---
 2 files changed, 9 insertions(+), 13 deletions(-)

Signature · 1 line

2.10.0

Nishanth Menon

author

2016-10-11 17:39 UTC #1 lore raw

PATCH 1/3 ti: common: board_detect: Replace hardcoded value with macro

We should have used TI_DEAD_EEPROM_MAGIC in the first place.

Fixes d3b98a9eb941 ("ti: common: dra7: Add standard access for board description EEPROM")

Signed-off-by Nishanth Menon <[email protected]>

---
 board/ti/common/board_detect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

board/ti/common/board_detect.c +1 -1

index e0ae1a51a6a4..7c552d20656a 100644
@@ -171,7 +171,7 @@ int __maybe_unused ti_i2c_eeprom_dra7_get(int bus_addr, int dev_addr)
 		goto already_read;
 
 	/* Initialize with a known bad marker for i2c fails.. */
-	ep->header = 0xADEAD12C;
+	ep->header = TI_DEAD_EEPROM_MAGIC;
 	ep->name[0] = 0x0;
 	ep->version[0] = 0x0;
 	ep->serial[0] = 0x0;

Signature · 1 line

2.10.0

Lokesh Vutla

2016-10-13 03:44 UTC #2 lore raw

PATCH 1/3 ti: common: board_detect: Replace hardcoded value with macro

Quoted 4 lines · Nishanth Menon

We should have used TI_DEAD_EEPROM_MAGIC in the first place.
Fixes d3b98a9eb941 ("ti: common: dra7: Add standard access for board description EEPROM")
Signed-off-by Nishanth Menon <[email protected]>

Reviewed-by Lokesh Vutla <[email protected]>

Thanks and regards,
Lokesh

Quoted 18 lines

---
 board/ti/common/board_detect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

board/ti/common/board_detect.c +1 -1

index e0ae1a51a6a4..7c552d20656a 100644
@@ -171,7 +171,7 @@ int __maybe_unused ti_i2c_eeprom_dra7_get(int bus_addr, int dev_addr)
 		goto already_read;
 
 	/* Initialize with a known bad marker for i2c fails.. */
-	ep->header = 0xADEAD12C;
+	ep->header = TI_DEAD_EEPROM_MAGIC;
 	ep->name[0] = 0x0;
 	ep->version[0] = 0x0;
 	ep->serial[0] = 0x0;

Tom Rini

2016-10-13 13:42 UTC #3 lore raw

PATCH 1/3 ti: common: board_detect: Replace hardcoded value with macro

In reply to Nishanth Menon

Quoted 4 lines · Nishanth Menon

We should have used TI_DEAD_EEPROM_MAGIC in the first place.
Fixes d3b98a9eb941 ("ti: common: dra7: Add standard access for board description EEPROM")
Signed-off-by Nishanth Menon <[email protected]>

Reviewed-by Tom Rini <[email protected]>

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161013/2bed6c5f/attachment.sig>

Tom Rini

2016-11-02 14:25 UTC #4 lore raw

PATCH 1/3 ti: common: board_detect: Replace hardcoded value with macro

In reply to Nishanth Menon

Quoted 6 lines · Nishanth Menon

We should have used TI_DEAD_EEPROM_MAGIC in the first place.
Fixes d3b98a9eb941 ("ti: common: dra7: Add standard access for board description EEPROM")
Signed-off-by Nishanth Menon <[email protected]>
Reviewed-by Lokesh Vutla <[email protected]>
Reviewed-by Tom Rini <[email protected]>

Applied to u-boot/master (before v2016.11-rc3), thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161102/3edad257/attachment.sig>

Nishanth Menon

author

2016-10-11 17:39 UTC #5 lore raw

PATCH 2/3 ti: common: board_detect: Setup initial default value for config as well

In reply to Nishanth Menon

config should have been initialized along with others as defaults.

Signed-off-by Nishanth Menon <[email protected]>

---
 board/ti/common/board_detect.c | 2 ++
 1 file changed, 2 insertions(+)

board/ti/common/board_detect.c +2 -0

index 7c552d20656a..13aac2f03037 100644
@@ -131,6 +131,7 @@ int __maybe_unused ti_i2c_eeprom_am_get(int bus_addr, int dev_addr)
 	ep->name[0] = 0x0;
 	ep->version[0] = 0x0;
 	ep->serial[0] = 0x0;
+	ep->config[0] = 0x0;
 
 	rc = ti_i2c_eeprom_get(bus_addr, dev_addr, TI_EEPROM_HEADER_MAGIC,
 			       sizeof(am_ep), (uint8_t *)&am_ep);

@@ -175,6 +176,7 @@ int __maybe_unused ti_i2c_eeprom_dra7_get(int bus_addr, int dev_addr)
 	ep->name[0] = 0x0;
 	ep->version[0] = 0x0;
 	ep->serial[0] = 0x0;
+	ep->config[0] = 0x0;
 	ep->emif1_size = 0;
 	ep->emif2_size = 0;

Signature · 1 line

2.10.0

Lokesh Vutla

2016-10-13 03:45 UTC #6 lore raw

PATCH 2/3 ti: common: board_detect: Setup initial default value for config as well

Quoted 3 lines · Nishanth Menon

config should have been initialized along with others as defaults.
Signed-off-by Nishanth Menon <[email protected]>

Reviewed-by Lokesh Vutla <[email protected]>

Thanks and regards,
Lokesh

Quoted 25 lines

---
 board/ti/common/board_detect.c | 2 ++
 1 file changed, 2 insertions(+)

board/ti/common/board_detect.c +2 -0

index 7c552d20656a..13aac2f03037 100644
@@ -131,6 +131,7 @@ int __maybe_unused ti_i2c_eeprom_am_get(int bus_addr, int dev_addr)
 	ep->name[0] = 0x0;
 	ep->version[0] = 0x0;
 	ep->serial[0] = 0x0;
+	ep->config[0] = 0x0;
 
 	rc = ti_i2c_eeprom_get(bus_addr, dev_addr, TI_EEPROM_HEADER_MAGIC,
 			       sizeof(am_ep), (uint8_t *)&am_ep);

@@ -175,6 +176,7 @@ int __maybe_unused ti_i2c_eeprom_dra7_get(int bus_addr, int dev_addr)
 	ep->name[0] = 0x0;
 	ep->version[0] = 0x0;
 	ep->serial[0] = 0x0;
+	ep->config[0] = 0x0;
 	ep->emif1_size = 0;
 	ep->emif2_size = 0;

Tom Rini

2016-10-13 13:43 UTC #7 lore raw

PATCH 2/3 ti: common: board_detect: Setup initial default value for config as well

In reply to Nishanth Menon

Quoted 3 lines · Nishanth Menon

config should have been initialized along with others as defaults.
Signed-off-by Nishanth Menon <[email protected]>

Reviewed-by Tom Rini <[email protected]>

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161013/8212e691/attachment.sig>

Tom Rini

2016-11-02 14:26 UTC #8 lore raw

PATCH 2/3 ti: common: board_detect: Setup initial default value for config as well

In reply to Nishanth Menon

Quoted 5 lines · Nishanth Menon

config should have been initialized along with others as defaults.
Signed-off-by Nishanth Menon <[email protected]>
Reviewed-by Lokesh Vutla <[email protected]>
Reviewed-by Tom Rini <[email protected]>

Applied to u-boot/master (before v2016.11-rc3), thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161102/0cc1f48e/attachment.sig>

Nishanth Menon

author

2016-10-11 17:39 UTC #9 lore raw

PATCH 3/3 ti: common: board_detect: Return a valid empty string for un-initialized eeprom

In reply to Nishanth Menon

Current logic for query of revision, board_name, config returns NULL. Users of these functions do a direct strncmp to compare. Unfortunately, as per conventions require two valid strings to compare against and the current implementation causes a crash when compared with NULL.

We'd still like to maintain the simplistic usage of these APIs instead of redundant if (string) res=strncmp(fn(),"cmp",n); flowing all over the place.

Hence, since the version, name and config is already pre-initialized with empty string, just dont check for invalid header in the first place and return the empty string to the caller.

Reported-by Brad Griffis <[email protected]>

Signed-off-by Nishanth Menon <[email protected]>

---
 board/ti/common/board_detect.c | 12 +++---------
 board/ti/common/board_detect.h |  6 +++---
 2 files changed, 6 insertions(+), 12 deletions(-)

board/ti/common/board_detect.c +3 -9

index 13aac2f03037..6e7ca9196d22 100644
@@ -231,9 +231,7 @@ char * __maybe_unused board_ti_get_rev(void)
 {
 	struct ti_common_eeprom *ep = TI_EEPROM_DATA;
 
-	if (ep->header == TI_DEAD_EEPROM_MAGIC)
-		return NULL;
-
+	/* if ep->header == TI_DEAD_EEPROM_MAGIC, this is empty already */
 	return ep->version;
 }

@@ -241,9 +239,7 @@ char * __maybe_unused board_ti_get_config(void)
 {
 	struct ti_common_eeprom *ep = TI_EEPROM_DATA;
 
-	if (ep->header == TI_DEAD_EEPROM_MAGIC)
-		return NULL;
-
+	/* if ep->header == TI_DEAD_EEPROM_MAGIC, this is empty already */
 	return ep->config;
 }

@@ -251,9 +247,7 @@ char * __maybe_unused board_ti_get_name(void)
 {
 	struct ti_common_eeprom *ep = TI_EEPROM_DATA;
 
-	if (ep->header == TI_DEAD_EEPROM_MAGIC)
-		return NULL;
-
+	/* if ep->header == TI_DEAD_EEPROM_MAGIC, this is empty already */
 	return ep->name;
 }

board/ti/common/board_detect.h +3 -3

index eb17f6f52a12..2ded3ed7764b 100644
@@ -141,7 +141,7 @@ bool board_ti_rev_is(char *rev_tag, int cmp_len);
 /**
  * board_ti_get_rev() - Get board revision for TI EVMs
  *
- * Return: NULL if eeprom was'nt read.
+ * Return: Empty string if eeprom was'nt read.
  *	   Board revision otherwise
  */
 char *board_ti_get_rev(void);

@@ -149,7 +149,7 @@ char *board_ti_get_rev(void);
 /**
  * board_ti_get_config() - Get board config for TI EVMs
  *
- * Return: NULL if eeprom was'nt read.
+ * Return: Empty string if eeprom was'nt read.
  *	   Board config otherwise
  */
 char *board_ti_get_config(void);

@@ -157,7 +157,7 @@ char *board_ti_get_config(void);
 /**
  * board_ti_get_name() - Get board name for TI EVMs
  *
- * Return: NULL if eeprom was'nt read.
+ * Return: Empty string if eeprom was'nt read.
  *	   Board name otherwise
  */
 char *board_ti_get_name(void);

Signature · 1 line

2.10.0

Lokesh Vutla

2016-10-13 03:45 UTC #10 lore raw

PATCH 3/3 ti: common: board_detect: Return a valid empty string for un-initialized eeprom

Quoted 10 lines · Nishanth Menon

Current logic for query of revision, board_name, config returns NULL. Users of these functions do a direct strncmp to compare. Unfortunately, as per conventions require two valid strings to compare against and the current implementation causes a crash when compared with NULL.
We'd still like to maintain the simplistic usage of these APIs instead of redundant if (string) res=strncmp(fn(),"cmp",n); flowing all over the place.

Quoted 6 lines

Hence, since the version, name and config is already pre-initialized with empty string, just dont check for invalid header in the first place and return the empty string to the caller.
Reported-by Brad Griffis <[email protected]>
Signed-off-by Nishanth Menon <[email protected]>

Reviewed-by Lokesh Vutla <[email protected]>

Thanks and regards,
Lokesh

Quoted 74 lines

---
 board/ti/common/board_detect.c | 12 +++---------
 board/ti/common/board_detect.h |  6 +++---
 2 files changed, 6 insertions(+), 12 deletions(-)

board/ti/common/board_detect.c +3 -9

index 13aac2f03037..6e7ca9196d22 100644
@@ -231,9 +231,7 @@ char * __maybe_unused board_ti_get_rev(void)
 {
 	struct ti_common_eeprom *ep = TI_EEPROM_DATA;
 
-	if (ep->header == TI_DEAD_EEPROM_MAGIC)
-		return NULL;
-
+	/* if ep->header == TI_DEAD_EEPROM_MAGIC, this is empty already */
 	return ep->version;
 }

@@ -241,9 +239,7 @@ char * __maybe_unused board_ti_get_config(void)
 {
 	struct ti_common_eeprom *ep = TI_EEPROM_DATA;
 
-	if (ep->header == TI_DEAD_EEPROM_MAGIC)
-		return NULL;
-
+	/* if ep->header == TI_DEAD_EEPROM_MAGIC, this is empty already */
 	return ep->config;
 }

@@ -251,9 +247,7 @@ char * __maybe_unused board_ti_get_name(void)
 {
 	struct ti_common_eeprom *ep = TI_EEPROM_DATA;
 
-	if (ep->header == TI_DEAD_EEPROM_MAGIC)
-		return NULL;
-
+	/* if ep->header == TI_DEAD_EEPROM_MAGIC, this is empty already */
 	return ep->name;
 }

board/ti/common/board_detect.h +3 -3

index eb17f6f52a12..2ded3ed7764b 100644
@@ -141,7 +141,7 @@ bool board_ti_rev_is(char *rev_tag, int cmp_len);
 /**
  * board_ti_get_rev() - Get board revision for TI EVMs
  *
- * Return: NULL if eeprom was'nt read.
+ * Return: Empty string if eeprom was'nt read.
  *	   Board revision otherwise
  */
 char *board_ti_get_rev(void);

@@ -149,7 +149,7 @@ char *board_ti_get_rev(void);
 /**
  * board_ti_get_config() - Get board config for TI EVMs
  *
- * Return: NULL if eeprom was'nt read.
+ * Return: Empty string if eeprom was'nt read.
  *	   Board config otherwise
  */
 char *board_ti_get_config(void);

@@ -157,7 +157,7 @@ char *board_ti_get_config(void);
 /**
  * board_ti_get_name() - Get board name for TI EVMs
  *
- * Return: NULL if eeprom was'nt read.
+ * Return: Empty string if eeprom was'nt read.
  *	   Board name otherwise
  */
 char *board_ti_get_name(void);

Tom Rini

2016-10-13 13:50 UTC #11 lore raw

PATCH 3/3 ti: common: board_detect: Return a valid empty string for un-initialized eeprom

In reply to Nishanth Menon

Quoted 10 lines · Nishanth Menon

Current logic for query of revision, board_name, config returns NULL. Users of these functions do a direct strncmp to compare. Unfortunately, as per conventions require two valid strings to compare against and the current implementation causes a crash when compared with NULL.
We'd still like to maintain the simplistic usage of these APIs instead of redundant if (string) res=strncmp(fn(),"cmp",n); flowing all over the place.

Quoted 6 lines

Hence, since the version, name and config is already pre-initialized with empty string, just dont check for invalid header in the first place and return the empty string to the caller.
Reported-by Brad Griffis <[email protected]>
Signed-off-by Nishanth Menon <[email protected]>

Reviewed-by Tom Rini <[email protected]>

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161013/f4734b72/attachment.sig>

Tom Rini

2016-11-02 14:26 UTC #12 lore raw

PATCH 3/3 ti: common: board_detect: Return a valid empty string for un-initialized eeprom

In reply to Nishanth Menon

Quoted 10 lines · Nishanth Menon

Current logic for query of revision, board_name, config returns NULL. Users of these functions do a direct strncmp to compare. Unfortunately, as per conventions require two valid strings to compare against and the current implementation causes a crash when compared with NULL.
We'd still like to maintain the simplistic usage of these APIs instead of redundant if (string) res=strncmp(fn(),"cmp",n); flowing all over the place.

Quoted 8 lines

Hence, since the version, name and config is already pre-initialized with empty string, just dont check for invalid header in the first place and return the empty string to the caller.
Reported-by Brad Griffis <[email protected]>
Signed-off-by Nishanth Menon <[email protected]>
Reviewed-by Lokesh Vutla <[email protected]>
Reviewed-by Tom Rini <[email protected]>

Applied to u-boot/master (before v2016.11-rc3), thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161102/d9f9e998/attachment.sig>