linux-kernel - Re: [PATCH v6] edac: synps: Added EDAC support for zynq ddr ecc controller

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]

Date:	Tue, 2 Dec 2014 11:36:40 +0100
From:	Borislav Petkov <bp@...en8.de>
To:	punnaiah choudary kalluri <punnaia@...inx.com>
Cc:	Punnaiah Choudary Kalluri <punnaiah.choudary.kalluri@...inx.com>,
	dougthompson@...ssion.com,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	linux-arm-kernel@...ts.infradead.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linux-edac@...r.kernel.org,
	"michal.simek@...inx.com" <michal.simek@...inx.com>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	"pawel.moll@....com" <pawel.moll@....com>,
	"mark.rutland@....com" <mark.rutland@....com>,
	"ijc+devicetree@...lion.org.uk" <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Rob Landley <rob@...dley.net>,
	Punnaiah Choudary <kpc528@...il.com>
Subject: Re: [PATCH v6] edac: synps: Added EDAC support for zynq ddr ecc
 controller

On Tue, Dec 02, 2014 at 10:51:22AM +0530, punnaiah choudary kalluri wrote:
> >> +/**
> >> + * synps_edac_get_eccstate - Return the controller ecc enable/disable status
> >> + * @base:    Pointer to the ddr memory contoller base address
> >> + *
> >> + * This routine returns the ECC enable/disable status for the controller
> >> + *
> >> + * Return: a ecc status boolean i.e true/false - enabled/disabled.
> >> + */
> >> +static bool synps_edac_get_eccstate(void __iomem *base)
> >> +{
> >> +     enum dev_type dt;
> >> +     u32 ecctype;
> >> +     bool state = false;
> >> +
> >> +     dt = synps_edac_get_dtype(base);
> >> +     if (dt == DEV_UNKNOWN)
> >> +             return state;
> >> +
> >> +     ecctype = readl(base + SCRUB_OFST) & SCRUB_MODE_MASK;
> >> +
> >> +     if ((ecctype == SCRUB_MODE_SECDED) && (dt == DEV_X2)) {
> >> +             state = true;
> >> +             writel(0x0, base + ECC_CTRL_OFST);
> >
> > Out of curiosity, why is that register write needed here? Maybe
> > forgotten? It looks unbalanced...
> 
> This is needed to start capturing the correctable and uncorrectable errors.
> Writing 1 to this register bits will clear the counters and writing 0 will start
> the counters.

So this definitely doesn't belong here then.

It should be the *last* thing you do after having initialized the whole
driver entirely and successfully and you're ready to start logging
errors.

Now you're calling it in synps_edac_mc_probe() so the counters will
start before you have even initialized the rest of the driver.

What is worse, if one of those things you do on the init path fails, you
enter prematurely and the counters are still running. Not good.

> It is crossing 80 cols. so, there is line break here. I feel the other
> way as the check patch throws warning for this.

And I'm saying you shouldn't follow checkpatch to the letter and think
for yourself instead.

What do you think is more readable?

This:

                       dimm = csi->channels[j]->dimm;
                       dimm->edac_mode = EDAC_FLAG_SECDED;
                       dimm->mtype = synps_edac_get_mtype(priv->baseaddr);
                       dimm->nr_pages =
                           (size >> PAGE_SHIFT) / csi->nr_channels;
                       dimm->grain = SYNPS_EDAC_ERR_GRAIN;
                       dimm->dtype = synps_edac_get_dtype(priv->baseaddr);

or this:

                       dimm		= csi->channels[j]->dimm;
                       dimm->mtype	= synps_edac_get_mtype(priv->baseaddr);
                       dimm->grain	= SYNPS_EDAC_ERR_GRAIN;
                       dimm->dtype	= synps_edac_get_dtype(priv->baseaddr);
                       dimm->nr_pages	= (size >> PAGE_SHIFT) / csi->nr_channels;
                       dimm->edac_mode	= EDAC_FLAG_SECDED;

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
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/