LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	dmidecode-devel@nongnu.org,
	Grant Likely <grant.likely@linaro.org>,
	Leif Lindholm <leif.lindholm@linaro.org>,
	Matt Fleming <matt.fleming@intel.com>,
	"x86@kernel.org" <x86@kernel.org>
Subject: Re: [PATCH 1/2] firmware: dmi_scan: add symbol to get SMBIOS entry area
Date: Mon, 26 Jan 2015 13:53:50 +0200	[thread overview]
Message-ID: <54C62ACE.1080503@linaro.org> (raw)
In-Reply-To: <CAKv+Gu-0Q74ZSUyMSfUT_kJ2cKRx9BcZkvHrrqw=7SyX+6RS5Q@mail.gmail.com>


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


  reply	other threads:[~2015-01-26 11:53 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-01-23 20:21 [PATCH 0/2] firmware: dmi-sysfs: add SMBIOS Ivan Khoronzhuk
2015-01-23 20:21 ` [PATCH 1/2] firmware: dmi_scan: add symbol to get SMBIOS entry area Ivan Khoronzhuk
2015-01-26  8:44   ` Ard Biesheuvel
2015-01-26 11:53     ` Ivan Khoronzhuk [this message]
2015-01-23 20:21 ` [PATCH 2/2] firmware: dmi-sysfs: add SMBIOS entry point area attribute Ivan Khoronzhuk

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=54C62ACE.1080503@linaro.org \
    --to=ivan.khoronzhuk@linaro.org \
    --cc=ard.biesheuvel@linaro.org \
    --cc=dmidecode-devel@nongnu.org \
    --cc=grant.likely@linaro.org \
    --cc=leif.lindholm@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=matt.fleming@intel.com \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH 1/2] firmware: dmi_scan: add symbol to get SMBIOS entry area' \
    /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).