| Message ID | 20250204222140.3883013-1-superm1@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/amd: Refactor find_system_memory() | expand |
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);
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); 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);