Date: Wed, 25 Nov 2015 12:47:43 +0100
From: Jean Delvare <jdelvare@suse.de>
To: Jordan Hargrave <Jordan_Hargrave@dell.com>
Cc: linux-kernel@vger.kernel.org, jharg93@gmail.com
Subject: Re: [PATCH 1/2] Save SMBIOS Type 9 System Slots during DMI Scan
Message-ID: <20151125124743.6919eb43@endymion.delvare>
In-Reply-To: <1447884134-31504-1-git-send-email-Jordan_Hargrave@dell.com>
References: <1447884134-31504-1-git-send-email-Jordan_Hargrave@dell.com>
Organization: SUSE Linux
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3793
Lines: 121

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/