Change in coreboot[master]: drivers/ipmi: Added function read_data_string() to make code cleaner

[Johnny Lin (Code Review)]

Johnny Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40621 )

Change subject: drivers/ipmi: Added function read_data_string() to make code cleaner
......................................................................

drivers/ipmi: Added function read_data_string() to make code cleaner

Tested on OCP Tioga Pass.

Signed-off-by: Johnny Lin 
Change-Id: I1da8abaa682af802e5cda65e5021069daf4ee717
---
M src/drivers/ipmi/ipmi_fru.c
1 file changed, 50 insertions(+), 124 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/40621/1

diff --git a/src/drivers/ipmi/ipmi_fru.c b/src/drivers/ipmi/ipmi_fru.c
index 7fd3d30..a6fb02a 100644
--- a/src/drivers/ipmi/ipmi_fru.c
+++ b/src/drivers/ipmi/ipmi_fru.c
@@ -92,6 +92,32 @@
    return length;
 }

+/* Read data string from data_ptr and store it to string, return the
+   length of the string or 0 when it's failed. */
+static int read_data_string(uint8_t *data_ptr, char **string)
+{
+	uint8_t length;
+
+	length = NUM_DATA_BYTES(data_ptr[0]);
+	if (length == 0) {
+		printk(BIOS_ERR, "%s:%d - failed due to length is zero\n", __func__, __LINE__);
+		return 0;
+	}
+
+	*string = malloc(length + 1);
+	if (!*string) {
+		printk(BIOS_ERR, "%s failed to malloc %d bytes for string data.\n", __func__,
+			length + 1);
+		return 0;
+	}
+	if (!data2str((const uint8_t *)data_ptr, *string, length)) {
+		printk(BIOS_ERR, "%s:%d - data2str failed\n", __func__, __LINE__);
+		free(*string);
+		return 0;
+	}
+
+	return length;
+}
 static void read_fru_board_info_area(const int port, const uint8_t id,
    			uint8_t offset, struct fru_board_info *info)
 {
@@ -128,61 +154,21 @@
    	printk(BIOS_ERR, "Bad FRU board info checksum.\n");
    	goto out;
    }
-	/* Read manufacturer string, bit[5:0] is the string length. */
-	length = NUM_DATA_BYTES(data_ptr[BOARD_MAN_TYPE_LEN_OFFSET]);
-	data_ptr += BOARD_MAN_TYPE_LEN_OFFSET;
-	if (length > 0) {
-		info->manufacturer = malloc(length + 1);
-		if (!info->manufacturer) {
-			printk(BIOS_ERR, "%s failed to malloc %d bytes for "
-				"manufacturer.\n", __func__, length + 1);
-			goto out;
-		}
-		if (!data2str((const uint8_t *)data_ptr, info->manufacturer, length))
-			free(info->manufacturer);
-	}
+	printk(BIOS_DEBUG, "Read board manufacturer string\n");
+	length = read_data_string(data_ptr + BOARD_MAN_TYPE_LEN_OFFSET,
+		&info->manufacturer);

-	/* Read product name string. */
-	data_ptr += length + 1;
-	length = NUM_DATA_BYTES(data_ptr[0]);
-	if (length > 0) {
-		info->product_name = malloc(length+1);
-		if (!info->product_name) {
-			printk(BIOS_ERR, "%s failed to malloc %d bytes for "
-				"product_name.\n", __func__, length + 1);
-			goto out;
-		}
-		if (!data2str((const uint8_t *)data_ptr, info->product_name, length))
-			free(info->product_name);
-	}
+	printk(BIOS_DEBUG, "Read board product name string.\n");
+	data_ptr += BOARD_MAN_TYPE_LEN_OFFSET + length + 1;
+	length = read_data_string(data_ptr, &info->product_name);

-	/* Read serial number string. */
+	printk(BIOS_DEBUG, "Read board serial number string.\n");
    data_ptr += length + 1;
-	length = NUM_DATA_BYTES(data_ptr[0]);
-	if (length > 0) {
-		info->serial_number = malloc(length + 1);
-		if (!info->serial_number) {
-			printk(BIOS_ERR, "%s failed to malloc %d bytes for "
-				"serial_number.\n", __func__, length + 1);
-			goto out;
-		}
-		if (!data2str((const uint8_t *)data_ptr, info->serial_number, length))
-			free(info->serial_number);
-	}
+	length = read_data_string(data_ptr, &info->serial_number);

-	/* Read part number string. */
+	printk(BIOS_DEBUG, "Read board part number string.\n");
    data_ptr += length + 1;
-	length = NUM_DATA_BYTES(data_ptr[0]);
-	if (length > 0) {
-		info->part_number = malloc(length + 1);
-		if (!info->part_number) {
-			printk(BIOS_ERR, "%s failed to malloc %d bytes for "
-				"part_number.\n", __func__, length + 1);
-			goto out;
-		}
-		if (!data2str((const uint8_t *)data_ptr, info->part_number, length))
-			free(info->part_number);
-	}
+	length = read_data_string(data_ptr, &info->part_number);

out:
    free(data_ptr);
@@ -225,89 +211,29 @@
    	printk(BIOS_ERR, "Bad FRU product info checksum.\n");
    	goto out;
    }
-	/* Read manufacturer string, bit[5:0] is the string length. */
-	length = NUM_DATA_BYTES(data_ptr[PRODUCT_MAN_TYPE_LEN_OFFSET]);
-	data_ptr += PRODUCT_MAN_TYPE_LEN_OFFSET;
-	if (length > 0) {
-		info->manufacturer = malloc(length + 1);
-		if (!info->manufacturer) {
-			printk(BIOS_ERR, "%s failed to malloc %d bytes for "
-				"manufacturer.\n", __func__, length + 1);
-			goto out;
-		}
-		if (!data2str((const uint8_t *)data_ptr, info->manufacturer, length))
-			free(info->manufacturer);
-	}
+	printk(BIOS_DEBUG, "Read product manufacturer string.\n");
+	length = read_data_string(data_ptr + PRODUCT_MAN_TYPE_LEN_OFFSET,
+		&info->manufacturer);

-	/* Read product_name string. */
-	data_ptr += length + 1;
-	length = NUM_DATA_BYTES(data_ptr[0]);
-	if (length > 0) {
-		info->product_name = malloc(length + 1);
-		if (!info->product_name) {
-			printk(BIOS_ERR, "%s failed to malloc %d bytes for "
-				"product_name.\n", __func__, length + 1);
-			goto out;
-		}
-		if (!data2str((const uint8_t *)data_ptr, info->product_name, length))
-			free(info->product_name);
-	}
+	data_ptr += PRODUCT_MAN_TYPE_LEN_OFFSET + length + 1;
+	printk(BIOS_DEBUG, "Read product_name string.\n");
+	length = read_data_string(data_ptr, &info->product_name);

-	/* Read product part/model number. */
    data_ptr += length + 1;
-	length = NUM_DATA_BYTES(data_ptr[0]);
-	if (length > 0) {
-		info->product_partnumber = malloc(length + 1);
-		if (!info->product_partnumber) {
-			printk(BIOS_ERR, "%s failed to malloc %d bytes for "
-				"product_partnumber.\n",	__func__, length + 1);
-			goto out;
-		}
-		if (!data2str((const uint8_t *)data_ptr, info->product_partnumber, length))
-			free(info->product_partnumber);
-	}
+	printk(BIOS_DEBUG, "Read product part/model number.\n");
+	length = read_data_string(data_ptr, &info->product_partnumber);

-	/* Read product version string. */
    data_ptr += length + 1;
-	length = NUM_DATA_BYTES(data_ptr[0]);
-	if (length > 0) {
-		info->product_version = malloc(length + 1);
-		if (!info->product_version) {
-			printk(BIOS_ERR, "%s failed to malloc %d bytes for "
-				"product_version.\n", __func__, length + 1);
-			goto out;
-		}
-		if (!data2str((const uint8_t *)data_ptr, info->product_version, length))
-			free(info->product_version);
-	}
+	printk(BIOS_DEBUG, "Read product version string.\n");
+	length = read_data_string(data_ptr, &info->product_version);

-	/* Read serial number string. */
    data_ptr += length + 1;
-	length = NUM_DATA_BYTES(data_ptr[0]);
-	if (length > 0) {
-		info->serial_number = malloc(length + 1);
-		if (!info->serial_number) {
-			printk(BIOS_ERR, "%s failed to malloc %d bytes for "
-				"serial_number.\n", __func__, length + 1);
-			goto out;
-		}
-		if (!data2str((const uint8_t *)data_ptr, info->serial_number, length))
-			free(info->serial_number);
-	}
+	printk(BIOS_DEBUG, "Read serial number string.\n");
+	length = read_data_string(data_ptr, &info->serial_number);

-	/* Read asset tag string. */
    data_ptr += length + 1;
-	length = NUM_DATA_BYTES(data_ptr[0]);
-	if (length > 0) {
-		info->asset_tag = malloc(length + 1);
-		if (!info->asset_tag) {
-			printk(BIOS_ERR, "%s failed to malloc %d bytes for "
-				"asset_tag.\n", __func__, length + 1);
-				goto out;
-			}
-		if (!data2str((const uint8_t *)data_ptr, info->asset_tag, length))
-			free(info->asset_tag);
-	}
+	printk(BIOS_DEBUG, "Read asset tag string.\n");
+	length = read_data_string(data_ptr, &info->asset_tag);

out:
    free(data_ptr);

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40621 )

Change subject: drivers/ipmi: Added function read_data_string() to make code cleaner
......................................................................

Patch Set 1:

Split adding read_data_string() from CB:40306 into this change.

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40621 )

Change subject: drivers/ipmi: Added function read_data_string() to make code cleaner
......................................................................

Patch Set 1: Code-Review+1

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40621 )

Change subject: drivers/ipmi: Added function read_data_string() to make code cleaner
......................................................................

Patch Set 2:

(5 comments)

https://review.coreboot.org/c/coreboot/+/40621/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40621/2//COMMIT_MSG@7 
PS2, Line 7: Added
Add

https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c 
File src/drivers/ipmi/ipmi_fru.c:

https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c... 
PS2, Line 96:    length of the string or 0 when it's failed. */
Concise multi-line comments are meant for comments in code blocks. Please use:

/*
     * …
     */

https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c... 
PS2, Line 97: uint8_t *data_ptr
Can it be const?

https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c... 
PS2, Line 103: 		printk(BIOS_ERR, "%s:%d - failed due to length is zero\n", __func__, __LINE__);
In my opinion error log messages should be more elaborate. But maybe the error should be printed by the caller, and here should only be a debug message?

https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c... 
PS2, Line 159: 		&info->manufacturer);
Shouldn’t you check if length is 0?

Change in coreboot[master]: drivers/ipmi: Add function read_data_string() to make code cleaner

[Johnny Lin (Code Review)]

Hello build bot (Jenkins), Jonathan Zhang, David Hendricks, Andrey Petrov, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/40621

to look at the new patch set (#3).

Change subject: drivers/ipmi: Add function read_data_string() to make code cleaner
......................................................................

drivers/ipmi: Add function read_data_string() to make code cleaner

Tested on OCP Tioga Pass.

Signed-off-by: Johnny Lin 
Change-Id: I1da8abaa682af802e5cda65e5021069daf4ee717
---
M src/drivers/ipmi/ipmi_fru.c
1 file changed, 53 insertions(+), 124 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/40621/3

Change in coreboot[master]: drivers/ipmi: Add function read_data_string() to make code cleaner

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40621 )

Change subject: drivers/ipmi: Add function read_data_string() to make code cleaner
......................................................................

Patch Set 3:

(5 comments)

https://review.coreboot.org/c/coreboot/+/40621/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40621/2//COMMIT_MSG@7 
PS2, Line 7: Added
> Add

Done

https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c 
File src/drivers/ipmi/ipmi_fru.c:

https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c... 
PS2, Line 96:    length of the string or 0 when it's failed. */
> Concise multi-line comments are meant for comments in code blocks. Please use: […]

Done

https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c... 
PS2, Line 97: uint8_t *data_ptr
> Can it be const?

Done

https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c... 
PS2, Line 103: 		printk(BIOS_ERR, "%s:%d - failed due to length is zero\n", __func__, __LINE__);
> In my opinion error log messages should be more elaborate. […]

Changed to DEBUG.

https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c... 
PS2, Line 159: 		&info->manufacturer);
> Shouldn’t you check if length is 0?

The check is done in read_data_string(), it would return 0 length if any error or there's no such data, and the code after that can work correctly.

Change in coreboot[master]: drivers/ipmi: Add function read_data_string() to make code cleaner

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40621 )

Change subject: drivers/ipmi: Add function read_data_string() to make code cleaner
......................................................................

Patch Set 3: Code-Review+1

LGTM.

Change in coreboot[master]: drivers/ipmi: Add function read_data_string() to make code cleaner

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40621 )

Change subject: drivers/ipmi: Add function read_data_string() to make code cleaner
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c 
File src/drivers/ipmi/ipmi_fru.c:

https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c... 
PS2, Line 159: 		&info->manufacturer);
> The check is done in read_data_string(), it would return 0 length if any error or there's no such da […]

In the subsequent printk, please print out length, so that the log message shows whether some data was read successfully.

Change in coreboot[master]: drivers/ipmi: Add function read_data_string() to make code cleaner

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40621 )

Change subject: drivers/ipmi: Add function read_data_string() to make code cleaner
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c 
File src/drivers/ipmi/ipmi_fru.c:

https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c... 
PS2, Line 159: 		&info->manufacturer);
> In the subsequent printk, please print out length, so that the log message shows whether some data w […]

In the current implementation, read_data_string() it would do some checks and print error message when failed, for example:
Read product FRU file ID string.
read_data_string:104 - failed due to length is zero

If the string is read successfully, no log would be printed, for example:
Read product manufacturer string.
memalign Enter, boundary 8, size 7, free_mem_ptr 0x6f9512f8
memalign 0x6f9512f8
Read product_name string. 
...
Does that meet the requirement?

Change in coreboot[master]: drivers/ipmi: Add function read_data_string() to make code cleaner

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40621 )

Change subject: drivers/ipmi: Add function read_data_string() to make code cleaner
......................................................................

Patch Set 9: Code-Review+2

Change in coreboot[master]: drivers/ipmi: Add function read_data_string() to make code cleaner

[Johnny Lin (Code Review)]

Hello build bot (Jenkins), Jonathan Zhang, David Hendricks, Jingle Hsu, Andrey Petrov, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/40621

to look at the new patch set (#10).

Change subject: drivers/ipmi: Add function read_data_string() to make code cleaner
......................................................................

drivers/ipmi: Add function read_data_string() to make code cleaner

Tested on OCP Tioga Pass.

Signed-off-by: Johnny Lin 
Change-Id: I1da8abaa682af802e5cda65e5021069daf4ee717
---
M src/drivers/ipmi/ipmi_fru.c
1 file changed, 53 insertions(+), 124 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/40621/10

Change in coreboot[master]: drivers/ipmi: Add function read_data_string() to make code cleaner

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40621 )

Change subject: drivers/ipmi: Add function read_data_string() to make code cleaner
......................................................................

Patch Set 10:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c 
File src/drivers/ipmi/ipmi_fru.c:

https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c... 
PS2, Line 159: 		&info->manufacturer);
> In the current implementation, read_data_string() it would do some checks and print error message wh […]

Done

Change in coreboot[master]: drivers/ipmi: Add function read_data_string() to make code cleaner

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40621 )

Change subject: drivers/ipmi: Add function read_data_string() to make code cleaner
......................................................................

drivers/ipmi: Add function read_data_string() to make code cleaner

Tested on OCP Tioga Pass.

Signed-off-by: Johnny Lin 
Change-Id: I1da8abaa682af802e5cda65e5021069daf4ee717
Reviewed-on: https://review.coreboot.org/c/coreboot/+/40621
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Rudolph 
Reviewed-by: Jonathan Zhang 
---
M src/drivers/ipmi/ipmi_fru.c
1 file changed, 53 insertions(+), 124 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Rudolph: Looks good to me, approved
  Jonathan Zhang: Looks good to me, but someone else must approve

diff --git a/src/drivers/ipmi/ipmi_fru.c b/src/drivers/ipmi/ipmi_fru.c
index 572c415..506e207 100644
--- a/src/drivers/ipmi/ipmi_fru.c
+++ b/src/drivers/ipmi/ipmi_fru.c
@@ -91,6 +91,35 @@
    return length;
 }

+/*
+ * Read data string from data_ptr and store it to string, return the
+ * length of the string or 0 when it's failed.
+ */
+static int read_data_string(const uint8_t *data_ptr, char **string)
+{
+	uint8_t length;
+
+	length = NUM_DATA_BYTES(data_ptr[0]);
+	if (length == 0) {
+		printk(BIOS_DEBUG, "%s:%d - failed due to length is zero\n", __func__,
+			__LINE__);
+		return 0;
+	}
+
+	*string = malloc(length + 1);
+	if (!*string) {
+		printk(BIOS_ERR, "%s failed to malloc %d bytes for string data.\n", __func__,
+			length + 1);
+		return 0;
+	}
+	if (!data2str((const uint8_t *)data_ptr, *string, length)) {
+		printk(BIOS_ERR, "%s:%d - data2str failed\n", __func__, __LINE__);
+		free(*string);
+		return 0;
+	}
+
+	return length;
+}
 static void read_fru_board_info_area(const int port, const uint8_t id,
    			uint8_t offset, struct fru_board_info *info)
 {
@@ -127,61 +156,21 @@
    	printk(BIOS_ERR, "Bad FRU board info checksum.\n");
    	goto out;
    }
-	/* Read manufacturer string, bit[5:0] is the string length. */
-	length = NUM_DATA_BYTES(data_ptr[BOARD_MAN_TYPE_LEN_OFFSET]);
-	data_ptr += BOARD_MAN_TYPE_LEN_OFFSET;
-	if (length > 0) {
-		info->manufacturer = malloc(length + 1);
-		if (!info->manufacturer) {
-			printk(BIOS_ERR, "%s failed to malloc %d bytes for "
-				"manufacturer.\n", __func__, length + 1);
-			goto out;
-		}
-		if (!data2str((const uint8_t *)data_ptr, info->manufacturer, length))
-			free(info->manufacturer);
-	}
+	printk(BIOS_DEBUG, "Read board manufacturer string\n");
+	length = read_data_string(data_ptr + BOARD_MAN_TYPE_LEN_OFFSET,
+		&info->manufacturer);

-	/* Read product name string. */
-	data_ptr += length + 1;
-	length = NUM_DATA_BYTES(data_ptr[0]);
-	if (length > 0) {
-		info->product_name = malloc(length+1);
-		if (!info->product_name) {
-			printk(BIOS_ERR, "%s failed to malloc %d bytes for "
-				"product_name.\n", __func__, length + 1);
-			goto out;
-		}
-		if (!data2str((const uint8_t *)data_ptr, info->product_name, length))
-			free(info->product_name);
-	}
+	printk(BIOS_DEBUG, "Read board product name string.\n");
+	data_ptr += BOARD_MAN_TYPE_LEN_OFFSET + length + 1;
+	length = read_data_string(data_ptr, &info->product_name);

-	/* Read serial number string. */
+	printk(BIOS_DEBUG, "Read board serial number string.\n");
    data_ptr += length + 1;
-	length = NUM_DATA_BYTES(data_ptr[0]);
-	if (length > 0) {
-		info->serial_number = malloc(length + 1);
-		if (!info->serial_number) {
-			printk(BIOS_ERR, "%s failed to malloc %d bytes for "
-				"serial_number.\n", __func__, length + 1);
-			goto out;
-		}
-		if (!data2str((const uint8_t *)data_ptr, info->serial_number, length))
-			free(info->serial_number);
-	}
+	length = read_data_string(data_ptr, &info->serial_number);

-	/* Read part number string. */
+	printk(BIOS_DEBUG, "Read board part number string.\n");
    data_ptr += length + 1;
-	length = NUM_DATA_BYTES(data_ptr[0]);
-	if (length > 0) {
-		info->part_number = malloc(length + 1);
-		if (!info->part_number) {
-			printk(BIOS_ERR, "%s failed to malloc %d bytes for "
-				"part_number.\n", __func__, length + 1);
-			goto out;
-		}
-		if (!data2str((const uint8_t *)data_ptr, info->part_number, length))
-			free(info->part_number);
-	}
+	length = read_data_string(data_ptr, &info->part_number);

out:
    free(data_ptr);
@@ -224,89 +213,29 @@
    	printk(BIOS_ERR, "Bad FRU product info checksum.\n");
    	goto out;
    }
-	/* Read manufacturer string, bit[5:0] is the string length. */
-	length = NUM_DATA_BYTES(data_ptr[PRODUCT_MAN_TYPE_LEN_OFFSET]);
-	data_ptr += PRODUCT_MAN_TYPE_LEN_OFFSET;
-	if (length > 0) {
-		info->manufacturer = malloc(length + 1);
-		if (!info->manufacturer) {
-			printk(BIOS_ERR, "%s failed to malloc %d bytes for "
-				"manufacturer.\n", __func__, length + 1);
-			goto out;
-		}
-		if (!data2str((const uint8_t *)data_ptr, info->manufacturer, length))
-			free(info->manufacturer);
-	}
+	printk(BIOS_DEBUG, "Read product manufacturer string.\n");
+	length = read_data_string(data_ptr + PRODUCT_MAN_TYPE_LEN_OFFSET,
+		&info->manufacturer);

-	/* Read product_name string. */
-	data_ptr += length + 1;
-	length = NUM_DATA_BYTES(data_ptr[0]);
-	if (length > 0) {
-		info->product_name = malloc(length + 1);
-		if (!info->product_name) {
-			printk(BIOS_ERR, "%s failed to malloc %d bytes for "
-				"product_name.\n", __func__, length + 1);
-			goto out;
-		}
-		if (!data2str((const uint8_t *)data_ptr, info->product_name, length))
-			free(info->product_name);
-	}
+	data_ptr += PRODUCT_MAN_TYPE_LEN_OFFSET + length + 1;
+	printk(BIOS_DEBUG, "Read product_name string.\n");
+	length = read_data_string(data_ptr, &info->product_name);

-	/* Read product part/model number. */
    data_ptr += length + 1;
-	length = NUM_DATA_BYTES(data_ptr[0]);
-	if (length > 0) {
-		info->product_partnumber = malloc(length + 1);
-		if (!info->product_partnumber) {
-			printk(BIOS_ERR, "%s failed to malloc %d bytes for "
-				"product_partnumber.\n",	__func__, length + 1);
-			goto out;
-		}
-		if (!data2str((const uint8_t *)data_ptr, info->product_partnumber, length))
-			free(info->product_partnumber);
-	}
+	printk(BIOS_DEBUG, "Read product part/model number.\n");
+	length = read_data_string(data_ptr, &info->product_partnumber);

-	/* Read product version string. */
    data_ptr += length + 1;
-	length = NUM_DATA_BYTES(data_ptr[0]);
-	if (length > 0) {
-		info->product_version = malloc(length + 1);
-		if (!info->product_version) {
-			printk(BIOS_ERR, "%s failed to malloc %d bytes for "
-				"product_version.\n", __func__, length + 1);
-			goto out;
-		}
-		if (!data2str((const uint8_t *)data_ptr, info->product_version, length))
-			free(info->product_version);
-	}
+	printk(BIOS_DEBUG, "Read product version string.\n");
+	length = read_data_string(data_ptr, &info->product_version);

-	/* Read serial number string. */
    data_ptr += length + 1;
-	length = NUM_DATA_BYTES(data_ptr[0]);
-	if (length > 0) {
-		info->serial_number = malloc(length + 1);
-		if (!info->serial_number) {
-			printk(BIOS_ERR, "%s failed to malloc %d bytes for "
-				"serial_number.\n", __func__, length + 1);
-			goto out;
-		}
-		if (!data2str((const uint8_t *)data_ptr, info->serial_number, length))
-			free(info->serial_number);
-	}
+	printk(BIOS_DEBUG, "Read serial number string.\n");
+	length = read_data_string(data_ptr, &info->serial_number);

-	/* Read asset tag string. */
    data_ptr += length + 1;
-	length = NUM_DATA_BYTES(data_ptr[0]);
-	if (length > 0) {
-		info->asset_tag = malloc(length + 1);
-		if (!info->asset_tag) {
-			printk(BIOS_ERR, "%s failed to malloc %d bytes for "
-				"asset_tag.\n", __func__, length + 1);
-				goto out;
-			}
-		if (!data2str((const uint8_t *)data_ptr, info->asset_tag, length))
-			free(info->asset_tag);
-	}
+	printk(BIOS_DEBUG, "Read asset tag string.\n");
+	length = read_data_string(data_ptr, &info->asset_tag);

out:
    free(data_ptr);