Re: [PATCH] Save SMBIOS Type 9 System Slots during DMI Scan

Re: [PATCH] Save SMBIOS Type 9 System Slots during DMI Scan

[Jordan Hargrave]

PCI address of onboard devices is currently saved but not for slots.

Created common onboard/slot save function. Verify that bus address is
not invalid.

Signed-off-by: Jordan Hargrave <jordan_hargrave@dell.com>
---
 drivers/firmware/dmi_scan.c | 53 +++++++++++++++++++++++++++++++--------------
 include/linux/dmi.h         |  1 +
 2 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index ac1ce4a..c6b2828 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -321,26 +321,30 @@ static void __init dmi_save_ipmi_device(const struct dmi_header *dm)
 	list_add_tail(&dev->list, &dmi_devices);
 }
 
-static void __init dmi_save_dev_onboard(int instance, int segment, int bus,
-					int devfn, const char *name)
+static void __init dmi_save_dev_pciaddr(int instance, int segment, int bus,
+					int devfn, const char *name, int type)
 {
-	struct dmi_dev_onboard *onboard_dev;
+	struct dmi_dev_onboard *dev;
 
-	onboard_dev = dmi_alloc(sizeof(*onboard_dev) + strlen(name) + 1);
-	if (!onboard_dev)
+	/* Ignore invalid values */
+	if ((bus == 0 && devfn == 0) || (bus == 0xFF && devfn == 0xFF))
 		return;
 
-	onboard_dev->instance = instance;
-	onboard_dev->segment = segment;
-	onboard_dev->bus = bus;
-	onboard_dev->devfn = devfn;
+	dev = dmi_alloc(sizeof(*dev) + strlen(name) + 1);
+	if (!dev)
+		return;
 
-	strcpy((char *)&onboard_dev[1], name);
-	onboard_dev->dev.type = DMI_DEV_TYPE_DEV_ONBOARD;
-	onboard_dev->dev.name = (char *)&onboard_dev[1];
-	onboard_dev->dev.device_data = onboard_dev;
+	dev->instance = instance;
+	dev->segment = segment;
+	dev->bus = bus;
+	dev->devfn = devfn;
 
-	list_add(&onboard_dev->dev.list, &dmi_devices);
+	strcpy((char *)&dev[1], name);
+	dev->dev.type = type;
+	dev->dev.name = (char *)&dev[1];
+	dev->dev.device_data = dev;
+
+	list_add(&dev->dev.list, &dmi_devices);
 }
 
 static void __init dmi_save_extended_devices(const struct dmi_header *dm)
@@ -351,11 +355,25 @@ static void __init dmi_save_extended_devices(const struct dmi_header *dm)
 	if ((*d & 0x80) == 0)
 		return;
 
-	dmi_save_dev_onboard(*(d+1), *(u16 *)(d+2), *(d+4), *(d+5),
-			     dmi_string_nosave(dm, *(d-1)));
+	dmi_save_dev_pciaddr(*(d+1), *(u16 *)(d+2), *(d+4), *(d+5),
+			     dmi_string_nosave(dm, *(d-1)),
+			     DMI_DEV_TYPE_DEV_ONBOARD);
 	dmi_save_one_device(*d & 0x7f, dmi_string_nosave(dm, *(d - 1)));
 }
 
+static void __init dmi_save_system_slot(const struct dmi_header *dm)
+{
+	const u8 *d = (u8 *)dm;
+
+	/* Need SMBIOS 2.6+ structure */
+	if (dm->length < 0x11)
+		return;
+	dmi_save_dev_pciaddr(*(u16 *)(d + 0x9), *(u16 *)(d + 0xD),
+			     *(d + 0xF), *(d + 0x10),
+			     dmi_string_nosave(dm, *(d + 0x4)),
+			     DMI_DEV_TYPE_DEV_SLOT);
+}
+
 static void __init count_mem_devices(const struct dmi_header *dm, void *v)
 {
 	if (dm->type != DMI_ENTRY_MEM_DEVICE)
@@ -426,6 +444,9 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
 		dmi_save_ident(dm, DMI_CHASSIS_SERIAL, 7);
 		dmi_save_ident(dm, DMI_CHASSIS_ASSET_TAG, 8);
 		break;
+	case 9:		/* System Slots */
+	       dmi_save_system_slot(dm);
+	       break;
 	case 10:	/* Onboard Devices Information */
 		dmi_save_devices(dm);
 		break;
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index 5055ac3..5e9c74c 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -22,6 +22,7 @@ enum dmi_device_type {
 	DMI_DEV_TYPE_IPMI = -1,
 	DMI_DEV_TYPE_OEM_STRING = -2,
 	DMI_DEV_TYPE_DEV_ONBOARD = -3,
+	DMI_DEV_TYPE_DEV_SLOT = -4,
 };
 
 enum dmi_entry_type {
-- 
1.9.1

Re: [PATCH] Save SMBIOS Type 9 System Slots during DMI Scan

[Jean Delvare]

Hi Jordan,

When sending a new version of a patch, please start a new thread, and
tag it with [PATCH v2] instead of [PATCH]. A changelog before the
diffstat is also appreciated.

On Thu, 26 Nov 2015 16:02:24 -0600, Jordan Hargrave wrote:
> PCI address of onboard devices is currently saved but not for slots.
> 
> Created common onboard/slot save function. Verify that bus address is
> not invalid.
> 
> Signed-off-by: Jordan Hargrave <jordan_hargrave@dell.com>
> ---
>  drivers/firmware/dmi_scan.c | 53 +++++++++++++++++++++++++++++++--------------
>  include/linux/dmi.h         |  1 +
>  2 files changed, 38 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index ac1ce4a..c6b2828 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -321,26 +321,30 @@ static void __init dmi_save_ipmi_device(const struct dmi_header *dm)
>  	list_add_tail(&dev->list, &dmi_devices);
>  }
>  
> -static void __init dmi_save_dev_onboard(int instance, int segment, int bus,
> -					int devfn, const char *name)
> +static void __init dmi_save_dev_pciaddr(int instance, int segment, int bus,
> +					int devfn, const char *name, int type)
>  {
> -	struct dmi_dev_onboard *onboard_dev;
> +	struct dmi_dev_onboard *dev;
>  
> -	onboard_dev = dmi_alloc(sizeof(*onboard_dev) + strlen(name) + 1);
> -	if (!onboard_dev)
> +	/* Ignore invalid values */
> +	if ((bus == 0 && devfn == 0) || (bus == 0xFF && devfn == 0xFF))
>  		return;

As discussed before, this is questionable, as you may filter out valid
entries (especially as you do not check the segment value.) It's
probably better to record invalid entries than to take the risk of
filtering out valid ones?

Also I said before that 00:00.0 could be considered invalid for slots
because it was always an on-board device, but now
dmi_save_dev_pciaddr() will also handle on-board devices. I'm not sure
why someone would want to record the root host bridge in the DMI table,
but the specification allows it.

>  
> -	onboard_dev->instance = instance;
> -	onboard_dev->segment = segment;
> -	onboard_dev->bus = bus;
> -	onboard_dev->devfn = devfn;
> +	dev = dmi_alloc(sizeof(*dev) + strlen(name) + 1);
> +	if (!dev)
> +		return;
>  
> -	strcpy((char *)&onboard_dev[1], name);
> -	onboard_dev->dev.type = DMI_DEV_TYPE_DEV_ONBOARD;
> -	onboard_dev->dev.name = (char *)&onboard_dev[1];
> -	onboard_dev->dev.device_data = onboard_dev;
> +	dev->instance = instance;
> +	dev->segment = segment;
> +	dev->bus = bus;
> +	dev->devfn = devfn;
>  
> -	list_add(&onboard_dev->dev.list, &dmi_devices);
> +	strcpy((char *)&dev[1], name);
> +	dev->dev.type = type;
> +	dev->dev.name = (char *)&dev[1];
> +	dev->dev.device_data = dev;
> +
> +	list_add(&dev->dev.list, &dmi_devices);
>  }
>  
>  static void __init dmi_save_extended_devices(const struct dmi_header *dm)
> @@ -351,11 +355,25 @@ static void __init dmi_save_extended_devices(const struct dmi_header *dm)
>  	if ((*d & 0x80) == 0)
>  		return;
>  
> -	dmi_save_dev_onboard(*(d+1), *(u16 *)(d+2), *(d+4), *(d+5),
> -			     dmi_string_nosave(dm, *(d-1)));
> +	dmi_save_dev_pciaddr(*(d+1), *(u16 *)(d+2), *(d+4), *(d+5),
> +			     dmi_string_nosave(dm, *(d-1)),
> +			     DMI_DEV_TYPE_DEV_ONBOARD);
>  	dmi_save_one_device(*d & 0x7f, dmi_string_nosave(dm, *(d - 1)));
>  }
>  
> +static void __init dmi_save_system_slot(const struct dmi_header *dm)
> +{
> +	const u8 *d = (u8 *)dm;
> +
> +	/* Need SMBIOS 2.6+ structure */
> +	if (dm->length < 0x11)
> +		return;
> +	dmi_save_dev_pciaddr(*(u16 *)(d + 0x9), *(u16 *)(d + 0xD),
> +			     *(d + 0xF), *(d + 0x10),
> +			     dmi_string_nosave(dm, *(d + 0x4)),
> +			     DMI_DEV_TYPE_DEV_SLOT);
> +}
> +
>  static void __init count_mem_devices(const struct dmi_header *dm, void *v)
>  {
>  	if (dm->type != DMI_ENTRY_MEM_DEVICE)
> @@ -426,6 +444,9 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
>  		dmi_save_ident(dm, DMI_CHASSIS_SERIAL, 7);
>  		dmi_save_ident(dm, DMI_CHASSIS_ASSET_TAG, 8);
>  		break;
> +	case 9:		/* System Slots */
> +	       dmi_save_system_slot(dm);
> +	       break;

Indentation is broken, you used spaces instead of tabs.

>  	case 10:	/* Onboard Devices Information */
>  		dmi_save_devices(dm);
>  		break;
> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
> index 5055ac3..5e9c74c 100644
> --- a/include/linux/dmi.h
> +++ b/include/linux/dmi.h
> @@ -22,6 +22,7 @@ enum dmi_device_type {
>  	DMI_DEV_TYPE_IPMI = -1,
>  	DMI_DEV_TYPE_OEM_STRING = -2,
>  	DMI_DEV_TYPE_DEV_ONBOARD = -3,
> +	DMI_DEV_TYPE_DEV_SLOT = -4,
>  };
>  
>  enum dmi_entry_type {


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] Save SMBIOS Type 9 System Slots during DMI Scan

[Jean Delvare]

Hi Jordan,

Once again: please keep the list included in your replies. Others may
be able to help. Also the list is archived for later reference.

On Fri, 27 Nov 2015 20:45:21 -0600, Jordan Hargrave wrote:
> On Fri, Nov 27, 2015 at 6:04 AM, Jean Delvare <jdelvare@suse.de> wrote:
> > When sending a new version of a patch, please start a new thread, and
> > tag it with [PATCH v2] instead of [PATCH]. A changelog before the
> > diffstat is also appreciated.
> 
> Hmm, not sure how to do that, it always generates N diffs for me whenever I
> do a new commit.

I can't parse this, sorry.

> > As discussed before, this is questionable, as you may filter out valid
> > entries (especially as you do not check the segment value.) It's
> > probably better to record invalid entries than to take the risk of
> > filtering out valid ones?
> 
> Segment is even more broken. it's either 0 or ffff, and bus maybe 0 or ff
> on top of that.
> And are there even any machines that use segments anyway? The last one I
> know of was 10+ years ago, and didn't support SMBIOS 2.6

We have a few machines in the SUSE Labs network which use PCI segments.
SGI UV3000 uses segments 0000, 0001, 1000, 1001, 1002 and 1003, and it
implements SMBIOS 2.7. SGI UV100 uses segments 0000, 0001, 1000 and
1001. Fujitsu PrimeQuest 2800E uses segments 0000 and 0001, and it
implements SMBIOS 2.7.

Also SGI Altix 450 (ia64) uses PCI segments 0001, 0002, 0011, 0012 and
0021. It doesn't seem to have a valid DMI table though.

So yes, PCI bus segments are used, although not often.

> > Also I said before that 00:00.0 could be considered invalid for slots
> > because it was always an on-board device, but now
> > dmi_save_dev_pciaddr() will also handle on-board devices. I'm not sure
> > why someone would want to record the root host bridge in the DMI table,
> > but the specification allows it.
>
> Never seen 00:00.0 as a valid value.  Yeah according to spec it's not
> invalid.... but real world BIOS is different.

It is the PCI root bridge on almost every x86 system I've seen, and
a few ia64 and arm64 systems too:

00:00.0  Host bridge [0600]: Intel Corporation 5520/5500/X58 I/O Hub to ESI Port [8086:3407] (rev 22)
00:00.0  Host bridge [0600]: Intel Corporation 5400 Chipset Memory Controller Hub [8086:4003] (rev 20)
00:00.0  Host bridge: Intel Corporation 5500 I/O Hub to ESI Port (rev 13)
00:00.0  Host bridge [0600]: Intel Corporation Xeon E7 v2/Xeon E5 v2/Core i7 DMI2 [8086:0e00] (rev 07)
00:00.0  Host bridge: ATI Technologies Inc RS690 Host Bridge
00:00.0  Host bridge [0600]: Advanced Micro Devices [AMD] nee ATI RD890 PCI to PCI bridge (external gfx0 port A) [1002:5a13] (rev 02)
00:00.0  Host bridge [0600]: ATI Technologies Inc RD890 Northbridge only dual slot (2x16) PCI-e GFX Hydra part [1002:5a10] (rev 02)
00:00.0  Host bridge: ATI Technologies Inc RD890 Northbridge only dual slot (2x16) PCI-e GFX Hydra part (rev 02)
00:00.0  Host bridge [0600]: Advanced Micro Devices, Inc. [AMD] Device [1022:1a00]

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] Save SMBIOS Type 9 System Slots during DMI Scan

[Jean Delvare]

Hi Jordan,

Le Saturday 28 November 2015 à 20:54 -0600, Jordan Hargrave a écrit :
> 
> 
> On Sat, Nov 28, 2015 at 2:46 AM, Jean Delvare <jdelvare@suse.de>
> wrote:
>         Hi Jordan,
>         
>         Once again: please keep the list included in your replies.
>         Others may
>         be able to help. Also the list is archived for later
>         reference.
>         
> 
> 
> The list wasn't in the cc:?  I sent the diff with git send-email --cc

When you send the patches, the list is there. However when I review and
then you reply using your regular email client, this is HTML and list is
dropped. Not good.

>(...)
>         We have a few machines in the SUSE Labs network which use PCI
>         segments.
>         SGI UV3000 uses segments 0000, 0001, 1000, 1001, 1002 and
>         1003, and it
>         implements SMBIOS 2.7. SGI UV100 uses segments 0000, 0001,
>         1000 and
>         1001. Fujitsu PrimeQuest 2800E uses segments 0000 and 0001,
>         and it
>         implements SMBIOS 2.7.
>         
>         Also SGI Altix 450 (ia64) uses PCI segments 0001, 0002, 0011,
>         0012 and
>         0021. It doesn't seem to have a valid DMI table though.
>         
>         So yes, PCI bus segments are used, although not often.
> 
> 
> Can you send me a full dmidecode -u and lspci -vvvxxxx on those
> systems? 

Sure, will send privately.


-- 
Jean Delvare
SUSE L3 Support