Re: [PATCH] drm/amd: Refactor find_system_memory()

[Felix Kuehling <felix.kuehling@amd.com>]

On 2025-02-04 17:21, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> find_system_memory() pulls out two fields from an SMBIOS type 17
> device and sets them on KFD devices. This however is pulling from
> the middle of the field in the SMBIOS device and leads to an unaligned
> access.
> 
> Instead use a struct representation to access the members and pull
> out the two specific fields.

Isn't that still an unaligned access? I don't understand the purpose of this patch.

One more comment inline.

> 
> Link: https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.8.0.pdf p99
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 27 +++++++++++------------
>  drivers/gpu/drm/amd/amdkfd/kfd_topology.h | 17 ++++++++++++++
>  2 files changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> index ceb9fb475ef13..93d3924dfcba0 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> @@ -968,24 +968,23 @@ static void kfd_update_system_properties(void)
>  	up_read(&topology_lock);
>  }
>  
> -static void find_system_memory(const struct dmi_header *dm,
> -	void *private)
> +static void find_system_memory(const struct dmi_header *dm, void *private)
>  {
> +	struct dmi_mem_device *memdev = (struct dmi_mem_device *)(dm);

I think it would be cleaner to use container_of to get a pointer to the structure containing the header.

Regards,
  Felix

>  	struct kfd_mem_properties *mem;
> -	u16 mem_width, mem_clock;
>  	struct kfd_topology_device *kdev =
>  		(struct kfd_topology_device *)private;
> -	const u8 *dmi_data = (const u8 *)(dm + 1);
> -
> -	if (dm->type == DMI_ENTRY_MEM_DEVICE && dm->length >= 0x15) {
> -		mem_width = (u16)(*(const u16 *)(dmi_data + 0x6));
> -		mem_clock = (u16)(*(const u16 *)(dmi_data + 0x11));
> -		list_for_each_entry(mem, &kdev->mem_props, list) {
> -			if (mem_width != 0xFFFF && mem_width != 0)
> -				mem->width = mem_width;
> -			if (mem_clock != 0)
> -				mem->mem_clk_max = mem_clock;
> -		}
> +
> +	if (memdev->header.type != DMI_ENTRY_MEM_DEVICE)
> +		return;
> +	if (memdev->header.length < sizeof(struct dmi_mem_device))
> +		return;
> +
> +	list_for_each_entry(mem, &kdev->mem_props, list) {
> +		if (memdev->total_width != 0xFFFF && memdev->total_width != 0)
> +			mem->width = memdev->total_width;
> +		if (memdev->speed != 0)
> +			mem->mem_clk_max = memdev->speed;
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.h b/drivers/gpu/drm/amd/amdkfd/kfd_topology.h
> index 155b5c410af16..f06c9db7ddde9 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.h
> @@ -24,6 +24,7 @@
>  #ifndef __KFD_TOPOLOGY_H__
>  #define __KFD_TOPOLOGY_H__
>  
> +#include <linux/dmi.h>
>  #include <linux/types.h>
>  #include <linux/list.h>
>  #include <linux/kfd_sysfs.h>
> @@ -179,6 +180,22 @@ struct kfd_system_properties {
>  	struct attribute	attr_props;
>  };
>  
> +struct dmi_mem_device {
> +	struct dmi_header header;
> +	u16 physical_handle;
> +	u16 error_handle;
> +	u16 total_width;
> +	u16 data_width;
> +	u16 size;
> +	u8 form_factor;
> +	u8 device_set;
> +	u8 device_locator;
> +	u8 bank_locator;
> +	u8 memory_type;
> +	u16 type_detail;
> +	u16 speed;
> +} __packed;
> +
>  struct kfd_topology_device *kfd_create_topology_device(
>  		struct list_head *device_list);
>  void kfd_release_topology_device_list(struct list_head *device_list);