On Wed, Nov 16, 2022 at 8:25 PM Luben Tuikov <luben.tuikov@xxxxxxx> wrote:
>
> Don't assume FRU MCU memory locations for the FRU data fields, or their sizes,
> instead reading and interpret the IPMI data, as stipulated in the IPMI spec
> version 1.0 rev 1.2.
>
> Extract the Product Name, Product Part/Model Number, and the Product Serial
> Number by interpreting the IPMI data.
>
> Check the checksums of the stored IPMI data to make sure we don't read and
> give corrupted data back the the user.
>
> Eliminate small I2C reads, and instead read the whole Product Info Area in one
> go, and then extract the information we're seeking from it.
>
> Eliminates a whole function, making this file smaller.
>
> v2: Clarify changes in the commit message.
Thanks for clarifying, Series is:
Reviewed-by: Alex Deucher <alexander.deucher@xxxxxxx>
>
> Cc: Alex Deucher <Alexander.Deucher@xxxxxxx>
> Cc: Kent Russell <kent.russell@xxxxxxx>
> Signed-off-by: Luben Tuikov <luben.tuikov@xxxxxxx>
> Tested-by: Kent Russell <kent.russell@xxxxxxx>
> Reviewed-by: Kent Russell <kent.russell@xxxxxxx>
> ---
> .../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
>
