| Message ID | 1582856183-5007-1-git-send-email-sherry.sun@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] EDAC: synopsys: Fix the wrong call of pinf->col parameter | expand |
Hi, > -----Original Message----- > From: Sherry Sun <sherry.sun@nxp.com> > Sent: Friday, February 28, 2020 7:46 AM > To: bp@alien8.de; mchehab@kernel.org; tony.luck@intel.com; > james.morse@arm.com; rrichter@marvell.com; Michal Simek > <michals@xilinx.com>; Manish Narani <MNARANI@xilinx.com> > Cc: linux-edac@vger.kernel.org; linux-kernel@vger.kernel.org; linux- > imx@nxp.com; frank.li@nxp.com > Subject: [PATCH v2] EDAC: synopsys: Fix the wrong call of pinf->col parameter > > Since ZynqMP platform call zynqmp_get_error_info() function to get ce/ue > information. In this function, pinf->col parameter is not used, this > parameter is only used by Zynq platforme in zynq_get_error_info(). So > here pinf->col should not be called and printed for ZynqMP, need remove > it. > > Fixes: b500b4a029d57 ("EDAC, synopsys: Add ECC support for ZynqMP DDR > controller") > Signed-off-by: Sherry Sun <sherry.sun@nxp.com> Reviewed-by: Manish Narani <manish.narani@xilinx.com> Thanks, Manish
On Fri, Feb 28, 2020 at 10:16:23AM +0800, Sherry Sun wrote: > Since ZynqMP platform call zynqmp_get_error_info() function to get ce/ue > information. In this function, pinf->col parameter is not used, this > parameter is only used by Zynq platforme in zynq_get_error_info(). So > here pinf->col should not be called and printed for ZynqMP, need remove > it. Err, the interrupt handler intr_handler() calls the ->get_error_info() function pointer and then calls handle_error(). I.e, the same path ending in handle_error() is called on both: Zynq and ZynqMP, one through the interrupt and the other through the poller. Because it looks like the interrupt support is only on ZynqMP? I'm looking at that DDR_ECC_INTR_SUPPORT thing. If so, then you need to rename the interrupt handler to zynqmp_intr_handler(). And normal Zynq platform uses check_errors() which then calls handle_error(). So still not good enough: you probably "fixed" it on ZynqMP but broke it on Zynq because check_errors() calls ->get_error_info() which is zynq_get_error_info() and will have populated pinf->col. You need to differentiate between Zynq and ZynqMP in handle_error() and issue the proper string depending on the platform. AFAICT.
Hi Borislav, > -----Original Message----- > From: Borislav Petkov <bp@alien8.de> > Sent: 2020年3月16日 3:50 > To: Sherry Sun <sherry.sun@nxp.com> > Cc: mchehab@kernel.org; tony.luck@intel.com; james.morse@arm.com; > rrichter@marvell.com; michal.simek@xilinx.com; manish.narani@xilinx.com; > linux-edac@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com>; Frank Li <frank.li@nxp.com> > Subject: Re: [PATCH v2] EDAC: synopsys: Fix the wrong call of pinf->col > parameter > > On Fri, Feb 28, 2020 at 10:16:23AM +0800, Sherry Sun wrote: > > Since ZynqMP platform call zynqmp_get_error_info() function to get > > ce/ue information. In this function, pinf->col parameter is not used, > > this parameter is only used by Zynq platforme in > > zynq_get_error_info(). So here pinf->col should not be called and > > printed for ZynqMP, need remove it. > > Err, the interrupt handler intr_handler() calls the ->get_error_info() function > pointer and then calls handle_error(). I.e, the same path ending in > handle_error() is called on both: Zynq and ZynqMP, one through the > interrupt and the other through the poller. > > Because it looks like the interrupt support is only on ZynqMP? I'm looking at > that DDR_ECC_INTR_SUPPORT thing. > > If so, then you need to rename the interrupt handler to > zynqmp_intr_handler(). > > And normal Zynq platform uses check_errors() which then calls > handle_error(). > > So still not good enough: you probably "fixed" it on ZynqMP but broke it on > Zynq because check_errors() calls ->get_error_info() which is > zynq_get_error_info() and will have populated pinf->col. > > You need to differentiate between Zynq and ZynqMP in handle_error() and > issue the proper string depending on the platform. Maybe you misunderstood the handle_error(), this function has already distinguished between Zynq and ZynqMP through priv->p_data->quirks. And what I am doing in this patch is to remove the useless pinf->col in ZynqMP part, which won't break the Zynq part. Thanks Sherry Sun > > AFAICT. > > -- > Regards/Gruss, > Boris. > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeopl > e.kernel.org%2Ftglx%2Fnotes-about- > netiquette&data=02%7C01%7Csherry.sun%40nxp.com%7C948bf1123f6f > 4717a61508d7c91a193b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0 > %7C637198986257175149&sdata=O8txcSkTpO3I34KUL0qboQ%2BvPme > D7lYPHsAhWavevWg%3D&reserved=0
On Mon, Mar 16, 2020 at 01:30:39AM +0000, Sherry Sun wrote: > Maybe you misunderstood the handle_error(), this function has already > distinguished between Zynq and ZynqMP through priv->p_data->quirks. > And what I am doing in this patch is to remove the useless pinf->col in ZynqMP > part, which won't break the Zynq part. Ok, I see it now, thanks for clarifying. Now, in your v2 pls fix this check in handle_error(): if (!priv->p_data->quirks) { to *actually* *explicitly* check that the platform for which this function is called, really supports interrupts, i.e., DDR_ECC_INTR_SUPPORT, and not that the ->quirks thing is != 0. Like the rest of the code does. Thx.
Hi Borislav, > -----Original Message----- > From: Borislav Petkov <bp@alien8.de> > Sent: 2020年3月16日 18:44 > To: Sherry Sun <sherry.sun@nxp.com> > Cc: mchehab@kernel.org; tony.luck@intel.com; james.morse@arm.com; > rrichter@marvell.com; michal.simek@xilinx.com; manish.narani@xilinx.com; > linux-edac@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com>; Frank Li <frank.li@nxp.com> > Subject: Re: [PATCH v2] EDAC: synopsys: Fix the wrong call of pinf->col > parameter > > On Mon, Mar 16, 2020 at 01:30:39AM +0000, Sherry Sun wrote: > > Maybe you misunderstood the handle_error(), this function has already > > distinguished between Zynq and ZynqMP through priv->p_data->quirks. > > And what I am doing in this patch is to remove the useless pinf->col > > in ZynqMP part, which won't break the Zynq part. > > Ok, I see it now, thanks for clarifying. > > Now, in your v2 pls fix this check in handle_error(): > > if (!priv->p_data->quirks) { > > to *actually* *explicitly* check that the platform for which this function is > called, really supports interrupts, i.e., DDR_ECC_INTR_SUPPORT, and not that > the ->quirks thing is != 0. Like the rest of the code does. > Ok, I will fix this check. Best regards Sherry Sun > Thx. > > -- > Regards/Gruss, > Boris. > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeopl > e.kernel.org%2Ftglx%2Fnotes-about- > netiquette&data=02%7C01%7Csherry.sun%40nxp.com%7Ccd843239ffd > d48a05ce408d7c996ebd5%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C > 0%7C637199522356909838&sdata=5p6JuXQQJgyjlaMOUH1cLLSDFbUzM > R%2Fjzs5HC%2FeU1Zc%3D&reserved=0
diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c index 880ffd833718..da707d539f7f 100644 --- a/drivers/edac/synopsys_edac.c +++ b/drivers/edac/synopsys_edac.c @@ -484,8 +484,8 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p) pinf->bitpos, pinf->data); } else { snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, - "DDR ECC error type:%s Row %d Bank %d Col %d BankGroup Number %d Block Number %d Bit Position: %d Data: 0x%08x", - "CE", pinf->row, pinf->bank, pinf->col, + "DDR ECC error type:%s Row %d Bank %d BankGroup Number %d Block Number %d Bit Position: %d Data: 0x%08x", + "CE", pinf->row, pinf->bank, pinf->bankgrpnr, pinf->blknr, pinf->bitpos, pinf->data); } @@ -503,8 +503,8 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p) "UE", pinf->row, pinf->bank, pinf->col); } else { snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, - "DDR ECC error type :%s Row %d Bank %d Col %d BankGroup Number %d Block Number %d", - "UE", pinf->row, pinf->bank, pinf->col, + "DDR ECC error type :%s Row %d Bank %d BankGroup Number %d Block Number %d", + "UE", pinf->row, pinf->bank, pinf->bankgrpnr, pinf->blknr); }
Since ZynqMP platform call zynqmp_get_error_info() function to get ce/ue information. In this function, pinf->col parameter is not used, this parameter is only used by Zynq platforme in zynq_get_error_info(). So here pinf->col should not be called and printed for ZynqMP, need remove it. Fixes: b500b4a029d57 ("EDAC, synopsys: Add ECC support for ZynqMP DDR controller") Signed-off-by: Sherry Sun <sherry.sun@nxp.com> --- This patch is based on [PATCH V2] EDAC: synopsys: Fix back to back snprintf() messages for CE/UE Changes in V2: - Separated this patch from the original patchset. --- drivers/edac/synopsys_edac.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)