| Message ID | CALdTtnt=aNYX3QqgsXLehfJ0KKx3k1CLGYbGyghgJBPEbrf43g@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Mar 23, 2017 at 08:17:31PM -0600, dann frazier wrote: > This addresses LP: #1675509. > > The HiSilicon D05 board and systems based on Cortex-A73 each have > errata for their architected timers which can cause reads to return > bogus data. One of the arm_arch_timer maintainers has developed an > errata infrastructure to enable workarounds for these. I've backported > the v2 revision from the developer's tree to the zesty kernel and > added the relevant ACKs from the parent tree maintainer from the > mailing list. I'll continue to track the review process and watch for > any fixes relevant to Ubuntu. > > Tested on HiSilicon D05; regression tested on HP m400 (APM X-Gene) and > a Cavium ThunderX CRB. Are you fairly confident all of this is going to make it upstream? +static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_type type, + void *arg) +{ + const struct arch_timer_erratum_workaround *wa; + ate_match_fn_t match_fn = NULL; + + if (static_branch_unlikely(&arch_timer_read_ool_enabled)) + return; + + switch (type) { + case ate_match_dt: + match_fn = arch_timer_check_dt_erratum; + break; + } + + wa = arch_timer_iterate_errata(type, match_fn, arg); + if (!wa) + return; + + arch_timer_enable_workaround(wa); + pr_info("Enabling global workaround for %s\n", wa->desc); +} This function makes me uncomfortable, because match_fn could conceivably be NULL when arch_timer_iterate_errata() is called, and that function doesn't check for NULL (I suspect that the initialization of match_fn to NULL is here was added just to silence a compiler warning). Not that it looks like any of the callers ultimately end up calling it in a way where it end up NULL, but it's not difficult to imagine that happening. I'd feel a lot better about it if the switch had a default case which just returned and possibly provided a warning, or something like that. Thanks, Seth
On Fri, Mar 24, 2017 at 8:02 AM, Seth Forshee <seth.forshee@canonical.com> wrote: > On Thu, Mar 23, 2017 at 08:17:31PM -0600, dann frazier wrote: >> This addresses LP: #1675509. >> >> The HiSilicon D05 board and systems based on Cortex-A73 each have >> errata for their architected timers which can cause reads to return >> bogus data. One of the arm_arch_timer maintainers has developed an >> errata infrastructure to enable workarounds for these. I've backported >> the v2 revision from the developer's tree to the zesty kernel and >> added the relevant ACKs from the parent tree maintainer from the >> mailing list. I'll continue to track the review process and watch for >> any fixes relevant to Ubuntu. >> >> Tested on HiSilicon D05; regression tested on HP m400 (APM X-Gene) and >> a Cavium ThunderX CRB. > > Are you fairly confident all of this is going to make it upstream? I am confident that this will evolve to be the series that lands upstream. The author is one of the driver maintainers, who has rejected previous errata-specific workarounds in favor of developing this workaround infrastructure. It is in active review by one of the clocksource maintainers (I've added his ACKs to these commits where supplied). The matching process is currently under discussion between the driver & subsystem maintainers: https://www.spinics.net/lists/arm-kernel/msg571498.html > +static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_type type, > + void *arg) > +{ > + const struct arch_timer_erratum_workaround *wa; > + ate_match_fn_t match_fn = NULL; > + > + if (static_branch_unlikely(&arch_timer_read_ool_enabled)) > + return; > + > + switch (type) { > + case ate_match_dt: > + match_fn = arch_timer_check_dt_erratum; > + break; > + } > + > + wa = arch_timer_iterate_errata(type, match_fn, arg); > + if (!wa) > + return; > + > + arch_timer_enable_workaround(wa); > + pr_info("Enabling global workaround for %s\n", wa->desc); > +} > > This function makes me uncomfortable, because match_fn could conceivably > be NULL when arch_timer_iterate_errata() is called, and that function > doesn't check for NULL (I suspect that the initialization of match_fn to > NULL is here was added just to silence a compiler warning). Not that it > looks like any of the callers ultimately end up calling it in a way > where it end up NULL, but it's not difficult to imagine that happening. *nod* > I'd feel a lot better about it if the switch had a default case which > just returned and possibly provided a warning, or something like that. OK - I'll send that feedback upstream and implement a local version of that suggestion for my series. -dann
This addresses LP: #1675509. The HiSilicon D05 board and systems based on Cortex-A73 each have errata for their architected timers which can cause reads to return bogus data. One of the arm_arch_timer maintainers has developed an errata infrastructure to enable workarounds for these. I've backported the v2 revision from the developer's tree to the zesty kernel and added the relevant ACKs from the parent tree maintainer from the mailing list. I'll continue to track the review process and watch for any fixes relevant to Ubuntu. Tested on HiSilicon D05; regression tested on HP m400 (APM X-Gene) and a Cavium ThunderX CRB. The following changes since commit 0cf75d7435609372dc1e3c2f32c4cb82a10189ce: pinctrl: intel: Add Intel Gemini Lake pin controller support (2017-03-22 09:23:38 -0600) are available in the git repository at: git://git.launchpad.net/~dannf/ubuntu/+source/linux/+git/linux arm_arch_timer-erratum for you to fetch changes up to 690752cbb5ebfd7b82b5e2bec6c87e90393fd5b6: UBUNTU: SAUCE: arm64: arch_timer: Add HISILICON_ERRATUM_161010101 ACPI matching data (2017-03-23 20:14:15 -0600) ---------------------------------------------------------------- Marc Zyngier (18): UBUNTU: SAUCE: arm64: Allow checking of a CPU-local erratum UBUNTU: SAUCE: arm64: Add CNTVCT_EL0 trap handler UBUNTU: SAUCE: arm64: Define Cortex-A73 MIDR UBUNTU: SAUCE: arm64: cpu_errata: Allow an erratum to be match for all revisions of a core UBUNTU: SAUCE: arm64: cpu_errata: Add capability to advertise Cortex-A73 erratum 858921 UBUNTU: SAUCE: arm64: arch_timer: Add infrastructure for multiple erratum detection methods UBUNTU: SAUCE: arm64: arch_timer: Add erratum handler for globally defined capability UBUNTU: SAUCE: arm64: arch_timer: Add erratum handler for CPU-specific capability UBUNTU: SAUCE: arm64: arch_timer: Move arch_timer_reg_read/write around UBUNTU: SAUCE: arm64: arch_timer: Get rid of erratum_workaround_set_sne UBUNTU: SAUCE: arm64: arch_timer: Rework the set_next_event workarounds UBUNTU: SAUCE: arm64: arch_timer: Make workaround methods optional UBUNTU: SAUCE: arm64: arch_timer: Allows a CPU-specific erratum to only affect a subset of CPUs UBUNTU: SAUCE: arm64: arch_timer: Move clocksource_counter and co around UBUNTU: SAUCE: arm64: arch_timer: Enable CNTVCT_EL0 trap if workaround is enabled UBUNTU: SAUCE: arm64: arch_timer: Workaround for Cortex-A73 erratum 858921 UBUNTU: SAUCE: arm64: arch_timer: Allow erratum matching with ACPI OEM information UBUNTU: SAUCE: arm64: arch_timer: Add HISILICON_ERRATUM_161010101 ACPI matching data Mark Rutland (1): arm64: ptrace: add XZR-safe regs accessors dann frazier (1): UBUNTU: [Config] CONFIG_ARM64_ERRATUM_858921=y Documentation/arm64/silicon-errata.txt | 1 + arch/arm64/include/asm/arch_timer.h | 44 ++- arch/arm64/include/asm/cpucaps.h | 3 +- arch/arm64/include/asm/cputype.h | 2 + arch/arm64/include/asm/esr.h | 2 + arch/arm64/include/asm/ptrace.h | 20 ++ arch/arm64/kernel/cpu_errata.c | 15 + arch/arm64/kernel/cpufeature.c | 13 +- arch/arm64/kernel/traps.c | 14 + debian.master/config/annotations | 2 + debian.master/config/config.common.ubuntu | 1 + drivers/clocksource/Kconfig | 11 + drivers/clocksource/arm_arch_timer.c | 535 +++++++++++++++++++++--------- 13 files changed, 494 insertions(+), 169 deletions(-)