Type 40 entries (Additional information) are summarized in section
7.41 as part of the SMBIOS specification.  Save these entries when
decoding the DMI tables.

Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
v2:
 * Drop some unneeded variables (LKP robot)
 * Allow any length strings, not just 5 and longer
---
 drivers/firmware/dmi_scan.c | 38 +++++++++++++++++++++++++++++++++++++
 include/linux/dmi.h         |  7 +++++++
 2 files changed, 45 insertions(+)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 80aded4c778bc..ec84fe3935c1e 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -393,6 +393,40 @@ static void __init dmi_save_dev_pciaddr(int instance, int segment, int bus,
 	list_add(&dev->dev.list, &dmi_devices);
 }
 
+static void __init dmi_save_additional(const struct dmi_additional_info *info)
+{
+	const u8 *data;
+	int i;
+
+	if (!info || info->header.length < 5 + info->count * 5)
+		return;
+
+	data = info->entries;
+
+	for (i = 0; i < info->count; i++) {
+		u8 string_num = data[i * 5 + 4];
+		const char *string_ptr;
+		char *value;
+		int len;
+
+		string_ptr = dmi_string_nosave(&info->header, string_num);
+		if (!string_ptr || !*string_ptr)
+			continue;
+
+		len = strlen(string_ptr);
+		if (len == 0)
+			continue;
+
+		value = dmi_alloc(len + 1);
+		if (!value)
+			continue;
+
+		strscpy(value, string_ptr, len + 1);
+
+		dmi_save_one_device(DMI_DEV_TYPE_ADDITIONAL, value);
+	}
+}
+
 static void __init dmi_save_extended_devices(const struct dmi_header *dm)
 {
 	const char *name;
@@ -526,8 +560,12 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
 	case DMI_ENTRY_IPMI_DEV:
 		dmi_save_ipmi_device(dm);
 		break;
+	case DMI_ENTRY_ADDITIONAL:
+		dmi_save_additional((const struct dmi_additional_info *)dm);
+		break;
 	case DMI_ENTRY_ONBOARD_DEV_EXT:
 		dmi_save_extended_devices(dm);
+		break;
 	}
 }
 
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index a809b5095c259..3fc3d334b321d 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -24,6 +24,7 @@ enum dmi_device_type {
 	DMI_DEV_TYPE_OEM_STRING = -2,
 	DMI_DEV_TYPE_DEV_ONBOARD = -3,
 	DMI_DEV_TYPE_DEV_SLOT = -4,
+	DMI_DEV_TYPE_ADDITIONAL = -5,
 };
 
 enum dmi_entry_type {
@@ -87,6 +88,12 @@ struct dmi_device {
 	void *device_data;	/* Type specific data */
 };
 
+struct dmi_additional_info {
+	struct dmi_header header;
+	u8 count;
+	u8 entries[];
+} __packed;
+
 #ifdef CONFIG_DMI
 
 struct dmi_dev_onboard {
-- 
2.43.0

Hi Mario,

On Tue, 16 Dec 2025 06:33:50 -0600, Mario Limonciello (AMD) wrote:
> Type 40 entries (Additional information) are summarized in section
> 7.41 as part of the SMBIOS specification.  Save these entries when
> decoding the DMI tables.
> 
> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> ---
> v2:
>  * Drop some unneeded variables (LKP robot)
>  * Allow any length strings, not just 5 and longer
> ---
>  drivers/firmware/dmi_scan.c | 38 +++++++++++++++++++++++++++++++++++++
>  include/linux/dmi.h         |  7 +++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 80aded4c778bc..ec84fe3935c1e 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -393,6 +393,40 @@ static void __init dmi_save_dev_pciaddr(int instance, int segment, int bus,
>  	list_add(&dev->dev.list, &dmi_devices);
>  }
>  
> +static void __init dmi_save_additional(const struct dmi_additional_info *info)
> +{
> +	const u8 *data;
> +	int i;
> +
> +	if (!info || info->header.length < 5 + info->count * 5)

Evaluating the second condition will access info->count, which is not
guaranteed to exist if info->header.length < 5. Of course if the DMI
table is compliant, it will be fine, but you can't rely on
firmware-provided data being sane.

Second problem is that you seem to assume that every entry has size 5
bytes. The SMBIOS specification says that the minimum size is 6 bytes,
and the actual size is stored in the first byte of each entry. So you
can't validate the total length upfront. You need to check as you walk
the table that the next entry would fit in the DMI record.

(The actual length will be 6 bytes if adding a value for an 8-bit
field, which is certainly the most frequent use case. But if adding a
value for a 16-bit field for example, then the entry would be 7 byte
long.)

> +		return;
> +
> +	data = info->entries;
> +
> +	for (i = 0; i < info->count; i++) {
> +		u8 string_num = data[i * 5 + 4];
> +		const char *string_ptr;
> +		char *value;
> +		int len;
> +
> +		string_ptr = dmi_string_nosave(&info->header, string_num);
> +		if (!string_ptr || !*string_ptr)
> +			continue;
> +
> +		len = strlen(string_ptr);
> +		if (len == 0)
> +			continue;
> +
> +		value = dmi_alloc(len + 1);
> +		if (!value)
> +			continue;
> +
> +		strscpy(value, string_ptr, len + 1);

Not sure why you allocate memory for the string, considering that
dmi_save_one_device() below does it too?

> +
> +		dmi_save_one_device(DMI_DEV_TYPE_ADDITIONAL, value);
> +	}
> +}

I'm not thrilled by the interface this offers. You end up arbitrarily
saving only one piece of information from the record (the free-form
string). You omit the reference handle (which isn't super-useful per
se, but could be used to find out the base DMI type, which would be
useful), nor the field offset within that DMI type, nor the new value
associated with that field.

This might be OK with your immediate use case, but lacks flexibility if
anyone else ever needs data from such a record.

Even for your use case, this seems pretty fragile. You end up
considering that any string starting with "AGESA" in a type 40 DMI
record is what you are looking for. While you don't have any guarantee
that the record in question relates to the CPU in general nor to one
specific type 4 record field. This could result in false positives.

> +
>  static void __init dmi_save_extended_devices(const struct dmi_header *dm)
>  {
>  	const char *name;
> @@ -526,8 +560,12 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
>  	case DMI_ENTRY_IPMI_DEV:
>  		dmi_save_ipmi_device(dm);
>  		break;
> +	case DMI_ENTRY_ADDITIONAL:
> +		dmi_save_additional((const struct dmi_additional_info *)dm);
> +		break;
>  	case DMI_ENTRY_ONBOARD_DEV_EXT:
>  		dmi_save_extended_devices(dm);
> +		break;
>  	}
>  }
>  
> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
> index a809b5095c259..3fc3d334b321d 100644
> --- a/include/linux/dmi.h
> +++ b/include/linux/dmi.h
> @@ -24,6 +24,7 @@ enum dmi_device_type {
>  	DMI_DEV_TYPE_OEM_STRING = -2,
>  	DMI_DEV_TYPE_DEV_ONBOARD = -3,
>  	DMI_DEV_TYPE_DEV_SLOT = -4,
> +	DMI_DEV_TYPE_ADDITIONAL = -5,
>  };
>  
>  enum dmi_entry_type {
> @@ -87,6 +88,12 @@ struct dmi_device {
>  	void *device_data;	/* Type specific data */
>  };
>  
> +struct dmi_additional_info {
> +	struct dmi_header header;
> +	u8 count;
> +	u8 entries[];
> +} __packed;
> +
>  #ifdef CONFIG_DMI
>  
>  struct dmi_dev_onboard {


-- 
Jean Delvare
SUSE L3 Support

Hi all,

On Tue, 2025-12-16 at 06:33 -0600, Mario Limonciello (AMD) wrote:
> Type 40 entries (Additional information) are summarized in section
> 7.41 as part of the SMBIOS specification.  Save these entries when
> decoding the DMI tables.
> 
> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> ---
> v2:
>  * Drop some unneeded variables (LKP robot)
>  * Allow any length strings, not just 5 and longer
> --- (...)

Having read the whole series now, my conclusion is that we don't want
to do that. The way AMD is storing this string in a type 40 record is
absolutely not how these records are supposed to be used. So having
generic code to store and retrieve strings from type 40 records is
neither needed nor desirable.

This is a hack from AMD, the best way to deal with it is with the
minimum amount of code in the kernel and smallest memory footprint.
Simply catch the string when you see it, log it, and be done with it.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

On Tue, Dec 16, 2025 at 06:33:50AM -0600, Mario Limonciello (AMD) wrote:
> Type 40 entries (Additional information) are summarized in section
> 7.41 as part of the SMBIOS specification.  Save these entries when
> decoding the DMI tables.
> 

Why can't an interested user just use dmidecode?

Thanks,
Yazen

On 12/17/25 3:03 PM, Yazen Ghannam wrote:
> On Tue, Dec 16, 2025 at 06:33:50AM -0600, Mario Limonciello (AMD) wrote:
>> Type 40 entries (Additional information) are summarized in section
>> 7.41 as part of the SMBIOS specification.  Save these entries when
>> decoding the DMI tables.
>>
> 
> Why can't an interested user just use dmidecode?
> 
> Thanks,
> Yazen

They could.  The reason for doing it in this series is the same reason 
for the one that we did the S5 bit.

It shows up in the logs, you can tie regressions to the AGESA version at 
specifically at the time of the failure if they've done BIOS updates 
since then.

On Wed, Dec 17, 2025 at 03:09:33PM -0600, Mario Limonciello wrote:
> On 12/17/25 3:03 PM, Yazen Ghannam wrote:
> > On Tue, Dec 16, 2025 at 06:33:50AM -0600, Mario Limonciello (AMD) wrote:
> > > Type 40 entries (Additional information) are summarized in section
> > > 7.41 as part of the SMBIOS specification.  Save these entries when
> > > decoding the DMI tables.
> > > 
> > 
> > Why can't an interested user just use dmidecode?
> > 
> > Thanks,
> > Yazen
> 
> They could.  The reason for doing it in this series is the same reason for
> the one that we did the S5 bit.
> 
> It shows up in the logs, you can tie regressions to the AGESA version at
> specifically at the time of the failure if they've done BIOS updates since
> then.

Yes, right. Sorry, I mixed this up with the debugfs patch.

We need to save it here so the init code can find it.

But why do we need a debugfs entry for it?

Thanks,
Yazen

Hi Yazen, Mario,

On Wed, 17 Dec 2025 16:21:34 -0500, Yazen Ghannam wrote:
> On Wed, Dec 17, 2025 at 03:09:33PM -0600, Mario Limonciello wrote:
> > On 12/17/25 3:03 PM, Yazen Ghannam wrote:  
> > > On Tue, Dec 16, 2025 at 06:33:50AM -0600, Mario Limonciello (AMD) wrote:  
> > > > Type 40 entries (Additional information) are summarized in section
> > > > 7.41 as part of the SMBIOS specification.  Save these entries when
> > > > decoding the DMI tables.
> > > 
> > > Why can't an interested user just use dmidecode?
> > 
> > They could.  The reason for doing it in this series is the same reason for
> > the one that we did the S5 bit.
> > 
> > It shows up in the logs, you can tie regressions to the AGESA version at
> > specifically at the time of the failure if they've done BIOS updates since
> > then.  
> 
> Yes, right. Sorry, I mixed this up with the debugfs patch.
> 
> We need to save it here so the init code can find it.
> 
> But why do we need a debugfs entry for it?

FWIW, I had the exact same feeling when first gazing at this patch
series. I believe that every problem that can be solved in user-space
should be solved in user-space. Now I get the reason (explained above)
for logging the information at boot time, and thus the requirement to
parse type 40 in the kernel, but if there's no strong reason to have a
debugfs interface then I'd just drop it.

Of course, we also want to improve support for type 40 in dmidecode. I
admit I didn't pay too much attention to it so far due to a lack of use
case. But now that there's a use case, I'll be happy to work on it (or
commit a contribution if someone else beats me to it).

-- 
Jean Delvare
SUSE L3 Support

On 12/17/25 3:21 PM, Yazen Ghannam wrote:
> On Wed, Dec 17, 2025 at 03:09:33PM -0600, Mario Limonciello wrote:
>> On 12/17/25 3:03 PM, Yazen Ghannam wrote:
>>> On Tue, Dec 16, 2025 at 06:33:50AM -0600, Mario Limonciello (AMD) wrote:
>>>> Type 40 entries (Additional information) are summarized in section
>>>> 7.41 as part of the SMBIOS specification.  Save these entries when
>>>> decoding the DMI tables.
>>>>
>>>
>>> Why can't an interested user just use dmidecode?
>>>
>>> Thanks,
>>> Yazen
>>
>> They could.  The reason for doing it in this series is the same reason for
>> the one that we did the S5 bit.
>>
>> It shows up in the logs, you can tie regressions to the AGESA version at
>> specifically at the time of the failure if they've done BIOS updates since
>> then.
> 
> Yes, right. Sorry, I mixed this up with the debugfs patch.
> 
> We need to save it here so the init code can find it.
> 
> But why do we need a debugfs entry for it?

Ah.  That one I don't feel strongly about.

I used it when I was getting the series working and it felt like a waste 
to toss.

On Wed, Dec 17, 2025 at 03:23:10PM -0600, Mario Limonciello wrote:
> On 12/17/25 3:21 PM, Yazen Ghannam wrote:
> > On Wed, Dec 17, 2025 at 03:09:33PM -0600, Mario Limonciello wrote:
> > > On 12/17/25 3:03 PM, Yazen Ghannam wrote:
> > > > On Tue, Dec 16, 2025 at 06:33:50AM -0600, Mario Limonciello (AMD) wrote:
> > > > > Type 40 entries (Additional information) are summarized in section
> > > > > 7.41 as part of the SMBIOS specification.  Save these entries when
> > > > > decoding the DMI tables.
> > > > > 
> > > > 
> > > > Why can't an interested user just use dmidecode?
> > > > 
> > > > Thanks,
> > > > Yazen
> > > 
> > > They could.  The reason for doing it in this series is the same reason for
> > > the one that we did the S5 bit.
> > > 
> > > It shows up in the logs, you can tie regressions to the AGESA version at
> > > specifically at the time of the failure if they've done BIOS updates since
> > > then.
> > 
> > Yes, right. Sorry, I mixed this up with the debugfs patch.
> > 
> > We need to save it here so the init code can find it.
> > 
> > But why do we need a debugfs entry for it?
> 
> Ah.  That one I don't feel strongly about.
> 
> I used it when I was getting the series working and it felt like a waste to
> toss.

Oh okay. Yeah, maybe "don't feel strongly about it" is a good reason to
drop it. Just trying to think like a more frugal reviewer. :)

Thanks,
Yazen