Messages in this thread | | | Date | Fri, 4 Sep 2020 15:32:58 +0200 | From | Gregor Herburger <> | Subject | Re: [PATCH v2 1/1] edac: fsl_ddr_edac: fix expected data message |
| |
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?
| |