linux-kernel - RE: [PATCH v7 5/7] edac: synopsys: Add EDAC ECC support for ZynqMP DDRC

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: <DM6PR02MB522717E2B74148A7069FA57EC1170@DM6PR02MB5227.namprd02.prod.outlook.com>
Date:   Mon, 24 Sep 2018 10:20:41 +0000
From:   Manish Narani <MNARANI@...inx.com>
To:     Borislav Petkov <bp@...en8.de>
CC:     "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "mchehab@...nel.org" <mchehab@...nel.org>,
        Michal Simek <michals@...inx.com>,
        "leoyang.li@....com" <leoyang.li@....com>,
        "sudeep.holla@....com" <sudeep.holla@....com>,
        "amit.kucheria@...aro.org" <amit.kucheria@...aro.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: RE: [PATCH v7 5/7] edac: synopsys: Add EDAC ECC support for ZynqMP
 DDRC

Hi Boris,

Thanks for the review.


> From: Borislav Petkov [mailto:bp@...en8.de]
> Sent: Friday, September 21, 2018 6:26 PM
> On Mon, Sep 17, 2018 at 07:55:03PM +0530, Manish Narani wrote:
> > Add EDAC ECC support for ZynqMP DDRC IP. The IP supports interrupts
> > for corrected and uncorrected errors. Add interrupt handlers for the same.
> >
> > Signed-off-by: Manish Narani <manish.narani@...inx.com>
> > ---
> >  drivers/edac/Kconfig         |   2 +-
> >  drivers/edac/synopsys_edac.c | 304
> > ++++++++++++++++++++++++++++++++++++++++---
> >  2 files changed, 287 insertions(+), 19 deletions(-)
> 
> ...
> > +	if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT) {
> 
> This whole chunk together with the if, looks like it could be a separate function
> called maybe request_irq() or so.
May be I can keep it like edac_setup_irq() ?

> 
> > +		irq = platform_get_irq(pdev, 0);
> > +		if (irq < 0) {
> > +			edac_printk(KERN_ERR, EDAC_MC,
> > +					"No irq %d in DT\n", irq);
> 
> Align arguments on opening brace.
Okay

> 
> Then, here it is "irq"...
> 
> > +			rc = irq;
> > +			goto free_edac_mc;
> > +		}
> > +
> > +		rc = devm_request_irq(&pdev->dev, irq,
> > +			synps_edac_intr_handler,
> > +			0, dev_name(&pdev->dev), mci);
> 
> Also align on opening brace.
> 
> > +		if (rc < 0) {
> > +			edac_printk(KERN_ERR, EDAC_MC, "Failed to request
> Irq\n");
> 
> ... here it is "Irq".
> 
> I'd say it should be "IRQ" everywhere.
Okay

> 
> > +			goto free_edac_mc;
> > +		}
> > +
> > +		/* Enable UE/CE Interrupts */
> > +		writel((DDR_QOSUE_MASK | DDR_QOSCE_MASK),
> > +			priv->baseaddr + DDR_QOS_IRQ_EN_OFST);
> > +	}
> > +
> >  	rc = edac_mc_add_mc(mci);
> >  	if (rc) {
> >  		edac_printk(KERN_ERR, EDAC_MC,
> > @@ -684,7 +945,8 @@ static int synps_edac_mc_probe(struct
> platform_device *pdev)
> >  	 * Start capturing the correctable and uncorrectable errors. A write of
> >  	 * 0 starts the counters.
> >  	 */
> > -	writel(0x0, baseaddr + ECC_CTRL_OFST);
> > +	if (!(priv->p_data->quirks & DDR_ECC_INTR_SUPPORT))
> > +		writel(0x0, baseaddr + ECC_CTRL_OFST);
> >  	return rc;
> >
> >  free_edac_mc:
> > @@ -702,7 +964,13 @@ static int synps_edac_mc_probe(struct
> > platform_device *pdev)  static int synps_edac_mc_remove(struct
> > platform_device *pdev)  {
> >  	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
> > +	struct synps_edac_priv *priv;
> >
> > +	priv = mci->pvt_info;
> > +	if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT)
> > +		/* Disable UE/CE Interrupts */
> > +		writel((DDR_QOSUE_MASK | DDR_QOSCE_MASK),
> > +			priv->baseaddr + DDR_QOS_IRQ_DB_OFST);
> 
> This could be a disable_irq() function. Makes the code easier to follow.
Okay. Will update this in v8.

Thanks,
Manish