[PATCH 6/8] dmi: Move memdev_dmi_entry definition to dmi.h

[PATCH 6/8] dmi: Move memdev_dmi_entry definition to dmi.h

[Matt Roper]

A couple of the EDAC drivers have a nice memdev_dmi_entry structure for
decoding DMI memory device entries.  Move the structure definition to
dmi.h so that it can be shared between those drivers and also other
parts of the kernel; the i915 graphics driver is going to need to use
this structure soon as well.

Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
Cc: Jean Delvare <jdelvare@suse.com>
Cc: linux-edac@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/edac/ghes_edac.c   | 26 --------------------------
 drivers/edac/i7core_edac.c | 25 -------------------------
 include/linux/dmi.h        | 25 +++++++++++++++++++++++++
 3 files changed, 25 insertions(+), 51 deletions(-)

diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index e3fa439..429be79 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -35,32 +35,6 @@ static DEFINE_MUTEX(ghes_edac_lock);
 static int ghes_edac_mc_num;
 
 
-/* Memory Device - Type 17 of SMBIOS spec */
-struct memdev_dmi_entry {
-	u8 type;
-	u8 length;
-	u16 handle;
-	u16 phys_mem_array_handle;
-	u16 mem_err_info_handle;
-	u16 total_width;
-	u16 data_width;
-	u16 size;
-	u8 form_factor;
-	u8 device_set;
-	u8 device_locator;
-	u8 bank_locator;
-	u8 memory_type;
-	u16 type_detail;
-	u16 speed;
-	u8 manufacturer;
-	u8 serial_number;
-	u8 asset_tag;
-	u8 part_number;
-	u8 attributes;
-	u32 extended_size;
-	u16 conf_mem_clk_speed;
-} __attribute__((__packed__));
-
 struct ghes_edac_dimm_fill {
 	struct mem_ctl_info *mci;
 	unsigned count;
diff --git a/drivers/edac/i7core_edac.c b/drivers/edac/i7core_edac.c
index 01087a3..b11d3be 100644
--- a/drivers/edac/i7core_edac.c
+++ b/drivers/edac/i7core_edac.c
@@ -1906,31 +1906,6 @@ static struct notifier_block i7_mce_dec = {
 	.notifier_call	= i7core_mce_check_error,
 };
 
-struct memdev_dmi_entry {
-	u8 type;
-	u8 length;
-	u16 handle;
-	u16 phys_mem_array_handle;
-	u16 mem_err_info_handle;
-	u16 total_width;
-	u16 data_width;
-	u16 size;
-	u8 form;
-	u8 device_set;
-	u8 device_locator;
-	u8 bank_locator;
-	u8 memory_type;
-	u16 type_detail;
-	u16 speed;
-	u8 manufacturer;
-	u8 serial_number;
-	u8 asset_tag;
-	u8 part_number;
-	u8 attributes;
-	u32 extended_size;
-	u16 conf_mem_clk_speed;
-} __attribute__((__packed__));
-
 
 /*
  * Decode the DRAM Clock Frequency, be paranoid, make sure that all
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index 5e9c74c..1eb2136 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -78,6 +78,31 @@ struct dmi_header {
 	u16 handle;
 } __packed;
 
+struct memdev_dmi_entry {
+	u8 type;
+	u8 length;
+	u16 handle;
+	u16 phys_mem_array_handle;
+	u16 mem_err_info_handle;
+	u16 total_width;
+	u16 data_width;
+	u16 size;
+	u8 form;
+	u8 device_set;
+	u8 device_locator;
+	u8 bank_locator;
+	u8 memory_type;
+	u16 type_detail;
+	u16 speed;
+	u8 manufacturer;
+	u8 serial_number;
+	u8 asset_tag;
+	u8 part_number;
+	u8 attributes;
+	u32 extended_size;
+	u16 conf_mem_clk_speed;
+} __attribute__((__packed__));
+
 struct dmi_device {
 	struct list_head list;
 	int type;
-- 
2.1.4

Re: [PATCH 6/8] dmi: Move memdev_dmi_entry definition to dmi.h

[Jean Delvare]

Hi Matt,

On Mon,  7 Mar 2016 17:05:44 -0800, Matt Roper wrote:
> A couple of the EDAC drivers have a nice memdev_dmi_entry structure for
> decoding DMI memory device entries.  Move the structure definition to
> dmi.h so that it can be shared between those drivers and also other
> parts of the kernel; the i915 graphics driver is going to need to use
> this structure soon as well.

Looks good in principle, a few suggestions inline below:

> Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: linux-edac@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/edac/ghes_edac.c   | 26 --------------------------
>  drivers/edac/i7core_edac.c | 25 -------------------------
>  include/linux/dmi.h        | 25 +++++++++++++++++++++++++
>  3 files changed, 25 insertions(+), 51 deletions(-)
> (...)
> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
> index 5e9c74c..1eb2136 100644
> --- a/include/linux/dmi.h
> +++ b/include/linux/dmi.h
> @@ -78,6 +78,31 @@ struct dmi_header {
>  	u16 handle;
>  } __packed;
>  
> +struct memdev_dmi_entry {

As a member of the API of the dmi subsystem, this symbol should start
with dmi_. What about dmi_entry_memdev?

> +	u8 type;
> +	u8 length;
> +	u16 handle;
> +	u16 phys_mem_array_handle;
> +	u16 mem_err_info_handle;
> +	u16 total_width;
> +	u16 data_width;
> +	u16 size;
> +	u8 form;
> +	u8 device_set;
> +	u8 device_locator;
> +	u8 bank_locator;
> +	u8 memory_type;
> +	u16 type_detail;
> +	u16 speed;
> +	u8 manufacturer;
> +	u8 serial_number;
> +	u8 asset_tag;
> +	u8 part_number;
> +	u8 attributes;
> +	u32 extended_size;
> +	u16 conf_mem_clk_speed;
> +} __attribute__((__packed__));

dmi_header is declared with __packed instead. I would appreciate
consistency within this header file, so please use __packed too.

> +
>  struct dmi_device {
>  	struct list_head list;
>  	int type;

Thanks,
-- 
Jean Delvare
SUSE L3 Support

[PATCH 6/8] dmi: Move memdev_dmi_entry definition to dmi.h (v2)

[Matt Roper]

A couple of the EDAC drivers have a nice memdev_dmi_entry structure for
decoding DMI memory device entries.  Move the structure definition to
dmi.h so that it can be shared between those drivers and also other
parts of the kernel; the i915 graphics driver is going to need to use
this structure soon as well.  As part of this move we rename the
structure s/memdev_dmi_entry/dmi_entry_memdev/ to ensure it has a proper
'dmi' prefix.

v2:
 - Rename structure to dmi_entry_memdev.  (Jean)
 - Use __packed instead of __attribute__((__packed__)) for consistency
   with the rest of the dmi.h header.  (Jean)

Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
Cc: Jean Delvare <jdelvare@suse.com>
Cc: linux-edac@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/edac/ghes_edac.c   | 28 +--------------------------
 drivers/edac/i7core_edac.c | 47 +++++++++++-----------------------------------
 include/linux/dmi.h        | 25 ++++++++++++++++++++++++
 3 files changed, 37 insertions(+), 63 deletions(-)

diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index e3fa439..39535bb 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -35,32 +35,6 @@ static DEFINE_MUTEX(ghes_edac_lock);
 static int ghes_edac_mc_num;
 
 
-/* Memory Device - Type 17 of SMBIOS spec */
-struct memdev_dmi_entry {
-	u8 type;
-	u8 length;
-	u16 handle;
-	u16 phys_mem_array_handle;
-	u16 mem_err_info_handle;
-	u16 total_width;
-	u16 data_width;
-	u16 size;
-	u8 form_factor;
-	u8 device_set;
-	u8 device_locator;
-	u8 bank_locator;
-	u8 memory_type;
-	u16 type_detail;
-	u16 speed;
-	u8 manufacturer;
-	u8 serial_number;
-	u8 asset_tag;
-	u8 part_number;
-	u8 attributes;
-	u32 extended_size;
-	u16 conf_mem_clk_speed;
-} __attribute__((__packed__));
-
 struct ghes_edac_dimm_fill {
 	struct mem_ctl_info *mci;
 	unsigned count;
@@ -80,7 +54,7 @@ static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg)
 	struct mem_ctl_info *mci = dimm_fill->mci;
 
 	if (dh->type == DMI_ENTRY_MEM_DEVICE) {
-		struct memdev_dmi_entry *entry = (struct memdev_dmi_entry *)dh;
+		struct dmi_entry_memdev *entry = (struct dmi_entry_memdev *)dh;
 		struct dimm_info *dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms,
 						       mci->n_layers,
 						       dimm_fill->count, 0, 0);
diff --git a/drivers/edac/i7core_edac.c b/drivers/edac/i7core_edac.c
index 01087a3..fbfb06f 100644
--- a/drivers/edac/i7core_edac.c
+++ b/drivers/edac/i7core_edac.c
@@ -1906,31 +1906,6 @@ static struct notifier_block i7_mce_dec = {
 	.notifier_call	= i7core_mce_check_error,
 };
 
-struct memdev_dmi_entry {
-	u8 type;
-	u8 length;
-	u16 handle;
-	u16 phys_mem_array_handle;
-	u16 mem_err_info_handle;
-	u16 total_width;
-	u16 data_width;
-	u16 size;
-	u8 form;
-	u8 device_set;
-	u8 device_locator;
-	u8 bank_locator;
-	u8 memory_type;
-	u16 type_detail;
-	u16 speed;
-	u8 manufacturer;
-	u8 serial_number;
-	u8 asset_tag;
-	u8 part_number;
-	u8 attributes;
-	u32 extended_size;
-	u16 conf_mem_clk_speed;
-} __attribute__((__packed__));
-
 
 /*
  * Decode the DRAM Clock Frequency, be paranoid, make sure that all
@@ -1946,28 +1921,28 @@ static void decode_dclk(const struct dmi_header *dh, void *_dclk_freq)
 		return;
 
 	if (dh->type == DMI_ENTRY_MEM_DEVICE) {
-		struct memdev_dmi_entry *memdev_dmi_entry =
-			(struct memdev_dmi_entry *)dh;
+		struct dmi_entry_memdev *dmi_entry_memdev =
+			(struct dmi_entry_memdev *)dh;
 		unsigned long conf_mem_clk_speed_offset =
-			(unsigned long)&memdev_dmi_entry->conf_mem_clk_speed -
-			(unsigned long)&memdev_dmi_entry->type;
+			(unsigned long)&dmi_entry_memdev->conf_mem_clk_speed -
+			(unsigned long)&dmi_entry_memdev->type;
 		unsigned long speed_offset =
-			(unsigned long)&memdev_dmi_entry->speed -
-			(unsigned long)&memdev_dmi_entry->type;
+			(unsigned long)&dmi_entry_memdev->speed -
+			(unsigned long)&dmi_entry_memdev->type;
 
 		/* Check that a DIMM is present */
-		if (memdev_dmi_entry->size == 0)
+		if (dmi_entry_memdev->size == 0)
 			return;
 
 		/*
 		 * Pick the configured speed if it's available, otherwise
 		 * pick the DIMM speed, or we don't have a speed.
 		 */
-		if (memdev_dmi_entry->length > conf_mem_clk_speed_offset) {
+		if (dmi_entry_memdev->length > conf_mem_clk_speed_offset) {
 			dmi_mem_clk_speed =
-				memdev_dmi_entry->conf_mem_clk_speed;
-		} else if (memdev_dmi_entry->length > speed_offset) {
-			dmi_mem_clk_speed = memdev_dmi_entry->speed;
+				dmi_entry_memdev->conf_mem_clk_speed;
+		} else if (dmi_entry_memdev->length > speed_offset) {
+			dmi_mem_clk_speed = dmi_entry_memdev->speed;
 		} else {
 			*dclk_freq = -1;
 			return;
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index 5e9c74c..60dcc31 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -78,6 +78,31 @@ struct dmi_header {
 	u16 handle;
 } __packed;
 
+struct dmi_entry_memdev {
+	u8 type;
+	u8 length;
+	u16 handle;
+	u16 phys_mem_array_handle;
+	u16 mem_err_info_handle;
+	u16 total_width;
+	u16 data_width;
+	u16 size;
+	u8 form;
+	u8 device_set;
+	u8 device_locator;
+	u8 bank_locator;
+	u8 memory_type;
+	u16 type_detail;
+	u16 speed;
+	u8 manufacturer;
+	u8 serial_number;
+	u8 asset_tag;
+	u8 part_number;
+	u8 attributes;
+	u32 extended_size;
+	u16 conf_mem_clk_speed;
+} __packed;
+
 struct dmi_device {
 	struct list_head list;
 	int type;
-- 
2.1.4

Re: [PATCH 6/8] dmi: Move memdev_dmi_entry definition to dmi.h (v2)

[Jean Delvare]

Hi Matt,

On Tue,  8 Mar 2016 10:32:37 -0800, Matt Roper wrote:
> A couple of the EDAC drivers have a nice memdev_dmi_entry structure for
> decoding DMI memory device entries.  Move the structure definition to
> dmi.h so that it can be shared between those drivers and also other
> parts of the kernel; the i915 graphics driver is going to need to use
> this structure soon as well.  As part of this move we rename the
> structure s/memdev_dmi_entry/dmi_entry_memdev/ to ensure it has a proper
> 'dmi' prefix.
> 
> v2:
>  - Rename structure to dmi_entry_memdev.  (Jean)
>  - Use __packed instead of __attribute__((__packed__)) for consistency
>    with the rest of the dmi.h header.  (Jean)

Looks better. One more comment inline below:

> (...)
> diff --git a/drivers/edac/i7core_edac.c b/drivers/edac/i7core_edac.c
> index 01087a3..fbfb06f 100644
> --- a/drivers/edac/i7core_edac.c
> +++ b/drivers/edac/i7core_edac.c
> (...)
> @@ -1946,28 +1921,28 @@ static void decode_dclk(const struct dmi_header *dh, void *_dclk_freq)
>  		return;
>  
>  	if (dh->type == DMI_ENTRY_MEM_DEVICE) {
> -		struct memdev_dmi_entry *memdev_dmi_entry =
> -			(struct memdev_dmi_entry *)dh;
> +		struct dmi_entry_memdev *dmi_entry_memdev =
> +			(struct dmi_entry_memdev *)dh;
>  		unsigned long conf_mem_clk_speed_offset =
> -			(unsigned long)&memdev_dmi_entry->conf_mem_clk_speed -
> -			(unsigned long)&memdev_dmi_entry->type;
> +			(unsigned long)&dmi_entry_memdev->conf_mem_clk_speed -
> +			(unsigned long)&dmi_entry_memdev->type;
>  		unsigned long speed_offset =
> -			(unsigned long)&memdev_dmi_entry->speed -
> -			(unsigned long)&memdev_dmi_entry->type;
> +			(unsigned long)&dmi_entry_memdev->speed -
> +			(unsigned long)&dmi_entry_memdev->type;
>  
>  		/* Check that a DIMM is present */
> -		if (memdev_dmi_entry->size == 0)
> +		if (dmi_entry_memdev->size == 0)
>  			return;
>  
>  		/*
>  		 * Pick the configured speed if it's available, otherwise
>  		 * pick the DIMM speed, or we don't have a speed.
>  		 */
> -		if (memdev_dmi_entry->length > conf_mem_clk_speed_offset) {
> +		if (dmi_entry_memdev->length > conf_mem_clk_speed_offset) {
>  			dmi_mem_clk_speed =
> -				memdev_dmi_entry->conf_mem_clk_speed;
> -		} else if (memdev_dmi_entry->length > speed_offset) {
> -			dmi_mem_clk_speed = memdev_dmi_entry->speed;
> +				dmi_entry_memdev->conf_mem_clk_speed;
> +		} else if (dmi_entry_memdev->length > speed_offset) {
> +			dmi_mem_clk_speed = dmi_entry_memdev->speed;
>  		} else {
>  			*dclk_freq = -1;
>  			return;

You do not need all of these changes. memdev_dmi_entry was both the
structure name and the variable name in the original code (something I
usually avoid, but C allows it.) Just because you renamed the structure
doesn't mean you have to rename the variable.

Other than that, looks good to me.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 6/8] dmi: Move memdev_dmi_entry definition to dmi.h (v2)

[Jean Delvare]

Hi Matt, Mauro,

On Thu, 17 Mar 2016 15:18:20 +0100, Jean Delvare wrote:
> On Tue,  8 Mar 2016 10:32:37 -0800, Matt Roper wrote:
> > A couple of the EDAC drivers have a nice memdev_dmi_entry structure for
> > decoding DMI memory device entries.  Move the structure definition to
> > dmi.h so that it can be shared between those drivers and also other
> > parts of the kernel; the i915 graphics driver is going to need to use
> > this structure soon as well.  As part of this move we rename the
> > structure s/memdev_dmi_entry/dmi_entry_memdev/ to ensure it has a proper
> > 'dmi' prefix.
> > 
> > v2:
> >  - Rename structure to dmi_entry_memdev.  (Jean)
> >  - Use __packed instead of __attribute__((__packed__)) for consistency
> >    with the rest of the dmi.h header.  (Jean)  
> 
> Looks better. (...)

What happened to this patch? I never received v3. Is it sill needed?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 6/8] dmi: Move memdev_dmi_entry definition to dmi.h (v2)

[Matt Roper]

On Mon, Jul 31, 2017 at 10:36:05AM +0200, Jean Delvare wrote:
> Hi Matt, Mauro,
> 
> On Thu, 17 Mar 2016 15:18:20 +0100, Jean Delvare wrote:
> > On Tue,  8 Mar 2016 10:32:37 -0800, Matt Roper wrote:
> > > A couple of the EDAC drivers have a nice memdev_dmi_entry structure for
> > > decoding DMI memory device entries.  Move the structure definition to
> > > dmi.h so that it can be shared between those drivers and also other
> > > parts of the kernel; the i915 graphics driver is going to need to use
> > > this structure soon as well.  As part of this move we rename the
> > > structure s/memdev_dmi_entry/dmi_entry_memdev/ to ensure it has a proper
> > > 'dmi' prefix.
> > > 
> > > v2:
> > >  - Rename structure to dmi_entry_memdev.  (Jean)
> > >  - Use __packed instead of __attribute__((__packed__)) for consistency
> > >    with the rest of the dmi.h header.  (Jean)  
> > 
> > Looks better. (...)
> 
> What happened to this patch? I never received v3. Is it sill needed?

We ended up going a different direction in the graphics driver and wound
up not needing access to this structure.  If there's still interest in
the general refactoring here, let me know and I can incorporate your
last feedback and respin a v3.


Matt

> -- 
> Jean Delvare
> SUSE L3 Support

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

Re: [PATCH 6/8] dmi: Move memdev_dmi_entry definition to dmi.h (v2)

[Jean Delvare]

On mer., 2017-08-09 at 09:18 -0700, Matt Roper wrote:
> On Mon, Jul 31, 2017 at 10:36:05AM +0200, Jean Delvare wrote:
> > 
> > Hi Matt, Mauro,
> > 
> > On Thu, 17 Mar 2016 15:18:20 +0100, Jean Delvare wrote:
> > > 
> > > On Tue,  8 Mar 2016 10:32:37 -0800, Matt Roper wrote:
> > > > 
> > > > A couple of the EDAC drivers have a nice memdev_dmi_entry structure for
> > > > decoding DMI memory device entries.  Move the structure definition to
> > > > dmi.h so that it can be shared between those drivers and also other
> > > > parts of the kernel; the i915 graphics driver is going to need to use
> > > > this structure soon as well.  As part of this move we rename the
> > > > structure s/memdev_dmi_entry/dmi_entry_memdev/ to ensure it has a proper
> > > > 'dmi' prefix.
> > > > 
> > > > v2:
> > > >  - Rename structure to dmi_entry_memdev.  (Jean)
> > > >  - Use __packed instead of __attribute__((__packed__)) for consistency
> > > >    with the rest of the dmi.h header.  (Jean)  
> > > 
> > > Looks better. (...)
> > 
> > What happened to this patch? I never received v3. Is it sill needed?
> 
> We ended up going a different direction in the graphics driver and wound
> up not needing access to this structure.  If there's still interest in
> the general refactoring here, let me know and I can incorporate your
> last feedback and respin a v3.

No, if you don't need it anymore I'll just drop it.

Thanks,
-- 
Jean Delvare
SUSE L3 Support