linux-kernel - Re: [PATCH 6/8] dmi: Move memdev_dmi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160308133713.24e0b71a@endymion>
Date:	Tue, 8 Mar 2016 13:37:13 +0100
From:	Jean Delvare <jdelvare@...e.de>
To:	Matt Roper <matthew.d.roper@...el.com>
Cc:	intel-gfx@...ts.freedesktop.org, mahesh1.kumar@...el.com,
	shobhit.kumar@...el.com,
	Mauro Carvalho Chehab <mchehab@....samsung.com>,
	Jean Delvare <jdelvare@...e.com>, linux-edac@...r.kernel.org,
	linux-kernel@...r.kernel.org
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 <mchehab@....samsung.com>
> Cc: Jean Delvare <jdelvare@...e.com>
> Cc: linux-edac@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> Signed-off-by: Matt Roper <matthew.d.roper@...el.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