RE: [PATCH v5 3/4] edac: synopsys: Add EDAC ECC support for ZynqMP DDRC

[Manish Narani]

Hi Boris,

Thanks for the review.

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@xxxxxxxxx]
> Sent: Wednesday, September 5, 2018 3:50 PM
> To: Manish Narani <MNARANI@xxxxxxxxxx>
> Cc: robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; Michal Simek
> <michals@xxxxxxxxxx>; mchehab@xxxxxxxxxx; leoyang.li@xxxxxxx;
> amit.kucheria@xxxxxxxxxx; olof@xxxxxxxxx; Srinivas Goud <sgoud@xxxxxxxxxx>;
> Anirudha Sarangi <anirudh@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> edac@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v5 3/4] edac: synopsys: Add EDAC ECC support for ZynqMP
> DDRC
> 
> On Fri, Aug 31, 2018 at 06:57:49PM +0530, Manish Narani wrote:
> > Add EDAC ECC support for ZynqMP DDRC IP. Also add support for ECC
> > Error Injection in ZynqMP. The corrected and uncorrected error
> > interrupts support is added. The Row, Column, Bank, Bank Group and
> > Rank bits positions are determined via Address Map registers of Synopsys
> DDRC.
> > Minor indentation changes are also done for better readability.
> >
> > Signed-off-by: Manish Narani <manish.narani@xxxxxxxxxx>
> > ---
> >  drivers/edac/Kconfig         |    2 +-
> >  drivers/edac/synopsys_edac.c | 1054
> > +++++++++++++++++++++++++++++++++++++-----
> >  2 files changed, 937 insertions(+), 119 deletions(-)
> 
> ...
> 
> > @@ -222,11 +408,77 @@ static int synps_edac_geterror_info(struct
> > synps_edac_priv *priv)  }
> >
> >  /**
> > + * zynqmp_geterror_info - Get the current ECC error info
> > + * @priv:	DDR memory controller private instance data
> > + *
> > + * Get the ECC error information from the ECC registers and clear the
> > +error
> > + * status bits in the ECC registers.
> > + *
> > + * Return: 0 if there is no error, otherwise return 1  */ static int
> > +zynqmp_geterror_info(struct synps_edac_priv *priv) {
> > +	void __iomem *base;
> > +	struct synps_ecc_status *p;
> > +	u32 regval, clearval = 0;
> > +
> > +	if (!priv)
> > +		return 1;
> 
> Same as for the previous patch: why are you testing this since you're
> dereferencing it before calling this function?

Okay.

> 
> ...
> 
> > @@ -258,10 +536,46 @@ static void synps_edac_handle_error(struct
> > mem_ctl_info *mci,  }
> >
> >  /**
> > + * synps_edac_intr_handler - Interrupt Handler for ECC interrupts
> > + * @irq:        irq number
> > + * @dev_id:     device id pointer
> > + *
> > + * This is the ISR called by EDAC core interrupt thread.
> 
> There's an "EDAC core interrupt thread"?!? This is the first time I hear of it.
> 
> Try again.

Okay. I will update this in v6.

> 
> > + * This is used to check and post ECC errors.
> > + *
> > + * Return: IRQ_NONE, if interrupt not set or IRQ_HANDLED otherwise.
> > + */
> > +static irqreturn_t synps_edac_intr_handler(int irq, void *dev_id) {
> > +	struct mem_ctl_info *mci = dev_id;
> > +	struct synps_edac_priv *priv = mci->pvt_info;
> > +	const struct synps_platform_data *p_data = priv->p_data;
> > +	int status, regval;
> > +
> > +	regval = readl(priv->baseaddr + DDR_QOS_IRQ_STAT_OFST);
> > +	regval &= (DDR_QOSCE_MASK | DDR_QOSUE_MASK);
> > +	if (!(regval & ECC_CE_UE_INTR_MASK))
> > +		return IRQ_NONE;
> > +
> > +	status = p_data->geterror_info(priv);
> > +	if (status)
> > +		return IRQ_NONE;
> > +
> > +	priv->ce_cnt += priv->stat.ce_cnt;
> > +	priv->ue_cnt += priv->stat.ue_cnt;
> > +	synps_edac_handle_error(mci, &priv->stat);
> > +
> > +	edac_dbg(3, "Total error count ce %d ue %d\n",
> 
> "ce" and "ue" are also abbreviations and should be in caps.

Okay. Will be taken care in v6.

> 
> ...
> 
> > +static DEVICE_ATTR_RW(inject_data_error);
> > +static DEVICE_ATTR_RW(inject_data_poison);
> > +
> > +static int synps_edac_create_sysfs_attributes(struct mem_ctl_info
> > +*mci) {
> > +	int rc;
> > +
> > +	rc = device_create_file(&mci->dev, &dev_attr_inject_data_error);
> > +	if (rc < 0)
> > +		return rc;
> > +	rc = device_create_file(&mci->dev, &dev_attr_inject_data_poison);
> > +	if (rc < 0)
> > +		return rc;
> > +	return 0;
> > +}
> 
> I think I'm having a deja-vu:
> 
> Last time I said:
> 
> "More importantly, you need to put all that injection functionality behind
> CONFIG_EDAC_DEBUG - you don't want to expose the injection capabilities on
> a production machine."
> 
> and you said:
> 
> "I agree. I will update the same by keeping the above mentioned macro."
> 
> But maybe you've misunderstood me.

I missed it in v5. Sorry for the inconvenience. Will update that in v6.

> 
> Grep the other EDAC drivers to find out how to hide the injection functionality
> behind CONFIG_EDAC_DEBUG.
> 
> And maybe this patch is becoming huuge and too unwieldy to review properly
> and for you to incorporate all the feedback.
> 
> Therefore, please split it in single patches, each of which is doing different
> things:
> 
> 1. fixup/change comments
> 2. add defines
> 3. add functionality X
> 4. add functionality Y
> ...
> 5. add injection
> 6. tie it all together

Will do the same in v6.

Thanks,
Manish Narani