[PATCH 0/4] Fix reading IPMI data on new Aldebaran boards

[PATCH 0/4] Fix reading IPMI data on new Aldebaran boards

[Luben Tuikov]

Mostly fix reading the product name, product number, and serial number
for newer Aldebaran boards.

Luben Tuikov (4):
  drm/amdgpu: Allow non-standard EEPROM I2C address
  drm/amdgpu: Set new default I2C FRU EEPROM address
  drm/amdgpu: Read IPMI data for product information
  drm/amdgpu: Add support for RAS table at 0x40000

 drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c    |   2 +-
 .../gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c    | 215 +++++++++---------
 .../gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c    |   7 +-
 3 files changed, 115 insertions(+), 109 deletions(-)


base-commit: f9cb9d9b7cfde813d3b2d19dc0016645f985b543
-- 
2.38.1

[PATCH 1/4] drm/amdgpu: Allow non-standard EEPROM I2C address

[Luben Tuikov]

Allow non-standard EEPROM I2C address of 0x58, where the Device Type
Identifier is 1011b, where we form 1011000b = 0x58 I2C address, as on some
ASICs the FRU lives there.

Cc: Alex Deucher <Alexander.Deucher@amd.com>
Cc: Kent Russell <kent.russell@amd.com>
Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
Tested-by: Kent Russell <kent.russell@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c
index d6c4293829aab1..7d2a908438e924 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c
@@ -87,7 +87,7 @@
  * "eeprom_addr", and set A10 to 0 to write into it, and A10 and A1 to
  * 1 to lock it permanently.
  */
-#define MAKE_I2C_ADDR(_aa) ((0xA << 3) | (((_aa) >> 16) & 7))
+#define MAKE_I2C_ADDR(_aa) ((0xA << 3) | (((_aa) >> 16) & 0xF))
 
 static int __amdgpu_eeprom_xfer(struct i2c_adapter *i2c_adap, u32 eeprom_addr,
 				u8 *eeprom_buf, u16 buf_size, bool read)
-- 
2.38.1

[PATCH 2/4] drm/amdgpu: Set new default I2C FRU EEPROM address

[Luben Tuikov]

Set the new default I2C FRU EEPROM address for newer ASICs.

Cc: Alex Deucher <Alexander.Deucher@amd.com>
Cc: Kent Russell <kent.russell@amd.com>
Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
Tested-by: Kent Russell <kent.russell@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c    | 36 +++++++++++++------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
index e325150879df7e..9b2ff386b7c4f8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
@@ -29,9 +29,10 @@
 #include "amdgpu_fru_eeprom.h"
 #include "amdgpu_eeprom.h"
 
-#define FRU_EEPROM_MADDR        0x60000
+#define FRU_EEPROM_MADDR_6      0x60000
+#define FRU_EEPROM_MADDR_8      0x80000
 
-static bool is_fru_eeprom_supported(struct amdgpu_device *adev)
+static bool is_fru_eeprom_supported(struct amdgpu_device *adev, u32 *fru_addr)
 {
 	/* Only server cards have the FRU EEPROM
 	 * TODO: See if we can figure this out dynamically instead of
@@ -45,6 +46,11 @@ static bool is_fru_eeprom_supported(struct amdgpu_device *adev)
 	if (amdgpu_sriov_vf(adev))
 		return false;
 
+	/* The default I2C EEPROM address of the FRU.
+	 */
+	if (fru_addr)
+		*fru_addr = FRU_EEPROM_MADDR_8;
+
 	/* VBIOS is of the format ###-DXXXYYYY-##. For SKU identification,
 	 * we can use just the "DXXX" portion. If there were more models, we
 	 * could convert the 3 characters to a hex integer and use a switch
@@ -57,21 +63,29 @@ static bool is_fru_eeprom_supported(struct amdgpu_device *adev)
 		if (strnstr(atom_ctx->vbios_version, "D161",
 			    sizeof(atom_ctx->vbios_version)) ||
 		    strnstr(atom_ctx->vbios_version, "D163",
-			    sizeof(atom_ctx->vbios_version)))
+			    sizeof(atom_ctx->vbios_version))) {
+			*fru_addr = FRU_EEPROM_MADDR_6;
 			return true;
-		else
+		} else {
 			return false;
+		}
 	case CHIP_ALDEBARAN:
-		/* All Aldebaran SKUs have the FRU */
+		/* All Aldebaran SKUs have an FRU */
+		if (!strnstr(atom_ctx->vbios_version, "D673",
+			     sizeof(atom_ctx->vbios_version)))
+			if (fru_addr)
+				*fru_addr = FRU_EEPROM_MADDR_6;
 		return true;
 	case CHIP_SIENNA_CICHLID:
 		if (strnstr(atom_ctx->vbios_version, "D603",
-		    sizeof(atom_ctx->vbios_version))) {
+			    sizeof(atom_ctx->vbios_version))) {
 			if (strnstr(atom_ctx->vbios_version, "D603GLXE",
-			    sizeof(atom_ctx->vbios_version)))
+				    sizeof(atom_ctx->vbios_version))) {
 				return false;
-			else
+			} else {
+				*fru_addr = FRU_EEPROM_MADDR_6;
 				return true;
+			}
 		} else {
 			return false;
 		}
@@ -111,10 +125,10 @@ static int amdgpu_fru_read_eeprom(struct amdgpu_device *adev, uint32_t addrptr,
 int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
 {
 	unsigned char buf[AMDGPU_PRODUCT_NAME_LEN];
-	u32 addrptr;
+	u32 addrptr, fru_addr;
 	int size, len;
 
-	if (!is_fru_eeprom_supported(adev))
+	if (!is_fru_eeprom_supported(adev, &fru_addr))
 		return 0;
 
 	/* If algo exists, it means that the i2c_adapter's initialized */
@@ -135,7 +149,7 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
 	 * Bytes 8-a are all 1-byte and refer to the size of the entire struct,
 	 * and the language field, so just start from 0xb, manufacturer size
 	 */
-	addrptr = FRU_EEPROM_MADDR + 0xb;
+	addrptr = fru_addr + 0xb;
 	size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
 	if (size < 1) {
 		DRM_ERROR("Failed to read FRU Manufacturer, ret:%d", size);
-- 
2.38.1

[PATCH 3/4] drm/amdgpu: Read IPMI data for product information

[Luben Tuikov]

Read and interpret IPMI data to get the product name, product model, and
product serial number.

Cc: Alex Deucher <Alexander.Deucher@amd.com>
Cc: Kent Russell <kent.russell@amd.com>
Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
Tested-by: Kent Russell <kent.russell@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c    | 183 ++++++++----------
 1 file changed, 85 insertions(+), 98 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
index 9b2ff386b7c4f8..2c38ac7bc643d5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
@@ -94,39 +94,12 @@ static bool is_fru_eeprom_supported(struct amdgpu_device *adev, u32 *fru_addr)
 	}
 }
 
-static int amdgpu_fru_read_eeprom(struct amdgpu_device *adev, uint32_t addrptr,
-				  unsigned char *buf, size_t buf_size)
-{
-	int ret;
-	u8 size;
-
-	ret = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addrptr, buf, 1);
-	if (ret < 1) {
-		DRM_WARN("FRU: Failed to get size field");
-		return ret;
-	}
-
-	/* The size returned by the i2c requires subtraction of 0xC0 since the
-	 * size apparently always reports as 0xC0+actual size.
-	 */
-	size = buf[0] & 0x3F;
-	size = min_t(size_t, size, buf_size);
-
-	ret = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addrptr + 1,
-				 buf, size);
-	if (ret < 1) {
-		DRM_WARN("FRU: Failed to get data field");
-		return ret;
-	}
-
-	return size;
-}
-
 int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
 {
-	unsigned char buf[AMDGPU_PRODUCT_NAME_LEN];
-	u32 addrptr, fru_addr;
+	unsigned char buf[8], *pia;
+	u32 addr, fru_addr;
 	int size, len;
+	u8 csum;
 
 	if (!is_fru_eeprom_supported(adev, &fru_addr))
 		return 0;
@@ -137,88 +110,102 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
 		return -ENODEV;
 	}
 
-	/* There's a lot of repetition here. This is due to the FRU having
-	 * variable-length fields. To get the information, we have to find the
-	 * size of each field, and then keep reading along and reading along
-	 * until we get all of the data that we want. We use addrptr to track
-	 * the address as we go
-	 */
-
-	/* The first fields are all of size 1-byte, from 0-7 are offsets that
-	 * contain information that isn't useful to us.
-	 * Bytes 8-a are all 1-byte and refer to the size of the entire struct,
-	 * and the language field, so just start from 0xb, manufacturer size
-	 */
-	addrptr = fru_addr + 0xb;
-	size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
-	if (size < 1) {
-		DRM_ERROR("Failed to read FRU Manufacturer, ret:%d", size);
-		return -EINVAL;
+	/* Read the IPMI Common header */
+	len = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, fru_addr, buf,
+				 sizeof(buf));
+	if (len != 8) {
+		DRM_ERROR("Couldn't read the IPMI Common Header: %d", len);
+		return len < 0 ? len : -EIO;
 	}
 
-	/* Increment the addrptr by the size of the field, and 1 due to the
-	 * size field being 1 byte. This pattern continues below.
-	 */
-	addrptr += size + 1;
-	size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
-	if (size < 1) {
-		DRM_ERROR("Failed to read FRU product name, ret:%d", size);
-		return -EINVAL;
+	if (buf[0] != 1) {
+		DRM_ERROR("Bad IPMI Common Header version: 0x%02x", buf[0]);
+		return -EIO;
 	}
 
-	len = size;
-	if (len >= AMDGPU_PRODUCT_NAME_LEN) {
-		DRM_WARN("FRU Product Name is larger than %d characters. This is likely a mistake",
-				AMDGPU_PRODUCT_NAME_LEN);
-		len = AMDGPU_PRODUCT_NAME_LEN - 1;
-	}
-	memcpy(adev->product_name, buf, len);
-	adev->product_name[len] = '\0';
-
-	addrptr += size + 1;
-	size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
-	if (size < 1) {
-		DRM_ERROR("Failed to read FRU product number, ret:%d", size);
-		return -EINVAL;
+	for (csum = 0; len > 0; len--)
+		csum += buf[len - 1];
+	if (csum) {
+		DRM_ERROR("Bad IPMI Common Header checksum: 0x%02x", csum);
+		return -EIO;
 	}
 
-	len = size;
-	/* Product number should only be 16 characters. Any more,
-	 * and something could be wrong. Cap it at 16 to be safe
-	 */
-	if (len >= sizeof(adev->product_number)) {
-		DRM_WARN("FRU Product Number is larger than 16 characters. This is likely a mistake");
-		len = sizeof(adev->product_number) - 1;
-	}
-	memcpy(adev->product_number, buf, len);
-	adev->product_number[len] = '\0';
+	/* Get the offset to the Product Info Area (PIA). */
+	addr = buf[4] * 8;
+	if (!addr)
+		return 0;
 
-	addrptr += size + 1;
-	size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
+	/* Get the absolute address to the PIA. */
+	addr += fru_addr;
 
-	if (size < 1) {
-		DRM_ERROR("Failed to read FRU product version, ret:%d", size);
-		return -EINVAL;
+	/* Read the header of the PIA. */
+	len = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addr, buf, 3);
+	if (len != 3) {
+		DRM_ERROR("Couldn't read the Product Info Area header: %d", len);
+		return len < 0 ? len : -EIO;
 	}
 
-	addrptr += size + 1;
-	size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
+	if (buf[0] != 1) {
+		DRM_ERROR("Bad IPMI Product Info Area version: 0x%02x", buf[0]);
+		return -EIO;
+	}
 
-	if (size < 1) {
-		DRM_ERROR("Failed to read FRU serial number, ret:%d", size);
-		return -EINVAL;
+	size = buf[1] * 8;
+	pia = kzalloc(size, GFP_KERNEL);
+	if (!pia)
+		return -ENOMEM;
+
+	/* Read the whole PIA. */
+	len = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addr, pia, size);
+	if (len != size) {
+		kfree(pia);
+		DRM_ERROR("Couldn't read the Product Info Area: %d", len);
+		return len < 0 ? len : -EIO;
 	}
 
-	len = size;
-	/* Serial number should only be 16 characters. Any more,
-	 * and something could be wrong. Cap it at 16 to be safe
-	 */
-	if (len >= sizeof(adev->serial)) {
-		DRM_WARN("FRU Serial Number is larger than 16 characters. This is likely a mistake");
-		len = sizeof(adev->serial) - 1;
+	for (csum = 0; size > 0; size--)
+		csum += pia[size - 1];
+	if (csum) {
+		DRM_ERROR("Bad Product Info Area checksum: 0x%02x", csum);
+		return -EIO;
 	}
-	memcpy(adev->serial, buf, len);
-	adev->serial[len] = '\0';
 
+	/* Now extract useful information from the PIA.
+	 *
+	 * Skip the Manufacturer Name at [3] and go directly to
+	 * the Product Name field.
+	 */
+	addr = 3 + 1 + (pia[3] & 0x3F);
+	if (addr + 1 >= len)
+		goto Out;
+	memcpy(adev->product_name, pia + addr + 1,
+	       min_t(size_t,
+		     sizeof(adev->product_name),
+		     pia[addr] & 0x3F));
+	adev->product_name[sizeof(adev->product_name) - 1] = '\0';
+
+	/* Go to the Product Part/Model Number field. */
+	addr += 1 + (pia[addr] & 0x3F);
+	if (addr + 1 >= len)
+		goto Out;
+	memcpy(adev->product_number, pia + addr + 1,
+	       min_t(size_t,
+		     sizeof(adev->product_number),
+		     pia[addr] & 0x3F));
+	adev->product_number[sizeof(adev->product_number) - 1] = '\0';
+
+	/* Go to the Product Version field. */
+	addr += 1 + (pia[addr] & 0x3F);
+
+	/* Go to the Product Serial Number field. */
+	addr += 1 + (pia[addr] & 0x3F);
+	if (addr + 1 >= len)
+		goto Out;
+	memcpy(adev->serial, pia + addr + 1, min_t(size_t,
+						   sizeof(adev->serial),
+						   pia[addr] & 0x3F));
+	adev->serial[sizeof(adev->serial) - 1] = '\0';
+Out:
+	kfree(pia);
 	return 0;
 }
-- 
2.38.1

[PATCH 4/4] drm/amdgpu: Add support for RAS table at 0x40000

[Luben Tuikov]

Add support for RAS table at I2C EEPROM address of 0x40000, since on some
ASICs it is not at 0, but at 0x40000.

Cc: Alex Deucher <Alexander.Deucher@amd.com>
Cc: Kent Russell <kent.russell@amd.com>
Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
Tested-by: Kent Russell <kent.russell@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
index f63bd31e199c8e..2d9f3f4cd79e92 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
@@ -157,6 +157,7 @@ static bool __get_eeprom_i2c_addr_ip_discovery(struct amdgpu_device *adev,
 static bool __get_eeprom_i2c_addr(struct amdgpu_device *adev,
 				  struct amdgpu_ras_eeprom_control *control)
 {
+	struct atom_context *atom_ctx = adev->mode_info.atom_context;
 	u8 i2c_addr;
 
 	if (!control)
@@ -190,7 +191,11 @@ static bool __get_eeprom_i2c_addr(struct amdgpu_device *adev,
 		break;
 
 	case CHIP_ALDEBARAN:
-		control->i2c_address = EEPROM_I2C_MADDR_0;
+		if (strnstr(atom_ctx->vbios_version, "D673",
+			    sizeof(atom_ctx->vbios_version)))
+			control->i2c_address = EEPROM_I2C_MADDR_4;
+		else
+			control->i2c_address = EEPROM_I2C_MADDR_0;
 		break;
 
 	case CHIP_IP_DISCOVERY:
-- 
2.38.1

Re: [PATCH 3/4] drm/amdgpu: Read IPMI data for product information

[Alex Deucher]

On Wed, Nov 16, 2022 at 2:49 PM Luben Tuikov <luben.tuikov@amd.com> wrote:
>
> Read and interpret IPMI data to get the product name, product model, and
> product serial number.

Patches 1,2,4 are:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
for patch 3:
It's not clear from the commit message what this change is doing.  Is
this just a rewrite/cleanup of the existing FRU parsing or is there a
bug fix in here related to the rest of this series?

Alex

>
> Cc: Alex Deucher <Alexander.Deucher@amd.com>
> Cc: Kent Russell <kent.russell@amd.com>
> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
> Tested-by: Kent Russell <kent.russell@amd.com>
> ---
>  .../gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c    | 183 ++++++++----------
>  1 file changed, 85 insertions(+), 98 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> index 9b2ff386b7c4f8..2c38ac7bc643d5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> @@ -94,39 +94,12 @@ static bool is_fru_eeprom_supported(struct amdgpu_device *adev, u32 *fru_addr)
>         }
>  }
>
> -static int amdgpu_fru_read_eeprom(struct amdgpu_device *adev, uint32_t addrptr,
> -                                 unsigned char *buf, size_t buf_size)
> -{
> -       int ret;
> -       u8 size;
> -
> -       ret = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addrptr, buf, 1);
> -       if (ret < 1) {
> -               DRM_WARN("FRU: Failed to get size field");
> -               return ret;
> -       }
> -
> -       /* The size returned by the i2c requires subtraction of 0xC0 since the
> -        * size apparently always reports as 0xC0+actual size.
> -        */
> -       size = buf[0] & 0x3F;
> -       size = min_t(size_t, size, buf_size);
> -
> -       ret = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addrptr + 1,
> -                                buf, size);
> -       if (ret < 1) {
> -               DRM_WARN("FRU: Failed to get data field");
> -               return ret;
> -       }
> -
> -       return size;
> -}
> -
>  int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
>  {
> -       unsigned char buf[AMDGPU_PRODUCT_NAME_LEN];
> -       u32 addrptr, fru_addr;
> +       unsigned char buf[8], *pia;
> +       u32 addr, fru_addr;
>         int size, len;
> +       u8 csum;
>
>         if (!is_fru_eeprom_supported(adev, &fru_addr))
>                 return 0;
> @@ -137,88 +110,102 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
>                 return -ENODEV;
>         }
>
> -       /* There's a lot of repetition here. This is due to the FRU having
> -        * variable-length fields. To get the information, we have to find the
> -        * size of each field, and then keep reading along and reading along
> -        * until we get all of the data that we want. We use addrptr to track
> -        * the address as we go
> -        */
> -
> -       /* The first fields are all of size 1-byte, from 0-7 are offsets that
> -        * contain information that isn't useful to us.
> -        * Bytes 8-a are all 1-byte and refer to the size of the entire struct,
> -        * and the language field, so just start from 0xb, manufacturer size
> -        */
> -       addrptr = fru_addr + 0xb;
> -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
> -       if (size < 1) {
> -               DRM_ERROR("Failed to read FRU Manufacturer, ret:%d", size);
> -               return -EINVAL;
> +       /* Read the IPMI Common header */
> +       len = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, fru_addr, buf,
> +                                sizeof(buf));
> +       if (len != 8) {
> +               DRM_ERROR("Couldn't read the IPMI Common Header: %d", len);
> +               return len < 0 ? len : -EIO;
>         }
>
> -       /* Increment the addrptr by the size of the field, and 1 due to the
> -        * size field being 1 byte. This pattern continues below.
> -        */
> -       addrptr += size + 1;
> -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
> -       if (size < 1) {
> -               DRM_ERROR("Failed to read FRU product name, ret:%d", size);
> -               return -EINVAL;
> +       if (buf[0] != 1) {
> +               DRM_ERROR("Bad IPMI Common Header version: 0x%02x", buf[0]);
> +               return -EIO;
>         }
>
> -       len = size;
> -       if (len >= AMDGPU_PRODUCT_NAME_LEN) {
> -               DRM_WARN("FRU Product Name is larger than %d characters. This is likely a mistake",
> -                               AMDGPU_PRODUCT_NAME_LEN);
> -               len = AMDGPU_PRODUCT_NAME_LEN - 1;
> -       }
> -       memcpy(adev->product_name, buf, len);
> -       adev->product_name[len] = '\0';
> -
> -       addrptr += size + 1;
> -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
> -       if (size < 1) {
> -               DRM_ERROR("Failed to read FRU product number, ret:%d", size);
> -               return -EINVAL;
> +       for (csum = 0; len > 0; len--)
> +               csum += buf[len - 1];
> +       if (csum) {
> +               DRM_ERROR("Bad IPMI Common Header checksum: 0x%02x", csum);
> +               return -EIO;
>         }
>
> -       len = size;
> -       /* Product number should only be 16 characters. Any more,
> -        * and something could be wrong. Cap it at 16 to be safe
> -        */
> -       if (len >= sizeof(adev->product_number)) {
> -               DRM_WARN("FRU Product Number is larger than 16 characters. This is likely a mistake");
> -               len = sizeof(adev->product_number) - 1;
> -       }
> -       memcpy(adev->product_number, buf, len);
> -       adev->product_number[len] = '\0';
> +       /* Get the offset to the Product Info Area (PIA). */
> +       addr = buf[4] * 8;
> +       if (!addr)
> +               return 0;
>
> -       addrptr += size + 1;
> -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
> +       /* Get the absolute address to the PIA. */
> +       addr += fru_addr;
>
> -       if (size < 1) {
> -               DRM_ERROR("Failed to read FRU product version, ret:%d", size);
> -               return -EINVAL;
> +       /* Read the header of the PIA. */
> +       len = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addr, buf, 3);
> +       if (len != 3) {
> +               DRM_ERROR("Couldn't read the Product Info Area header: %d", len);
> +               return len < 0 ? len : -EIO;
>         }
>
> -       addrptr += size + 1;
> -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
> +       if (buf[0] != 1) {
> +               DRM_ERROR("Bad IPMI Product Info Area version: 0x%02x", buf[0]);
> +               return -EIO;
> +       }
>
> -       if (size < 1) {
> -               DRM_ERROR("Failed to read FRU serial number, ret:%d", size);
> -               return -EINVAL;
> +       size = buf[1] * 8;
> +       pia = kzalloc(size, GFP_KERNEL);
> +       if (!pia)
> +               return -ENOMEM;
> +
> +       /* Read the whole PIA. */
> +       len = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addr, pia, size);
> +       if (len != size) {
> +               kfree(pia);
> +               DRM_ERROR("Couldn't read the Product Info Area: %d", len);
> +               return len < 0 ? len : -EIO;
>         }
>
> -       len = size;
> -       /* Serial number should only be 16 characters. Any more,
> -        * and something could be wrong. Cap it at 16 to be safe
> -        */
> -       if (len >= sizeof(adev->serial)) {
> -               DRM_WARN("FRU Serial Number is larger than 16 characters. This is likely a mistake");
> -               len = sizeof(adev->serial) - 1;
> +       for (csum = 0; size > 0; size--)
> +               csum += pia[size - 1];
> +       if (csum) {
> +               DRM_ERROR("Bad Product Info Area checksum: 0x%02x", csum);
> +               return -EIO;
>         }
> -       memcpy(adev->serial, buf, len);
> -       adev->serial[len] = '\0';
>
> +       /* Now extract useful information from the PIA.
> +        *
> +        * Skip the Manufacturer Name at [3] and go directly to
> +        * the Product Name field.
> +        */
> +       addr = 3 + 1 + (pia[3] & 0x3F);
> +       if (addr + 1 >= len)
> +               goto Out;
> +       memcpy(adev->product_name, pia + addr + 1,
> +              min_t(size_t,
> +                    sizeof(adev->product_name),
> +                    pia[addr] & 0x3F));
> +       adev->product_name[sizeof(adev->product_name) - 1] = '\0';
> +
> +       /* Go to the Product Part/Model Number field. */
> +       addr += 1 + (pia[addr] & 0x3F);
> +       if (addr + 1 >= len)
> +               goto Out;
> +       memcpy(adev->product_number, pia + addr + 1,
> +              min_t(size_t,
> +                    sizeof(adev->product_number),
> +                    pia[addr] & 0x3F));
> +       adev->product_number[sizeof(adev->product_number) - 1] = '\0';
> +
> +       /* Go to the Product Version field. */
> +       addr += 1 + (pia[addr] & 0x3F);
> +
> +       /* Go to the Product Serial Number field. */
> +       addr += 1 + (pia[addr] & 0x3F);
> +       if (addr + 1 >= len)
> +               goto Out;
> +       memcpy(adev->serial, pia + addr + 1, min_t(size_t,
> +                                                  sizeof(adev->serial),
> +                                                  pia[addr] & 0x3F));
> +       adev->serial[sizeof(adev->serial) - 1] = '\0';
> +Out:
> +       kfree(pia);
>         return 0;
>  }
> --
> 2.38.1
>

RE: [PATCH 3/4] drm/amdgpu: Read IPMI data for product information

[Russell, Kent]

[AMD Official Use Only - General]

You can add my 

Reviewed-by: Kent Russell <kent.russell@amd.com>
as well. And I have no issue with a little elaboration on the commit. We definitely changed a few things here.

 Kent

> -----Original Message-----
> From: Alex Deucher <alexdeucher@gmail.com>
> Sent: Wednesday, November 16, 2022 3:59 PM
> To: Tuikov, Luben <Luben.Tuikov@amd.com>
> Cc: AMD Graphics <amd-gfx@lists.freedesktop.org>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Russell, Kent <Kent.Russell@amd.com>
> Subject: Re: [PATCH 3/4] drm/amdgpu: Read IPMI data for product information
> 
> On Wed, Nov 16, 2022 at 2:49 PM Luben Tuikov <luben.tuikov@amd.com>
> wrote:
> >
> > Read and interpret IPMI data to get the product name, product model, and
> > product serial number.
> 
> Patches 1,2,4 are:
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> for patch 3:
> It's not clear from the commit message what this change is doing.  Is
> this just a rewrite/cleanup of the existing FRU parsing or is there a
> bug fix in here related to the rest of this series?
> 
> Alex
> 
> >
> > Cc: Alex Deucher <Alexander.Deucher@amd.com>
> > Cc: Kent Russell <kent.russell@amd.com>
> > Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
> > Tested-by: Kent Russell <kent.russell@amd.com>
> > ---
> >  .../gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c    | 183 ++++++++----------
> >  1 file changed, 85 insertions(+), 98 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> > index 9b2ff386b7c4f8..2c38ac7bc643d5 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> > @@ -94,39 +94,12 @@ static bool is_fru_eeprom_supported(struct
> amdgpu_device *adev, u32 *fru_addr)
> >         }
> >  }
> >
> > -static int amdgpu_fru_read_eeprom(struct amdgpu_device *adev, uint32_t
> addrptr,
> > -                                 unsigned char *buf, size_t buf_size)
> > -{
> > -       int ret;
> > -       u8 size;
> > -
> > -       ret = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addrptr, buf,
> 1);
> > -       if (ret < 1) {
> > -               DRM_WARN("FRU: Failed to get size field");
> > -               return ret;
> > -       }
> > -
> > -       /* The size returned by the i2c requires subtraction of 0xC0 since the
> > -        * size apparently always reports as 0xC0+actual size.
> > -        */
> > -       size = buf[0] & 0x3F;
> > -       size = min_t(size_t, size, buf_size);
> > -
> > -       ret = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addrptr + 1,
> > -                                buf, size);
> > -       if (ret < 1) {
> > -               DRM_WARN("FRU: Failed to get data field");
> > -               return ret;
> > -       }
> > -
> > -       return size;
> > -}
> > -
> >  int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
> >  {
> > -       unsigned char buf[AMDGPU_PRODUCT_NAME_LEN];
> > -       u32 addrptr, fru_addr;
> > +       unsigned char buf[8], *pia;
> > +       u32 addr, fru_addr;
> >         int size, len;
> > +       u8 csum;
> >
> >         if (!is_fru_eeprom_supported(adev, &fru_addr))
> >                 return 0;
> > @@ -137,88 +110,102 @@ int amdgpu_fru_get_product_info(struct
> amdgpu_device *adev)
> >                 return -ENODEV;
> >         }
> >
> > -       /* There's a lot of repetition here. This is due to the FRU having
> > -        * variable-length fields. To get the information, we have to find the
> > -        * size of each field, and then keep reading along and reading along
> > -        * until we get all of the data that we want. We use addrptr to track
> > -        * the address as we go
> > -        */
> > -
> > -       /* The first fields are all of size 1-byte, from 0-7 are offsets that
> > -        * contain information that isn't useful to us.
> > -        * Bytes 8-a are all 1-byte and refer to the size of the entire struct,
> > -        * and the language field, so just start from 0xb, manufacturer size
> > -        */
> > -       addrptr = fru_addr + 0xb;
> > -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
> > -       if (size < 1) {
> > -               DRM_ERROR("Failed to read FRU Manufacturer, ret:%d", size);
> > -               return -EINVAL;
> > +       /* Read the IPMI Common header */
> > +       len = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, fru_addr,
> buf,
> > +                                sizeof(buf));
> > +       if (len != 8) {
> > +               DRM_ERROR("Couldn't read the IPMI Common Header: %d", len);
> > +               return len < 0 ? len : -EIO;
> >         }
> >
> > -       /* Increment the addrptr by the size of the field, and 1 due to the
> > -        * size field being 1 byte. This pattern continues below.
> > -        */
> > -       addrptr += size + 1;
> > -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
> > -       if (size < 1) {
> > -               DRM_ERROR("Failed to read FRU product name, ret:%d", size);
> > -               return -EINVAL;
> > +       if (buf[0] != 1) {
> > +               DRM_ERROR("Bad IPMI Common Header version: 0x%02x", buf[0]);
> > +               return -EIO;
> >         }
> >
> > -       len = size;
> > -       if (len >= AMDGPU_PRODUCT_NAME_LEN) {
> > -               DRM_WARN("FRU Product Name is larger than %d characters. This is
> likely a mistake",
> > -                               AMDGPU_PRODUCT_NAME_LEN);
> > -               len = AMDGPU_PRODUCT_NAME_LEN - 1;
> > -       }
> > -       memcpy(adev->product_name, buf, len);
> > -       adev->product_name[len] = '\0';
> > -
> > -       addrptr += size + 1;
> > -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
> > -       if (size < 1) {
> > -               DRM_ERROR("Failed to read FRU product number, ret:%d", size);
> > -               return -EINVAL;
> > +       for (csum = 0; len > 0; len--)
> > +               csum += buf[len - 1];
> > +       if (csum) {
> > +               DRM_ERROR("Bad IPMI Common Header checksum: 0x%02x",
> csum);
> > +               return -EIO;
> >         }
> >
> > -       len = size;
> > -       /* Product number should only be 16 characters. Any more,
> > -        * and something could be wrong. Cap it at 16 to be safe
> > -        */
> > -       if (len >= sizeof(adev->product_number)) {
> > -               DRM_WARN("FRU Product Number is larger than 16 characters. This
> is likely a mistake");
> > -               len = sizeof(adev->product_number) - 1;
> > -       }
> > -       memcpy(adev->product_number, buf, len);
> > -       adev->product_number[len] = '\0';
> > +       /* Get the offset to the Product Info Area (PIA). */
> > +       addr = buf[4] * 8;
> > +       if (!addr)
> > +               return 0;
> >
> > -       addrptr += size + 1;
> > -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
> > +       /* Get the absolute address to the PIA. */
> > +       addr += fru_addr;
> >
> > -       if (size < 1) {
> > -               DRM_ERROR("Failed to read FRU product version, ret:%d", size);
> > -               return -EINVAL;
> > +       /* Read the header of the PIA. */
> > +       len = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addr, buf,
> 3);
> > +       if (len != 3) {
> > +               DRM_ERROR("Couldn't read the Product Info Area header: %d", len);
> > +               return len < 0 ? len : -EIO;
> >         }
> >
> > -       addrptr += size + 1;
> > -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
> > +       if (buf[0] != 1) {
> > +               DRM_ERROR("Bad IPMI Product Info Area version: 0x%02x", buf[0]);
> > +               return -EIO;
> > +       }
> >
> > -       if (size < 1) {
> > -               DRM_ERROR("Failed to read FRU serial number, ret:%d", size);
> > -               return -EINVAL;
> > +       size = buf[1] * 8;
> > +       pia = kzalloc(size, GFP_KERNEL);
> > +       if (!pia)
> > +               return -ENOMEM;
> > +
> > +       /* Read the whole PIA. */
> > +       len = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addr, pia,
> size);
> > +       if (len != size) {
> > +               kfree(pia);
> > +               DRM_ERROR("Couldn't read the Product Info Area: %d", len);
> > +               return len < 0 ? len : -EIO;
> >         }
> >
> > -       len = size;
> > -       /* Serial number should only be 16 characters. Any more,
> > -        * and something could be wrong. Cap it at 16 to be safe
> > -        */
> > -       if (len >= sizeof(adev->serial)) {
> > -               DRM_WARN("FRU Serial Number is larger than 16 characters. This is
> likely a mistake");
> > -               len = sizeof(adev->serial) - 1;
> > +       for (csum = 0; size > 0; size--)
> > +               csum += pia[size - 1];
> > +       if (csum) {
> > +               DRM_ERROR("Bad Product Info Area checksum: 0x%02x", csum);
> > +               return -EIO;
> >         }
> > -       memcpy(adev->serial, buf, len);
> > -       adev->serial[len] = '\0';
> >
> > +       /* Now extract useful information from the PIA.
> > +        *
> > +        * Skip the Manufacturer Name at [3] and go directly to
> > +        * the Product Name field.
> > +        */
> > +       addr = 3 + 1 + (pia[3] & 0x3F);
> > +       if (addr + 1 >= len)
> > +               goto Out;
> > +       memcpy(adev->product_name, pia + addr + 1,
> > +              min_t(size_t,
> > +                    sizeof(adev->product_name),
> > +                    pia[addr] & 0x3F));
> > +       adev->product_name[sizeof(adev->product_name) - 1] = '\0';
> > +
> > +       /* Go to the Product Part/Model Number field. */
> > +       addr += 1 + (pia[addr] & 0x3F);
> > +       if (addr + 1 >= len)
> > +               goto Out;
> > +       memcpy(adev->product_number, pia + addr + 1,
> > +              min_t(size_t,
> > +                    sizeof(adev->product_number),
> > +                    pia[addr] & 0x3F));
> > +       adev->product_number[sizeof(adev->product_number) - 1] = '\0';
> > +
> > +       /* Go to the Product Version field. */
> > +       addr += 1 + (pia[addr] & 0x3F);
> > +
> > +       /* Go to the Product Serial Number field. */
> > +       addr += 1 + (pia[addr] & 0x3F);
> > +       if (addr + 1 >= len)
> > +               goto Out;
> > +       memcpy(adev->serial, pia + addr + 1, min_t(size_t,
> > +                                                  sizeof(adev->serial),
> > +                                                  pia[addr] & 0x3F));
> > +       adev->serial[sizeof(adev->serial) - 1] = '\0';
> > +Out:
> > +       kfree(pia);
> >         return 0;
> >  }
> > --
> > 2.38.1
> >

Re: [PATCH 3/4] drm/amdgpu: Read IPMI data for product information

[Luben Tuikov]

It is actually reading the IPMI data as it should. Previously to this, it was
just assuming where the data would be, it's size, and so on. In talking
to some engineers internally, we concluded that it shouldn't do that, and it
should instead follow the IPMI spec to read the data--in the same way
that data was written there--folloing the IPMI spec.

Regards,
Luben

On 2022-11-16 15:58, Alex Deucher wrote:
> On Wed, Nov 16, 2022 at 2:49 PM Luben Tuikov <luben.tuikov@amd.com> wrote:
>>
>> Read and interpret IPMI data to get the product name, product model, and
>> product serial number.
> 
> Patches 1,2,4 are:
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> for patch 3:
> It's not clear from the commit message what this change is doing.  Is
> this just a rewrite/cleanup of the existing FRU parsing or is there a
> bug fix in here related to the rest of this series?
> 
> Alex
> 
>>
>> Cc: Alex Deucher <Alexander.Deucher@amd.com>
>> Cc: Kent Russell <kent.russell@amd.com>
>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
>> Tested-by: Kent Russell <kent.russell@amd.com>
>> ---
>>  .../gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c    | 183 ++++++++----------
>>  1 file changed, 85 insertions(+), 98 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
>> index 9b2ff386b7c4f8..2c38ac7bc643d5 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
>> @@ -94,39 +94,12 @@ static bool is_fru_eeprom_supported(struct amdgpu_device *adev, u32 *fru_addr)
>>         }
>>  }
>>
>> -static int amdgpu_fru_read_eeprom(struct amdgpu_device *adev, uint32_t addrptr,
>> -                                 unsigned char *buf, size_t buf_size)
>> -{
>> -       int ret;
>> -       u8 size;
>> -
>> -       ret = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addrptr, buf, 1);
>> -       if (ret < 1) {
>> -               DRM_WARN("FRU: Failed to get size field");
>> -               return ret;
>> -       }
>> -
>> -       /* The size returned by the i2c requires subtraction of 0xC0 since the
>> -        * size apparently always reports as 0xC0+actual size.
>> -        */
>> -       size = buf[0] & 0x3F;
>> -       size = min_t(size_t, size, buf_size);
>> -
>> -       ret = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addrptr + 1,
>> -                                buf, size);
>> -       if (ret < 1) {
>> -               DRM_WARN("FRU: Failed to get data field");
>> -               return ret;
>> -       }
>> -
>> -       return size;
>> -}
>> -
>>  int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
>>  {
>> -       unsigned char buf[AMDGPU_PRODUCT_NAME_LEN];
>> -       u32 addrptr, fru_addr;
>> +       unsigned char buf[8], *pia;
>> +       u32 addr, fru_addr;
>>         int size, len;
>> +       u8 csum;
>>
>>         if (!is_fru_eeprom_supported(adev, &fru_addr))
>>                 return 0;
>> @@ -137,88 +110,102 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
>>                 return -ENODEV;
>>         }
>>
>> -       /* There's a lot of repetition here. This is due to the FRU having
>> -        * variable-length fields. To get the information, we have to find the
>> -        * size of each field, and then keep reading along and reading along
>> -        * until we get all of the data that we want. We use addrptr to track
>> -        * the address as we go
>> -        */
>> -
>> -       /* The first fields are all of size 1-byte, from 0-7 are offsets that
>> -        * contain information that isn't useful to us.
>> -        * Bytes 8-a are all 1-byte and refer to the size of the entire struct,
>> -        * and the language field, so just start from 0xb, manufacturer size
>> -        */
>> -       addrptr = fru_addr + 0xb;
>> -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
>> -       if (size < 1) {
>> -               DRM_ERROR("Failed to read FRU Manufacturer, ret:%d", size);
>> -               return -EINVAL;
>> +       /* Read the IPMI Common header */
>> +       len = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, fru_addr, buf,
>> +                                sizeof(buf));
>> +       if (len != 8) {
>> +               DRM_ERROR("Couldn't read the IPMI Common Header: %d", len);
>> +               return len < 0 ? len : -EIO;
>>         }
>>
>> -       /* Increment the addrptr by the size of the field, and 1 due to the
>> -        * size field being 1 byte. This pattern continues below.
>> -        */
>> -       addrptr += size + 1;
>> -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
>> -       if (size < 1) {
>> -               DRM_ERROR("Failed to read FRU product name, ret:%d", size);
>> -               return -EINVAL;
>> +       if (buf[0] != 1) {
>> +               DRM_ERROR("Bad IPMI Common Header version: 0x%02x", buf[0]);
>> +               return -EIO;
>>         }
>>
>> -       len = size;
>> -       if (len >= AMDGPU_PRODUCT_NAME_LEN) {
>> -               DRM_WARN("FRU Product Name is larger than %d characters. This is likely a mistake",
>> -                               AMDGPU_PRODUCT_NAME_LEN);
>> -               len = AMDGPU_PRODUCT_NAME_LEN - 1;
>> -       }
>> -       memcpy(adev->product_name, buf, len);
>> -       adev->product_name[len] = '\0';
>> -
>> -       addrptr += size + 1;
>> -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
>> -       if (size < 1) {
>> -               DRM_ERROR("Failed to read FRU product number, ret:%d", size);
>> -               return -EINVAL;
>> +       for (csum = 0; len > 0; len--)
>> +               csum += buf[len - 1];
>> +       if (csum) {
>> +               DRM_ERROR("Bad IPMI Common Header checksum: 0x%02x", csum);
>> +               return -EIO;
>>         }
>>
>> -       len = size;
>> -       /* Product number should only be 16 characters. Any more,
>> -        * and something could be wrong. Cap it at 16 to be safe
>> -        */
>> -       if (len >= sizeof(adev->product_number)) {
>> -               DRM_WARN("FRU Product Number is larger than 16 characters. This is likely a mistake");
>> -               len = sizeof(adev->product_number) - 1;
>> -       }
>> -       memcpy(adev->product_number, buf, len);
>> -       adev->product_number[len] = '\0';
>> +       /* Get the offset to the Product Info Area (PIA). */
>> +       addr = buf[4] * 8;
>> +       if (!addr)
>> +               return 0;
>>
>> -       addrptr += size + 1;
>> -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
>> +       /* Get the absolute address to the PIA. */
>> +       addr += fru_addr;
>>
>> -       if (size < 1) {
>> -               DRM_ERROR("Failed to read FRU product version, ret:%d", size);
>> -               return -EINVAL;
>> +       /* Read the header of the PIA. */
>> +       len = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addr, buf, 3);
>> +       if (len != 3) {
>> +               DRM_ERROR("Couldn't read the Product Info Area header: %d", len);
>> +               return len < 0 ? len : -EIO;
>>         }
>>
>> -       addrptr += size + 1;
>> -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
>> +       if (buf[0] != 1) {
>> +               DRM_ERROR("Bad IPMI Product Info Area version: 0x%02x", buf[0]);
>> +               return -EIO;
>> +       }
>>
>> -       if (size < 1) {
>> -               DRM_ERROR("Failed to read FRU serial number, ret:%d", size);
>> -               return -EINVAL;
>> +       size = buf[1] * 8;
>> +       pia = kzalloc(size, GFP_KERNEL);
>> +       if (!pia)
>> +               return -ENOMEM;
>> +
>> +       /* Read the whole PIA. */
>> +       len = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addr, pia, size);
>> +       if (len != size) {
>> +               kfree(pia);
>> +               DRM_ERROR("Couldn't read the Product Info Area: %d", len);
>> +               return len < 0 ? len : -EIO;
>>         }
>>
>> -       len = size;
>> -       /* Serial number should only be 16 characters. Any more,
>> -        * and something could be wrong. Cap it at 16 to be safe
>> -        */
>> -       if (len >= sizeof(adev->serial)) {
>> -               DRM_WARN("FRU Serial Number is larger than 16 characters. This is likely a mistake");
>> -               len = sizeof(adev->serial) - 1;
>> +       for (csum = 0; size > 0; size--)
>> +               csum += pia[size - 1];
>> +       if (csum) {
>> +               DRM_ERROR("Bad Product Info Area checksum: 0x%02x", csum);
>> +               return -EIO;
>>         }
>> -       memcpy(adev->serial, buf, len);
>> -       adev->serial[len] = '\0';
>>
>> +       /* Now extract useful information from the PIA.
>> +        *
>> +        * Skip the Manufacturer Name at [3] and go directly to
>> +        * the Product Name field.
>> +        */
>> +       addr = 3 + 1 + (pia[3] & 0x3F);
>> +       if (addr + 1 >= len)
>> +               goto Out;
>> +       memcpy(adev->product_name, pia + addr + 1,
>> +              min_t(size_t,
>> +                    sizeof(adev->product_name),
>> +                    pia[addr] & 0x3F));
>> +       adev->product_name[sizeof(adev->product_name) - 1] = '\0';
>> +
>> +       /* Go to the Product Part/Model Number field. */
>> +       addr += 1 + (pia[addr] & 0x3F);
>> +       if (addr + 1 >= len)
>> +               goto Out;
>> +       memcpy(adev->product_number, pia + addr + 1,
>> +              min_t(size_t,
>> +                    sizeof(adev->product_number),
>> +                    pia[addr] & 0x3F));
>> +       adev->product_number[sizeof(adev->product_number) - 1] = '\0';
>> +
>> +       /* Go to the Product Version field. */
>> +       addr += 1 + (pia[addr] & 0x3F);
>> +
>> +       /* Go to the Product Serial Number field. */
>> +       addr += 1 + (pia[addr] & 0x3F);
>> +       if (addr + 1 >= len)
>> +               goto Out;
>> +       memcpy(adev->serial, pia + addr + 1, min_t(size_t,
>> +                                                  sizeof(adev->serial),
>> +                                                  pia[addr] & 0x3F));
>> +       adev->serial[sizeof(adev->serial) - 1] = '\0';
>> +Out:
>> +       kfree(pia);
>>         return 0;
>>  }
>> --
>> 2.38.1
>>

Re: [PATCH 3/4] drm/amdgpu: Read IPMI data for product information

[Luben Tuikov]

Not sure how much more to say, than what the commit description says now:

	Read and interpret IPMI data to get the product name, product model, and
	product serial number.

It reads IPMI data, and interpets it, to get the prodcut name, product model,
and product serial number.

What else is missing which should be part of the commit description?

The change of this commmit doesn't really do anything else.

Regards,
Luben

On 2022-11-16 16:19, Russell, Kent wrote:
> [AMD Official Use Only - General]
> 
> You can add my 
> 
> Reviewed-by: Kent Russell <kent.russell@amd.com>
> as well. And I have no issue with a little elaboration on the commit. We definitely changed a few things here.
> 
>  Kent
> 
>> -----Original Message-----
>> From: Alex Deucher <alexdeucher@gmail.com>
>> Sent: Wednesday, November 16, 2022 3:59 PM
>> To: Tuikov, Luben <Luben.Tuikov@amd.com>
>> Cc: AMD Graphics <amd-gfx@lists.freedesktop.org>; Deucher, Alexander
>> <Alexander.Deucher@amd.com>; Russell, Kent <Kent.Russell@amd.com>
>> Subject: Re: [PATCH 3/4] drm/amdgpu: Read IPMI data for product information
>>
>> On Wed, Nov 16, 2022 at 2:49 PM Luben Tuikov <luben.tuikov@amd.com>
>> wrote:
>>>
>>> Read and interpret IPMI data to get the product name, product model, and
>>> product serial number.
>>
>> Patches 1,2,4 are:
>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>> for patch 3:
>> It's not clear from the commit message what this change is doing.  Is
>> this just a rewrite/cleanup of the existing FRU parsing or is there a
>> bug fix in here related to the rest of this series?
>>
>> Alex
>>
>>>
>>> Cc: Alex Deucher <Alexander.Deucher@amd.com>
>>> Cc: Kent Russell <kent.russell@amd.com>
>>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
>>> Tested-by: Kent Russell <kent.russell@amd.com>
>>> ---
>>>  .../gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c    | 183 ++++++++----------
>>>  1 file changed, 85 insertions(+), 98 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
>>> index 9b2ff386b7c4f8..2c38ac7bc643d5 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
>>> @@ -94,39 +94,12 @@ static bool is_fru_eeprom_supported(struct
>> amdgpu_device *adev, u32 *fru_addr)
>>>         }
>>>  }
>>>
>>> -static int amdgpu_fru_read_eeprom(struct amdgpu_device *adev, uint32_t
>> addrptr,
>>> -                                 unsigned char *buf, size_t buf_size)
>>> -{
>>> -       int ret;
>>> -       u8 size;
>>> -
>>> -       ret = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addrptr, buf,
>> 1);
>>> -       if (ret < 1) {
>>> -               DRM_WARN("FRU: Failed to get size field");
>>> -               return ret;
>>> -       }
>>> -
>>> -       /* The size returned by the i2c requires subtraction of 0xC0 since the
>>> -        * size apparently always reports as 0xC0+actual size.
>>> -        */
>>> -       size = buf[0] & 0x3F;
>>> -       size = min_t(size_t, size, buf_size);
>>> -
>>> -       ret = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addrptr + 1,
>>> -                                buf, size);
>>> -       if (ret < 1) {
>>> -               DRM_WARN("FRU: Failed to get data field");
>>> -               return ret;
>>> -       }
>>> -
>>> -       return size;
>>> -}
>>> -
>>>  int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
>>>  {
>>> -       unsigned char buf[AMDGPU_PRODUCT_NAME_LEN];
>>> -       u32 addrptr, fru_addr;
>>> +       unsigned char buf[8], *pia;
>>> +       u32 addr, fru_addr;
>>>         int size, len;
>>> +       u8 csum;
>>>
>>>         if (!is_fru_eeprom_supported(adev, &fru_addr))
>>>                 return 0;
>>> @@ -137,88 +110,102 @@ int amdgpu_fru_get_product_info(struct
>> amdgpu_device *adev)
>>>                 return -ENODEV;
>>>         }
>>>
>>> -       /* There's a lot of repetition here. This is due to the FRU having
>>> -        * variable-length fields. To get the information, we have to find the
>>> -        * size of each field, and then keep reading along and reading along
>>> -        * until we get all of the data that we want. We use addrptr to track
>>> -        * the address as we go
>>> -        */
>>> -
>>> -       /* The first fields are all of size 1-byte, from 0-7 are offsets that
>>> -        * contain information that isn't useful to us.
>>> -        * Bytes 8-a are all 1-byte and refer to the size of the entire struct,
>>> -        * and the language field, so just start from 0xb, manufacturer size
>>> -        */
>>> -       addrptr = fru_addr + 0xb;
>>> -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
>>> -       if (size < 1) {
>>> -               DRM_ERROR("Failed to read FRU Manufacturer, ret:%d", size);
>>> -               return -EINVAL;
>>> +       /* Read the IPMI Common header */
>>> +       len = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, fru_addr,
>> buf,
>>> +                                sizeof(buf));
>>> +       if (len != 8) {
>>> +               DRM_ERROR("Couldn't read the IPMI Common Header: %d", len);
>>> +               return len < 0 ? len : -EIO;
>>>         }
>>>
>>> -       /* Increment the addrptr by the size of the field, and 1 due to the
>>> -        * size field being 1 byte. This pattern continues below.
>>> -        */
>>> -       addrptr += size + 1;
>>> -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
>>> -       if (size < 1) {
>>> -               DRM_ERROR("Failed to read FRU product name, ret:%d", size);
>>> -               return -EINVAL;
>>> +       if (buf[0] != 1) {
>>> +               DRM_ERROR("Bad IPMI Common Header version: 0x%02x", buf[0]);
>>> +               return -EIO;
>>>         }
>>>
>>> -       len = size;
>>> -       if (len >= AMDGPU_PRODUCT_NAME_LEN) {
>>> -               DRM_WARN("FRU Product Name is larger than %d characters. This is
>> likely a mistake",
>>> -                               AMDGPU_PRODUCT_NAME_LEN);
>>> -               len = AMDGPU_PRODUCT_NAME_LEN - 1;
>>> -       }
>>> -       memcpy(adev->product_name, buf, len);
>>> -       adev->product_name[len] = '\0';
>>> -
>>> -       addrptr += size + 1;
>>> -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
>>> -       if (size < 1) {
>>> -               DRM_ERROR("Failed to read FRU product number, ret:%d", size);
>>> -               return -EINVAL;
>>> +       for (csum = 0; len > 0; len--)
>>> +               csum += buf[len - 1];
>>> +       if (csum) {
>>> +               DRM_ERROR("Bad IPMI Common Header checksum: 0x%02x",
>> csum);
>>> +               return -EIO;
>>>         }
>>>
>>> -       len = size;
>>> -       /* Product number should only be 16 characters. Any more,
>>> -        * and something could be wrong. Cap it at 16 to be safe
>>> -        */
>>> -       if (len >= sizeof(adev->product_number)) {
>>> -               DRM_WARN("FRU Product Number is larger than 16 characters. This
>> is likely a mistake");
>>> -               len = sizeof(adev->product_number) - 1;
>>> -       }
>>> -       memcpy(adev->product_number, buf, len);
>>> -       adev->product_number[len] = '\0';
>>> +       /* Get the offset to the Product Info Area (PIA). */
>>> +       addr = buf[4] * 8;
>>> +       if (!addr)
>>> +               return 0;
>>>
>>> -       addrptr += size + 1;
>>> -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
>>> +       /* Get the absolute address to the PIA. */
>>> +       addr += fru_addr;
>>>
>>> -       if (size < 1) {
>>> -               DRM_ERROR("Failed to read FRU product version, ret:%d", size);
>>> -               return -EINVAL;
>>> +       /* Read the header of the PIA. */
>>> +       len = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addr, buf,
>> 3);
>>> +       if (len != 3) {
>>> +               DRM_ERROR("Couldn't read the Product Info Area header: %d", len);
>>> +               return len < 0 ? len : -EIO;
>>>         }
>>>
>>> -       addrptr += size + 1;
>>> -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
>>> +       if (buf[0] != 1) {
>>> +               DRM_ERROR("Bad IPMI Product Info Area version: 0x%02x", buf[0]);
>>> +               return -EIO;
>>> +       }
>>>
>>> -       if (size < 1) {
>>> -               DRM_ERROR("Failed to read FRU serial number, ret:%d", size);
>>> -               return -EINVAL;
>>> +       size = buf[1] * 8;
>>> +       pia = kzalloc(size, GFP_KERNEL);
>>> +       if (!pia)
>>> +               return -ENOMEM;
>>> +
>>> +       /* Read the whole PIA. */
>>> +       len = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addr, pia,
>> size);
>>> +       if (len != size) {
>>> +               kfree(pia);
>>> +               DRM_ERROR("Couldn't read the Product Info Area: %d", len);
>>> +               return len < 0 ? len : -EIO;
>>>         }
>>>
>>> -       len = size;
>>> -       /* Serial number should only be 16 characters. Any more,
>>> -        * and something could be wrong. Cap it at 16 to be safe
>>> -        */
>>> -       if (len >= sizeof(adev->serial)) {
>>> -               DRM_WARN("FRU Serial Number is larger than 16 characters. This is
>> likely a mistake");
>>> -               len = sizeof(adev->serial) - 1;
>>> +       for (csum = 0; size > 0; size--)
>>> +               csum += pia[size - 1];
>>> +       if (csum) {
>>> +               DRM_ERROR("Bad Product Info Area checksum: 0x%02x", csum);
>>> +               return -EIO;
>>>         }
>>> -       memcpy(adev->serial, buf, len);
>>> -       adev->serial[len] = '\0';
>>>
>>> +       /* Now extract useful information from the PIA.
>>> +        *
>>> +        * Skip the Manufacturer Name at [3] and go directly to
>>> +        * the Product Name field.
>>> +        */
>>> +       addr = 3 + 1 + (pia[3] & 0x3F);
>>> +       if (addr + 1 >= len)
>>> +               goto Out;
>>> +       memcpy(adev->product_name, pia + addr + 1,
>>> +              min_t(size_t,
>>> +                    sizeof(adev->product_name),
>>> +                    pia[addr] & 0x3F));
>>> +       adev->product_name[sizeof(adev->product_name) - 1] = '\0';
>>> +
>>> +       /* Go to the Product Part/Model Number field. */
>>> +       addr += 1 + (pia[addr] & 0x3F);
>>> +       if (addr + 1 >= len)
>>> +               goto Out;
>>> +       memcpy(adev->product_number, pia + addr + 1,
>>> +              min_t(size_t,
>>> +                    sizeof(adev->product_number),
>>> +                    pia[addr] & 0x3F));
>>> +       adev->product_number[sizeof(adev->product_number) - 1] = '\0';
>>> +
>>> +       /* Go to the Product Version field. */
>>> +       addr += 1 + (pia[addr] & 0x3F);
>>> +
>>> +       /* Go to the Product Serial Number field. */
>>> +       addr += 1 + (pia[addr] & 0x3F);
>>> +       if (addr + 1 >= len)
>>> +               goto Out;
>>> +       memcpy(adev->serial, pia + addr + 1, min_t(size_t,
>>> +                                                  sizeof(adev->serial),
>>> +                                                  pia[addr] & 0x3F));
>>> +       adev->serial[sizeof(adev->serial) - 1] = '\0';
>>> +Out:
>>> +       kfree(pia);
>>>         return 0;
>>>  }
>>> --
>>> 2.38.1
>>>

Re: [PATCH 3/4] drm/amdgpu: Read IPMI data for product information

[Luben Tuikov]

On 2022-11-16 15:58, Alex Deucher wrote:
> On Wed, Nov 16, 2022 at 2:49 PM Luben Tuikov <luben.tuikov@amd.com> wrote:
>>
>> Read and interpret IPMI data to get the product name, product model, and
>> product serial number.
> 
> Patches 1,2,4 are:
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> for patch 3:
> It's not clear from the commit message what this change is doing.  Is
> this just a rewrite/cleanup of the existing FRU parsing or is there a
> bug fix in here related to the rest of this series?

The previous code just hacked away at reading the data, assuming
locations and sizes of the data. It shouldn't do that. It should
instead interpret the IPMI data as per the IPMI spec and find out
the data stored by the formatted tables as per the IPMI spec. It
should've been written like so from the outset.

The commit description says:

    Read and interpret IPMI data to get the product name, product model, and
    product serial number.

And that's all that the commit is doing really. Nothing more, nothing less.

Regards,
Luben

> 
> Alex
> 
>>
>> Cc: Alex Deucher <Alexander.Deucher@amd.com>
>> Cc: Kent Russell <kent.russell@amd.com>
>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
>> Tested-by: Kent Russell <kent.russell@amd.com>
>> ---
>>  .../gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c    | 183 ++++++++----------
>>  1 file changed, 85 insertions(+), 98 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
>> index 9b2ff386b7c4f8..2c38ac7bc643d5 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
>> @@ -94,39 +94,12 @@ static bool is_fru_eeprom_supported(struct amdgpu_device *adev, u32 *fru_addr)
>>         }
>>  }
>>
>> -static int amdgpu_fru_read_eeprom(struct amdgpu_device *adev, uint32_t addrptr,
>> -                                 unsigned char *buf, size_t buf_size)
>> -{
>> -       int ret;
>> -       u8 size;
>> -
>> -       ret = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addrptr, buf, 1);
>> -       if (ret < 1) {
>> -               DRM_WARN("FRU: Failed to get size field");
>> -               return ret;
>> -       }
>> -
>> -       /* The size returned by the i2c requires subtraction of 0xC0 since the
>> -        * size apparently always reports as 0xC0+actual size.
>> -        */
>> -       size = buf[0] & 0x3F;
>> -       size = min_t(size_t, size, buf_size);
>> -
>> -       ret = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addrptr + 1,
>> -                                buf, size);
>> -       if (ret < 1) {
>> -               DRM_WARN("FRU: Failed to get data field");
>> -               return ret;
>> -       }
>> -
>> -       return size;
>> -}
>> -
>>  int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
>>  {
>> -       unsigned char buf[AMDGPU_PRODUCT_NAME_LEN];
>> -       u32 addrptr, fru_addr;
>> +       unsigned char buf[8], *pia;
>> +       u32 addr, fru_addr;
>>         int size, len;
>> +       u8 csum;
>>
>>         if (!is_fru_eeprom_supported(adev, &fru_addr))
>>                 return 0;
>> @@ -137,88 +110,102 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
>>                 return -ENODEV;
>>         }
>>
>> -       /* There's a lot of repetition here. This is due to the FRU having
>> -        * variable-length fields. To get the information, we have to find the
>> -        * size of each field, and then keep reading along and reading along
>> -        * until we get all of the data that we want. We use addrptr to track
>> -        * the address as we go
>> -        */
>> -
>> -       /* The first fields are all of size 1-byte, from 0-7 are offsets that
>> -        * contain information that isn't useful to us.
>> -        * Bytes 8-a are all 1-byte and refer to the size of the entire struct,
>> -        * and the language field, so just start from 0xb, manufacturer size
>> -        */
>> -       addrptr = fru_addr + 0xb;
>> -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
>> -       if (size < 1) {
>> -               DRM_ERROR("Failed to read FRU Manufacturer, ret:%d", size);
>> -               return -EINVAL;
>> +       /* Read the IPMI Common header */
>> +       len = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, fru_addr, buf,
>> +                                sizeof(buf));
>> +       if (len != 8) {
>> +               DRM_ERROR("Couldn't read the IPMI Common Header: %d", len);
>> +               return len < 0 ? len : -EIO;
>>         }
>>
>> -       /* Increment the addrptr by the size of the field, and 1 due to the
>> -        * size field being 1 byte. This pattern continues below.
>> -        */
>> -       addrptr += size + 1;
>> -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
>> -       if (size < 1) {
>> -               DRM_ERROR("Failed to read FRU product name, ret:%d", size);
>> -               return -EINVAL;
>> +       if (buf[0] != 1) {
>> +               DRM_ERROR("Bad IPMI Common Header version: 0x%02x", buf[0]);
>> +               return -EIO;
>>         }
>>
>> -       len = size;
>> -       if (len >= AMDGPU_PRODUCT_NAME_LEN) {
>> -               DRM_WARN("FRU Product Name is larger than %d characters. This is likely a mistake",
>> -                               AMDGPU_PRODUCT_NAME_LEN);
>> -               len = AMDGPU_PRODUCT_NAME_LEN - 1;
>> -       }
>> -       memcpy(adev->product_name, buf, len);
>> -       adev->product_name[len] = '\0';
>> -
>> -       addrptr += size + 1;
>> -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
>> -       if (size < 1) {
>> -               DRM_ERROR("Failed to read FRU product number, ret:%d", size);
>> -               return -EINVAL;
>> +       for (csum = 0; len > 0; len--)
>> +               csum += buf[len - 1];
>> +       if (csum) {
>> +               DRM_ERROR("Bad IPMI Common Header checksum: 0x%02x", csum);
>> +               return -EIO;
>>         }
>>
>> -       len = size;
>> -       /* Product number should only be 16 characters. Any more,
>> -        * and something could be wrong. Cap it at 16 to be safe
>> -        */
>> -       if (len >= sizeof(adev->product_number)) {
>> -               DRM_WARN("FRU Product Number is larger than 16 characters. This is likely a mistake");
>> -               len = sizeof(adev->product_number) - 1;
>> -       }
>> -       memcpy(adev->product_number, buf, len);
>> -       adev->product_number[len] = '\0';
>> +       /* Get the offset to the Product Info Area (PIA). */
>> +       addr = buf[4] * 8;
>> +       if (!addr)
>> +               return 0;
>>
>> -       addrptr += size + 1;
>> -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
>> +       /* Get the absolute address to the PIA. */
>> +       addr += fru_addr;
>>
>> -       if (size < 1) {
>> -               DRM_ERROR("Failed to read FRU product version, ret:%d", size);
>> -               return -EINVAL;
>> +       /* Read the header of the PIA. */
>> +       len = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addr, buf, 3);
>> +       if (len != 3) {
>> +               DRM_ERROR("Couldn't read the Product Info Area header: %d", len);
>> +               return len < 0 ? len : -EIO;
>>         }
>>
>> -       addrptr += size + 1;
>> -       size = amdgpu_fru_read_eeprom(adev, addrptr, buf, sizeof(buf));
>> +       if (buf[0] != 1) {
>> +               DRM_ERROR("Bad IPMI Product Info Area version: 0x%02x", buf[0]);
>> +               return -EIO;
>> +       }
>>
>> -       if (size < 1) {
>> -               DRM_ERROR("Failed to read FRU serial number, ret:%d", size);
>> -               return -EINVAL;
>> +       size = buf[1] * 8;
>> +       pia = kzalloc(size, GFP_KERNEL);
>> +       if (!pia)
>> +               return -ENOMEM;
>> +
>> +       /* Read the whole PIA. */
>> +       len = amdgpu_eeprom_read(adev->pm.fru_eeprom_i2c_bus, addr, pia, size);
>> +       if (len != size) {
>> +               kfree(pia);
>> +               DRM_ERROR("Couldn't read the Product Info Area: %d", len);
>> +               return len < 0 ? len : -EIO;
>>         }
>>
>> -       len = size;
>> -       /* Serial number should only be 16 characters. Any more,
>> -        * and something could be wrong. Cap it at 16 to be safe
>> -        */
>> -       if (len >= sizeof(adev->serial)) {
>> -               DRM_WARN("FRU Serial Number is larger than 16 characters. This is likely a mistake");
>> -               len = sizeof(adev->serial) - 1;
>> +       for (csum = 0; size > 0; size--)
>> +               csum += pia[size - 1];
>> +       if (csum) {
>> +               DRM_ERROR("Bad Product Info Area checksum: 0x%02x", csum);
>> +               return -EIO;
>>         }
>> -       memcpy(adev->serial, buf, len);
>> -       adev->serial[len] = '\0';
>>
>> +       /* Now extract useful information from the PIA.
>> +        *
>> +        * Skip the Manufacturer Name at [3] and go directly to
>> +        * the Product Name field.
>> +        */
>> +       addr = 3 + 1 + (pia[3] & 0x3F);
>> +       if (addr + 1 >= len)
>> +               goto Out;
>> +       memcpy(adev->product_name, pia + addr + 1,
>> +              min_t(size_t,
>> +                    sizeof(adev->product_name),
>> +                    pia[addr] & 0x3F));
>> +       adev->product_name[sizeof(adev->product_name) - 1] = '\0';
>> +
>> +       /* Go to the Product Part/Model Number field. */
>> +       addr += 1 + (pia[addr] & 0x3F);
>> +       if (addr + 1 >= len)
>> +               goto Out;
>> +       memcpy(adev->product_number, pia + addr + 1,
>> +              min_t(size_t,
>> +                    sizeof(adev->product_number),
>> +                    pia[addr] & 0x3F));
>> +       adev->product_number[sizeof(adev->product_number) - 1] = '\0';
>> +
>> +       /* Go to the Product Version field. */
>> +       addr += 1 + (pia[addr] & 0x3F);
>> +
>> +       /* Go to the Product Serial Number field. */
>> +       addr += 1 + (pia[addr] & 0x3F);
>> +       if (addr + 1 >= len)
>> +               goto Out;
>> +       memcpy(adev->serial, pia + addr + 1, min_t(size_t,
>> +                                                  sizeof(adev->serial),
>> +                                                  pia[addr] & 0x3F));
>> +       adev->serial[sizeof(adev->serial) - 1] = '\0';
>> +Out:
>> +       kfree(pia);
>>         return 0;
>>  }
>> --
>> 2.38.1
>>