Linux-EDAC Archive on lore.kernel.org
 help / color / Atom feed
From: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
To: Robert Richter <rrichter@marvell.com>
Cc: Borislav Petkov <bp@alien8.de>, Tony Luck <tony.luck@intel.com>,
	James Morse <james.morse@arm.com>,
	"linux-edac@vger.kernel.org" <linux-edac@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 08/19] EDAC, mc: Split edac_mc_alloc() into smaller functions
Date: Fri, 11 Oct 2019 07:43:42 -0300
Message-ID: <20191011074342.71e443a9@coco.lan> (raw)
In-Reply-To: <20191010202418.25098-9-rrichter@marvell.com>

Em Thu, 10 Oct 2019 20:25:20 +0000
Robert Richter <rrichter@marvell.com> escreveu:

> edac_mc_alloc() is huge. Factor out code by moving it to the two new
> functions edac_mc_alloc_csrows() and edac_mc_alloc_dimms(). Do not
> move code yet for better review.
> 
> Signed-off-by: Robert Richter <rrichter@marvell.com>

Reviewed-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

> ---
>  drivers/edac/edac_mc.c | 104 +++++++++++++++++++++++++++--------------
>  1 file changed, 69 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
> index a893f793be8a..0db504cb3419 100644
> --- a/drivers/edac/edac_mc.c
> +++ b/drivers/edac/edac_mc.c
> @@ -305,6 +305,9 @@ static void _edac_mc_free(struct mem_ctl_info *mci)
>  	kfree(mci);
>  }
>  
> +static int edac_mc_alloc_csrows(struct mem_ctl_info *mci);
> +static int edac_mc_alloc_dimms(struct mem_ctl_info *mci);
> +
>  struct mem_ctl_info *edac_mc_alloc(unsigned int mc_num,
>  				   unsigned int n_layers,
>  				   struct edac_mc_layer *layers,
> @@ -312,14 +315,9 @@ struct mem_ctl_info *edac_mc_alloc(unsigned int mc_num,
>  {
>  	struct mem_ctl_info *mci;
>  	struct edac_mc_layer *layer;
> -	struct csrow_info *csr;
> -	struct rank_info *chan;
> -	struct dimm_info *dimm;
> -	unsigned int pos[EDAC_MAX_LAYERS];
>  	unsigned int idx, size, tot_dimms = 1;
>  	unsigned int tot_csrows = 1, tot_channels = 1;
> -	void *pvt, *p, *ptr = NULL;
> -	int j, row, chn, n, len;
> +	void *pvt, *ptr = NULL;
>  	bool per_rank = false;
>  
>  	if (WARN_ON(n_layers > EDAC_MAX_LAYERS || n_layers == 0))
> @@ -377,16 +375,43 @@ struct mem_ctl_info *edac_mc_alloc(unsigned int mc_num,
>  	mci->num_cschannel = tot_channels;
>  	mci->csbased = per_rank;
>  
> +	if (edac_mc_alloc_csrows(mci))
> +		goto error;
> +
> +	if (edac_mc_alloc_dimms(mci))
> +		goto error;
> +
> +	mci->op_state = OP_ALLOC;
> +
> +	return mci;
> +
> +error:
> +	_edac_mc_free(mci);
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(edac_mc_alloc);
> +
> +static int edac_mc_alloc_csrows(struct mem_ctl_info *mci)
> +{
> +	unsigned int tot_csrows = mci->nr_csrows;
> +	unsigned int tot_channels = mci->num_cschannel;
> +	unsigned int row, chn;
> +
>  	/*
>  	 * Alocate and fill the csrow/channels structs
>  	 */
>  	mci->csrows = kcalloc(tot_csrows, sizeof(*mci->csrows), GFP_KERNEL);
>  	if (!mci->csrows)
> -		goto error;
> +		return -ENOMEM;
> +
>  	for (row = 0; row < tot_csrows; row++) {
> +		struct csrow_info *csr;
> +
>  		csr = kzalloc(sizeof(**mci->csrows), GFP_KERNEL);
>  		if (!csr)
> -			goto error;
> +			return -ENOMEM;
> +
>  		mci->csrows[row] = csr;
>  		csr->csrow_idx = row;
>  		csr->mci = mci;
> @@ -394,34 +419,51 @@ struct mem_ctl_info *edac_mc_alloc(unsigned int mc_num,
>  		csr->channels = kcalloc(tot_channels, sizeof(*csr->channels),
>  					GFP_KERNEL);
>  		if (!csr->channels)
> -			goto error;
> +			return -ENOMEM;
>  
>  		for (chn = 0; chn < tot_channels; chn++) {
> +			struct rank_info *chan;
> +
>  			chan = kzalloc(sizeof(**csr->channels), GFP_KERNEL);
>  			if (!chan)
> -				goto error;
> +				return -ENOMEM;
> +
>  			csr->channels[chn] = chan;
>  			chan->chan_idx = chn;
>  			chan->csrow = csr;
>  		}
>  	}
>  
> +	return 0;
> +}
> +
> +static int edac_mc_alloc_dimms(struct mem_ctl_info *mci)
> +{
> +	void *p;
> +	unsigned int pos[EDAC_MAX_LAYERS];
> +	unsigned int row, chn, idx;
> +	int layer;
> +
>  	/*
>  	 * Allocate and fill the dimm structs
>  	 */
> -	mci->dimms  = kcalloc(tot_dimms, sizeof(*mci->dimms), GFP_KERNEL);
> +	mci->dimms  = kcalloc(mci->tot_dimms, sizeof(*mci->dimms), GFP_KERNEL);
>  	if (!mci->dimms)
> -		goto error;
> +		return -ENOMEM;
>  
>  	memset(&pos, 0, sizeof(pos));
>  	row = 0;
>  	chn = 0;
> -	for (idx = 0; idx < tot_dimms; idx++) {
> +	for (idx = 0; idx < mci->tot_dimms; idx++) {
> +		struct dimm_info *dimm;
> +		struct rank_info *chan;
> +		int n, len;
> +
>  		chan = mci->csrows[row]->channels[chn];
>  
>  		dimm = kzalloc(sizeof(**mci->dimms), GFP_KERNEL);
>  		if (!dimm)
> -			goto error;
> +			return -ENOMEM;
>  		mci->dimms[idx] = dimm;
>  		dimm->mci = mci;
>  		dimm->idx = idx;
> @@ -431,16 +473,16 @@ struct mem_ctl_info *edac_mc_alloc(unsigned int mc_num,
>  		 */
>  		len = sizeof(dimm->label);
>  		p = dimm->label;
> -		n = snprintf(p, len, "mc#%u", mc_num);
> +		n = snprintf(p, len, "mc#%u", mci->mc_idx);
>  		p += n;
>  		len -= n;
> -		for (j = 0; j < n_layers; j++) {
> +		for (layer = 0; layer < mci->n_layers; layer++) {
>  			n = snprintf(p, len, "%s#%u",
> -				     edac_layer_name[layers[j].type],
> -				     pos[j]);
> +				     edac_layer_name[mci->layers[layer].type],
> +				     pos[layer]);
>  			p += n;
>  			len -= n;
> -			dimm->location[j] = pos[j];
> +			dimm->location[layer] = pos[layer];
>  
>  			if (len <= 0)
>  				break;
> @@ -452,39 +494,31 @@ struct mem_ctl_info *edac_mc_alloc(unsigned int mc_num,
>  		dimm->cschannel = chn;
>  
>  		/* Increment csrow location */
> -		if (layers[0].is_virt_csrow) {
> +		if (mci->layers[0].is_virt_csrow) {
>  			chn++;
> -			if (chn == tot_channels) {
> +			if (chn == mci->num_cschannel) {
>  				chn = 0;
>  				row++;
>  			}
>  		} else {
>  			row++;
> -			if (row == tot_csrows) {
> +			if (row == mci->nr_csrows) {
>  				row = 0;
>  				chn++;
>  			}
>  		}
>  
>  		/* Increment dimm location */
> -		for (j = n_layers - 1; j >= 0; j--) {
> -			pos[j]++;
> -			if (pos[j] < layers[j].size)
> +		for (layer = mci->n_layers - 1; layer >= 0; layer--) {
> +			pos[layer]++;
> +			if (pos[layer] < mci->layers[layer].size)
>  				break;
> -			pos[j] = 0;
> +			pos[layer] = 0;
>  		}
>  	}
>  
> -	mci->op_state = OP_ALLOC;
> -
> -	return mci;
> -
> -error:
> -	_edac_mc_free(mci);
> -
> -	return NULL;
> +	return 0;
>  }
> -EXPORT_SYMBOL_GPL(edac_mc_alloc);
>  
>  void edac_mc_free(struct mem_ctl_info *mci)
>  {



Thanks,
Mauro

  reply index

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-10 20:25 [PATCH 00/19] EDAC: Rework edac_mc and ghes drivers Robert Richter
2019-10-10 20:25 ` [PATCH 01/19] EDAC: Replace EDAC_DIMM_PTR() macro with edac_get_dimm() function Robert Richter
2019-10-11  9:58   ` Mauro Carvalho Chehab
2019-10-11 11:38     ` Robert Richter
2019-10-10 20:25 ` [PATCH 02/19] EDAC: Remove EDAC_DIMM_OFF() macro Robert Richter
2019-10-11 10:09   ` Mauro Carvalho Chehab
2019-10-11 11:36     ` Robert Richter
2019-10-10 20:25 ` [PATCH 03/19] EDAC: Introduce mci_for_each_dimm() iterator Robert Richter
2019-10-11 10:14   ` Mauro Carvalho Chehab
2019-10-10 20:25 ` [PATCH 04/19] EDAC, mc: Do not BUG_ON() in edac_mc_alloc() Robert Richter
2019-10-11 10:15   ` Mauro Carvalho Chehab
2019-10-10 20:25 ` [PATCH 05/19] EDAC, mc: Reduce indentation level in edac_mc_handle_error() Robert Richter
2019-10-10 22:10   ` Joe Perches
2019-10-11  6:50     ` Robert Richter
2019-10-11 10:20     ` Mauro Carvalho Chehab
2019-10-11 10:50       ` Joe Perches
2019-10-11 12:08         ` Robert Richter
2019-10-11 14:49           ` Joe Perches
2019-10-11 10:17   ` Mauro Carvalho Chehab
2019-10-10 20:25 ` [PATCH 06/19] EDAC, mc: Remove per layer counters Robert Richter
2019-10-11 10:40   ` Mauro Carvalho Chehab
2019-10-14 11:12     ` Robert Richter
2019-10-10 20:25 ` [PATCH 07/19] EDAC, mc: Rename iterator variable to idx Robert Richter
2019-10-11 10:41   ` Mauro Carvalho Chehab
2019-10-10 20:25 ` [PATCH 08/19] EDAC, mc: Split edac_mc_alloc() into smaller functions Robert Richter
2019-10-11 10:43   ` Mauro Carvalho Chehab [this message]
2019-10-10 20:25 ` [PATCH 09/19] EDAC, mc: Reorder functions edac_mc_alloc*() Robert Richter
2019-10-11 10:45   ` Mauro Carvalho Chehab
2019-10-10 20:25 ` [PATCH 10/19] EDAC, mc: Rework edac_raw_mc_handle_error() to use struct dimm_info Robert Richter
2019-10-11 10:48   ` Mauro Carvalho Chehab
2019-10-10 20:25 ` [PATCH 11/19] EDAC: Remove misleading comment in struct edac_raw_error_desc Robert Richter
2019-10-11 10:49   ` Mauro Carvalho Chehab
2019-10-10 20:25 ` [PATCH 12/19] EDAC: Store error type " Robert Richter
2019-10-11 10:54   ` Mauro Carvalho Chehab
2019-10-14 11:47     ` Robert Richter
2019-10-10 20:25 ` [PATCH 13/19] EDAC, mc: Determine mci pointer from the error descriptor Robert Richter
2019-10-11 10:56   ` Mauro Carvalho Chehab
2019-10-10 20:25 ` [PATCH 14/19] EDAC, mc: Create new function edac_inc_csrow() Robert Richter
2019-10-11 11:08   ` Mauro Carvalho Chehab
2019-10-14 11:58     ` Robert Richter
2019-10-10 20:25 ` [PATCH 15/19] EDAC, ghes: Use standard kernel macros for page calculations Robert Richter
2019-10-11 11:10   ` Mauro Carvalho Chehab
2019-10-10 20:25 ` [PATCH 16/19] EDAC, ghes: Fix grain calculation Robert Richter
2019-10-11 11:22   ` Mauro Carvalho Chehab
2019-10-10 20:25 ` [PATCH 17/19] EDAC, ghes: Remove intermediate buffer pvt->detail_location Robert Richter
2019-10-11 11:20   ` Mauro Carvalho Chehab
2019-10-10 20:25 ` [PATCH 18/19] EDAC, ghes: Unify trace_mc_event() code with edac_mc driver Robert Richter
2019-10-11 11:23   ` Mauro Carvalho Chehab
2019-10-10 20:25 ` [PATCH 19/19] EDAC, Documentation: Describe CPER module definition and DIMM ranks Robert Richter
2019-10-11 11:29   ` Mauro Carvalho Chehab
2019-10-10 20:36 ` [PATCH 00/19] EDAC: Rework edac_mc and ghes drivers Robert Richter
2019-10-14 12:00 ` 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=20191011074342.71e443a9@coco.lan \
    --to=mchehab+samsung@kernel.org \
    --cc=bp@alien8.de \
    --cc=james.morse@arm.com \
    --cc=linux-edac@vger.kernel.org \
    --cc=linux-kernel@vger.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
	public-inbox-index linux-edac

Example config snippet for mirrors

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.git