Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Robert Richter <rrichter@marvell.com>
To:     Borislav Petkov <bp@alien8.de>
CC:     James Morse <james.morse@arm.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        "linux-edac@vger.kernel.org" <linux-edac@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 08/24] EDAC: Introduce mci_for_each_dimm() iterator
Thread-Topic: [PATCH v2 08/24] EDAC: Introduce mci_for_each_dimm() iterator
Thread-Index: AQHVXXk8O9FaCqNi1UGTsoT9yxk98Q==
Date:   Wed, 28 Aug 2019 08:18:56 +0000
Message-ID: <20190828081849.xwbfgro6dfrrtdac@rric.localdomain>
References: <20190624150758.6695-1-rrichter@marvell.com>
 <20190624150758.6695-9-rrichter@marvell.com> <20190814151824.GB1841@zn.tnic>
In-Reply-To: <20190814151824.GB1841@zn.tnic>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-messagesentrepresentingtype: 1
x-originating-ip: [31.208.96.227]
x-ms-publictraffictype: Email
x-ms-office365-filtering-correlation-id: 175d6b4a-1ef7-425d-7023-08d72b905efc
x-microsoft-antispam: BCL:0;PCL:0;RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600166)(711020)(4605104)(1401327)(2017052603328)(7193020);SRVR:MN2PR18MB2479;
x-ms-traffictypediagnostic: MN2PR18MB2479:
x-microsoft-antispam-prvs: <MN2PR18MB2479A079E059831403C7996CD9A30@MN2PR18MB2479.namprd18.prod.outlook.com>
x-ms-oob-tlc-oobclassifiers: OLM:2887;
x-forefront-prvs: 014304E855
x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(4636009)(136003)(376002)(346002)(39860400002)(366004)(396003)(199004)(189003)(229853002)(6916009)(3846002)(81156014)(81166006)(6116002)(8676002)(476003)(486006)(9686003)(6512007)(8936002)(54906003)(66556008)(478600001)(316002)(71200400001)(66066001)(66446008)(66476007)(66946007)(64756008)(71190400001)(6436002)(99286004)(26005)(305945005)(76176011)(2906002)(4326008)(5660300002)(7736002)(53546011)(6506007)(14454004)(1076003)(386003)(6486002)(25786009)(11346002)(446003)(14444005)(102836004)(6246003)(186003)(53936002)(86362001)(52116002)(256004);DIR:OUT;SFP:1101;SCL:1;SRVR:MN2PR18MB2479;H:MN2PR18MB3408.namprd18.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;A:1;MX:1;
received-spf: None (protection.outlook.com: marvell.com does not designate
 permitted sender hosts)
x-ms-exchange-senderadcheck: 1
x-microsoft-antispam-message-info: WYKA7ujOZJUwFHf7iTYsaAEAowj9rhlc6n6JfxpqlF7ZFHEM6KqlukDfKsv3e7vmld+PFBPnI/K3HmW35q8SmXjexE2bWSTLHVrOWOeIOjxgvdx6pb3o9Kgs2q20lzuZL4UtEMcCmad73utnSpfoeS6dEze9IY9eHhInMUriSj4BVoZvmDY58fL7t8aDJnwomOzpu/asxMlZoB8BcubtYXzshH0Nz2LcOZOYDNyNv14P6Y6LoB5Wdzp6lVp0HrucoCkhECd7pt7PvAnO8Sp5JDxY+7P3/a7vVnOwqulN/Bh+MWZkkm0KEnlWhHnB4tAGrNe5cW8eU1rxEp5BFWM7z9dNVltpCh2nmb7iTipiBoknnQ4HQqMjygAceBWoOeYwGtGrZyovM0FnnKLE9K1pGB+gcg8+3puJrGiQwVfEDCA=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="us-ascii"
Content-ID: <4D7470063D0414459A3EBA9A3D923573@namprd18.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 175d6b4a-1ef7-425d-7023-08d72b905efc
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Aug 2019 08:18:57.1707
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 3iEyVCfrvTAc6PlI/TssNfAektTO+e4zZasTY92OAiQI5JqS6HOUMlHt6oETTXQmeWu9vu6Ccg90w5oiRn0L/g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR18MB2479
X-OriginatorOrg: marvell.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:5.22.84,1.0.8
 definitions=2019-08-28_02:2019-08-27,2019-08-28 signatures=0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 14.08.19 17:18:24, Borislav Petkov wrote:
> On Mon, Jun 24, 2019 at 03:09:11PM +0000, Robert Richter wrote:
> > Make code more readable by introducing a mci_for_each_dimm() iterator.
> > Now, we just get a pointer to a struct dimm_info. Direct array access
> > using an index is no longer needed to iterate.
> >=20
> > Signed-off-by: Robert Richter <rrichter@marvell.com>
> > ---
> >  drivers/edac/edac_mc.c       | 18 ++++++++++--------
> >  drivers/edac/edac_mc_sysfs.c | 34 +++++++++++++++-------------------
> >  drivers/edac/ghes_edac.c     |  8 ++++----
> >  drivers/edac/i5100_edac.c    | 11 +++++------
> >  include/linux/edac.h         |  7 +++++++
> >  5 files changed, 41 insertions(+), 37 deletions(-)
> >=20
> > diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
> > index c44bc83e8502..27277ca46ab3 100644
> > --- a/drivers/edac/edac_mc.c
> > +++ b/drivers/edac/edac_mc.c
> > @@ -145,15 +145,18 @@ static void edac_mc_dump_channel(struct rank_info=
 *chan)
> >  	edac_dbg(4, "    channel->dimm =3D %p\n", chan->dimm);
> >  }
> > =20
> > -static void edac_mc_dump_dimm(struct dimm_info *dimm, int number)
> > +static void edac_mc_dump_dimm(struct dimm_info *dimm)
> >  {
> >  	char location[80];
> > =20
> > +	if (!dimm->nr_pages)
> > +		return;
> > +
>=20
> What's that for?

This is moved from the iterator loop below to here. It limits the dump
to only populated dimm slots.

>=20
> >  	edac_dimm_info_location(dimm, location, sizeof(location));
> > =20
> >  	edac_dbg(4, "%s%i: %smapped as virtual row %d, chan %d\n",
> >  		 dimm->mci->csbased ? "rank" : "dimm",
> > -		 number, location, dimm->csrow, dimm->cschannel);
> > +		 dimm->idx, location, dimm->csrow, dimm->cschannel);
> >  	edac_dbg(4, "  dimm =3D %p\n", dimm);
> >  	edac_dbg(4, "  dimm->label =3D '%s'\n", dimm->label);
> >  	edac_dbg(4, "  dimm->nr_pages =3D 0x%x\n", dimm->nr_pages);

> > diff --git a/drivers/edac/i5100_edac.c b/drivers/edac/i5100_edac.c
> > index 39ba7f2414ae..7ec42b26a716 100644
> > --- a/drivers/edac/i5100_edac.c
> > +++ b/drivers/edac/i5100_edac.c
> > @@ -846,14 +846,13 @@ static void i5100_init_interleaving(struct pci_de=
v *pdev,
> > =20
> >  static void i5100_init_csrows(struct mem_ctl_info *mci)
> >  {
> > -	int i;
> >  	struct i5100_priv *priv =3D mci->pvt_info;
> > +	struct dimm_info *dimm;
> > =20
> > -	for (i =3D 0; i < mci->tot_dimms; i++) {
> > -		struct dimm_info *dimm;
> > -		const unsigned long npages =3D i5100_npages(mci, i);
> > -		const unsigned chan =3D i5100_csrow_to_chan(mci, i);
> > -		const unsigned rank =3D i5100_csrow_to_rank(mci, i);
> > +	mci_for_each_dimm(mci, dimm) {
> > +		const unsigned long npages =3D i5100_npages(mci, dimm->idx);
> > +		const unsigned chan =3D i5100_csrow_to_chan(mci, dimm->idx);
> > +		const unsigned rank =3D i5100_csrow_to_rank(mci, dimm->idx);
> > =20
> >  		if (!npages)
> >  			continue;
>=20
> This cannot be right: the code here under this does now:
>=20
> 	dimm =3D edac_get_dimm(mci, chan, rank, 0);
>=20
> but dimm is already set. It used to get the DIMM using chan and rank but
> now you're iterating over the already initialized pointers. So I think
> you don't need the edac_get_dimm() anymore.

Right, it should calculate to the same pointer we already have and can
be removed. Good catch.

>=20
> Also fix those up too, while at it pls:
>=20
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
> #235: FILE: drivers/edac/i5100_edac.c:854:
> +               const unsigned chan =3D i5100_csrow_to_chan(mci, dimm->id=
x);
>=20
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
> #236: FILE: drivers/edac/i5100_edac.c:855:
> +               const unsigned rank =3D i5100_csrow_to_rank(mci, dimm->id=
x);

I am going to fix this is a separate patch, though I will exclude
rework of struct i5100_priv.

I have reworked the patch according to all your other comments.

Thanks for review.

-Robert