Re: [PATCH]: SMBIOS: Add initial code and export version via sysfs

[Prarit Bhargava <prarit@redhat.com>]

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
>