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