On 2025-02-05 15:36, Mario Limonciello wrote: > On 2/5/2025 14:33, Felix Kuehling wrote: >> >> >> On 2025-02-05 14:31, Mario Limonciello wrote: >>> On 2/4/2025 17:19, Felix Kuehling wrote: >>>> >>>> On 2025-02-04 17:21, Mario Limonciello wrote: >>>>> From: Mario Limonciello <mario.limoncie...@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. >> >> The way I see it, the offsets that were used before were correct and match >> the offsets in the packed structure definition. I'm annotating the offsets >> from the end of the header in the structure definition below as proof. >> > > The problem I saw was that the dmi_data field starts a byte late though. > That's why I was thinking this is the source of the unaligned access and the > mistake. > > Let me annotate below. > >>> >>>> >>>> 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.limoncie...@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); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ The type of dm is struct dmi_header *. dm + 1 is the first byte after the dmi_header. In other words dmi_data points to the physical_handle field, which is the field with offset 0 as I annotated it below and all the other offsets match my annotations, which match the original code. Regards, Felix >>>>> - >>>>> - 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; >> >> Comments below to annotate the byte offset of each field from the end of the >> header. >> >>>>> + u16 physical_handle; // 0x0 >>>>> + u16 error_handle; // 0x2 >>>>> + u16 total_width; // 0x4 >>>>> + u16 data_width; // 0x6 (matches the original code) >>>>> + u16 size; // 0x8 >>>>> + u8 form_factor; // 0xa >>>>> + u8 device_set; // 0xb >>>>> + u8 device_locator; // 0xc >>>>> + u8 bank_locator; // 0xd >>>>> + u8 memory_type; // 0xe >>>>> + u16 type_detail; // 0xf >>>>> + u16 speed; // 0x11 (matches the original code) >>>>> +} __packed; >> >> The bottom line is, this patch doesn't change anything about which DMI data >> is accessed. It's still an unaligned access. Now I think your patch is still >> a decent cleanup. But the justification in the commit description doesn't >> make sense. >> >> Regards, >> Felix >> >>>>> + >>>>> struct kfd_topology_device *kfd_create_topology_device( >>>>> struct list_head *device_list); >>>>> void kfd_release_topology_device_list(struct list_head *device_list); >>>> >>> >> >
