LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Ivan.khoronzhuk" <ivan.khoronzhuk@globallogic.com>
To: Jean Delvare <jdelvare@suse.de>
Cc: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>,
	linux-kernel@vger.kernel.org, ard.biesheuvel@linaro.org,
	grant.likely@linaro.org, matt.fleming@intel.com,
	linux-api@vger.kernel.org, linux-doc@vger.kernel.org,
	dmidecode-devel@nongnu.org, leif.lindholm@linaro.org,
	msalter@redhat.com
Subject: Re: [dmidecode] [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute
Date: Thu, 19 Mar 2015 19:33:56 +0200	[thread overview]
Message-ID: <550B0884.6010902@globallogic.com> (raw)
In-Reply-To: <1425978807.24849.22.camel@chaos.site>



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


      parent reply	other threads:[~2015-03-19 17:34 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-02-04 17:06 Ivan Khoronzhuk
2015-02-10  9:51 ` Ivan Khoronzhuk
2015-02-11 14:17   ` Matt Fleming
2015-02-11 14:43     ` Matt Fleming
2015-02-12  2:33       ` Ivan Khoronzhuk
2015-02-26  9:36 ` [dmidecode] " Jean Delvare
2015-03-04 12:30   ` Ivan.khoronzhuk
2015-03-10  9:13     ` Jean Delvare
     [not found]       ` <1426162975.2784.31.camel@mfleming-mobl1.ger.corp.intel.com>
2015-03-16 21:02         ` Ivan.khoronzhuk
2015-03-26 13:05           ` Matt Fleming
2015-03-26 13:06             ` Ivan.khoronzhuk
2015-03-26 14:23               ` Jean Delvare
2015-03-19 17:33       ` Ivan.khoronzhuk [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=550B0884.6010902@globallogic.com \
    --to=ivan.khoronzhuk@globallogic.com \
    --cc=ard.biesheuvel@linaro.org \
    --cc=dmidecode-devel@nongnu.org \
    --cc=grant.likely@linaro.org \
    --cc=ivan.khoronzhuk@linaro.org \
    --cc=jdelvare@suse.de \
    --cc=leif.lindholm@linaro.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=matt.fleming@intel.com \
    --cc=msalter@redhat.com \
    --subject='Re: [dmidecode] [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).