[PATCH 1/2] Save SMBIOS Type 9 System Slots during DMI Scan

[PATCH 1/2] Save SMBIOS Type 9 System Slots during DMI Scan

[Jordan Hargrave]

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

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

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index ac1ce4a..43cb7db 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -356,6 +356,41 @@ static void __init dmi_save_extended_devices(const struct dmi_header *dm)
 	dmi_save_one_device(*d & 0x7f, dmi_string_nosave(dm, *(d - 1)));
 }
 
+static void __init dmi_save_dev_slot(int instance, int segment, int bus, int devfn, const char *name)
+{
+	struct dmi_dev_onboard *slot;
+
+	slot = dmi_alloc(sizeof(*slot) + strlen(name) + 1);
+	if (!slot) {
+		printk(KERN_ERR "dmi_save_system_slot: out of memory.\n");
+		return;
+	}
+	slot->instance = instance;
+	slot->segment = segment;
+	slot->bus = bus;
+	slot->devfn = devfn;
+
+	strcpy((char *)&slot[1], name);
+	slot->dev.type = DMI_DEV_TYPE_SYSTEM_SLOT;
+	slot->dev.name = (char *)&slot[1];
+	slot->dev.device_data = slot;
+
+	list_add(&slot->dev.list, &dmi_devices);
+}
+
+
+static void __init dmi_save_system_slot(const struct dmi_header *dm)
+{
+	const char *name;
+	const u8 *d = (u8*)dm;
+
+	if (dm->type == DMI_ENTRY_SYSTEM_SLOT && dm->length >= 0x11) {
+		name = dmi_string_nosave(dm, *(d + 0x04));
+		dmi_save_dev_slot(*(u16 *)(d + 0x09), *(u16 *)(d + 0xD),
+				  *(d + 0xF), *(d + 0x10), name);
+	}
+}
+
 static void __init count_mem_devices(const struct dmi_header *dm, void *v)
 {
 	if (dm->type != DMI_ENTRY_MEM_DEVICE)
@@ -437,6 +472,10 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
 		break;
 	case 41:	/* Onboard Devices Extended Information */
 		dmi_save_extended_devices(dm);
+		break;
+	case 9:         /* System Slots */
+		dmi_save_system_slot(dm);
+		break;
 	}
 }
 
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index 5055ac3..09f42e7 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_SYSTEM_SLOT = -4,
 };
 
 enum dmi_entry_type {
-- 
2.5.0

[PATCH 1/2] Save SMBIOS Type 9 System Slots during DMI Scan

[Jordan_Hargrave]

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

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

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index ac1ce4a..43cb7db 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -356,6 +356,41 @@ static void __init dmi_save_extended_devices(const struct dmi_header *dm)
        dmi_save_one_device(*d & 0x7f, dmi_string_nosave(dm, *(d - 1)));
 }

+static void __init dmi_save_dev_slot(int instance, int segment, int bus, int devfn, const char *name)
+{
+       struct dmi_dev_onboard *slot;
+
+       slot = dmi_alloc(sizeof(*slot) + strlen(name) + 1);
+       if (!slot) {
+               printk(KERN_ERR "dmi_save_system_slot: out of memory.\n");
+               return;
+       }
+       slot->instance = instance;
+       slot->segment = segment;
+       slot->bus = bus;
+       slot->devfn = devfn;
+
+       strcpy((char *)&slot[1], name);
+       slot->dev.type = DMI_DEV_TYPE_SYSTEM_SLOT;
+       slot->dev.name = (char *)&slot[1];
+       slot->dev.device_data = slot;
+
+       list_add(&slot->dev.list, &dmi_devices);
+}
+
+
+static void __init dmi_save_system_slot(const struct dmi_header *dm)
+{
+       const char *name;
+       const u8 *d = (u8*)dm;
+
+       if (dm->type == DMI_ENTRY_SYSTEM_SLOT && dm->length >= 0x11) {
+               name = dmi_string_nosave(dm, *(d + 0x04));
+               dmi_save_dev_slot(*(u16 *)(d + 0x09), *(u16 *)(d + 0xD),
+                                 *(d + 0xF), *(d + 0x10), name);
+       }
+}
+
 static void __init count_mem_devices(const struct dmi_header *dm, void *v)
 {
        if (dm->type != DMI_ENTRY_MEM_DEVICE)
@@ -437,6 +472,10 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
                break;
        case 41:        /* Onboard Devices Extended Information */
                dmi_save_extended_devices(dm);
+               break;
+       case 9:         /* System Slots */
+               dmi_save_system_slot(dm);
+               break;
        }
 }

diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index 5055ac3..09f42e7 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_SYSTEM_SLOT = -4,
 };

 enum dmi_entry_type {
--
2.5.0

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

[Jean Delvare]

Hi Jordan,

On Wed, 18 Nov 2015 16:02:14 -0600, Jordan Hargrave wrote:
> PCI address of onboard devices is currently saved but not for slots.
> 
> Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com>
> ---
>  drivers/firmware/dmi_scan.c | 39 +++++++++++++++++++++++++++++++++++++++
>  include/linux/dmi.h         |  1 +
>  2 files changed, 40 insertions(+)

First of all: scripts/checkpatch.pl found 1 error and 2 warnings in
your patch. You should really check this before you send the patch out
for review.

> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index ac1ce4a..43cb7db 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -356,6 +356,41 @@ static void __init dmi_save_extended_devices(const struct dmi_header *dm)
>  	dmi_save_one_device(*d & 0x7f, dmi_string_nosave(dm, *(d - 1)));
>  }
>  
> +static void __init dmi_save_dev_slot(int instance, int segment, int bus, int devfn, const char *name)
> +{
> +	struct dmi_dev_onboard *slot;
> +
> +	slot = dmi_alloc(sizeof(*slot) + strlen(name) + 1);
> +	if (!slot) {
> +		printk(KERN_ERR "dmi_save_system_slot: out of memory.\n");

Memory allocation errors are already logged at a lower level so you
don't have to do it again. Plus you did not even get the function name
right ;-)

> +		return;
> +	}
> +	slot->instance = instance;
> +	slot->segment = segment;
> +	slot->bus = bus;
> +	slot->devfn = devfn;
> +
> +	strcpy((char *)&slot[1], name);
> +	slot->dev.type = DMI_DEV_TYPE_SYSTEM_SLOT;
> +	slot->dev.name = (char *)&slot[1];
> +	slot->dev.device_data = slot;
> +
> +	list_add(&slot->dev.list, &dmi_devices);
> +}

This function is very similar to dmi_save_dev_onboard so it would make
sense to reuse that function and add an extra parameter to pass the
type (DMI_DEV_TYPE_DEV_ONBOARD or DMI_DEV_TYPE_SYSTEM_SLOT.) This
avoids some code duplication.

> +
> +

Extra blank line, please remove it.

> +static void __init dmi_save_system_slot(const struct dmi_header *dm)
> +{
> +	const char *name;
> +	const u8 *d = (u8*)dm;
> +
> +	if (dm->type == DMI_ENTRY_SYSTEM_SLOT && dm->length >= 0x11) {

The first half of the test will always succeed so it can be omitted.
OTOH you do not check the value of d + 0x07 (current usage.) As I
understand it, you should only consider slots which are in use, so
where *(d + 0x07) == 0x04?

> +		name = dmi_string_nosave(dm, *(d + 0x04));
> +		dmi_save_dev_slot(*(u16 *)(d + 0x09), *(u16 *)(d + 0xD),
> +				  *(d + 0xF), *(d + 0x10), name);
> +	}
> +}
> +
>  static void __init count_mem_devices(const struct dmi_header *dm, void *v)
>  {
>  	if (dm->type != DMI_ENTRY_MEM_DEVICE)
> @@ -437,6 +472,10 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
>  		break;
>  	case 41:	/* Onboard Devices Extended Information */
>  		dmi_save_extended_devices(dm);
> +		break;
> +	case 9:         /* System Slots */
> +		dmi_save_system_slot(dm);
> +		break;

Other entries are in numeric order, would be nice to stick to that.

>  	}
>  }
>  
> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
> index 5055ac3..09f42e7 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_SYSTEM_SLOT = -4,

You are really storing device information, not slot information, so I
believe DMI_DEV_TYPE_DEV_SLOT would be a more appropriate name.

>  };
>  
>  enum dmi_entry_type {


-- 
Jean Delvare
SUSE L3 Support

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

[Jean Delvare]

On Thu, 26 Nov 2015 02:02:52 -0600, Jordan Hargrave wrote:
> On Wed, Nov 25, 2015 at 5:47 AM, Jean Delvare <jdelvare@suse.de> wrote:
> > On Wed, 18 Nov 2015 16:02:14 -0600, Jordan Hargrave wrote:
> > > +static void __init dmi_save_system_slot(const struct dmi_header *dm)
> > > +{
> > > +     const char *name;
> > > +     const u8 *d = (u8*)dm;
> > > +
> > > +     if (dm->type == DMI_ENTRY_SYSTEM_SLOT && dm->length >= 0x11) {
> >
> > The first half of the test will always succeed so it can be omitted.
> > OTOH you do not check the value of d + 0x07 (current usage.) As I
> > understand it, you should only consider slots which are in use, so
> > where *(d + 0x07) == 0x04?
>
> Unfortunately BIOS often get 'usage' wrong.  And I'd like to keep all slots
> recorded if user hotplugs PCIe card.

OK, then please add a comment saying so.

I'm curious how the BIOS can assign a device number and function when
there is no card in the slot. I'm also curious how multi-function
cards are handled. I suppose that only the first function has a DMI
record?

-- 
Jean Delvare
SUSE L3 Support

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

[Jean Delvare]

Hi Jordan,

Please leave the list in Cc and avoid HTML formatting.

Le Thursday 26 November 2015 à 16:00 -0600, Jordan Hargrave a écrit :
> 
> 
> On Thu, Nov 26, 2015 at 2:26 AM, Jean Delvare <jdelvare@suse.de>
> wrote:
>         I'm curious how the BIOS can assign a device number and
>         function when there is no card in the slot. I'm also curious
>         how multi-function cards are handled. I suppose that only the
>         first function has a DMI record?


> BIOS can leave holes in PCI bridge numbers during PCI enumeration for
> hotplug slots.  
> 
> 
> From what I've seen of dmidecode/lspci dumps, there's basically 4
> cases that can occur.
> 
> a) Inuse, Bus Address correct
> 
> b) Inuse, Bus Address invalid
> 
> c) Not Inuse, Bus Address correct
> 
> d) Not Inuse, Bus Address invalid

Just checked how it looks like on my Lenovo laptop. Two slots are
listed, both with "Current Usage: Available" (which BTW is probably
wrong for one of them which holds the wireless card as far as I know)
and both have Bus Address: 0000:00:00.0. This is case d) above. However
both also say "Hot-plug devices are supported". I suppose that in theory
you should check this flag and treat unused slots with hot-plug
supported as "In use".

> 
> If the BIOS is functioning correctly, you should only see cases a or
> d.  That assumes BIOS gets it right of course (which non-Dell systems
> usually don't).

Well if I read you correctly, c) would be valid too for hot-plug
scenarios? In the DMI table dumps I checked, lots have "Not Inuse, Bus
Address correct" entries, without hot-plug support declared. I don't
think it is much of a problem anyway, worse case we record an entry
which will never be used?

> The Bus Address invalid follow three different cases:
> 00:00.0
> 
> ff:1f.7
> 
> Some random value :/  (where there isn't a PCI device at that
> address).  This is the worst case. It could be BIOS has reserved a bus
> address for an empty slot, or it could just be bad BIOS data.
> 
> 
> I can put the check in for ff:1f.7 or 00:00.0 at least.

I don't think you can reject ff:1f.7 as it is potentially valid (as a
matter of fact I have on Dell PowerEdge 300 DMI dump with a "RAC" PCIe
device declared at bus address 0000:ff:1f.7, I can share it with you if
needed.) OTOH I don't know if bus segment ffff is valid.

00:00.0 is also valid but at least you know it will never be assigned to
a slot (at least I think it is always the host bridge, which is on-board
by definition? but I could be wrong...)

Remember, what really matters is that the code works with systems which
stick to the specification. If the behavior is suboptimal on systems
which do not follow the specification, they deserve it.

Also note that the SMBIOS specification says that the segment/bus/devfn
fields should be ffs for non-PCI-like slots. dmidecode decodes these
fields regardless of the type, but that is incorrect (although not much
of an issue in practice I suppose as PCI rules the world.) I suppose you
don't want to check this in the kernel anyway because 1* the list of
slot types gets new values every now and then and that would be a pain
to maintain and 2* I suppose it doesn't matter if you add other devices
to the list as nobody will ever look them up.

>    That will eliminate most of cases b, d.  Hotplugged cards won't get
> the right slot number though.  Not a problem on Dell servers as we
> don't support PCIe hotplug.
> 
> 
> 
> Multi-function cards will only have an entry for function 0.  The
> other patch that uses this code will lookup func 0 if there isn't an
> exact match and it's not a PCIE root port.

OK, makes sense (although it is regrettable that the SMBIOS spec did not
provide a way to handle this case gracefully.)


-- 
Jean Delvare
SUSE L3 Support

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

[Jean Delvare]

Le Friday 27 November 2015 à 10:25 +0100, Jean Delvare a écrit :
> Also note that the SMBIOS specification says that the segment/bus/devfn
> fields should be ffs for non-PCI-like slots. dmidecode decodes these
> fields regardless of the type, but that is incorrect (although not much
> of an issue in practice I suppose as PCI rules the world.)

Correction... dmidecode checks that fields aren't all ffs, and if not,
decodes the segment/bus/devfn. Apparently I decided it was easier to do
it that way than to check the slot type. Even if it does not follow the
specification stricto sensu.

-- 
Jean Delvare
SUSE L3 Support