LinuxLists.cc - [PATCH 6/8] dmi: Move memdev_dmi

2016-03-08 01:07:45

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

2016-03-08 12:39:28

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

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 <[email protected]>
> Cc: Jean Delvare <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Matt Roper <[email protected]>
> ---
> 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

2016-03-08 18:33:29

by Matt Roper

[permalink] [raw]

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

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 <[email protected]>
Cc: Jean Delvare <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Matt Roper <[email protected]>
---
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

2016-03-17 14:18:27

by Jean Delvare

[permalink] [raw]

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

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 <[email protected]>

--
Jean Delvare
SUSE L3 Support

2017-07-31 08:36:10

by Jean Delvare

[permalink] [raw]

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

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

2017-08-09 16:18:51

by Matt Roper

[permalink] [raw]

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

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

2017-08-10 09:39:22

by Jean Delvare

[permalink] [raw]

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

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