Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers, second version

[Borislav Petkov <bp@amd64.org>]

breaking thread because it grew too big.

On Fri, May 04, 2012 at 07:48:42AM -0300, Mauro Carvalho Chehab wrote:

[ … ]

> +	memset(&pos, 0, sizeof(pos));
> +	row = 0;
> +	chn = 0;
> +	debugf4("%s: initializing %d %s\n", __func__, tot_dimms,
> +		per_rank ? "ranks" : "dimms");
> +	for (i = 0; i < tot_dimms; i++) {
> +		chan = &csi[row].channels[chn];
> +		dimm = EDAC_DIMM_PTR(layer, mci->dimms, n_layers,
> +			       pos[0], pos[1], pos[2]);
> +		dimm->mci = mci;
> +
> +		debugf2("%s: %d: %s%zd (%d:%d:%d): row %d, chan %d\n", __func__,
> +			i, per_rank ? "rank" : "dimm", (dimm - mci->dimms),
> +			pos[0], pos[1], pos[2], row, chn);
> +
> +		/* Copy DIMM location */
> +		for (j = 0; j < n_layers; j++)
> +			dimm->location[j] = pos[j];
> +
> +		/* Link it to the csrows old API data */
> +		chan->dimm = dimm;
> +		dimm->csrow = row;
> +		dimm->cschannel = chn;
> +
> +		/* Increment csrow location */
> +		for (j = n_layers - 1; j >= 0; j--)
> +			if (layers[j].is_virt_csrow)
> +				break;

This looks fishy: the for-loop above iterates over layers[j] to break on
the first ->is_virt_csrow.

> +		row++;

And row here gets incremented unconditionally, independent from the loop
above. And you're not using any results from the loop: j gets reset
below in the next loop.

What's going on?

> +		if (row == tot_csrows) {
> +			row = 0;
> +			chn++;
> +		}
> +
> +		/* Increment dimm location */
> +		for (j = n_layers - 1; j >= 0; j--) {
> +			pos[j]++;
> +			if (pos[j] < layers[j].size)
> +				break;
> +			pos[j] = 0;
>  		}
>  	}
>  
> @@ -263,6 +373,46 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
>  	 */
>  	return mci;
>  }
> +EXPORT_SYMBOL_GPL(new_edac_mc_alloc);
> +
> +/**
> + * edac_mc_alloc: Allocate and partially fill a struct mem_ctl_info structure
> + * @mc_num:		Memory controller number
> + * @n_layers:		Number of layers at the MC hierarchy
> + * layers:		Describes each layer as seen by the Memory Controller
> + * @size_pvt:		Size of private storage needed
> + *
> + *
> + * FIXME: drivers handle multi-rank memories in different ways: some
> + * drivers map multi-ranked DIMMs as one DIMM while others
> + * as several DIMMs.
> + *
> + * Everything is kmalloc'ed as one big chunk - more efficient.
> + * It can only be used if all structures have the same lifetime - otherwise
> + * you have to allocate and initialize your own structures.
> + *
> + * Use edac_mc_free() to free mc structures allocated by this function.
> + *
> + * Returns:
> + *	On failure: NULL
> + *	On success: struct mem_ctl_info pointer
> + */
> +

[ … ]

> +void edac_mc_handle_error(const enum hw_event_mc_err_type type,
> +			  struct mem_ctl_info *mci,
> +			  const unsigned long page_frame_number,
> +			  const unsigned long offset_in_page,
> +			  const unsigned long syndrome,
> +			  const int layer0,
> +			  const int layer1,
> +			  const int layer2,
> +			  const char *msg,
> +			  const char *other_detail,
> +			  const void *mcelog)
>  {
> -	int len = EDAC_MC_LABEL_LEN * 4;
> -	char labels[len + 1];
> -	char *pos = labels;
> -	int chan;
> -	int chars;
> -	char *label = NULL;
> +	/* FIXME: too much for stack: move it to some pre-alocated area */

I'm assuming all those new FIXMEs are going to be addressed soonish :)

Rest looks ok,
thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551