On Fri, Sep 04, 2020 at 11:17:18AM +0200, Borislav Petkov wrote:
> Your mail client broke threading...
>
Indeed. Guess I have to change the mail client. Sorry for that.
> On Fri, Sep 04, 2020 at 06:52:24AM +0000, Gregor Herburger wrote:
>
> > The cap_low, cap_high and syndrome are used in the printk following the
> > if-Block.
> > This will make expected data / captured data look the same.
>
> Right.
>
> > @@ -334,18 +337,32 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
> > sbe_ecc_decode(cap_high, cap_low, syndrome,
> > &bad_data_bit, &bad_ecc_bit);
> >
> > + exp_high = cap_high;
> > + exp_low = cap_low;
> > + exp_syndrome = syndrome;
> > +
> > if (bad_data_bit != -1)
> > + {
>
> Opening brace is on the same line for if-statements.
>
> > fsl_mc_printk(mci, KERN_ERR,
> > "Faulty Data bit: %d\n", bad_data_bit);
> > +
> > + if (bad_data_bit < 32)
> > + exp_low = cap_low ^ (1 << bad_data_bit);
> > + else
> > + exp_high = cap_high ^ (1 << (bad_data_bit -
> > 32));
> > + }
> > +
> > if (bad_ecc_bit != -1)
> > + {
>
> Ditto.
>
> > fsl_mc_printk(mci, KERN_ERR,
> > "Faulty ECC bit: %d\n", bad_ecc_bit);
> >
> > + exp_syndrome = syndrome ^ (1 << bad_ecc_bit);
> > + }
> > +
> > fsl_mc_printk(mci, KERN_ERR,
> > "Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
> > - cap_high ^ (1 << (bad_data_bit - 32)),
> > - cap_low ^ (1 << bad_data_bit),
> > - syndrome ^ (1 << bad_ecc_bit));
> > + exp_high, exp_low, exp_syndrome);
> > }
> >
> > fsl_mc_printk(mci, KERN_ERR,
> > "Captured Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
> > cap_high, cap_low, syndrome);
> >
> > How about something like this?
>
> My only concern here is that you'll be printing "Expected Data ..."
> unconditionally even if either or both - bad_data_bit and bad_ecc_bit
> - are -1.
That shouldn't happen. The whole if-block is only executed when a single
bit correctable error has occured (DDR_EDE_SBE). So we always should have
bad_data_bit or bad_ecc_bit (exclusively).
>
> If the driver cannot decode the data and/or ECC syndrome bits, then it
> should say so - not dump expected data and claim that it is a valid
> information.
>
Ok, that is reaonable. But that shouldn't that go into sbe_ecc_decode()?.
Currently sbe_ecc_decude() returns on the first error it finds. So we would
have to rework this function.
> So maybe in addition to the above:
>
> if (bad_data_bit != -1) {
> ...
> } else {
> fsl_mc_printk(..., "Unable to decode the Faulty Data bit");
> }
>
> and the same for the ECC bit.
>
I suggest adding such an message to sbe_ecc_decode(). Also to add an
return 0 on success and to check that before printing infos about single
bit errors.
> And then print only the expected data for the bit which sbe_ecc_decode()
> found correctly and not say anything otherwise.
>
Also i just noticed in the kernel log is no hint that this is an
single bit error. Maybe we should add this too?
