Re: [PATCH 1/2] drivers/edac: Add L1 and L2 error detection for A53 and A57

[Sascha Hauer <s.hauer@pengutronix.de>]

Hi Boris,

Sorry for the long delay, your mail was buried in my inbox.

On Wed, Aug 26, 2020 at 10:41:35AM +0200, Borislav Petkov wrote:
> On Thu, Aug 13, 2020 at 09:57:20AM +0200, Sascha Hauer wrote:
> > The Cortex A53 and A57 cores have error detection capabilities for the
> > L1/L2 Caches, this patch adds a driver for them.
> > 
> > Unfortunately there is no robust way to inject errors into the caches,
> > so this driver doesn't contain any code to actually test it. It has
> > been tested though with code taken from an older version of this driver
> > found here: https://lkml.org/lkml/2018/3/14/1203. For reasons stated
> > in this thread the error injection code is not suitable for mainline,
> > so it is removed from the driver.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  .../bindings/edac/arm,cortex-a5x-edac.yaml    |  32 +++
> >  drivers/edac/Kconfig                          |   6 +
> >  drivers/edac/Makefile                         |   1 +
> >  drivers/edac/cortex_arm64_l1_l2.c             | 208 ++++++++++++++++++
> >  4 files changed, 247 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/edac/arm,cortex-a5x-edac.yaml
> >  create mode 100644 drivers/edac/cortex_arm64_l1_l2.c
> 
> Just nitpicks below. James'd need to look at this too before it goes
> anywhere.
> 
> Checkpatch is trying to tell me something here:
> 
> WARNING: DT compatible string "arm,cortex-a53-edac" appears un-documented -- check ./Documentation/devicetree/bindings/
> #296: FILE: drivers/edac/cortex_arm64_l1_l2.c:190:
> +       { .compatible = "arm,cortex-a53-edac" },
> 
> WARNING: DT compatible string "arm,cortex-a57-edac" appears un-documented -- check ./Documentation/devicetree/bindings/
> #297: FILE: drivers/edac/cortex_arm64_l1_l2.c:191:
> +       { .compatible = "arm,cortex-a57-edac" },
> 
> for 2/2 too:
> 
> WARNING: DT compatible string "arm,cortex-a53-edac" appears un-documented -- check ./Documentation/devicetree/bindings/
> #39: FILE: arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi:842:
> +               compatible = "arm,cortex-a53-edac";
> 
> WARNING: DT compatible string "arm,cortex-a57-edac" appears un-documented -- check ./Documentation/devicetree/bindings/
> #56: FILE: arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi:805:
> +               compatible = "arm,cortex-a57-edac";
> 
> 
> False positive or valid?

Have you run checkpatch on a tree with this patch applied? If not, then
yes, it's undocumented as the docs are added with this patch.

> 
> ...
> 
> > +static void read_errors(void *data)
> > +{
> > +	u64 cpumerr, l2merr;
> > +	int cpu = smp_processor_id();
> > +	char msg[MESSAGE_SIZE];
> > +	struct edac_device_ctl_info *edac_ctl = data;
> 
> Please sort function local variables declaration in a reverse christmas
> tree order:
> 
> 	<type A> longest_variable_name;
> 	<type B> shorter_var_name;
> 	<type C> even_shorter;
> 	<type D> i;

I never heard of such a requirement. How is the length defined? Is it
only the length of the variable name or is it the length of the name
including the type? Including the array braces [] or not? What if a
variable shall be initialized with the value of an earlier declared
variable, do I have to make up variable names with a suitable length in
that case?  What if shorter_var_name and even_shorter are of same type,
can I still write them in a single line? Finally, Is this documented
somewhere?

I hope that was a joke from you that I didn't understand.

> 
> Check your other functions too pls.
> 
> > +	/* cpumerrsr_el1 */
> > +	asm volatile("mrs %0, s3_1_c15_c2_2" : "=r" (cpumerr));
> > +	asm volatile("msr s3_1_c15_c2_2, %0" :: "r" (0));
> > +
> > +	if (cpumerr & CPUMERRSR_EL1_VALID) {
> > +		const char *str;
> > +		int fatal = (cpumerr & CPUMERRSR_EL1_FATAL) != 0;
> 
> Don't need "!= 0" and fatal can be bool.

Ok.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |