Re: [PATCH 01/16] arm64: capabilities: Update prototype for enable call back

[Dave Martin]

On Tue, Jan 23, 2018 at 03:38:37PM +0000, Suzuki K Poulose wrote:
> On 23/01/18 14:52, Dave Martin wrote:
> >On Tue, Jan 23, 2018 at 12:27:54PM +0000, Suzuki K Poulose wrote:
> >>From: Dave Martin <dave.martin@xxxxxxx>
> >>
> >>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. Update the prototype for enable to accept a const pointer.
> >>
> >>Cc: Will Deacon <will.deacon@xxxxxxx>
> >>Cc: Robin Murphy <robin.murphy@xxxxxxx>
> >>Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> >>Cc: Mark Rutland <mark.rutland@xxxxxxx>
> >>Cc: Andre Przywara <andre.przywara@xxxxxxx>
> >>Cc: James Morse <james.morse@xxxxxxx>
> >>Reviewed-by: Julien Thierry <julien.thierry@xxxxxxx>
> >>Signed-off-by: Dave Martin <dave.martin@xxxxxxx>
> >>[ Rebased to for-next/core converting more users ]
> >>Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> >>---
> >>  arch/arm64/include/asm/cpufeature.h |  3 ++-
> >>  arch/arm64/include/asm/fpsimd.h     |  4 +++-
> >>  arch/arm64/include/asm/processor.h  |  7 ++++---
> >>  arch/arm64/kernel/cpu_errata.c      | 14 ++++++--------
> >>  arch/arm64/kernel/cpufeature.c      | 16 ++++++++++++----
> >>  arch/arm64/kernel/fpsimd.c          |  3 ++-
> >>  arch/arm64/kernel/traps.c           |  3 ++-
> >>  arch/arm64/mm/fault.c               |  2 +-
> >>  8 files changed, 32 insertions(+), 20 deletions(-)
> >>
> >>diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> >>index ac67cfc2585a..cefbd685292c 100644
> >>--- a/arch/arm64/include/asm/cpufeature.h
> >>+++ b/arch/arm64/include/asm/cpufeature.h
> >>@@ -97,7 +97,8 @@ 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 */
> >>+	/* Called on all active CPUs for all  "available" capabilities */
> >
> >Nit: Odd spacing?  Also, "available" doesn't really make sense for errata
> >workarounds.
> >
> 
> Thanks for spotting, will fix it.
> 
> >Maybe applicable would be a better word?
> >
> 
> There is a subtle difference. If there are two entries for a capability,
> with only one of them matches, we end up calling the enable() for both
> the entries. "Applicable" could potentially be misunderstood, leading
> to assumption that the enable() is called only if that "entry" matches,
> which is not true. I accept that "available" doesn't sound any better either.
> 
> 
> >>+	int (*enable)(const struct arm64_cpu_capabilities *caps);

This probably shouldn't be "caps" here: this argument refers a single
capability, not an array.  Also, this shouldn't be any random capability,
but the one corresponding to the enable method:

	cap->enable(cap) 

(i.e., cap1->enable(cap2) is invalid, and the cpufeature framework won't
do that).

> >Alternatively, if the comment is liable to be ambiguous, maybe it would
> >be better to delete it.  The explicit argument type already makes this
> >more self-documenting than previously.
> 
> I think we still need to make it clear that the enable is called on
> all active CPUs. It is not about the argument anymore.
> 
> How about :
> 
> /*
>  * Called on all active CPUs if the capability associated with
>  * this entry is set.
>  */

Maybe, but now we have the new concept of "setting" a capability.

Really, this is enabling the capability for a CPU, not globally, so
maybe it could be renamed to "cpu_enable".

Could we describe the method in terms of what it is required to do,
as well as the circumstances of the call, e.g.:

/*
 * Take the appropriate actions to enable this capability for this cpu.
 * Every time a cpu is booted, this method is called under stop_machine()
 * for each globally enabled capability.
 */

(I'm hoping that "globally enabled" is meaningful wording, though
perhaps not.)

Also, what does the return value of this method mean?

Previously, the return value was ignored, but other patches in this
series might change that.

[...]

Cheers
---Dave