Messages in this thread | | | Date | Thu, 17 Mar 2011 15:55:04 -0400 | From | Prarit Bhargava <> | Subject | Re: [PATCH]: SMBIOS: Add initial code and export version via sysfs |
| |
On 03/17/2011 03:30 PM, Greg KH wrote: > On Thu, Mar 17, 2011 at 09:57:54AM -0400, Prarit Bhargava wrote: > >> The existing DMI code, drivers/firmware/dmi.c, is not really parsing DMI. It >> is actually SMBIOS code that is using the old DMI infrastructure to expose >> SMBIOS entries. >> >> We should be doing the following: >> >> 1. Looking for SMBIOS (either EFI or mapped to it's standard location, >> 0xF0000) >> 2. If found, look for the SMBIOS' Intermediate Structure (aka "DMI" entry) >> 3. If not found, look for an "old" DMI structure. >> >> DMI is a predecessor of SMBIOS, so we should start treating it as such. >> > Why? What is this change going to buy us in the end? Heck, right now, > what's the benefit? >
It's wrong. DMI doesn't give us SMBIOS information -- it's the other way around. There's also a bunch of other stuff from SMBIOS being used in the kernel (see pci slots and hotplug drivers).
It would be nice to have a single place for this, no?
> >> For this patch I have introduced drivers/firmware/smbios.c, exported the >> SMBIOS version through sysfs, and modified the DMI code such that >> >> a) dmi_scan_machine() is now called from init_smbios(), and >> b) if an SMBIOS is found, the values in the SMBIOS STEP structure are used >> in dmi_scan_machine(). >> >> Right now, removing the DMI code is not an option. It is used by common >> initscripts, etc., to bringup the system. Later modifications in this area will >> include additional parsing of the SMBIOS structs and a simultaneous cleanup >> of the DMI code. >> > Care to show follow-on patches that convert things to a manner which you > think it should look like? > >
I haven't written anything yet :) -- I was planning on exporting the SMBIOS structs (all types) though.
>> --- /dev/null >> +++ b/drivers/firmware/smbios.c >> @@ -0,0 +1,175 @@ >> +#include <linux/dmi.h> >> +#include <linux/efi.h> >> +#include <linux/init.h> >> +#include <linux/io.h> >> +#include <linux/kernel.h> >> +#include <linux/smbios.h> >> + >> +#include <asm/setup.h> >> + >> +int smbios_present = 0; >> +struct smbios_step *smbios_step; >> +static struct device *smbios_dev; >> > A "raw" struct device? Why? You should never do this, that's not the > way to use a 'struct device'. > >
Ah, okay -- my familiarity with sysfs is very minimal. I'll ask on kernel-mentors for a pointer to "good" code.
> Let's back up here, what exactly are you trying to show in sysfs? Where > are your Documentation/ABI/ entries that show these new files, what they > contain, and how to use them? >
Obviously missing -- I will add this on my next patch.
> >> +/* The actual address of the SMBIOS */ >> +static unsigned long smbios_addr = 0; >> + >> +#ifdef CONFIG_SYSFS >> > You should never need this in your .c file. > >
Will remove.
>> +static ssize_t version_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + return snprintf(buf, PAGE_SIZE, "%u.%u\n", smbios_step->major, >> + smbios_step->minor); >> > As you "know" you are only returning a simple value, why the snprintf()? > Hint, don't, it's overkill and how do you "know" that the buffer is > PAGE_SIZE big? Hint, you don't, so stick to a small buffer like you > are, and all is fine. >
:) Will do. Thanks :)
> >> +} >> + >> +/* add additional sysfs files here */ >> +static struct device_attribute smbios_attrs[] = { >> + __ATTR_RO(version), >> +}; >> > So a whole new class just for a single version file in sysfs? > > I'm confused, what exactly are you trying to show here that you can't > show in the existing device/class structure? > > >> + >> +static struct class smbios_class = { >> + .name = "smbios", >> + .dev_release = (void(*)(struct device *)) kfree, >> > As stated before, nice try, but no. :) > >
Right -- I'll go off and try to figure out the proper way to do this. Sorry ... I was following along with what the existing DMI code does.
> >> +}; >> + >> +static struct device *smbios_dev; >> + >> +static int __init smbios_setup_sysfs(void) >> +{ >> + int attr, i, ret = 0; >> + >> + ret = class_register(&smbios_class); >> + if (ret) >> + goto out; >> + >> + smbios_dev = kzalloc(sizeof(*smbios_dev), GFP_KERNEL); >> + if (!smbios_dev) { >> + ret = -ENOMEM; >> + goto out_cleanup_class; >> + } >> + >> + smbios_dev->class = &smbios_class; >> + dev_set_name(smbios_dev, "id"); >> + >> + ret = device_register(smbios_dev); >> + if (ret) >> + goto out_cleanup_smbios_dev; >> + >> + for (attr = 0; attr < ARRAY_SIZE(smbios_attrs); attr++) { >> + ret = device_create_file(smbios_dev, &smbios_attrs[attr]); >> > You just raced userspace when you created this file, causing it massive > problems if it was expecting when the hotplug event happened that this > file would be present. Please don't do that, we have the infrastructure > to do this properly, please use it. > >
np -- thanks for the info Greg.
P.
> thanks, > > greg k-h >
| |