Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1322185 > unrolled thread

[PATCH 2/4] dmi: Add a DMI firmware node and handling

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/4] dmi: Add a DMI firmware node and handling minyard@acm.org - 2016-01-30 00:50 +0100
    Re: [PATCH 2/4] dmi: Add a DMI firmware node and handling kbuild test robot <lkp@intel.com> - 2016-01-30 01:10 +0100
      Re: [PATCH 2/4] dmi: Add a DMI firmware node and handling Corey Minyard <minyard@acm.org> - 2016-01-30 02:40 +0100
    Re: [PATCH 2/4] dmi: Add a DMI firmware node and handling kbuild test robot <lkp@intel.com> - 2016-01-30 01:50 +0100
    Re: [PATCH 2/4] dmi: Add a DMI firmware node and handling Andy Lutomirski <luto@amacapital.net> - 2016-01-31 23:50 +0100
      Re: [PATCH 2/4] dmi: Add a DMI firmware node and handling Corey Minyard <minyard@acm.org> - 2016-02-01 01:40 +0100
    Re: [PATCH 2/4] dmi: Add a DMI firmware node and handling Jean Delvare <jdelvare@suse.de> - 2016-02-01 10:30 +0100
      Re: [PATCH 2/4] dmi: Add a DMI firmware node and handling Corey Minyard <minyard@acm.org> - 2016-02-02 14:40 +0100
        Re: [PATCH 2/4] dmi: Add a DMI firmware node and handling Andy Lutomirski <luto@amacapital.net> - 2016-02-02 19:30 +0100
          Re: [PATCH 2/4] dmi: Add a DMI firmware node and handling Corey Minyard <minyard@acm.org> - 2016-02-03 18:00 +0100

#1322185 — [PATCH 2/4] dmi: Add a DMI firmware node and handling

From: Corey Minyard <cminyard@mvista.com>

This is so that an IPMI platform device can be created from a
DMI firmware entry.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Jean Delvare <jdelvare@suse.de>
Cc: Andy Lutomirski <luto@kernel.org>
---
 drivers/firmware/dmi_scan.c | 34 ++++++++++++++++++++++++----------
 include/linux/dmi.h         | 24 ++++++++++++++++++++++++
 include/linux/fwnode.h      |  1 +
 3 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index da471b2..13d9bca 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -41,6 +41,16 @@ static struct dmi_memdev_info {
 } *dmi_memdev;
 static int dmi_memdev_nr;
 
+static void *dmi_zalloc(unsigned len)
+{
+	void *ret = dmi_alloc(len);
+
+	if (ret)
+		memset(ret, 0, len);
+
+	return ret;
+}
+
 static const char * __init dmi_string_nosave(const struct dmi_header *dm, u8 s)
 {
 	const u8 *bp = ((u8 *) dm) + dm->length;
@@ -242,6 +252,12 @@ static void __init dmi_save_type(const struct dmi_header *dm, int slot,
 	dmi_ident[slot] = s;
 }
 
+static void __init dmi_devices_list_add(struct dmi_device *dev)
+{
+	dev->fwnode.type = FWNODE_DMI;
+	list_add(&dev->list, &dmi_devices);
+}
+
 static void __init dmi_save_one_device(int type, const char *name)
 {
 	struct dmi_device *dev;
@@ -250,15 +266,14 @@ static void __init dmi_save_one_device(int type, const char *name)
 	if (dmi_find_device(type, name, NULL))
 		return;
 
-	dev = dmi_alloc(sizeof(*dev) + strlen(name) + 1);
+	dev = dmi_zalloc(sizeof(*dev) + strlen(name) + 1);
 	if (!dev)
 		return;
 
 	dev->type = type;
 	strcpy((char *)(dev + 1), name);
 	dev->name = (char *)(dev + 1);
-	dev->device_data = NULL;
-	list_add(&dev->list, &dmi_devices);
+	dmi_devices_list_add(dev);
 }
 
 static void __init dmi_save_devices(const struct dmi_header *dm)
@@ -287,15 +302,14 @@ static void __init dmi_save_oem_strings_devices(const struct dmi_header *dm)
 		if (devname == dmi_empty_string)
 			continue;
 
-		dev = dmi_alloc(sizeof(*dev));
+		dev = dmi_zalloc(sizeof(*dev));
 		if (!dev)
 			break;
 
 		dev->type = DMI_DEV_TYPE_OEM_STRING;
 		dev->name = devname;
-		dev->device_data = NULL;
 
-		list_add(&dev->list, &dmi_devices);
+		dmi_devices_list_add(dev);
 	}
 }
 
@@ -310,7 +324,7 @@ static void __init dmi_save_ipmi_device(const struct dmi_header *dm)
 
 	memcpy(data, dm, dm->length);
 
-	dev = dmi_alloc(sizeof(*dev));
+	dev = dmi_zalloc(sizeof(*dev));
 	if (!dev)
 		return;
 
@@ -318,7 +332,7 @@ static void __init dmi_save_ipmi_device(const struct dmi_header *dm)
 	dev->name = "IPMI controller";
 	dev->device_data = data;
 
-	list_add_tail(&dev->list, &dmi_devices);
+	dmi_devices_list_add(dev);
 }
 
 static void __init dmi_save_dev_pciaddr(int instance, int segment, int bus,
@@ -331,7 +345,7 @@ static void __init dmi_save_dev_pciaddr(int instance, int segment, int bus,
 	    segment == 0xFFFF && bus == 0xFF && devfn == 0xFF)
 		return;
 
-	dev = dmi_alloc(sizeof(*dev) + strlen(name) + 1);
+	dev = dmi_zalloc(sizeof(*dev) + strlen(name) + 1);
 	if (!dev)
 		return;
 
@@ -345,7 +359,7 @@ static void __init dmi_save_dev_pciaddr(int instance, int segment, int bus,
 	dev->dev.name = (char *)&dev[1];
 	dev->dev.device_data = dev;
 
-	list_add(&dev->dev.list, &dmi_devices);
+	dmi_devices_list_add(&dev->dev);
 }
 
 static void __init dmi_save_extended_devices(const struct dmi_header *dm)
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index a930a4d..e130c38 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -4,6 +4,7 @@
 #include <linux/list.h>
 #include <linux/kobject.h>
 #include <linux/mod_devicetable.h>
+#include <linux/fwnode.h>
 
 /* enum dmi_field is in mod_devicetable.h */
 
@@ -80,6 +81,7 @@ struct dmi_header {
 
 struct dmi_device {
 	struct list_head list;
+	struct fwnode_handle fwnode;
 	int type;
 	const char *name;
 	void *device_data;	/* Type specific data */
@@ -113,6 +115,18 @@ extern int dmi_walk(void (*decode)(const struct dmi_header *, void *),
 extern bool dmi_match(enum dmi_field f, const char *str);
 extern void dmi_memdev_name(u16 handle, const char **bank, const char **device);
 
+static inline bool is_dmi_fwnode(struct fwnode_handle *fwnode)
+{
+	return fwnode && fwnode->type == FWNODE_DMI;
+}
+
+static inline struct dmi_device *to_dmi_device(struct fwnode_handle *fwnode)
+{
+	if (is_dmi_fwnode(fwnode))
+		return container_of(fwnode, struct dmi_device, fwnode);
+	return NULL;
+}
+
 #else
 
 static inline int dmi_check_system(const struct dmi_system_id *list) { return 0; }
@@ -144,6 +158,16 @@ static inline void dmi_memdev_name(u16 handle, const char **bank,
 static inline const struct dmi_system_id *
 	dmi_first_match(const struct dmi_system_id *list) { return NULL; }
 
+static inline bool is_dmi_fwnode(struct fwnode_handle *fwnode)
+{
+	return false
+}
+
+static inline struct dmi_fwnode *to_dmi_device(struct fwnode_handle *fwnode)
+{
+	return NULL;
+}
+
 #endif
 
 #endif	/* __DMI_H__ */
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 8516717..8690de2 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -19,6 +19,7 @@ enum fwnode_type {
 	FWNODE_ACPI_DATA,
 	FWNODE_PDATA,
 	FWNODE_IRQCHIP,
+	FWNODE_DMI,
 };
 
 struct fwnode_handle {
-- 
2.5.0

[toc] | [next] | [standalone]

#1322193

[Multipart message — attachments visible in raw view] — view raw

Hi Corey,

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on v4.5-rc1 next-20160129]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/minyard-acm-org/dmi-Rework-to-get-IPMI-autoloading-from-DMI-tables/20160130-074830
config: i386-tinyconfig (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   In file included from arch/x86/kernel/setup.c:46:0:
   include/linux/dmi.h: In function 'is_dmi_fwnode':
>> include/linux/dmi.h:164:1: error: expected ';' before '}' token
    }
    ^

vim +164 include/linux/dmi.h

   158	static inline const struct dmi_system_id *
   159		dmi_first_match(const struct dmi_system_id *list) { return NULL; }
   160	
   161	static inline bool is_dmi_fwnode(struct fwnode_handle *fwnode)
   162	{
   163		return false
 > 164	}
   165	
   166	static inline struct dmi_fwnode *to_dmi_device(struct fwnode_handle *fwnode)
   167	{

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1322227

Dang, I've been doing too much Python. I've fixed this, but I guess I'll 
wait for more comments.

-corey

On 01/29/2016 05:59 PM, kbuild test robot wrote:
> Hi Corey,
>
> [auto build test ERROR on char-misc/char-misc-testing]
> [also build test ERROR on v4.5-rc1 next-20160129]
> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
>
> url:    https://github.com/0day-ci/linux/commits/minyard-acm-org/dmi-Rework-to-get-IPMI-autoloading-from-DMI-tables/20160130-074830
> config: i386-tinyconfig (attached as .config)
> reproduce:
>          # save the attached .config to linux build tree
>          make ARCH=i386
>
> All errors (new ones prefixed by >>):
>
>     In file included from arch/x86/kernel/setup.c:46:0:
>     include/linux/dmi.h: In function 'is_dmi_fwnode':
>>> include/linux/dmi.h:164:1: error: expected ';' before '}' token
>      }
>      ^
>
> vim +164 include/linux/dmi.h
>
>     158	static inline const struct dmi_system_id *
>     159		dmi_first_match(const struct dmi_system_id *list) { return NULL; }
>     160	
>     161	static inline bool is_dmi_fwnode(struct fwnode_handle *fwnode)
>     162	{
>     163		return false
>   > 164	}
>     165	
>     166	static inline struct dmi_fwnode *to_dmi_device(struct fwnode_handle *fwnode)
>     167	{
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1322211

[Multipart message — attachments visible in raw view] — view raw

Hi Corey,

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on v4.5-rc1 next-20160129]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/minyard-acm-org/dmi-Rework-to-get-IPMI-autoloading-from-DMI-tables/20160130-074830
config: i386-randconfig-s0-201604 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

>> WARNING: vmlinux.o(.text.unlikely+0xe634): Section mismatch in reference from the function dmi_zalloc() to the function .init.text:extend_brk()
   The function dmi_zalloc() references
   the function __init extend_brk().
   This is often because dmi_zalloc lacks a __init
   annotation or the annotation of extend_brk is wrong.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1322753

On Fri, Jan 29, 2016 at 2:43 PM,  <minyard@acm.org> wrote:
> From: Corey Minyard <cminyard@mvista.com>
>
> This is so that an IPMI platform device can be created from a
> DMI firmware entry.

This doesn't apply for me.  What's it based on?

--Andy

[toc] | [prev] | [next] | [standalone]

#1322764

On 01/31/2016 04:46 PM, Andy Lutomirski wrote:
> On Fri, Jan 29, 2016 at 2:43 PM,  <minyard@acm.org> wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> This is so that an IPMI platform device can be created from a
>> DMI firmware entry.
> This doesn't apply for me.  What's it based on?
>
> --Andy

It was off of Linus' master a couple of days ago.  I just rebased to the 
latest and it applied cleanly.

I can resend, if you like.

Thanks,

-corey

[toc] | [prev] | [next] | [standalone]

#1322938

Hi Corey,

I won't comment on the IPMI side of this as this isn't my area. However
I have a comment on the DMI part:

Le Friday 29 January 2016 à 16:43 -0600, minyard@acm.org a écrit :
> From: Corey Minyard <cminyard@mvista.com>
> 
> This is so that an IPMI platform device can be created from a
> DMI firmware entry.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Jean Delvare <jdelvare@suse.de>
> Cc: Andy Lutomirski <luto@kernel.org>
> ---
>  drivers/firmware/dmi_scan.c | 34 ++++++++++++++++++++++++----------
>  include/linux/dmi.h         | 24 ++++++++++++++++++++++++
>  include/linux/fwnode.h      |  1 +
>  3 files changed, 49 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index da471b2..13d9bca 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -41,6 +41,16 @@ static struct dmi_memdev_info {
>  } *dmi_memdev;
>  static int dmi_memdev_nr;
>  
> +static void *dmi_zalloc(unsigned len)
> +{
> +	void *ret = dmi_alloc(len);
> +
> +	if (ret)
> +		memset(ret, 0, len);
> +
> +	return ret;
> +}
> +
>  static const char * __init dmi_string_nosave(const struct dmi_header *dm, u8 s)
>  {
>  	const u8 *bp = ((u8 *) dm) + dm->length;
> @@ -242,6 +252,12 @@ static void __init dmi_save_type(const struct dmi_header *dm, int slot,
> (...)
> @@ -250,15 +266,14 @@ static void __init dmi_save_one_device(int type, const char *name)
>  	if (dmi_find_device(type, name, NULL))
>  		return;
>  
> -	dev = dmi_alloc(sizeof(*dev) + strlen(name) + 1);
> +	dev = dmi_zalloc(sizeof(*dev) + strlen(name) + 1);
>  	if (!dev)
>  		return;
>  
>  	dev->type = type;
>  	strcpy((char *)(dev + 1), name);
>  	dev->name = (char *)(dev + 1);
> -	dev->device_data = NULL;

This change seems rather unrelated, and I'm not sure what purpose it
serves. On ia64 and arm64 it is clearly redundant as dmi_alloc calls
kzalloc directly. On x86_64, extend_brk is called instead (don't ask me
why, I have no clue) but looking at the code I see that it does
memset(ret, 0, size) as well so memory is also zeroed there. Which makes
dmi_alloc the same as dmi_zalloc on all 3 architectures.

So please revert this change. This will make your patch easier to
review, too.

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [next] | [standalone]

#1324097

On 02/01/2016 03:25 AM, Jean Delvare wrote:
> Hi Corey,
>
> I won't comment on the IPMI side of this as this isn't my area. However
> I have a comment on the DMI part:
>
> Le Friday 29 January 2016 à 16:43 -0600, minyard@acm.org a écrit :
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> This is so that an IPMI platform device can be created from a
>> DMI firmware entry.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> Cc: Jean Delvare <jdelvare@suse.de>
>> Cc: Andy Lutomirski <luto@kernel.org>
>> ---
>>   drivers/firmware/dmi_scan.c | 34 ++++++++++++++++++++++++----------
>>   include/linux/dmi.h         | 24 ++++++++++++++++++++++++
>>   include/linux/fwnode.h      |  1 +
>>   3 files changed, 49 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
>> index da471b2..13d9bca 100644
>> --- a/drivers/firmware/dmi_scan.c
>> +++ b/drivers/firmware/dmi_scan.c
>> @@ -41,6 +41,16 @@ static struct dmi_memdev_info {
>>   } *dmi_memdev;
>>   static int dmi_memdev_nr;
>>   
>> +static void *dmi_zalloc(unsigned len)
>> +{
>> +	void *ret = dmi_alloc(len);
>> +
>> +	if (ret)
>> +		memset(ret, 0, len);
>> +
>> +	return ret;
>> +}
>> +
>>   static const char * __init dmi_string_nosave(const struct dmi_header *dm, u8 s)
>>   {
>>   	const u8 *bp = ((u8 *) dm) + dm->length;
>> @@ -242,6 +252,12 @@ static void __init dmi_save_type(const struct dmi_header *dm, int slot,
>> (...)
>> @@ -250,15 +266,14 @@ static void __init dmi_save_one_device(int type, const char *name)
>>   	if (dmi_find_device(type, name, NULL))
>>   		return;
>>   
>> -	dev = dmi_alloc(sizeof(*dev) + strlen(name) + 1);
>> +	dev = dmi_zalloc(sizeof(*dev) + strlen(name) + 1);
>>   	if (!dev)
>>   		return;
>>   
>>   	dev->type = type;
>>   	strcpy((char *)(dev + 1), name);
>>   	dev->name = (char *)(dev + 1);
>> -	dev->device_data = NULL;
> This change seems rather unrelated, and I'm not sure what purpose it
> serves. On ia64 and arm64 it is clearly redundant as dmi_alloc calls
> kzalloc directly. On x86_64, extend_brk is called instead (don't ask me
> why, I have no clue) but looking at the code I see that it does
> memset(ret, 0, size) as well so memory is also zeroed there. Which makes
> dmi_alloc the same as dmi_zalloc on all 3 architectures.
>
> So please revert this change. This will make your patch easier to
> review, too.
>
Ok.  I had assumed extend_break wasn't zeroing since there were all the 
NULL assignments,
I should have looked.

I was thinking about this, and the fwnode could just be added to the 
IPMI device.  I'm not
sure if you would prefer that over adding it to dmi_device.  The fwnode 
is in acpi_device,
and I was modelling these changes after that, but maybe that's not 
required here.

Thanks,

-corey

[toc] | [prev] | [next] | [standalone]

#1324408

On Feb 2, 2016 5:37 AM, "Corey Minyard" <minyard@acm.org> wrote:
>
> On 02/01/2016 03:25 AM, Jean Delvare wrote:
>>
>> Hi Corey,
>>
>> I won't comment on the IPMI side of this as this isn't my area. However
>> I have a comment on the DMI part:
>>
>> Le Friday 29 January 2016 à 16:43 -0600, minyard@acm.org a écrit :
>>>
>>> From: Corey Minyard <cminyard@mvista.com>
>>>
>>> This is so that an IPMI platform device can be created from a
>>> DMI firmware entry.
>>>
>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>> Cc: Jean Delvare <jdelvare@suse.de>
>>> Cc: Andy Lutomirski <luto@kernel.org>
>>> ---
>>>   drivers/firmware/dmi_scan.c | 34 ++++++++++++++++++++++++----------
>>>   include/linux/dmi.h         | 24 ++++++++++++++++++++++++
>>>   include/linux/fwnode.h      |  1 +
>>>   3 files changed, 49 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
>>> index da471b2..13d9bca 100644
>>> --- a/drivers/firmware/dmi_scan.c
>>> +++ b/drivers/firmware/dmi_scan.c
>>> @@ -41,6 +41,16 @@ static struct dmi_memdev_info {
>>>   } *dmi_memdev;
>>>   static int dmi_memdev_nr;
>>>   +static void *dmi_zalloc(unsigned len)
>>> +{
>>> +       void *ret = dmi_alloc(len);
>>> +
>>> +       if (ret)
>>> +               memset(ret, 0, len);
>>> +
>>> +       return ret;
>>> +}
>>> +
>>>   static const char * __init dmi_string_nosave(const struct dmi_header *dm, u8 s)
>>>   {
>>>         const u8 *bp = ((u8 *) dm) + dm->length;
>>> @@ -242,6 +252,12 @@ static void __init dmi_save_type(const struct dmi_header *dm, int slot,
>>> (...)
>>> @@ -250,15 +266,14 @@ static void __init dmi_save_one_device(int type, const char *name)
>>>         if (dmi_find_device(type, name, NULL))
>>>                 return;
>>>   -     dev = dmi_alloc(sizeof(*dev) + strlen(name) + 1);
>>> +       dev = dmi_zalloc(sizeof(*dev) + strlen(name) + 1);
>>>         if (!dev)
>>>                 return;
>>>         dev->type = type;
>>>         strcpy((char *)(dev + 1), name);
>>>         dev->name = (char *)(dev + 1);
>>> -       dev->device_data = NULL;
>>
>> This change seems rather unrelated, and I'm not sure what purpose it
>> serves. On ia64 and arm64 it is clearly redundant as dmi_alloc calls
>> kzalloc directly. On x86_64, extend_brk is called instead (don't ask me
>> why, I have no clue) but looking at the code I see that it does
>> memset(ret, 0, size) as well so memory is also zeroed there. Which makes
>> dmi_alloc the same as dmi_zalloc on all 3 architectures.
>>
>> So please revert this change. This will make your patch easier to
>> review, too.
>>
> Ok.  I had assumed extend_break wasn't zeroing since there were all the NULL assignments,
> I should have looked.
>
> I was thinking about this, and the fwnode could just be added to the IPMI device.  I'm not
> sure if you would prefer that over adding it to dmi_device.  The fwnode is in acpi_device,
> and I was modelling these changes after that, but maybe that's not required here.

I think dmi_device is right, especially if your patches result in a
firmware_node sysfs link being created.  That way the link will point
to the right place.


--Andy

[toc] | [prev] | [next] | [standalone]

#1325688

On 02/02/2016 12:25 PM, Andy Lutomirski wrote:
> On Feb 2, 2016 5:37 AM, "Corey Minyard" <minyard@acm.org> wrote:
>> On 02/01/2016 03:25 AM, Jean Delvare wrote:
>>> Hi Corey,
>>>
>>> I won't comment on the IPMI side of this as this isn't my area. However
>>> I have a comment on the DMI part:
>>>
>>> Le Friday 29 January 2016 à 16:43 -0600, minyard@acm.org a écrit :
>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>
>>>> This is so that an IPMI platform device can be created from a
>>>> DMI firmware entry.
>>>>
>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>>> Cc: Jean Delvare <jdelvare@suse.de>
>>>> Cc: Andy Lutomirski <luto@kernel.org>
>>>> ---
>>>>    drivers/firmware/dmi_scan.c | 34 ++++++++++++++++++++++++----------
>>>>    include/linux/dmi.h         | 24 ++++++++++++++++++++++++
>>>>    include/linux/fwnode.h      |  1 +
>>>>    3 files changed, 49 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
>>>> index da471b2..13d9bca 100644
>>>> --- a/drivers/firmware/dmi_scan.c
>>>> +++ b/drivers/firmware/dmi_scan.c
>>>> @@ -41,6 +41,16 @@ static struct dmi_memdev_info {
>>>>    } *dmi_memdev;
>>>>    static int dmi_memdev_nr;
>>>>    +static void *dmi_zalloc(unsigned len)
>>>> +{
>>>> +       void *ret = dmi_alloc(len);
>>>> +
>>>> +       if (ret)
>>>> +               memset(ret, 0, len);
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +
>>>>    static const char * __init dmi_string_nosave(const struct dmi_header *dm, u8 s)
>>>>    {
>>>>          const u8 *bp = ((u8 *) dm) + dm->length;
>>>> @@ -242,6 +252,12 @@ static void __init dmi_save_type(const struct dmi_header *dm, int slot,
>>>> (...)
>>>> @@ -250,15 +266,14 @@ static void __init dmi_save_one_device(int type, const char *name)
>>>>          if (dmi_find_device(type, name, NULL))
>>>>                  return;
>>>>    -     dev = dmi_alloc(sizeof(*dev) + strlen(name) + 1);
>>>> +       dev = dmi_zalloc(sizeof(*dev) + strlen(name) + 1);
>>>>          if (!dev)
>>>>                  return;
>>>>          dev->type = type;
>>>>          strcpy((char *)(dev + 1), name);
>>>>          dev->name = (char *)(dev + 1);
>>>> -       dev->device_data = NULL;
>>> This change seems rather unrelated, and I'm not sure what purpose it
>>> serves. On ia64 and arm64 it is clearly redundant as dmi_alloc calls
>>> kzalloc directly. On x86_64, extend_brk is called instead (don't ask me
>>> why, I have no clue) but looking at the code I see that it does
>>> memset(ret, 0, size) as well so memory is also zeroed there. Which makes
>>> dmi_alloc the same as dmi_zalloc on all 3 architectures.
>>>
>>> So please revert this change. This will make your patch easier to
>>> review, too.
>>>
>> Ok.  I had assumed extend_break wasn't zeroing since there were all the NULL assignments,
>> I should have looked.
>>
>> I was thinking about this, and the fwnode could just be added to the IPMI device.  I'm not
>> sure if you would prefer that over adding it to dmi_device.  The fwnode is in acpi_device,
>> and I was modelling these changes after that, but maybe that's not required here.
> I think dmi_device is right, especially if your patches result in a
> firmware_node sysfs link being created.  That way the link will point
> to the right place.

Yeah, that's the conclusion I had come to, I think.  It doesn't 
currently add the
firmware_node to sysfs, but that's easily added and probably a next logical
step.

I'll have a new set of patches out today after I compile test at each step.

Thanks,

-corey

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web