Linux-EDAC Archive on lore.kernel.org
 help / color / Atom feed
From: James Morse <james.morse@arm.com>
To: Robert Richter <rrichter@marvell.com>
Cc: Borislav Petkov <bp@alien8.de>, Tony Luck <tony.luck@intel.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	"linux-edac@vger.kernel.org" <linux-edac@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 13/21] EDAC, ghes: Rework memory hierarchy detection
Date: Wed, 29 May 2019 16:06:55 +0100
Message-ID: <068e31b4-43e8-c1ab-194e-f2b41c1534f9@arm.com> (raw)
In-Reply-To: <20190529084344.28562-14-rrichter@marvell.com>

Hi Robert,

On 29/05/2019 09:44, Robert Richter wrote:
> In a later patch we want add more information about the memory
> hierarchy (NUMA topology, DIMM label information). Rework memory
> hierarchy detection to make the code extendable for this.
> 
> The general approach is roughly like:
> 
> 	mem_info_setup();
> 	for_each_node(nid) {
> 		mci = edac_mc_alloc(nid);
> 		mci_add_dimm_info(mci);
> 		edac_mc_add_mc(mci);
> 	};
> 
> This patch introduces mem_info_setup() and mci_add_dimm_info().
> 
> All data of the memory hierarchy is collected in a local struct
> ghes_mem_info.
> 
> Note: Per (NUMA) node registration will be implemented in a later
> patch.


> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> index ea4d53043199..50f4ee36b755 100644
> --- a/drivers/edac/ghes_edac.c
> +++ b/drivers/edac/ghes_edac.c
> @@ -67,17 +67,38 @@ struct memdev_dmi_entry {
>  	u16 conf_mem_clk_speed;
>  } __attribute__((__packed__));
>  
> -struct ghes_edac_dimm_fill {
> -	struct mem_ctl_info *mci;
> -	unsigned count;

> +struct ghes_dimm_info {
> +	struct dimm_info dimm_info;
> +	int		idx;
> +};

> +struct ghes_mem_info {
> +	int num_dimm;
> +	struct ghes_dimm_info *dimms;
>  };
>  
> +struct ghes_mem_info mem_info;

static?


> @@ -94,18 +115,17 @@ static int get_dimm_smbios_index(u16 handle)
>  
>  static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg)
>  {
> -	struct ghes_edac_dimm_fill *dimm_fill = arg;
> -	struct mem_ctl_info *mci = dimm_fill->mci;
> -
>  	if (dh->type == DMI_ENTRY_MEM_DEVICE) {
> +		int *idx = arg;
>  		struct memdev_dmi_entry *entry = (struct memdev_dmi_entry *)dh;
> -		struct dimm_info *dimm = edac_get_dimm(mci, dimm_fill->count,
> -						       0, 0);
> +		struct ghes_dimm_info *mi = &mem_info.dimms[*idx];
> +		struct dimm_info *dimm = &mi->dimm_info;
>  		u16 rdr_mask = BIT(7) | BIT(13);


> +		mi->phys_handle = entry->phys_mem_array_handle;

Where did this come from, and what is it for?

...

Should this be in a later patch? (did you bisect build this?)


>  		if (entry->size == 0xffff) {
> -			pr_info("Can't get DIMM%i size\n",
> -				dimm_fill->count);
> +			pr_info("Can't get DIMM%i size\n", mi->idx);
>  			dimm->nr_pages = MiB_TO_PAGES(32);/* Unknown */
>  		} else if (entry->size == 0x7fff) {
>  			dimm->nr_pages = MiB_TO_PAGES(entry->extended_size);


> +static int mem_info_setup(void)
> +{
> +	int idx = 0;
> +
> +	memset(&mem_info, 0, sizeof(mem_info));

Is this necessary? Isn't mem_info in the BSS, it will zero'd already.


> +	/* Get the number of DIMMs */
> +	dmi_walk(ghes_edac_count_dimms, NULL);
> +	if (!mem_info.num_dimm)
> +		return -EINVAL;

> +	mem_info.dimms = kcalloc(mem_info.num_dimm,
> +				sizeof(*mem_info.dimms), GFP_KERNEL);
> +	if (!mem_info.dimms)
> +		return -ENOMEM;

> +	ghes_dimm_info_init();

Could you move the kcalloc() into ghes_dimm_info_init()? This would save you having a
unnecessarily-different version in mem_info_setup_fake().


> +	dmi_walk(ghes_edac_dmidecode, &idx);
> +
> +	return 0;
> +}

> +static int mem_info_setup_fake(void)
> +{
> +	struct ghes_dimm_info *ghes_dimm;
> +	struct dimm_info *dimm;
> +
> +	memset(&mem_info, 0, sizeof(mem_info));

Is this necessary? Its only been touched by mem_info_setup(), and you get in here because
mem_info.num_dimm == 0...


> +	ghes_dimm = kzalloc(sizeof(*mem_info.dimms), GFP_KERNEL);
> +	if (!ghes_dimm)
> +		return -ENOMEM;

This is common with mem_info_setup(). If ghes_dimm_info_init() read mem_info.num_dimm and
did the rest, you'd avoid some duplication here.


> +	mem_info.num_dimm = 1;
> +	mem_info.dimms = ghes_dimm;
> +
> +	ghes_dimm_info_init();
> +
> +	dimm = &ghes_dimm->dimm_info;
> +	dimm->nr_pages = 1;
> +	dimm->grain = 128;
> +	dimm->mtype = MEM_UNKNOWN;
> +	dimm->dtype = DEV_UNKNOWN;
> +	dimm->edac_mode = EDAC_SECDED;
> +
> +	return 0;
> +}


> +static void mci_add_dimm_info(struct mem_ctl_info *mci)

(From the name I expected this to be in edac_mc.c)


> +{
> +	struct dimm_info *mci_dimm, *dmi_dimm;
> +	struct ghes_dimm_info *dimm;
> +	int index = 0;
> +
> +	for_each_dimm(dimm) {
> +		dmi_dimm = &dimm->dimm_info;
> +		mci_dimm = edac_get_dimm_by_index(mci, index);
> +
> +		index++;
> +		if (index > mci->tot_dimms)
> +			break;
> +
> +		mci_dimm->nr_pages	= dmi_dimm->nr_pages;
> +		mci_dimm->mtype		= dmi_dimm->mtype;
> +		mci_dimm->edac_mode	= dmi_dimm->edac_mode;
> +		mci_dimm->dtype		= dmi_dimm->dtype;
> +		mci_dimm->grain		= dmi_dimm->grain;
> +		mci_dimm->smbios_handle = dmi_dimm->smbios_handle;
>  	}

This isn't fun. I guess 'numa' is the reason for generating a shadow copy of all this, and
then having to copy it into edac. But surely that isn't a problem unique to ghes_edac.c?

Can't you add the nid, and any other properties to struct dimm_info? It already has
smbios_handle, which is hardly useful to other drivers!


> +	if (index != mci->tot_dimms)
> +		pr_warn("Unexpected number of DIMMs: %d (exp. %d)\n",
> +			index, mci->tot_dimms);
>  }


> @@ -472,22 +566,24 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)

>  	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, sizeof(struct ghes_edac_pvt));
>  	if (!mci) {
> -		pr_info("Can't allocate memory for EDAC data\n");
> +		pr_err("Can't allocate memory for EDAC data\n");

Leftover debug?

		kfree(mem_info.dimms); ?

>  		return -ENOMEM;
>  	}
>  
> @@ -513,26 +609,14 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)

> -	if (!fake) {
> -		dimm_fill.count = 0;
> -		dimm_fill.mci = mci;
> -		dmi_walk(ghes_edac_dmidecode, &dimm_fill);
> -	} else {
> -		struct dimm_info *dimm = edac_get_dimm(mci, 0, 0, 0);
> -
> -		dimm->nr_pages = 1;
> -		dimm->grain = 128;
> -		dimm->mtype = MEM_UNKNOWN;
> -		dimm->dtype = DEV_UNKNOWN;
> -		dimm->edac_mode = EDAC_SECDED;
> -	}
> +	mci_add_dimm_info(mci);
>  
>  	rc = edac_mc_add_mc(mci);
>  	if (rc < 0) {
> -		pr_info("Can't register at EDAC core\n");
> +		pr_err("Can't register at EDAC core\n");

Leftover debug?

>  		edac_mc_free(mci);

		kfree(mem_info.dimms); ?

>  		return -ENODEV;
>  	}


Thanks!

James

  reply index

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-29  8:44 [PATCH 00/21] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Robert Richter
2019-05-29  8:44 ` [PATCH 01/21] EDAC, mc: Fix edac_mc_find() in case no device is found Robert Richter
2019-05-29  8:44 ` [PATCH 02/21] EDAC: Fixes to use put_device() after device_add() errors Robert Richter
2019-06-11 17:28   ` Borislav Petkov
2019-06-12 17:17     ` Robert Richter
2019-05-29  8:44 ` [PATCH 03/21] EDAC: Kill EDAC_DIMM_PTR() macro Robert Richter
2019-05-29  8:44 ` [PATCH 04/21] EDAC: Kill EDAC_DIMM_OFF() macro Robert Richter
2019-05-29  8:44 ` [PATCH 05/21] EDAC: Introduce mci_for_each_dimm() iterator Robert Richter
2019-05-29  8:44 ` [PATCH 06/21] EDAC, mc: Cleanup _edac_mc_free() code Robert Richter
2019-05-29  8:44 ` [PATCH 07/21] EDAC, mc: Remove per layer counters Robert Richter
2019-05-29  8:44 ` [PATCH 08/21] EDAC, mc: Rework edac_raw_mc_handle_error() to use struct dimm_info Robert Richter
2019-05-29  8:44 ` [PATCH 09/21] EDAC, ghes: Use standard kernel macros for page calculations Robert Richter
2019-05-29 15:13   ` James Morse
2019-05-29  8:44 ` [PATCH 10/21] EDAC, ghes: Remove pvt->detail_location string Robert Richter
2019-05-29 15:13   ` James Morse
2019-06-12 18:13     ` Robert Richter
2019-05-29  8:44 ` [PATCH 11/21] EDAC, ghes: Unify trace_mc_event() code with edac_mc driver Robert Richter
2019-05-29 15:12   ` James Morse
2019-06-03 13:10     ` Robert Richter
2019-06-04 17:15       ` James Morse
2019-06-13 22:23         ` Robert Richter
2019-05-29  8:44 ` [PATCH 12/21] EDAC, ghes: Add support for legacy API counters Robert Richter
2019-05-29 15:13   ` James Morse
2019-06-12 18:41     ` Robert Richter
2019-06-19 17:22       ` James Morse
2019-06-20  6:55         ` Robert Richter
2019-06-26  9:33           ` James Morse
2019-06-26 10:27             ` Robert Richter
2019-05-29  8:44 ` [PATCH 13/21] EDAC, ghes: Rework memory hierarchy detection Robert Richter
2019-05-29 15:06   ` James Morse [this message]
2019-05-31 13:41     ` Robert Richter
2019-05-29  8:44 ` [PATCH 14/21] EDAC, ghes: Extract numa node information for each dimm Robert Richter
2019-05-29 17:51   ` James Morse
2019-06-13 20:52     ` Robert Richter
2019-05-29  8:44 ` [PATCH 15/21] EDAC, ghes: Moving code around ghes_edac_register() Robert Richter
2019-05-29  8:44 ` [PATCH 16/21] EDAC, ghes: Create one memory controller device per node Robert Richter
2019-05-29  8:44 ` [PATCH 17/21] EDAC, ghes: Fill sysfs with the DMI DIMM label information Robert Richter
2019-05-29  8:44 ` [PATCH 18/21] EDAC, mc: Introduce edac_mc_alloc_by_dimm() for per dimm allocation Robert Richter
2019-05-29  8:44 ` [PATCH 19/21] EDAC, ghes: Identify dimm by node, card, module and handle Robert Richter
2019-05-29  8:44 ` [PATCH 20/21] EDAC, ghes: Enable per-layer reporting based on card/module Robert Richter
2019-05-29  8:44 ` [PATCH 21/21] EDAC, Documentation: Describe CPER module definition and DIMM ranks Robert Richter
2019-05-29 14:54 ` [PATCH 00/21] EDAC, mc, ghes: Fixes and updates to improve memory error reporting Borislav Petkov
2019-05-31 14:48   ` Robert Richter

Reply instructions:

You may reply publically 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=068e31b4-43e8-c1ab-194e-f2b41c1534f9@arm.com \
    --to=james.morse@arm.com \
    --cc=bp@alien8.de \
    --cc=linux-edac@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=rrichter@marvell.com \
    --cc=tony.luck@intel.com \
    /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

Linux-EDAC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-edac/0 linux-edac/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-edac linux-edac/ https://lore.kernel.org/linux-edac \
		linux-edac@vger.kernel.org linux-edac@archiver.kernel.org
	public-inbox-index linux-edac


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-edac


AGPL code for this site: git clone https://public-inbox.org/ public-inbox