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

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-next>] [day] [month] [year] [list]

Message-ID: <20120507133204.GC11462@aftab.osrc.amd.com>
Date:	Mon, 7 May 2012 15:32:04 +0200
From:	Borislav Petkov <bp@...64.org>
To:	Mauro Carvalho Chehab <mchehab@...hat.com>
Cc:	Borislav Petkov <bp@...64.org>,
	Linux Edac Mailing List <linux-edac@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Aristeu Rozanski <arozansk@...hat.com>,
	Doug Thompson <norsk5@...oo.com>,
	Mark Gross <mark.gross@...el.com>,
	Jason Uhlenkott <juhlenko@...mai.com>,
	Tim Small <tim@...tersideup.com>,
	Ranganathan Desikan <ravi@...ztechnologies.com>,
	"Arvind R." <arvino55@...il.com>, Olof Johansson <olof@...om.net>,
	Egor Martovetsky <egor@...emi.com>,
	Chris Metcalf <cmetcalf@...era.com>,
	Michal Marek <mmarek@...e.cz>, Jiri Kosina <jkosina@...e.cz>,
	Joe Perches <joe@...ches.com>,
	Dmitry Eremin-Solenikov <dbaryshkov@...il.com>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Hitoshi Mitake <h.mitake@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Niklas Söderlund 
	<niklas.soderlund@...csson.com>,
	Shaohui Xie <Shaohui.Xie@...escale.com>,
	Josh Boyer <jwboyer@...il.com>, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH EDACv16 1/2] edac: Change internal representation to work
 with layers, second version

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/