Messages in this thread | | | From | Sherry Sun <> | Subject | RE: [PATCH V2 2/2] EDAC: synopsys: re-enable the interrupts in intr_handler for V3.X Synopsys EDAC DDR | Date | Sat, 23 Apr 2022 07:03:45 +0000 |
| |
> -----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
| |