Messages in this thread | | | Date | Thu, 19 Mar 2015 19:33:56 +0200 | From | "Ivan.khoronzhuk" <> | Subject | Re: [dmidecode] [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute |
| |
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
| |