From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753030Ab2D3H7u (ORCPT ); Mon, 30 Apr 2012 03:59:50 -0400 Received: from s15943758.onlinehome-server.info ([217.160.130.188]:38989 "EHLO mail.x86-64.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751688Ab2D3H7t (ORCPT ); Mon, 30 Apr 2012 03:59:49 -0400 Date: Mon, 30 Apr 2012 09:59:40 +0200 From: Borislav Petkov To: Mauro Carvalho Chehab Cc: Linux Edac Mailing List , Linux Kernel Mailing List , Aristeu Rozanski , Doug Thompson , Mark Gross , Jason Uhlenkott , Tim Small , Ranganathan Desikan , "Arvind R." , Olof Johansson , Egor Martovetsky , Chris Metcalf , Michal Marek , Jiri Kosina , Joe Perches , Dmitry Eremin-Solenikov , Benjamin Herrenschmidt , Hitoshi Mitake , Andrew Morton , Niklas =?iso-8859-1?Q?S=F6derlund?= , Shaohui Xie , Josh Boyer , linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers Message-ID: <20120430075940.GC8182@aftab.osrc.amd.com> References: <1335289087-11337-1-git-send-email-mchehab@redhat.com> <1335291342-14922-1-git-send-email-mchehab@redhat.com> <20120427133304.GE9626@aftab.osrc.amd.com> <4F9ADCE3.2030506@redhat.com> <20120428091621.GE26065@aftab.osrc.amd.com> <4F9D4D55.9070705@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4F9D4D55.9070705@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Apr 29, 2012 at 11:16:53AM -0300, Mauro Carvalho Chehab wrote: > > Hey, are you looking at compiled code or at source code? Because I'm > > looking at source code, and it is a pretty safe bet the majority of the > > people here do that too. > > What I said is that, from source code POV, a code where the loop variables are > initialized just before the loop is easier to read it when the initialization > of those vars are on another part of the code. > > That's basically why the "for" syntax starts with a var initialization clause. > > The tot_dimms & friends are loop vars: their value is calculated within the loop. > > At the object code, this won't bring any difference. > > > > >> it, either by using registers for those vars or by moving the initialization > >> to the top of the function. > >> > >> This function is too complex, so it is better to initialize those vars > >> just before the loops that are calculating those totals. > > > > Simply initialize those variables at declaration time and that's it. > > Initializing them before the loop doesn't make the function less complex > > - splitting it and sanitizing it does. > > Initializing loop-calculated vars just before the loop makes the code easier > to read, and may avoid issues that might happen during code lifecycle. This is getting ridiculous: the variable declaration and initialization are on the same screen as the loop (unless one uses a screen which can only show less than 40ish lines). So the argument about making the code easier to read is bogus. This function is already cluttered with a lot of crap, and is very large so adding more lines which can simply be stashed away at declaration time is better readability. Besides, every modern editor can jump to the declaration of a local variable so that the user can see to what it is initialized to. > +struct mem_ctl_info *new_edac_mc_alloc(unsigned edac_index, > + unsigned n_layers, > + struct edac_mc_layer *layers, > + bool rev_order, > + unsigned sz_pvt) > { > void *ptr = NULL; > struct mem_ctl_info *mci; > - struct csrow_info *csi, *csrow; > + struct edac_mc_layer *layer; > + struct csrow_info *csi, *csr; > struct rank_info *chi, *chp, *chan; > struct dimm_info *dimm; > + u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS]; > void *pvt; > - unsigned size; > - int row, chn; > + unsigned size, tot_dimms, count, pos[EDAC_MAX_LAYERS]; > + unsigned tot_csrows, tot_channels, tot_errcount = 0; > + int i, j; > int err; > + int row, chn; > + bool per_rank = false; > + > + BUG_ON(n_layers > EDAC_MAX_LAYERS || n_layers == 0); > + /* > + * Calculate the total amount of dimms and csrows/cschannels while > + * in the old API emulation mode > + */ > + tot_dimms = 1; > + tot_channels = 1; > + tot_csrows = 1; > + for (i = 0; i < n_layers; i++) { > + tot_dimms *= layers[i].size; > + if (layers[i].is_virt_csrow) > + tot_csrows *= layers[i].size; > + else > + tot_channels *= layers[i].size; > + > + if (layers[i].type == EDAC_MC_LAYER_CHIP_SELECT) > + per_rank = true; -- 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