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.limonciello@xxxxxxx> >>>>> >>>>> 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.limonciello@xxxxxxx> >>>>> --- >>>>> 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); >>>> >>> >> >
