| Message ID | 20220421015313.5747-3-sherry.sun@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | EDAC: synopsys: fix some bugs in V3.X Synopsys EDAC DDR driver | expand |
On Thu, Apr 21, 2022 at 09:53:13AM +0800, Sherry Sun wrote: > Since zynqmp_get_error_info() is called during CE/UE interrupt, at the This also needs to be made human-readable: for example, "zynqmp_get_error_info() reads the error information from the registers when an interrupt for a {un-,}correctable error is raised." > end of zynqmp_get_error_info(), it wirtes 0 to ECC_CLR_OFST, which cause Unknown word [wirtes] in commit message. Suggestions: ['writes', Please introduce a spellchecker into your patch creation workflow. > the CE/UE interrupts of V3.X Synopsys EDAC DDR been disabled, then the "which disables the error interrupts" - make it simple - no need for the V3.X marketing bla. > interrupt handler will be called only once, so need to re-enable the "Therefore, reenable the error interrupt line ..." > interrupts at the end of intr_handler for V3.X Synopsys EDAC DDR. I think you're catching my drift: our commit messages need to be understandable and when read months, years from now, still to make sense. > Signed-off-by: Sherry Sun <sherry.sun@nxp.com> > Reviewed-by: Shubhrajyoti Datta <Shubhrajyoti.datta@xilinx.com> > Acked-by: Michal Simek <michal.simek@xilinx.com> > --- > Changes in V2: > 1. Add the Reviewed-by and Acked-by tag. > 2. Add the newline as suggested by Michal. > --- > drivers/edac/synopsys_edac.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c > index 88a481043d4c..ae1cf02a92f5 100644 > --- a/drivers/edac/synopsys_edac.c > +++ b/drivers/edac/synopsys_edac.c > @@ -527,6 +527,8 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p) > memset(p, 0, sizeof(*p)); > } > > +static void enable_intr(struct synps_edac_priv *priv); Why the forward declaration? Why not simply move {enable,disable}_intr() upwards in that file? Also, for both fixes: do you want them backported in stable kernels? I think you do because they look like you'd want that v3.x support to work with older kernels too. If so, read the section about "Fixes:" in Documentation/process/submitting-patches.rst Thx.
> -----Original Message----- > From: Borislav Petkov <bp@alien8.de> > Sent: 2022年4月21日 17:07 > To: Sherry Sun <sherry.sun@nxp.com> > Cc: michal.simek@xilinx.com; Shubhrajyoti.datta@xilinx.com; > mchehab@kernel.org; tony.luck@intel.com; james.morse@arm.com; > rric@kernel.org; linux-edac@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH V2 2/2] EDAC: synopsys: re-enable the interrupts in > intr_handler for V3.X Synopsys EDAC DDR > > On Thu, Apr 21, 2022 at 09:53:13AM +0800, Sherry Sun wrote: > > Since zynqmp_get_error_info() is called during CE/UE interrupt, at the > > This also needs to be made human-readable: for example, > "zynqmp_get_error_info() reads the error information from the registers > when an interrupt for a {un-,}correctable error is raised." > > > end of zynqmp_get_error_info(), it wirtes 0 to ECC_CLR_OFST, which > > cause > > Unknown word [wirtes] in commit message. > Suggestions: ['writes', > > Please introduce a spellchecker into your patch creation workflow. > > > the CE/UE interrupts of V3.X Synopsys EDAC DDR been disabled, then the > > "which disables the error interrupts" - make it simple - no need for the V3.X > marketing bla. > > > interrupt handler will be called only once, so need to re-enable the > > "Therefore, reenable the error interrupt line ..." > > > interrupts at the end of intr_handler for V3.X Synopsys EDAC DDR. > > I think you're catching my drift: our commit messages need to be > understandable and when read months, years from now, still to make sense. Hi Borislav, thanks for the detailed suggestions on this patchset, will improve them in V3. > > > Signed-off-by: Sherry Sun <sherry.sun@nxp.com> > > Reviewed-by: Shubhrajyoti Datta <Shubhrajyoti.datta@xilinx.com> > > Acked-by: Michal Simek <michal.simek@xilinx.com> > > --- > > Changes in V2: > > 1. Add the Reviewed-by and Acked-by tag. > > 2. Add the newline as suggested by Michal. > > --- > > drivers/edac/synopsys_edac.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/edac/synopsys_edac.c > > b/drivers/edac/synopsys_edac.c index 88a481043d4c..ae1cf02a92f5 > 100644 > > --- a/drivers/edac/synopsys_edac.c > > +++ b/drivers/edac/synopsys_edac.c > > @@ -527,6 +527,8 @@ static void handle_error(struct mem_ctl_info *mci, > struct synps_ecc_status *p) > > memset(p, 0, sizeof(*p)); > > } > > > > +static void enable_intr(struct synps_edac_priv *priv); > > Why the forward declaration? > > Why not simply move {enable,disable}_intr() upwards in that file? I wanted minimal code changes here, but if you think it's better to simply move {enable,disable}_intr() upwards, I will do that way in V3. > > Also, for both fixes: do you want them backported in stable kernels? > > I think you do because they look like you'd want that v3.x support to work > with older kernels too. > > If so, read the section about "Fixes:" in Documentation/process/submitting- > patches.rst My fix patches are based on Dinh's patch: f7824ded4149 ("EDAC/synopsys: Add support for version 3 of the Synopsys EDAC DDR"), as this patch was introduced since L5.17, it's quite new, so I think we don't need to backport them to the stable kernels. Thanks~ Best regards Sherry > > Thx. > > -- > Regards/Gruss, > Boris. > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeopl > e.kernel.org%2Ftglx%2Fnotes-about- > netiquette&data=05%7C01%7Csherry.sun%40nxp.com%7C91d3a08c4e0 > d43aeac9108da23764aec%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C > 0%7C637861288219637795%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wL > jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C% > 7C%7C&sdata=wnCvrZJ2%2FZKd%2Ff2X5xptEg7zfDqy5sLmyDCq8Rh3QB > M%3D&reserved=0
On Sat, Apr 23, 2022 at 07:03:45AM +0000, Sherry Sun wrote: > My fix patches are based on Dinh's patch: f7824ded4149 > ("EDAC/synopsys: Add support for version 3 of the Synopsys EDAC DDR"), > as this patch was introduced since L5.17, it's quite new, so I think > we don't need to backport them to the stable kernels. What do you mean, quite new? There are 5.17.x stable releases so they will need those fixes too.
> On Sat, Apr 23, 2022 at 07:03:45AM +0000, Sherry Sun wrote: > > My fix patches are based on Dinh's patch: f7824ded4149 > > ("EDAC/synopsys: Add support for version 3 of the Synopsys EDAC DDR"), > > as this patch was introduced since L5.17, it's quite new, so I think > > we don't need to backport them to the stable kernels. > > What do you mean, quite new? > > There are 5.17.x stable releases so they will need those fixes too. Hi Borislav, got it, so we can add the fix tag for the two patches. I will send V4. Fixes: f7824ded4149 ("EDAC/synopsys: Add support for version 3 of the Synopsys EDAC DDR") Best regards Sherry > > -- > Regards/Gruss, > Boris. >
diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c index 88a481043d4c..ae1cf02a92f5 100644 --- a/drivers/edac/synopsys_edac.c +++ b/drivers/edac/synopsys_edac.c @@ -527,6 +527,8 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p) memset(p, 0, sizeof(*p)); } +static void enable_intr(struct synps_edac_priv *priv); + /** * intr_handler - Interrupt Handler for ECC interrupts. * @irq: IRQ number. @@ -568,6 +570,9 @@ static irqreturn_t intr_handler(int irq, void *dev_id) /* v3.0 of the controller does not have this register */ if (!(priv->p_data->quirks & DDR_ECC_INTR_SELF_CLEAR)) writel(regval, priv->baseaddr + DDR_QOS_IRQ_STAT_OFST); + else + enable_intr(priv); + return IRQ_HANDLED; }