lkml.org 
[lkml]   [2011]   [Mar]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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
>


\
 
 \ /
  Last update: 2011-03-17 20:57    [W:0.091 / U:0.700 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site