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

[Suzuki K Poulose]

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);
>
> 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.
 */


> I don't feel that strongly either way though; probably not worth a
> respin unless you have other things to change.
>
> Also please note that I didn't test the original patch here (in case
> I didn't point that out already...)

I think I did test it using the HW DBM feature (see the patch at the end).
However, will do some more testing with it. Thanks for mentioning.

Suzuki