linux-kernel - RE: [PATCH v7 2/2] EDAC/zynqmp: Add EDAC support for Xilinx ZynqMP OCM

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: <BY5PR12MB42589C326796149216AA84D6DBFE9@BY5PR12MB4258.namprd12.prod.outlook.com>
Date:   Mon, 9 Jan 2023 06:09:02 +0000
From:   "Potthuri, Sai Krishna" <sai.krishna.potthuri@....com>
To:     Borislav Petkov <bp@...en8.de>
CC:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Michal Simek <michal.simek@...inx.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Tony Luck <tony.luck@...el.com>,
        James Morse <james.morse@....com>,
        Robert Richter <rric@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
        "saikrishna12468@...il.com" <saikrishna12468@...il.com>,
        "git (AMD-Xilinx)" <git@....com>,
        "Datta, Shubhrajyoti" <shubhrajyoti.datta@....com>,
        kernel test robot <lkp@...el.com>
Subject: RE: [PATCH v7 2/2] EDAC/zynqmp: Add EDAC support for Xilinx ZynqMP
 OCM

Hi Boris,

> -----Original Message-----
> From: Borislav Petkov <bp@...en8.de>
> Sent: Sunday, January 8, 2023 10:28 PM
> To: Potthuri, Sai Krishna <sai.krishna.potthuri@....com>
> Cc: Rob Herring <robh+dt@...nel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@...aro.org>; Michal Simek
> <michal.simek@...inx.com>; Mauro Carvalho Chehab
> <mchehab@...nel.org>; Tony Luck <tony.luck@...el.com>; James Morse
> <james.morse@....com>; Robert Richter <rric@...nel.org>;
> devicetree@...r.kernel.org; linux-arm-kernel@...ts.infradead.org; linux-
> kernel@...r.kernel.org; linux-edac@...r.kernel.org;
> saikrishna12468@...il.com; git (AMD-Xilinx) <git@....com>; Datta,
> Shubhrajyoti <shubhrajyoti.datta@....com>; kernel test robot
> <lkp@...el.com>
> Subject: Re: [PATCH v7 2/2] EDAC/zynqmp: Add EDAC support for Xilinx
> ZynqMP OCM
> 
> On Wed, Jan 04, 2023 at 02:15:12PM +0530, Sai Krishna Potthuri wrote:
> > Add EDAC support for Xilinx ZynqMP OCM Controller, so this driver
> > reports CE and UE errors upon interrupt generation, and also creates
> > UE/CE debugfs entries for error injection.
> > On Xilinx ZynqMP platform, both OCM Controller driver(zynqmp_edac) and
> > DDR Memory Controller driver(synopsys_edac) co-exist which means both
> > can be loaded at a time. This scenario is tested on Xilinx ZynqMP
> > platform.
> >
> > Fix following issue reported by the robot.
> > "MAINTAINERS references a file that doesn't exist:
> > Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml"
> >
> > Co-developed-by: Shubhrajyoti Datta <shubhrajyoti.datta@....com>
> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@....com>
> > Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@....com>
> > Reported-by: kernel test robot <lkp@...el.com>
> > ---
> >  MAINTAINERS                |   7 +
> >  drivers/edac/Kconfig       |   9 +
> >  drivers/edac/Makefile      |   1 +
> >  drivers/edac/zynqmp_edac.c | 465
> > +++++++++++++++++++++++++++++++++++++
> >  4 files changed, 482 insertions(+)
> >  create mode 100644 drivers/edac/zynqmp_edac.c
> 
> Some touchups ontop, see below.
> 
> I had to revert back to the #ifdeffery because IS_ENABLED doesn't prevent
> the compiler from looking inside the conditional...
> 
> Anyway, inter-diff below. Holler if something's still amiss.
> 
> Thx.
> 
> ---
> 
>  diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig -index
> 58ab63642e72..7944e40c67da 100644
> +index 4cfdefbd744d..68f576700911 100644
>  --- a/drivers/edac/Kconfig
>  +++ b/drivers/edac/Kconfig
> -@@ -539,4 +539,13 @@ config EDAC_DMC520
> +@@ -542,4 +542,12 @@ config EDAC_DMC520
>   	  Support for error detection and correction on the
>   	  SoCs with ARM DMC-520 DRAM controller.
> 
> -+config EDAC_ZYNQMP_OCM
> ++config EDAC_ZYNQMP
>  +	tristate "Xilinx ZynqMP OCM Controller"
>  +	depends on ARCH_ZYNQMP || COMPILE_TEST
>  +	help
>  +	  This driver supports error detection and correction for the
> -+	  Xilinx ZynqMP OCM (On Chip Memory) controller.
> -+	  This driver can also be built as a module. If so, the module
> -+	  will be called zynqmp_ocm_edac.
> ++	  Xilinx ZynqMP OCM (On Chip Memory) controller. It can also be
> ++	  built as a module. In that case it will be called zynqmp_edac.
>  +
>   endif # EDAC
>  diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile -index
> 2d1641a27a28..47cbda06d7b0 100644
> +index 2d1641a27a28..9b025c5b3061 100644
>  --- a/drivers/edac/Makefile
>  +++ b/drivers/edac/Makefile
>  @@ -84,3 +84,4 @@ obj-$(CONFIG_EDAC_QCOM)			+=
> qcom_edac.o
>   obj-$(CONFIG_EDAC_ASPEED)		+= aspeed_edac.o
>   obj-$(CONFIG_EDAC_BLUEFIELD)		+= bluefield_edac.o
>   obj-$(CONFIG_EDAC_DMC520)		+= dmc520_edac.o
> -+obj-$(CONFIG_EDAC_ZYNQMP_OCM)		+= zynqmp_edac.o
> ++obj-$(CONFIG_EDAC_ZYNQMP)		+= zynqmp_edac.o
>  diff --git a/drivers/edac/zynqmp_edac.c b/drivers/edac/zynqmp_edac.c
> new file mode 100644 -index 000000000000..69069028457b
> +index 000000000000..b11f1157d4bb
>  --- /dev/null
>  +++ b/drivers/edac/zynqmp_edac.c
> -@@ -0,0 +1,465 @@
> +@@ -0,0 +1,469 @@
>  +// SPDX-License-Identifier: GPL-2.0
>  +/*
>  + * Xilinx ZynqMP OCM ECC Driver
> @@ -220,12 +224,14 @@ index 000000000000..69069028457b
>  +		p->ceinfo.fault_hi = readl(base + CE_FFD1_OFST);
>  +		p->ceinfo.addr = (OCM_BASEVAL | readl(base +
> CE_FFA_OFST));
>  +		writel(ECC_CTRL_CLR_CE_ERR, base + OCM_ISR_OFST);
> -+	} else {
> ++	} else if (mask & OCM_UEINTR_MASK) {
>  +		p->ue_cnt++;
>  +		p->ueinfo.fault_lo = readl(base + UE_FFD0_OFST);
>  +		p->ueinfo.fault_hi = readl(base + UE_FFD1_OFST);
>  +		p->ueinfo.addr = (OCM_BASEVAL | readl(base +
> UE_FFA_OFST));
>  +		writel(ECC_CTRL_CLR_UE_ERR, base + OCM_ISR_OFST);
> ++	} else {
> ++		WARN_ON_ONCE(1);
>  +	}
As we are raising a warning message in intr_handler() if the flagged interrupt
is not UE or CE and we return from there, so do we really need else if{} and
WARN_ON_ONCE() in else{} here?

Regards
Sai Krishna