Message-ID: <550B08E6.5050200@globallogic.com>
Date:	Thu, 19 Mar 2015 19:35:34 +0200
From:	"Ivan.khoronzhuk" <ivan.khoronzhuk@...ballogic.com>
To:	Jean Delvare <jdelvare@...e.de>
CC:	linux-kernel@...r.kernel.org, matt.fleming@...el.com,
	ard.biesheuvel@...aro.org, grant.likely@...aro.org,
	linux-api@...r.kernel.org, linux-doc@...r.kernel.org,
	mikew@...gle.com, dmidecode-devel@...gnu.org,
	leif.lindholm@...aro.org, msalter@...hat.com
Subject: Re: [Patch] firmware: dmi_scan: split dmisubsystem from dmi-sysfs



On 19.03.15 17:30, Jean Delvare wrote:
> Hi Ivan,
>
> Le Monday 16 March 2015 à 22:57 +0200, Ivan Khoronzhuk a écrit :
>> Some utils, like dmidecode and smbios, need to access SMBIOS entry
>> table area in order to get information like SMBIOS version, size, etc.
>> Currently it's done via /dev/mem. But for situation when /dev/mem
>> usage is disabled, the utils have to use dmi sysfs instead, which
>> doesn't represent SMBIOS entry and adds code/delay redundancy when direct
>> access for table is needed.
>>
>> So this patch creates dmi subsystem and adds SMBIOS entry point to allow
>> utils in question to work correctly without /dev/mem. Also patch adds
>> raw dmi table to simplify dmi table processing in user space, as were
>> proposed by Jean Delvare.
> "as was proposed" or even "as proposed" sounds more correct.
>
> BTW, which tree is your patch based on? I can't get it to apply cleanly
> on top of any kernel version I tried. I adjusted the patch to my tree
> but it means I'm not reviewing your code exactly. Please send patches
> which can be applied on top of some recent vanilla tree (3.19 or 4.0-rc4
> would be OK at the moment.)

Oh, sorry I forgot to mention, it's based on efi/next, but with consumption
that Matt will remove old version:
  85c83ea firmware: dmi-sysfs: Add SMBIOS entry point area attribute

As you remember, your propositions were slightly late,
and patch had been applied on efi/next when you commented.

Matt, Could you please remove old version.
I hope it will be replaced by new one.

>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@...ballogic.com>
>> ---
>>
>> This patch is logical continuation of
>> "[dmidecode] [Patch v4] firmware: dmi-sysfs: add SMBIOS entry point area attribute"
>> https://lkml.org/lkml/2015/2/4/475
>>
>> Pay attention that this includes /sys/firmware/dmi for holding tables instead of
>> /sys/firmware/dmi/table as were proposed.
> Why is that? I proposed /sys/firmware/dmi/tables because the acpi
> subsystem puts its own tables in /sys/firmware/acpi/tables. It seemed
> reasonable to follow that example for consistency. What is your reason
> for doing it differently?

I just like it more instead of catalog/catalog/catalog...
But it's only proposition. Let's it be under tables.

>
>>   Documentation/ABI/testing/sysfs-firmware-dmi       | 122 +++------------------
>>   .../ABI/testing/sysfs-firmware-dmi-entries         | 110 +++++++++++++++++++
> This is adding a lot of noise to your patch, making it harder to review
> and backport. If you think that the contents of sysfs-firmware-dmi would
> rather go in a documentation file named sysfs-firmware-dmi-entries, I
> have no objection, but it should be done in a separate patch.

yes.

>>   drivers/firmware/dmi-sysfs.c                       |  12 +-
>>   drivers/firmware/dmi_scan.c                        | 115 +++++++++++++++++--
>>   include/linux/dmi.h                                |   2 +
>>   5 files changed, 238 insertions(+), 123 deletions(-)
>>   create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi-entries
>>
>> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi b/Documentation/ABI/testing/sysfs-firmware-dmi
>> index c78f9ab..6413128 100644
>> --- a/Documentation/ABI/testing/sysfs-firmware-dmi
>> +++ b/Documentation/ABI/testing/sysfs-firmware-dmi
>> @@ -1,110 +1,16 @@
>>   What:		/sys/firmware/dmi/
>> -Date:		February 2011
>> -Contact:	Mike Waychison <mikew@...gle.com>
>> +Date:		March 2015
>> +Contact:	Ivan Khoronzhuk <ivan.khoronzhuk@...ballogic.com>
>>   Description:
>> (...)
>> +		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_entry_point. The format of SMBIOS
>> +		entry point header can be read in SMBIOS specification.
>> +		To simplify access and processing delay in user space,
> "processing delay" doesn't really describe the problem that was present
> with the previous patch set. It was more a problem of processing time,
> CPU and memory usage, and code complexity. Anyway I see no reason to
> explain that here, given that this proposal was rejected.
>
> You'd rather just explain that the raw data is provided through sysfs as
> an alternative to utilities reading them from /dev/mem (as you already
> correctly explain in the patch description.) That's what your new patch
> is all about.

yes

>
>> +		subsystem provides also raw dmi table under
> DMI better spelled capitalized. I'd also avoid mentioning "subsystem",
> I'm not sure why you're using that word (and also in the subject), sysfs
> is a virtual file system, and there is no such thing as a "dmi

dmi -> DMI.
According subsystem, it was erroneously noticed from efi.c.
It seems confusing to me too, let avoid it.

> subsystem".
>
>> +		/sys/firmware/dmi/dmi_table.
> As said before I'd make it /sys/firmware/dmi/tables/DMI to mimic what
> acpi does.
>
>> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
>> index e0f1cb3..390067d 100644
>> --- a/drivers/firmware/dmi-sysfs.c
>> +++ b/drivers/firmware/dmi-sysfs.c
>> @@ -566,7 +566,6 @@ static struct kobj_type dmi_sysfs_entry_ktype = {
>>   	.default_attrs = dmi_sysfs_entry_attrs,
>>   };
>>   
>> -static struct kobject *dmi_kobj;
>>   static struct kset *dmi_kset;
>>   
>>   /* Global count of all instances seen.  Only for setup */
>> @@ -651,10 +650,10 @@ static int __init dmi_sysfs_init(void)
>>   	int error = -ENOMEM;
>>   	int val;
>>   
>> -	/* Set up our directory */
>> -	dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>> -	if (!dmi_kobj)
>> -		goto err;
>> +	if (!dmi_kobj) {
>> +		pr_err("dmi-sysfs: dmi subsysterm is absent.\n");
> Typo: subsystem. Then again the use of this word keeps confusing me, but
> it's a minor thing.
>
>> +		return -EINVAL;
> The original code had a single error path and I see no reason to change
> that. -EINVAL seems inappropriate for this case, I'd rather return
> -ENOSYS.

ENOSYS better.

>
>> +	}
>>   
>>   	dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
>>   	if (!dmi_kset)
>> @@ -675,7 +674,6 @@ static int __init dmi_sysfs_init(void)
>>   err:
>>   	cleanup_entry_list();
>>   	kset_unregister(dmi_kset);
>> -	kobject_put(dmi_kobj);
>>   	return error;
>>   }
>>   
>> @@ -685,8 +683,6 @@ static void __exit dmi_sysfs_exit(void)
>>   	pr_debug("dmi-sysfs: unloading.\n");
>>   	cleanup_entry_list();
>>   	kset_unregister(dmi_kset);
>> -	kobject_del(dmi_kobj);
>> -	kobject_put(dmi_kobj);
>>   }
>>   
>>   module_init(dmi_sysfs_init);
>> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
>> index c9cb725..3fca52a 100644
>> --- a/drivers/firmware/dmi_scan.c
>> +++ b/drivers/firmware/dmi_scan.c
>> @@ -10,6 +10,9 @@
>>   #include <asm/dmi.h>
>>   #include <asm/unaligned.h>
>>   
>> +struct kobject *dmi_kobj;
>> +EXPORT_SYMBOL_GPL(dmi_kobj);
>> +
>>   /*
>>    * DMI stands for "Desktop Management Interface".  It is part
>>    * of and an antecedent to, SMBIOS, which stands for System
>> @@ -20,6 +23,9 @@ static const char dmi_empty_string[] = "        ";
>>   static u32 dmi_ver __initdata;
>>   static u32 dmi_len;
>>   static u16 dmi_num;
>> +static u8 smbios_entry_point[32];
>> +static int smbios_entry_point_size;
>> +
>>   /*
>>    * Catch too early calls to dmi_check_system():
>>    */
>> @@ -118,6 +124,7 @@ static void dmi_table(u8 *buf,
>>   }
>>   
>>   static phys_addr_t dmi_base;
>> +static u8 *dmi_tb;
> Where "tb" stands for... "table", but you couldn't use that because a
> function by that name already exist, right? I can think of two less
> cryptic ways to solve this: either you rename function dmi_table to,
> say, dmi_decode_table (which would be a better name anyway IMHO), or you
> name your variable dmi_table_p or dmi_raw_table. But "tb" is not
> immediate enough to understand.

If others are OK, I'll better rename dmi_table function to 
dmi_decode_table()
(or maybe dmidecode_table :), joke)
as it's more appropriate to what it's doing.
And accordingly dmi_tb to dmi_table.

>>   
>>   static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
>>   		void *))
>> @@ -476,6 +483,8 @@ static int __init dmi_present(const u8 *buf)
>>   	if (memcmp(buf, "_SM_", 4) == 0 &&
>>   	    buf[5] < 32 && dmi_checksum(buf, buf[5])) {
>>   		smbios_ver = get_unaligned_be16(buf + 6);
>> +		smbios_entry_point_size = buf[5];
>> +		memcpy(smbios_entry_point, buf, smbios_entry_point_size);
>>   
>>   		/* Some BIOS report weird SMBIOS version, fix that up */
>>   		switch (smbios_ver) {
>> @@ -508,6 +517,8 @@ static int __init dmi_present(const u8 *buf)
>>   					dmi_ver >> 8, dmi_ver & 0xFF,
>>   					(dmi_ver < 0x0300) ? "" : ".x");
>>   			} else {
>> +				smbios_entry_point_size = 15;
>> +				memcpy(smbios_entry_point, buf, 15);
>>   				dmi_ver = (buf[14] & 0xF0) << 4 |
>>   					   (buf[14] & 0x0F);
>>   				pr_info("Legacy DMI %d.%d present.\n",
>> @@ -535,6 +546,8 @@ static int __init dmi_smbios3_present(const u8 *buf)
>>   		dmi_ver &= 0xFFFFFF;
>>   		dmi_len = get_unaligned_le32(buf + 12);
>>   		dmi_base = get_unaligned_le64(buf + 16);
>> +		smbios_entry_point_size = buf[6];
>> +		memcpy(smbios_entry_point, buf, smbios_entry_point_size);
>>   
>>   		/*
>>   		 * The 64-bit SMBIOS 3.0 entry point no longer has a field
> This is inconsistent. You should either use smbios_entry_point_size as
> the last argument to memcpy() in all 3 cases (my preference), or you
> should use buf[5], 15 and buf[6] respectively, but not mix.

You probably meant "memcpy(smbios_entry_point, buf, 15)"
Just wanted to avoid redundant line break.

>
>> @@ -638,6 +651,95 @@ void __init dmi_scan_machine(void)
>>   	dmi_initialized = 1;
>>   }
>>   
>> +static ssize_t smbios_entry_point_read(struct file *filp,
>> +				       struct kobject *kobj,
>> +				       struct bin_attribute *bin_attr,
>> +				       char *buf, loff_t pos, size_t count)
>> +{
>> +	ssize_t size;
>> +
>> +	size = bin_attr->size;
>> +
>> +	if (size > pos)
>> +		size -= pos;
>> +	else
>> +		return 0;
>> +
>> +	if (count < size)
>> +		size = count;
>> +
>> +	memcpy(buf, &smbios_entry_point[pos], size);
>> +
>> +	return size;
>> +}
>> +
>> +static ssize_t dmi_table_read(struct file *filp,
>> +			      struct kobject *kobj,
>> +			      struct bin_attribute *bin_attr,
>> +			      char *buf, loff_t pos, size_t count)
>> +{
>> +	ssize_t size;
>> +
>> +	size = bin_attr->size;
>> +
>> +	if (size > pos)
>> +		size -= pos;
>> +	else
>> +		return 0;
>> +
>> +	if (count < size)
>> +		size = count;
>> +
>> +	memcpy(buf, &dmi_tb[pos], size);
>> +
>> +	return size;
>> +}
> Looking at drivers/acpi/bgrt.c, there seems to be a more simple way to
> implement this: fill in the file size and contents (.private) at
> run-time and let sysfs handle all the rest. You already do the former so
> you're actually very close.

I'll look.

>
>> +BIN_ATTR_RO(dmi_table, 0);
>> +BIN_ATTR_RO(smbios_entry_point, 0);
> These should both be static.

Yes.

>
> This will make dmi_table readable by all users, instead of just root as
> it should be. The DMI data contains private information (serial numbers,
> UUID...) You will see that some files under /sys/devices/virtual/dmi/id
> can't be read by regular users for this reason.

Yes.

>
>> +/*
>> + * Register the dmi subsystem under the firmware subsysterm
> Same typo again: subsystem.
>
>> + */
>> +static int __init dmisubsys_init(void)
>> +{
>> +	int ret = -ENOMEM;
> There's only one error case which returns that value so it would be
> better set in that error path.

yep.

>
>> +
>> +	if (!smbios_entry_point_size || !dmi_available) {
> I already mentioned in a previous review that I don't think you need to
> check for !dmi_available, and you said you agreed.

No.
Previously only smbios entry point was exported.
Now DMI table was added.
smbios_entry_point_size doesn't mean DMI table is present.


>
>> +		ret = -EINVAL;
> Again -ENOSYS or similar would be more appropriate (not that it matters
> a lot in practice though as I don't think there is any way to actually
> retrieve this value.)

ENOSYS better

>
>> +		goto err;
>> +	}
>> +
>> +	/* Set up dmi directory at /sys/firmware/dmi */
>> +	dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
>> +	if (!dmi_kobj)
>> +		goto err;
>> +
>> +	bin_attr_smbios_entry_point.size = smbios_entry_point_size;
>> +	ret = sysfs_create_bin_file(dmi_kobj, &bin_attr_smbios_entry_point);
>> +	if (ret)
>> +		goto err;
>> +
>> +	if (!dmi_tb) {
> I don't like this. You should know which of this function and dmi_walk()
> is called first. If you don't, then how can you guarantee that a race
> condition can't happen?

Shit.
Maybe better to leave dmi_walk as it was
and map it only here.

>
> This makes me wonder if that code wouldn't rather go in
> dmi_scan_machine() rather than a separate function.
>
>> +		dmi_tb = dmi_remap(dmi_base, dmi_len);
>> +		if (!dmi_tb)
>> +			goto err;
>> +	}
>> +
>> +	bin_attr_dmi_table.size = dmi_len;
>> +	ret = sysfs_create_bin_file(dmi_kobj, &bin_attr_dmi_table);
>> +	if (ret)
>> +		goto err;
>> +
>> +	return 0;
>> +err:
>> +	pr_err("dmi: Firmware registration failed.\n");
> n
> I said in a previous review that files that have been created should be
> explicitly deleted, and I still think so.

I dislike it, but if you insist I'll do this.

>
>> +	kobject_del(dmi_kobj);
>> +	kobject_put(dmi_kobj);
> I think you also need to explicitly set dmi_kobj to NULL here, otherwise
> dmi-sysfs will try to use an object which no longer exists.

Yes.

>
> An alternative approach would be to leave dmi_kobj available even if the
> binary files could not be created. As this case is very unlikely to
> happen, I don't care which way you choose.

I also thought about such approach. But imaged a situation when DMI 
catalog is
empty and no one uses dmi-sysfs (which probably is more frequently), decided
to delete it. So dmi_kobj = 0.

>
>> +	return ret;
>> +}
>> +subsys_initcall(dmisubsys_init);
>> +
>>   /**
>>    * dmi_set_dump_stack_arch_desc - set arch description for dump_stack()
>>    *
>> @@ -897,18 +999,17 @@ EXPORT_SYMBOL(dmi_get_date);
>>   int dmi_walk(void (*decode)(const struct dmi_header *, void *),
>>   	     void *private_data)
>>   {
>> -	u8 *buf;
>> -
>>   	if (!dmi_available)
>>   		return -1;
>>   
>> -	buf = dmi_remap(dmi_base, dmi_len);
>> -	if (buf == NULL)
>> -		return -1;
>> +	if (!dmi_tb) {
>> +		dmi_tb = dmi_remap(dmi_base, dmi_len);
>> +		if (!dmi_tb)
>> +			return -1;
>> +	}
>>   
>> -	dmi_table(buf, decode, private_data);
>> +	dmi_table(dmi_tb, decode, private_data);
>>   
>> -	dmi_unmap(buf);
>>   	return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(dmi_walk);
>> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
>> index f820f0a..316293e 100644
>> --- a/include/linux/dmi.h
>> +++ b/include/linux/dmi.h
>> @@ -93,6 +93,7 @@ struct dmi_dev_onboard {
>>   	int devfn;
>>   };
>>   
>> +extern struct kobject *dmi_kobj;
>>   extern int dmi_check_system(const struct dmi_system_id *list);
>>   const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list);
>>   extern const char * dmi_get_system_info(int field);
>> @@ -112,6 +113,7 @@ extern void dmi_memdev_name(u16 handle, const char **bank, const char **device);
>>   
>>   #else
>>   
>> +extern struct kobject *dmi_kobj;
> No, if CONFIG_DMI is not set then dmi_scan isn't built and dmi_kobj does
> not exist.

Yep

>
>>   static inline int dmi_check_system(const struct dmi_system_id *list) { return 0; }
>>   static inline const char * dmi_get_system_info(int field) { return NULL; }
>>   static inline const struct dmi_device * dmi_find_device(int type, const char *name,
>

-- 
Regards,
Ivan Khoronzhuk

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists