linux-kernel - Re: [EDAC PATCH v13 6/7] edac.h: Prepare to handle with generic layers

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: <20120424104059.GA11559@aftab.osrc.amd.com>
Date:	Tue, 24 Apr 2012 12:40:59 +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>,
	Doug Thompson <norsk5@...oo.com>
Subject: Re: [EDAC PATCH v13 6/7] edac.h: Prepare to handle with generic
 layers

On Mon, Apr 23, 2012 at 06:30:54PM +0000, Mauro Carvalho Chehab wrote:
> >> +};
> >> +
> >> +/**
> >> + * struct edac_mc_layer - describes the memory controller hierarchy
> >> + * @layer:		layer type
> >> + * @size:maximum size of the layer
> >> + * @is_csrow:		This layer is part of the "csrow" when old API
> >> + *			compatibility mode is enabled. Otherwise, it is
> >> + *			a channel
> >> + */
> >> +struct edac_mc_layer {
> >> +	enum edac_mc_layer_type	type;
> >> +	unsigned		size;
> >> +	bool			is_csrow;
> >> +};
> > 
> > Huh, why do you need is_csrow? Can't do
> > 
> > 	type = EDAC_MC_LAYER_CHIP_SELECT;
> > 
> > ?
> 
> No, that's different. For a csrow-based memory controller, is_csrow is equal to
> type == EDAC_MC_LAYER_CHIP_SELECT, but, for the other memory controllers, this
> is used to mark with layers will be used for the "fake csrow" exported by the
> EDAC core by the legacy API.

I don't understand this, do you mean: "this will be used to mark which
layer will be used to fake a csrow"...?

[..]

> With regards to the changes at edac_mc_sysfs,  it will likely affect all per-dimm
> routines, plus the counters reset logic. The problem of pointing to a set of
> routines that need changes is that this list can/will change with time.
> 
> So, the intention behind this note is not to give an exhaustive list of what should
> be changed, if EDAC_MAX_LAYERS is incremented. Instead, it is meant to give a
> clue that incrementing the number of layers is not as easy as just changing
> it: it would require to change the number of layers also at the code.

Then write that instead of adding a clueless note which only confuses readers.

> 
> > 
> >> + */
> >> +#define EDAC_MAX_LAYERS		3
> >> +
> >> +/*
> >> + * A loop could be used here to make it more generic, but, as we only have
> >> + * 3 layers, this is a little faster. By design, layers can never be 0 or
> >> + * more than 3. If that ever happens, a NULL is returned, causing an OOPS
> >> + * during the memory allocation routine, with would point to the developer
> >> + * that he's doing something wrong.
> >> + */
> >> +#define GET_POS(layers, var, nlayers, lay0, lay1, lay2) ({		\
> > 
> > This is returning size per layers so it cannot be GET_POS(), AFAICT.
> > EDAC_GET_SIZE or similar maybe?
> 
> This is not returning the size, per layers. It is returning a pointer to the
> structure that holds the dimm.
> 
> > 
> >> +	typeof(var) __p;						\
> >> +	if ((nlayers) == 1)						\
> >> +		__p = &var[lay0];					\
> >> +	else if ((nlayers) == 2)					\
> >> +		__p = &var[(lay1) + ((layers[1]).size * (lay0))];	\
> >> +	else if ((nlayers) == 3)					\
> >> +		__p = &var[(lay2) + ((layers[2]).size * ((lay1) +	\
> >> +			    ((layers[1]).size * (lay0))))];		\
> >> +	else								\
> >> +		__p = NULL;						\
> >> +	__p;								\
> >> +})

Ok, I'm looking at your next patch trying to understand this thing:

+	/*
+	 * Fills the dimm struct
+	 */
+	memset(&pos, 0, sizeof(pos));
+	row = 0;
+	chn = 0;
+	debugf4("%s: initializing %d dimms\n", __func__, tot_dimms);
+	for (i = 0; i < tot_dimms; i++) {
+		chan = &csi[row].channels[chn];
+		dimm = GET_POS(lay, mci->dimms, n_layers,
+			       pos[0], pos[1], pos[2]);

pos is an unsigned[3] array with all its elements set to 0 in the memset
above. Which means I need a run-variable like that all the time whenever
I iterate over the layers.

Now, say nlayers == 3, then your macro does this:

	__p = &var[(lay2) + ((layers[2]).size * ((lay1) + ((layers[1]).size * (lay0))))];

So I'm multiplying a loop variable with layers[i].size which is the
maximum size of the layer. What does that mean, where is this size
initialized?

I can imagine that I'll get an element in the mci->dimms array in the
end but this is very confusing.

So please explain what each argument of this macro exactly means.

-- 
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/