Re: [dmidecode] [Patch] firmware: dmi_scan: split dmisubsystem from dmi-sysfs

[Jean Delvare]

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.)

> Signed-off-by: Ivan Khoronzhuk <address@hidden>
> ---
> 
> 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?

>  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.

>  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 <address@hidden>
> +Date:                March 2015
> +Contact:     Ivan Khoronzhuk <address@hidden>
>  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.

> +             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
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.

> +     }
>  
>       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.

>  
>  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.

> @@ -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. 

> +BIN_ATTR_RO(dmi_table, 0);
> +BIN_ATTR_RO(smbios_entry_point, 0);

These should both be static.

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.

> +/*
> + * 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.

> +
> +     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.

> +             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.)

> +             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?

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");

I said in a previous review that files that have been created should be
explicitly deleted, and I still think so.

> +     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.

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.

> +     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.

>  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,


-- 
Jean Delvare
SUSE L3 Support