Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754694AbbK0MEP (ORCPT ); Fri, 27 Nov 2015 07:04:15 -0500 Received: from mx2.suse.de ([195.135.220.15]:51210 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752478AbbK0MEJ (ORCPT ); Fri, 27 Nov 2015 07:04:09 -0500 Date: Fri, 27 Nov 2015 13:04:05 +0100 From: Jean Delvare To: Jordan Hargrave Cc: linux-kernel@vger.kernel.org, Jordan Hargrave Subject: Re: [PATCH] Save SMBIOS Type 9 System Slots during DMI Scan Message-ID: <20151127130405.1217375d@endymion.delvare> In-Reply-To: <1448575344-19107-1-git-send-email-jordan_hargrave@dell.com> References: <1448575344-19107-1-git-send-email-jordan_hargrave@dell.com> Organization: SUSE Linux X-Mailer: Claws Mail 3.10.1 (GTK+ 2.24.23; x86_64-suse-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4908 Lines: 142 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 > --- > 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 -- 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/