Re: [dmidecode] [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute

["Ivan.khoronzhuk" <ivan.khoronzhuk@globallogic.com>]

On 10.03.15 11:13, Jean Delvare wrote:
> Hi Ivan,
>
> Sorry for the late reply. I think I addressed some points in other
> replies already, but for completeness let me reply to this post too.
>
> Le Wednesday 04 March 2015 à 14:30 +0200, Ivan.khoronzhuk a écrit :
>> On 02/26/2015 11:36 AM, Jean Delvare wrote:
>>> On Wed,  4 Feb 2015 19:06:03 +0200, Ivan Khoronzhuk wrote:
>>>> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi b/Documentation/ABI/testing/sysfs-firmware-dmi
>>>> index c78f9ab..3a9ffe8 100644
>>>> --- a/Documentation/ABI/testing/sysfs-firmware-dmi
>>>> +++ b/Documentation/ABI/testing/sysfs-firmware-dmi
>>>> @@ -12,6 +12,16 @@ Description:
>>>>    		cannot ensure that the data as exported to userland is
>>>>    		without error either.
>>>>    
>>>> +		The firmware provides DMI structures as a packed list of
>>>> +		data referenced by a SMBIOS table entry point. The SMBIOS
>>>> +		entry point contains general information, like SMBIOS
>>>> +		version, DMI table size, etc. The structure, content and
>>>> +		size of SMBIOS entry point is dependent on SMBIOS version.
>>>> +		That's why SMBIOS entry point is represented in dmi sysfs
>>>> +		like a raw attribute and is accessible via
>>>> +		/sys/firmware/dmi/smbios_raw_header. The format of SMBIOS
>>> As mentioned before, I don't like the name "smbios_raw_header". I think
>>> it should be "smbios_entry_point" or similar.
>> If Matt is OK to get another version,
>> Let it be smbios_entry_point.
>> If it's more convenient, it should be changed while it's possible.
> Great, thanks.
>
>>>> @@ -669,6 +699,18 @@ static int __init dmi_sysfs_init(void)
>>>>    		goto err;
>>>>    	}
>>>>    
>>>> +	smbios_raw_header = dmi_get_smbios_entry_area(&size);
>>>> +	if (!smbios_raw_header) {
>>>> +		pr_debug("dmi-sysfs: SMBIOS raw data is not available.\n");
>>>> +		error = -EINVAL;
>>>> +		goto err;
>>>> +	}
>>> I don't think this should have been a fatal error. Just because for
>>> some reason dmi_get_smbios_entry_area() returned NULL is no good reason
>>> for nor exposing /sys/firmware/dmi/entries as we used to.
>> It issues an error only in case of when entry table is not available,
>> if entry point is absent then dmi table is not available a fortiori.
>> So there is no reason to continue from that point.
> Well it could also fail because of an error in the code ;-) But OK, I
> agree with you here.
>
>>> But anyway this is no longer relevant if the code is moved to dmi_scan
>>> as I suggested.
>>>
>>>> +
>>>> +	/* Create the raw binary file to access the entry area */
>>>> +	smbios_raw_area_attr.size = size;
>>>> +	if (sysfs_create_bin_file(dmi_kobj, &smbios_raw_area_attr))
>>>> +		goto err;
>>> I think this should have had a corresponding call to
>>> sysfs_remove_bin_file() in dmi_sysfs_exit(). (Again no longer relevant
>>> if the code is moved.)
>> The removing is done in kobject_del().
>> Doesn't it? In another way it should be done for
>> dmi/entries/*/raw attributes also.
> It _is_ done for other attributes already:
>
> 	kset_unregister(dmi_kset);
>
> Which is exactly why I believe it should be done for
> smbios_raw_area_attr too. All other kernel drivers are calling
> sysfs_create_bin_file() or equivalent in their cleanup function and I
> see no reason why this driver would be an exception.

kset_unregister() uses kobject_del()
no see difference.

>
>>> There's one thing I do not understand. I seem to understand that the
>>> goal behind this patch is to be able to run dmidecode without /dev/mem.
>>> Dmidecode currently reads 2 areas from /dev/mem: the 0xF0000-0xFFFFF
>>> area in search of the entry point, and the DMI data table itself. With
>>> this patch you make the entry point available through sysfs. But
>>> dmidecode will still need to access /dev/mem to access the DMI data
>>> table. So that does not really solve anything, does it?
>> It's supposed to read DMI table via entries presented by dmi-sysfs.
>> It contains raw attributes that can be used for these purposes.
>> No need to use /dev/mem.
> Yes, I understood this meanwhile, sorry.
>
>> Another case if you want to add binary of whole dmi table to be able to
>> read it directly in order to parse in dmidecode w/o any additional headache
>> with open/close. Well, it partly dupes currently present dmi-sysfs.
> In fact dmi-sysfs already duplicates a lot of code which was already
> present in dmidecode and libsmbios. And exporting the raw table will
> require way less code than the implementation you proposed originally.
> So the code duplication argument doesn't hold, sorry.
>
>> But it simplifies dmi table parsing for dmidecode, and who wants to use
>> dmi-sysfs, let them use it, but dmidecode will be reading raw entry.
>> Well let it be. Why not.
> Yes, this is exactly my point.
>
>> If others are OK, for dmidecode, and probably others tools also,
>> kernel can constantly expose two tables under /sys/firmware/dmi/tables/
>> smbios_entry_point and dmi_table. Independently on dmi-sysfs.
> That's what I would like to see implemented, yes, thank you very much.
>

-- 
Regards,
Ivan Khoronzhuk