Re: [PATCH v2 0/2] arm64: Run enable method for errata work arounds on late CPUs

[Dave Martin]

On Wed, Jan 17, 2018 at 05:42:19PM +0000, Suzuki K Poulose wrote:
> We issue the enable() call back for all CPU hwcaps capabilities
> available on the system, on all the CPUs. So far we have ignored
> the argument passed to the call back, which had a prototype to
> accept a "void *" for use with on_each_cpu() and later with
> stop_machine(). However, with commit 0a0d111d40fd1
> ("arm64: cpufeature: Pass capability structure to ->enable callback"),
> there are some users of the argument who wants the matching capability
> struct pointer where there are multiple matching criteria for a single
> capability. Changing the prototype is quite an invasive change and

It's not that bad, though it's debatable whether it's really worth
it.  See the appended patch below.

> will be part of a future series. For now, add a comment to clarify
> what is expected.

With the type change, it becomes more obvious what should be passed,
and the comment can probably be trimmed down.  I omit the comment
from my patch (I'm lazy).

Without it, I would prefer a comment alongside the (void *) cast,
something like "enable methods expect this to be passed as a void *,
for compatibility with stop_machine()".

For consistency, the stop_machine() call should also be changed to
pass the cap pointer instead of NULL, even if we don't actually rely
on that for any purpose today -- it will help avoid surprises in the
future.  (My patch does makes an equivalent change.)

[...]

> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index ac67cfc2585a..c049e28274d4 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -97,7 +97,14 @@ struct arm64_cpu_capabilities {
>  	u16 capability;
>  	int def_scope;			/* default scope */
>  	bool (*matches)(const struct arm64_cpu_capabilities *caps, int scope);
> -	int (*enable)(void *);		/* Called on all active CPUs */
> +	/*
> +	 * For each @capability set in CPU hwcaps, @enable() is called on all
> +	 * active CPUs with const struct arm64_cpu_capabilities * as argument.

The declaration tells us the type, so we don't need that in the comment.
But what object should the pointer point to?

> +	 * It is upto the callback (especially when multiple entries for the

up to

> +	 * same capability exists) to determine if any action should be taken
> +	 * based on @matches() applies to thie CPU.
> +	 */
> +	int (*enable)(void *caps);
>  	union {
>  		struct {	/* To be used for erratum handling only */
>  			u32 midr_model;

Cheers
---Dave


--8<--