Re: [PATCH 1/2] firmware: dmi_scan: add symbol to get SMBIOS entry area

[Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>]

On 01/26/2015 10:44 AM, Ard Biesheuvel wrote:
> On 23 January 2015 at 20:21, Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>> There are situations when code needs to access SMBIOS entry table
>> area. For example, to pass it via sysfs to userspace when it's not
>> allowed to get SMBIOS info via /dev/mem.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> ---
>>   drivers/firmware/dmi_scan.c | 34 ++++++++++++++++++++++++++++++++++
>>   include/linux/dmi.h         |  2 ++
>>   2 files changed, 36 insertions(+)
>>
>> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
>> index 420c8d8..ae9204a 100644
>> --- a/drivers/firmware/dmi_scan.c
>> +++ b/drivers/firmware/dmi_scan.c
>> @@ -113,6 +113,7 @@ static void dmi_table(u8 *buf, int len, int num,
>>          }
>>   }
>>
>> +static unsigned char smbios_header[32];
>>   static phys_addr_t dmi_base;
>>   static u16 dmi_len;
>>   static u16 dmi_num;
>> @@ -474,6 +475,7 @@ 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);
>> +               memcpy(smbios_header, buf, buf[5]);
>>
>>                  /* Some BIOS report weird SMBIOS version, fix that up */
>>                  switch (smbios_ver) {
>> @@ -505,6 +507,7 @@ static int __init dmi_present(const u8 *buf)
>>                                  pr_info("SMBIOS %d.%d present.\n",
>>                                         dmi_ver >> 8, dmi_ver & 0xFF);
>>                          } else {
>> +                               memcpy(smbios_header, buf, 15);
>>                                  dmi_ver = (buf[14] & 0xF0) << 4 |
>>                                             (buf[14] & 0x0F);
>>                                  pr_info("Legacy DMI %d.%d present.\n",
>> @@ -531,6 +534,7 @@ 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);
>> +               memcpy(smbios_header, buf, buf[6]);
>>
>>                  /*
>>                   * The 64-bit SMBIOS 3.0 entry point no longer has a field
>> @@ -944,3 +948,33 @@ void dmi_memdev_name(u16 handle, const char **bank, const char **device)
>>          }
>>   }
>>   EXPORT_SYMBOL_GPL(dmi_memdev_name);
>> +
>> +/**
>> + * dmi_get_smbios_entry_area - copy SMBIOS entry point area to array.
>> + * @entry - pointer on array to read area in, current max size is 32 bytes.
>> + *
>> + * returns -ENODATA if table is not available, otherwise returns actual
>> + * size of SMBIOS entry point area.
>> + */
>> +int dmi_get_smbios_entry_area(char *table)
>> +{
> What about
>
> const u8 *dmi_get_smbios_header(int *size)
>
> and return the buffer (or NULL if no data) and the size previously
> recorded in *size (see below)
>
> The reason is that, in the second patch, you are copying the data into
> yet another char[32], which doesn't make a great deal of sense is the
> data is not captured right at that time
>
> (I suggested earlier that the correct way to implement this was to
> populate the header at the same time you traverse the dmi tree to
> populate the sysfs entries. If you capture the data at init time,
> there is no reason to copy it yet again at dmi_sysfs module_init time)


yes. Why I don't trust to return pointer even in case of const.
Even don't remember.
I'll update with const.
Thanks.

>> +       int size = 0;
>> +
>> +       if (!dmi_available)
>> +               return -ENODATA;
>> +
>> +       if (memcmp(smbios_header, "_SM3_", 5) == 0)
>> +               size = smbios_header[6];
>> +       else if (memcmp(smbios_header, "_SM_", 4) == 0)
>> +               size = smbios_header[5];
>> +       else if (memcmp(smbios_header, "_DMI_", 5) == 0)
>> +               size = 15;
>> +
> You are duplicating work here: could we record smbios_header_size when
> you capture the data itself?

~
I'll update soon.

>
>> +       memcpy(table, smbios_header, size);
>> +
>> +       if (!size)
>> +               return -ENODATA;
>> +
>> +       return size;
>> +}
>> +EXPORT_SYMBOL_GPL(dmi_get_smbios_entry_area);
>> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
>> index f820f0a..7cae713 100644
>> --- a/include/linux/dmi.h
>> +++ b/include/linux/dmi.h
>> @@ -109,6 +109,7 @@ extern int dmi_walk(void (*decode)(const struct dmi_header *, void *),
>>          void *private_data);
>>   extern bool dmi_match(enum dmi_field f, const char *str);
>>   extern void dmi_memdev_name(u16 handle, const char **bank, const char **device);
>> +int dmi_get_smbios_entry_area(char *table);
>>
>>   #else
>>
>> @@ -140,6 +141,7 @@ static inline void dmi_memdev_name(u16 handle, const char **bank,
>>                  const char **device) { }
>>   static inline const struct dmi_system_id *
>>          dmi_first_match(const struct dmi_system_id *list) { return NULL; }
>> +static inline int dmi_get_smbios_entry_area(char *table) { return -ENODATA; }
>>
>>   #endif
>>
>> --
>> 1.9.1
>>