Messages in this thread | | | Subject | Re: [PATCH 2/2] edac: add support for Amazon's Annapurna Labs EDAC | From | "Hawa, Hanna" <> | Date | Tue, 11 Jun 2019 22:56:02 +0300 |
| |
Hi James,
> > Allowing linux to access these implementation-defined registers has come up before: > https://www.spinics.net/lists/kernel/msg2750349.html > > It looks like you've navigated most of the issues. Accessing implementation-defined > registers is frowned on, but this stuff can't be done generically until v8.2. Sure, no planning to do this generally for all ARM a57/a72. I'm doing this specific for alpine SoCs.
> > This can't be done on 'all A57/A72' because some platforms may not have been integrated to > have error-checking at L1/L2, (1.3 'Features' of [0] says the ECC protection for L1 data > cache etc is optional). Even if they did, this stuff needs turning on in L2CTLR_EL1. > These implementation-defined registers may be trapped by the hypervisor, I assume for your > platform this is linux booted at EL2, so no hypervisor. In Alpine-v2/Alpine-v3 Bit[21]-"ECC and parity enable" in L2CTRL_EL1 is enabled in the firmware.
> > >> +#define ARM_CA57_CPUMERRSR_INDEX_OFF (0) >> +#define ARM_CA57_CPUMERRSR_INDEX_MASK (0x3FFFF) > > (GENMASK() would make it quicker and easier to compare this with the datasheet) Will be used in next patchset.
> > >> +#define ARM_CA57_L2_INC_PLRU_RAM 0x18 > > A57 describes this one as 'PF RAM'... will be updated.
> > > > Linux supports versions of binutils that choke on this syntax. > See the sys_reg() definitions in arm64's asm/sysreg.h that define something you can feed > to read_sysreg_s(). It would save having these wrapper functions. > > commit 72c583951526 ("arm64: gicv3: Allow GICv3 compilation with older binutils") for the > story. Great, I'll use sys_reg(), read_sysreg_s(), and remove the wrapper functions.
> > > > | #define ARM_CA57_CPUMERRSR_VALID BIT(31) > | if (!(val & ARM_CA57_CPUMERRSR_VALID)) > > would be easier to read, the same goes for 'fatal' as its a single bit. Will be fixed, here and in al_a57_edac_l2merrsr.
> > >> + edac_device_handle_ce(edac_dev, 0, 0, "L2 Error"); > > How do we know this was corrected? > > 6.4.8 "Error Correction Code" has "Double-bit ECC errors set the fatal bit." in a > paragraph talking about the L1 memory system. I'll check fatal field to check if it's uncorrected/corrected.
> > "L2 Error" ? Copy and paste? copy/paste mistake, will be fixed.
> > >> + edac_printk(KERN_CRIT, DRV_NAME, "CPU%d L1 %serror detected\n", >> + cpu, (fatal) ? "Fatal " : ""); >> + edac_printk(KERN_CRIT, DRV_NAME, "RAMID="); >> + >> + switch (ramid) { >> + case ARM_CA57_L1_I_TAG_RAM: >> + pr_cont("'L1-I Tag RAM' index=%d way=%d", index, way); >> + break; >> + case ARM_CA57_L1_I_DATA_RAM: >> + pr_cont("'L1-I Data RAM' index=%d bank= %d", index, way); >> + break; > > Is index/way information really useful? I can't replace way-3 on the system, nor can I > stop it being used. If its useless, I'd rather we don't bother parsing and printing it out. I'll remove the index/way information, and keep CPUMERRSR_EL1 value print (who want this information can parse the value and get the index/bank status)
> > >> + pr_cont(", repeat=%d, other=%d (CPUMERRSR_EL1=0x%llx)\n", repeat, other, >> + val); > > 'other' here is another error, but we don't know the ramid. > 'repeat' is another error for the same ramid. > > Could we still feed this stuff into edac? This would make the counters accurate if the > polling frequency isn't quite fast enough. There is no API in EDAC to increase the counters, I can increase by accessing the ce_count/ue_count from edac_device_ctl_info/edac_device_instance structures if it's okay..
> > >> +static void al_a57_edac_l2merrsr(void *arg) >> +{ > >> + edac_device_handle_ce(edac_dev, 0, 0, "L2 Error"); > > How do we know this is corrected? > > If looks like L2CTLR_EL1[20] might force fatal 1/0 to map to uncorrected/corrected. Is > this what you are depending on here? No - not on this. Reporting all the errors as corrected seems to be bad.
Can i be depends on fatal field?
if (fatal) edac_device_handle_ue(edac_dev, 0, 0, "L2 Error"); else edac_device_handle_ce(edac_dev, 0, 0, "L2 Error");
How can L2CTLR_EL1[20] force fatal?
> > (it would be good to have a list of integration-time and firmware dependencies this driver > has, for the next person who tries to enable it on their system and complains it doesn't > work for them) Where can i add such information?
> > >> + case ARM_CA57_L2_INC_PLRU_RAM: >> + pr_cont("'L2 Inclusion PLRU RAM'"); > > The A57 TRM describes this as "Inclusion PF RAM", and notes its only in r1p0 or later, > (but doesn't say what it is). The A72 TRM describes the same encoding as "Inclusion PLRU > RAM", which is something to do with its replacement policy. It has control bits that A57's > version doesn't, so these are not the same thing. > > Disambiguating A57/A72 here is a load of faff, 'L2 internal metadata' probably covers both > cases, but unless these RAMs are replaceable or can be disabled, there isn't much point > working out which one it was. Will be fixed to 'L2 internal metadata'
> >> + pr_cont(", cpuid/way=%d, repeat=%d, other=%d (L2MERRSR_EL1=0x%llx)\n", >> + way, repeat, other, val); > > cpuid could be useful if you can map it back to the cpu number linux has. > If you can spot that cpu-7 is experiencing more errors than it should, you can leave it > offline. > > To do this you'd need to map each L2MERRSR_EL1's '0-3' range back to the CPUs they > actually are. The gic's 'ppi-partitions' does this with phandles, e.g. > Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml. You could add a > similar shaped thing to the l2-cacheX node in the DT, (or in your edac node, but it is a > property of the cache integration). As in L1 prints, I'll remove non-relevant prints.
> > >> + /* >> + * Use get_online_cpus/put_online_cpus to prevent the online CPU map >> + * changing while reads the L1/L2 error status > > For walking the list of offline cpus, this makes sense. But you schedule work without > waiting, it may get run after you drop the cpus_read_lock()..., Will update the smp_call_function_single() call function to wait.
> > >> + get_online_cpus(); > > The comment above these helpers is: > | /* Wrappers which go away once all code is converted */ > > cpus_read_lock()? Will be updated.
> > >> + for_each_online_cpu(cpu) { >> + /* Check L1 errors */ >> + smp_call_function_single(cpu, al_a57_edac_cpumerrsr, edac_dev, >> + 0); > > As you aren't testing for big/little, wouldn't on_each_cpu() here be simpler? Could be simpler for L1, how can be implemented for L2?
> > As you don't wait, what stops al_a57_edac_cpumerrsr() feeding two errors into > edac_device_handle_ce() at the same time? Do you need a spinlock in al_a57_edac_cpumerrsr()? Will call smp_call_function_single() with wait.
> > >> + cluster = topology_physical_package_id(cpu); > > Hmm, I'm not sure cluster==package is guaranteed to be true forever. > > If you describe the L2MERRSR_EL1 cpu mapping in your DT you could use that. Otherwise > pulling out the DT using something like the arch code's parse_cluster(). I rely on that it's alpine SoC specific driver.
> > >> + if (cluster != last_cluster) { >> + smp_call_function_single(cpu, al_a57_edac_l2merrsr, >> + edac_dev, 0); >> + last_cluster = cluster; >> + } > > Here you depend on the CPUs being listed in cluster-order in the DT. I'm fairly sure the > numbering is arbitrary: On my Juno 0,3,4,5 are the A53 cluster, and 1,2 are the A57 cluster. > > If 1,3,5 were cluster-a and 2,4,6 were cluster-b, you would end up calling > al_a57_edac_l2merrsr() for each cpu. As you don't wait, they could race. > > If you can get a cpu-mask for each cluster, smp_call_function_any() would to the > pick-one-online-cpu work for you. Again, I rely on that it's alpine SoC specific driver. How can I get cpu-mask for each cluster? from DT?
> > >> +static int al_a57_edac_remove(struct platform_device *pdev) >> +{ >> + struct edac_device_ctl_info *edac_dev = platform_get_drvdata(pdev); >> + >> + edac_device_del_device(edac_dev->dev); >> + edac_device_free_ctl_info(edac_dev); > > Your poll function schedule work on other CPUs and didn't wait, is it possible > al_a57_edac_l2merrsr() is still using this memory when you free it? This will be okay, after using wait in smp_call_function_single(). > > >> +MODULE_LICENSE("GPL"); > > | MODULE_LICENSE("GPL v2"); > > To match the SPDX header? Will be fixed.
Thanks for your detailed review.
Thanks, Hanna > > > > Thanks, > > James > > > [0] > http://infocenter.arm.com/help/topic/com.arm.doc.ddi0488c/DDI0488C_cortex_a57_mpcore_r1p0_trm.pdf >
| |