[PATCH] drm/amd: Refactor find_system_memory()

[Mario Limonciello]

On 2/4/2025 17:19, Felix Kuehling wrote:
> 
> On 2025-02-04 17:21, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello at 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.

Unless I added wrong, it looked to me that the offset it was pulling 
from previously was incorrect.  So I was expecting it should be aligned 
(and less error prone) to pull from the correct offset from a struct.

> 
> 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 at 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.

Ack.

> 
> 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);
>