[PULL][Zesty] Errata workarounds for arm_arch_timer

[dann frazier]

On Fri, Mar 24, 2017 at 8:02 AM, Seth Forshee
<seth.forshee at 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