[PATCH 1/4] drm/amdgpu: Increase potential product_name to 64 characters

[PATCH 1/4] drm/amdgpu: Increase potential product_name to 64 characters

[Kent Russell]

Having seen at least 1 42-character product_name, bump the number up to
64.

Signed-off-by: Kent Russell <kent.russell@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h            | 2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index e701dedce344..1afb3066f6dd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1083,7 +1083,7 @@ struct amdgpu_device {
 
 	/* Chip product information */
 	char				product_number[16];
-	char				product_name[32];
+	char				product_name[64];
 	char				serial[20];
 
 	atomic_t			throttling_logging_enabled;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
index 7709caeb233d..b3b951fe0861 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
@@ -88,7 +88,7 @@ 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 buff[34];
+	unsigned char buff[66];
 	u32 addrptr;
 	int size, len;
 
@@ -131,11 +131,11 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
 	}
 
 	len = size;
-	/* Product name should only be 32 characters. Any more,
-	 * and something could be wrong. Cap it at 32 to be safe
+	/* Product name should logically be < 64 characters. Any more,
+	 * and something could be wrong. Cap it at 64 to be safe
 	 */
 	if (len >= sizeof(adev->product_name)) {
-		DRM_WARN("FRU Product Number is larger than 32 characters. This is likely a mistake");
+		DRM_WARN("FRU Product Name is larger than 64 characters. This is likely a mistake");
 		len = sizeof(adev->product_name) - 1;
 	}
 	/* Start at 2 due to buff using fields 0 and 1 for the address */
-- 
2.25.1

[PATCH 2/4] drm/amdgpu: Enable unique_id for Aldebaran

[Kent Russell]

It's supported, so support the unique_id sysfs file

Signed-off-by: Kent Russell <kent.russell@amd.com>
---
 drivers/gpu/drm/amd/pm/amdgpu_pm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
index 082539c70fd4..dfefb147ac2c 100644
--- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
@@ -2090,7 +2090,8 @@ static int default_attr_update(struct amdgpu_device *adev, struct amdgpu_device_
 	} else if (DEVICE_ATTR_IS(unique_id)) {
 		if (asic_type != CHIP_VEGA10 &&
 		    asic_type != CHIP_VEGA20 &&
-		    asic_type != CHIP_ARCTURUS)
+		    asic_type != CHIP_ARCTURUS &&
+		    asic_type != CHIP_ALDEBARAN)
 			*states = ATTR_STATE_UNSUPPORTED;
 	} else if (DEVICE_ATTR_IS(pp_features)) {
 		if (adev->flags & AMD_IS_APU || asic_type < CHIP_VEGA10)
-- 
2.25.1

[PATCH 3/4] drm/amdgpu: Only overwrite serial if field is empty

[Kent Russell]

On Aldebaran, the serial may be obtained from the FRU. Only overwrite
the serial with the unique_id if the serial is empty. This will support
printing serial numbers for mGPU devices where there are 2 unique_ids
for the 2 GPUs, but only one serial number for the board

Signed-off-by: Kent Russell <kent.russell@amd.com>
---
 drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
index 6e781cee8bb6..dfcf9a53e5ea 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
@@ -1615,7 +1615,8 @@ static void aldebaran_get_unique_id(struct smu_context *smu)
 	mutex_unlock(&smu->metrics_lock);
 
 	adev->unique_id = ((uint64_t)upper32 << 32) | lower32;
-	sprintf(adev->serial, "%016llx", adev->unique_id);
+	if (adev->serial[0] == '\0')
+		sprintf(adev->serial, "%016llx", adev->unique_id);
 }
 
 static bool aldebaran_is_baco_supported(struct smu_context *smu)
-- 
2.25.1

[PATCH 4/4] drm/amdgpu: Access the FRU on Aldebaran

[Kent Russell]

This is supported, although the offset is different from VG20, so fix
that with a variable and enable getting the product name and serial
number from the FRU.

Signed-off-by: Kent Russell <kent.russell@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
index b3b951fe0861..124376b666fd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
@@ -56,6 +56,8 @@ static bool is_fru_eeprom_supported(struct amdgpu_device *adev)
 			return true;
 		else
 			return false;
+	case CHIP_ALDEBARAN:
+		return true;
 	default:
 		return false;
 	}
@@ -91,6 +93,10 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
 	unsigned char buff[66];
 	u32 addrptr;
 	int size, len;
+	int offset = 2;
+
+	if (adev->asic_type == CHIP_ALDEBARAN)
+		offset = 0;
 
 	if (!is_fru_eeprom_supported(adev))
 		return 0;
@@ -139,7 +145,7 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
 		len = sizeof(adev->product_name) - 1;
 	}
 	/* Start at 2 due to buff using fields 0 and 1 for the address */
-	memcpy(adev->product_name, &buff[2], len);
+	memcpy(adev->product_name, &buff[offset], len);
 	adev->product_name[len] = '\0';
 
 	addrptr += size + 1;
@@ -157,7 +163,7 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
 		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, &buff[2], len);
+	memcpy(adev->product_number, &buff[offset], len);
 	adev->product_number[len] = '\0';
 
 	addrptr += size + 1;
@@ -184,7 +190,7 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
 		DRM_WARN("FRU Serial Number is larger than 16 characters. This is likely a mistake");
 		len = sizeof(adev->serial) - 1;
 	}
-	memcpy(adev->serial, &buff[2], len);
+	memcpy(adev->serial, &buff[offset], len);
 	adev->serial[len] = '\0';
 
 	return 0;
-- 
2.25.1

RE: [PATCH 4/4] drm/amdgpu: Access the FRU on Aldebaran

[Chen, Guchun]

[Public]

+	if (adev->asic_type == CHIP_ALDEBARAN)
+		offset = 0;
 
 	if (!is_fru_eeprom_supported(adev))
 		return 0;

I assume the logic should be adjusted. It's better to put the asic_type check after is_fru_eeprom_supported.

Regards,
Guchun

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Kent Russell
Sent: Tuesday, December 14, 2021 3:34 AM
To: amd-gfx@lists.freedesktop.org
Cc: Russell, Kent <Kent.Russell@amd.com>
Subject: [PATCH 4/4] drm/amdgpu: Access the FRU on Aldebaran

This is supported, although the offset is different from VG20, so fix that with a variable and enable getting the product name and serial number from the FRU.

Signed-off-by: Kent Russell <kent.russell@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
index b3b951fe0861..124376b666fd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
@@ -56,6 +56,8 @@ static bool is_fru_eeprom_supported(struct amdgpu_device *adev)
 			return true;
 		else
 			return false;
+	case CHIP_ALDEBARAN:
+		return true;
 	default:
 		return false;
 	}
@@ -91,6 +93,10 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
 	unsigned char buff[66];
 	u32 addrptr;
 	int size, len;
+	int offset = 2;
+
+	if (adev->asic_type == CHIP_ALDEBARAN)
+		offset = 0;
 
 	if (!is_fru_eeprom_supported(adev))
 		return 0;
@@ -139,7 +145,7 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
 		len = sizeof(adev->product_name) - 1;
 	}
 	/* Start at 2 due to buff using fields 0 and 1 for the address */
-	memcpy(adev->product_name, &buff[2], len);
+	memcpy(adev->product_name, &buff[offset], len);
 	adev->product_name[len] = '\0';
 
 	addrptr += size + 1;
@@ -157,7 +163,7 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
 		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, &buff[2], len);
+	memcpy(adev->product_number, &buff[offset], len);
 	adev->product_number[len] = '\0';
 
 	addrptr += size + 1;
@@ -184,7 +190,7 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
 		DRM_WARN("FRU Serial Number is larger than 16 characters. This is likely a mistake");
 		len = sizeof(adev->serial) - 1;
 	}
-	memcpy(adev->serial, &buff[2], len);
+	memcpy(adev->serial, &buff[offset], len);
 	adev->serial[len] = '\0';
 
 	return 0;
--
2.25.1

RE: [PATCH 1/4] drm/amdgpu: Increase potential product_name to 64 characters

[Chen, Guchun]

[Public]

How about set a define like PRODUCT_NAME_LEN to be 64, and use it in FRU code? In this case, if it needs to bump string length of product name later on, it will be simple.

#define PRODUCT_NAME_LEN 64

unsigned char buff[PRODUCT_NAME_LEN + 2];

Regards,
Guchun

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Kent Russell
Sent: Tuesday, December 14, 2021 3:34 AM
To: amd-gfx@lists.freedesktop.org
Cc: Russell, Kent <Kent.Russell@amd.com>
Subject: [PATCH 1/4] drm/amdgpu: Increase potential product_name to 64 characters

Having seen at least 1 42-character product_name, bump the number up to 64.

Signed-off-by: Kent Russell <kent.russell@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h            | 2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index e701dedce344..1afb3066f6dd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1083,7 +1083,7 @@ struct amdgpu_device {
 
 	/* Chip product information */
 	char				product_number[16];
-	char				product_name[32];
+	char				product_name[64];
 	char				serial[20];
 
 	atomic_t			throttling_logging_enabled;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
index 7709caeb233d..b3b951fe0861 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
@@ -88,7 +88,7 @@ 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 buff[34];
+	unsigned char buff[66];
 	u32 addrptr;
 	int size, len;
 
@@ -131,11 +131,11 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
 	}
 
 	len = size;
-	/* Product name should only be 32 characters. Any more,
-	 * and something could be wrong. Cap it at 32 to be safe
+	/* Product name should logically be < 64 characters. Any more,
+	 * and something could be wrong. Cap it at 64 to be safe
 	 */
 	if (len >= sizeof(adev->product_name)) {
-		DRM_WARN("FRU Product Number is larger than 32 characters. This is likely a mistake");
+		DRM_WARN("FRU Product Name is larger than 64 characters. This is 
+likely a mistake");
 		len = sizeof(adev->product_name) - 1;
 	}
 	/* Start at 2 due to buff using fields 0 and 1 for the address */
--
2.25.1

RE: [PATCH 4/4] drm/amdgpu: Access the FRU on Aldebaran

[Chen, Guchun]

[Public]

BTW, does FRU exists on all the Aldebaran SKUs?

This patch acks FRU's presence when it's Aldebaran. So if some Aldebaran SKUs do not have a FRU, some i2c access failures will be observed during boot up. This is not friendly to user.

Please check Vega20 case we talked before, and there are some more strict checks for FRU on several Vega20 SKUs.

case CHIP_VEGA20:
		/* D161 and D163 are the VG20 server SKUs */
		if (strnstr(atom_ctx->vbios_version, "D161",
			    sizeof(atom_ctx->vbios_version)) ||
		    strnstr(atom_ctx->vbios_version, "D163",
			    sizeof(atom_ctx->vbios_version)))
			return true;
		else
			return false;

Regards,
Guchun

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Chen, Guchun
Sent: Tuesday, December 14, 2021 11:17 AM
To: Russell, Kent <Kent.Russell@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Russell, Kent <Kent.Russell@amd.com>
Subject: RE: [PATCH 4/4] drm/amdgpu: Access the FRU on Aldebaran

[Public]

+	if (adev->asic_type == CHIP_ALDEBARAN)
+		offset = 0;
 
 	if (!is_fru_eeprom_supported(adev))
 		return 0;

I assume the logic should be adjusted. It's better to put the asic_type check after is_fru_eeprom_supported.

Regards,
Guchun

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Kent Russell
Sent: Tuesday, December 14, 2021 3:34 AM
To: amd-gfx@lists.freedesktop.org
Cc: Russell, Kent <Kent.Russell@amd.com>
Subject: [PATCH 4/4] drm/amdgpu: Access the FRU on Aldebaran

This is supported, although the offset is different from VG20, so fix that with a variable and enable getting the product name and serial number from the FRU.

Signed-off-by: Kent Russell <kent.russell@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
index b3b951fe0861..124376b666fd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
@@ -56,6 +56,8 @@ static bool is_fru_eeprom_supported(struct amdgpu_device *adev)
 			return true;
 		else
 			return false;
+	case CHIP_ALDEBARAN:
+		return true;
 	default:
 		return false;
 	}
@@ -91,6 +93,10 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
 	unsigned char buff[66];
 	u32 addrptr;
 	int size, len;
+	int offset = 2;
+
+	if (adev->asic_type == CHIP_ALDEBARAN)
+		offset = 0;
 
 	if (!is_fru_eeprom_supported(adev))
 		return 0;
@@ -139,7 +145,7 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
 		len = sizeof(adev->product_name) - 1;
 	}
 	/* Start at 2 due to buff using fields 0 and 1 for the address */
-	memcpy(adev->product_name, &buff[2], len);
+	memcpy(adev->product_name, &buff[offset], len);
 	adev->product_name[len] = '\0';
 
 	addrptr += size + 1;
@@ -157,7 +163,7 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
 		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, &buff[2], len);
+	memcpy(adev->product_number, &buff[offset], len);
 	adev->product_number[len] = '\0';
 
 	addrptr += size + 1;
@@ -184,7 +190,7 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
 		DRM_WARN("FRU Serial Number is larger than 16 characters. This is likely a mistake");
 		len = sizeof(adev->serial) - 1;
 	}
-	memcpy(adev->serial, &buff[2], len);
+	memcpy(adev->serial, &buff[offset], len);
 	adev->serial[len] = '\0';
 
 	return 0;
--
2.25.1

RE: [PATCH 4/4] drm/amdgpu: Access the FRU on Aldebaran

[Russell, Kent]

I'm not aware of any Aldebaran SKUs that don't have the FRU, but I can look into it before submitting this patch.

 Kent

> -----Original Message-----
> From: Chen, Guchun <Guchun.Chen@amd.com>
> Sent: Monday, December 13, 2021 10:39 PM
> To: Chen, Guchun <Guchun.Chen@amd.com>; Russell, Kent <Kent.Russell@amd.com>;
> amd-gfx@lists.freedesktop.org
> Cc: Russell, Kent <Kent.Russell@amd.com>
> Subject: RE: [PATCH 4/4] drm/amdgpu: Access the FRU on Aldebaran
> 
> [Public]
> 
> BTW, does FRU exists on all the Aldebaran SKUs?
> 
> This patch acks FRU's presence when it's Aldebaran. So if some Aldebaran SKUs do not have
> a FRU, some i2c access failures will be observed during boot up. This is not friendly to user.
> 
> Please check Vega20 case we talked before, and there are some more strict checks for FRU
> on several Vega20 SKUs.
> 
> case CHIP_VEGA20:
> 		/* D161 and D163 are the VG20 server SKUs */
> 		if (strnstr(atom_ctx->vbios_version, "D161",
> 			    sizeof(atom_ctx->vbios_version)) ||
> 		    strnstr(atom_ctx->vbios_version, "D163",
> 			    sizeof(atom_ctx->vbios_version)))
> 			return true;
> 		else
> 			return false;
> 
> Regards,
> Guchun
> 
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Chen, Guchun
> Sent: Tuesday, December 14, 2021 11:17 AM
> To: Russell, Kent <Kent.Russell@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Russell, Kent <Kent.Russell@amd.com>
> Subject: RE: [PATCH 4/4] drm/amdgpu: Access the FRU on Aldebaran
> 
> [Public]
> 
> +	if (adev->asic_type == CHIP_ALDEBARAN)
> +		offset = 0;
> 
>  	if (!is_fru_eeprom_supported(adev))
>  		return 0;
> 
> I assume the logic should be adjusted. It's better to put the asic_type check after
> is_fru_eeprom_supported.
> 
> Regards,
> Guchun
> 
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Kent Russell
> Sent: Tuesday, December 14, 2021 3:34 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Russell, Kent <Kent.Russell@amd.com>
> Subject: [PATCH 4/4] drm/amdgpu: Access the FRU on Aldebaran
> 
> This is supported, although the offset is different from VG20, so fix that with a variable and
> enable getting the product name and serial number from the FRU.
> 
> Signed-off-by: Kent Russell <kent.russell@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> index b3b951fe0861..124376b666fd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> @@ -56,6 +56,8 @@ static bool is_fru_eeprom_supported(struct amdgpu_device *adev)
>  			return true;
>  		else
>  			return false;
> +	case CHIP_ALDEBARAN:
> +		return true;
>  	default:
>  		return false;
>  	}
> @@ -91,6 +93,10 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
>  	unsigned char buff[66];
>  	u32 addrptr;
>  	int size, len;
> +	int offset = 2;
> +
> +	if (adev->asic_type == CHIP_ALDEBARAN)
> +		offset = 0;
> 
>  	if (!is_fru_eeprom_supported(adev))
>  		return 0;
> @@ -139,7 +145,7 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
>  		len = sizeof(adev->product_name) - 1;
>  	}
>  	/* Start at 2 due to buff using fields 0 and 1 for the address */
> -	memcpy(adev->product_name, &buff[2], len);
> +	memcpy(adev->product_name, &buff[offset], len);
>  	adev->product_name[len] = '\0';
> 
>  	addrptr += size + 1;
> @@ -157,7 +163,7 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
>  		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, &buff[2], len);
> +	memcpy(adev->product_number, &buff[offset], len);
>  	adev->product_number[len] = '\0';
> 
>  	addrptr += size + 1;
> @@ -184,7 +190,7 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
>  		DRM_WARN("FRU Serial Number is larger than 16 characters. This is likely a
> mistake");
>  		len = sizeof(adev->serial) - 1;
>  	}
> -	memcpy(adev->serial, &buff[2], len);
> +	memcpy(adev->serial, &buff[offset], len);
>  	adev->serial[len] = '\0';
> 
>  	return 0;
> --
> 2.25.1

RE: [PATCH 1/4] drm/amdgpu: Increase potential product_name to 64 characters

[Russell, Kent]

Yeah that would make things a little clearer, and make it easier to change going forward. Thanks!

 Kent

> -----Original Message-----
> From: Chen, Guchun <Guchun.Chen@amd.com>
> Sent: Monday, December 13, 2021 10:33 PM
> To: Russell, Kent <Kent.Russell@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Russell, Kent <Kent.Russell@amd.com>
> Subject: RE: [PATCH 1/4] drm/amdgpu: Increase potential product_name to 64 characters
> 
> [Public]
> 
> How about set a define like PRODUCT_NAME_LEN to be 64, and use it in FRU code? In this
> case, if it needs to bump string length of product name later on, it will be simple.
> 
> #define PRODUCT_NAME_LEN 64
> 
> unsigned char buff[PRODUCT_NAME_LEN + 2];
> 
> Regards,
> Guchun
> 
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Kent Russell
> Sent: Tuesday, December 14, 2021 3:34 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Russell, Kent <Kent.Russell@amd.com>
> Subject: [PATCH 1/4] drm/amdgpu: Increase potential product_name to 64 characters
> 
> Having seen at least 1 42-character product_name, bump the number up to 64.
> 
> Signed-off-by: Kent Russell <kent.russell@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h            | 2 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c | 8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index e701dedce344..1afb3066f6dd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1083,7 +1083,7 @@ struct amdgpu_device {
> 
>  	/* Chip product information */
>  	char				product_number[16];
> -	char				product_name[32];
> +	char				product_name[64];
>  	char				serial[20];
> 
>  	atomic_t			throttling_logging_enabled;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> index 7709caeb233d..b3b951fe0861 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> @@ -88,7 +88,7 @@ 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 buff[34];
> +	unsigned char buff[66];
>  	u32 addrptr;
>  	int size, len;
> 
> @@ -131,11 +131,11 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
>  	}
> 
>  	len = size;
> -	/* Product name should only be 32 characters. Any more,
> -	 * and something could be wrong. Cap it at 32 to be safe
> +	/* Product name should logically be < 64 characters. Any more,
> +	 * and something could be wrong. Cap it at 64 to be safe
>  	 */
>  	if (len >= sizeof(adev->product_name)) {
> -		DRM_WARN("FRU Product Number is larger than 32 characters. This is likely
> a mistake");
> +		DRM_WARN("FRU Product Name is larger than 64 characters. This is
> +likely a mistake");
>  		len = sizeof(adev->product_name) - 1;
>  	}
>  	/* Start at 2 due to buff using fields 0 and 1 for the address */
> --
> 2.25.1

RE: [PATCH 1/4] drm/amdgpu: Increase potential product_name to 64 characters

[Russell, Kent]

[AMD Official Use Only]

Will do. Thanks!

 Kent

> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Saturday, December 18, 2021 9:36 AM
> To: Russell, Kent <Kent.Russell@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 1/4] drm/amdgpu: Increase potential product_name to 64 characters
>
>
>
> Am 17.12.21 um 16:31 schrieb Kent Russell:
> > Having seen at least 1 42-character product_name, bump the number up to
> > 64, and put that definition into amdgpu.h to make future adjustments
> > simpler.
> >
> > Signed-off-by: Kent Russell <kent.russell@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu.h            |  3 ++-
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c | 12 +++++-------
> >   2 files changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > index e701dedce344..4e6737e4c36c 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > @@ -813,6 +813,7 @@ struct amd_powerplay {
> >
> >   #define AMDGPU_RESET_MAGIC_NUM 64
> >   #define AMDGPU_MAX_DF_PERFMONS 4
> > +#define PRODUCT_NAME_LEN 64
>
> Please prefix all defines with AMDGPU_, apart from that looks good to me.
>
> Regards,
> Christian.
>
> >   struct amdgpu_device {
> >     struct device                   *dev;
> >     struct pci_dev                  *pdev;
> > @@ -1083,7 +1084,7 @@ struct amdgpu_device {
> >
> >     /* Chip product information */
> >     char                            product_number[16];
> > -   char                            product_name[32];
> > +   char                            product_name[PRODUCT_NAME_LEN];
> >     char                            serial[20];
> >
> >     atomic_t                        throttling_logging_enabled;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> > index 7709caeb233d..5ed24701f9cf 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> > @@ -88,7 +88,7 @@ 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 buff[34];
> > +   unsigned char buff[PRODUCT_NAME_LEN+2];
> >     u32 addrptr;
> >     int size, len;
> >
> > @@ -131,12 +131,10 @@ int amdgpu_fru_get_product_info(struct amdgpu_device
> *adev)
> >     }
> >
> >     len = size;
> > -   /* Product name should only be 32 characters. Any more,
> > -    * and something could be wrong. Cap it at 32 to be safe
> > -    */
> > -   if (len >= sizeof(adev->product_name)) {
> > -           DRM_WARN("FRU Product Number is larger than 32 characters. This is likely
> a mistake");
> > -           len = sizeof(adev->product_name) - 1;
> > +   if (len >= PRODUCT_NAME_LEN) {
> > +           DRM_WARN("FRU Product Name is larger than %d characters. This is likely a
> mistake",
> > +                           PRODUCT_NAME_LEN);
> > +           len = PRODUCT_NAME_LEN - 1;
> >     }
> >     /* Start at 2 due to buff using fields 0 and 1 for the address */
> >     memcpy(adev->product_name, &buff[2], len);

Re: [PATCH 1/4] drm/amdgpu: Increase potential product_name to 64 characters

[Christian König]

Am 17.12.21 um 16:31 schrieb Kent Russell:
> Having seen at least 1 42-character product_name, bump the number up to
> 64, and put that definition into amdgpu.h to make future adjustments
> simpler.
>
> Signed-off-by: Kent Russell <kent.russell@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h            |  3 ++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c | 12 +++++-------
>   2 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index e701dedce344..4e6737e4c36c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -813,6 +813,7 @@ struct amd_powerplay {
>   
>   #define AMDGPU_RESET_MAGIC_NUM 64
>   #define AMDGPU_MAX_DF_PERFMONS 4
> +#define PRODUCT_NAME_LEN 64

Please prefix all defines with AMDGPU_, apart from that looks good to me.

Regards,
Christian.

>   struct amdgpu_device {
>   	struct device			*dev;
>   	struct pci_dev			*pdev;
> @@ -1083,7 +1084,7 @@ struct amdgpu_device {
>   
>   	/* Chip product information */
>   	char				product_number[16];
> -	char				product_name[32];
> +	char				product_name[PRODUCT_NAME_LEN];
>   	char				serial[20];
>   
>   	atomic_t			throttling_logging_enabled;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> index 7709caeb233d..5ed24701f9cf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
> @@ -88,7 +88,7 @@ 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 buff[34];
> +	unsigned char buff[PRODUCT_NAME_LEN+2];
>   	u32 addrptr;
>   	int size, len;
>   
> @@ -131,12 +131,10 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
>   	}
>   
>   	len = size;
> -	/* Product name should only be 32 characters. Any more,
> -	 * and something could be wrong. Cap it at 32 to be safe
> -	 */
> -	if (len >= sizeof(adev->product_name)) {
> -		DRM_WARN("FRU Product Number is larger than 32 characters. This is likely a mistake");
> -		len = sizeof(adev->product_name) - 1;
> +	if (len >= PRODUCT_NAME_LEN) {
> +		DRM_WARN("FRU Product Name is larger than %d characters. This is likely a mistake",
> +				PRODUCT_NAME_LEN);
> +		len = PRODUCT_NAME_LEN - 1;
>   	}
>   	/* Start at 2 due to buff using fields 0 and 1 for the address */
>   	memcpy(adev->product_name, &buff[2], len);

[PATCH 1/4] drm/amdgpu: Increase potential product_name to 64 characters

[Kent Russell]

Having seen at least 1 42-character product_name, bump the number up to
64, and put that definition into amdgpu.h to make future adjustments
simpler.

Signed-off-by: Kent Russell <kent.russell@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h            |  3 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c | 12 +++++-------
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index e701dedce344..4e6737e4c36c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -813,6 +813,7 @@ struct amd_powerplay {
 
 #define AMDGPU_RESET_MAGIC_NUM 64
 #define AMDGPU_MAX_DF_PERFMONS 4
+#define PRODUCT_NAME_LEN 64
 struct amdgpu_device {
 	struct device			*dev;
 	struct pci_dev			*pdev;
@@ -1083,7 +1084,7 @@ struct amdgpu_device {
 
 	/* Chip product information */
 	char				product_number[16];
-	char				product_name[32];
+	char				product_name[PRODUCT_NAME_LEN];
 	char				serial[20];
 
 	atomic_t			throttling_logging_enabled;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
index 7709caeb233d..5ed24701f9cf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
@@ -88,7 +88,7 @@ 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 buff[34];
+	unsigned char buff[PRODUCT_NAME_LEN+2];
 	u32 addrptr;
 	int size, len;
 
@@ -131,12 +131,10 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
 	}
 
 	len = size;
-	/* Product name should only be 32 characters. Any more,
-	 * and something could be wrong. Cap it at 32 to be safe
-	 */
-	if (len >= sizeof(adev->product_name)) {
-		DRM_WARN("FRU Product Number is larger than 32 characters. This is likely a mistake");
-		len = sizeof(adev->product_name) - 1;
+	if (len >= PRODUCT_NAME_LEN) {
+		DRM_WARN("FRU Product Name is larger than %d characters. This is likely a mistake",
+				PRODUCT_NAME_LEN);
+		len = PRODUCT_NAME_LEN - 1;
 	}
 	/* Start at 2 due to buff using fields 0 and 1 for the address */
 	memcpy(adev->product_name, &buff[2], len);
-- 
2.25.1