On Thu, Feb 08, 2018 at 12:32:56PM +0000, Robin Murphy wrote:

On 08/02/18 12:01, Dave Martin wrote:

quoted

On Thu, Feb 08, 2018 at 10:53:52AM +0000, Suzuki K Poulose wrote:

quoted

On 07/02/18 10:39, Dave Martin wrote:

quoted

On Wed, Jan 31, 2018 at 06:28:03PM +0000, Suzuki K Poulose wrote:

quoted

Some capabilities have different criteria for detection and associated
actions based on the matching criteria, even though they all share the
same capability bit. So far we have used multiple entries with the same
capability bit to handle this. This is prone to errors, as the
cpu_enable is invoked for each entry, irrespective of whether the
detection rule applies to the CPU or not. And also this complicates
other helpers, e.g, __this_cpu_has_cap.

This patch adds a wrapper entry to cover all the possible variations
of a capability and ensures :
 1) The capabilitiy is set when at least one of the entry detects
 2) Action is only taken for the entries that detects.

I guess this means that where we have a single cpu_enable() method
but complex match criteria that require multiple entries, then that
cpu_enable() method might get called multiple times on a given CPU.

A CPU executes cpu_enable() only for the "matching" entries in the list,
unlike earlier. So as long as there is a single entry "matching" the given
CPU, the cpu_enable() will not be duplicated. May be we *should* mandate
that a CPU cannot be matched by multiple entries.

quoted

Could be worth a comment if cpu_enable() methods must be robust
against this.

Could we say something like:

"Where a single capability has multiple entries, mutiple cpu_enable()
methods may be called if more than one entry matches.  Where this is
not desired, care should be taken to ensure that the entries are
mutually exclusive: for example, two entries for a single capability
that match on MIDR should be configured to match MIDR ranges that do
not overlap."

This is more verbose than I would like however.  Maybe there's a simpler
way to say it.

If we're not also talking about a capability having mutually exclusive
enable methods (it doesn't seem so, but I'm not necessarily 100% clear), how
about:

"If a cpu_enable() method is associated with multiple matches for a single
capability, care should be taken that either the match criteria are mutually
exclusive, or that the method is robust against being called multiple
times."

This is one reason why I wondered if we could pull cpu_enable() out of
the match criteria struct so that we don't duplicate it: in that case
there's no chance of multiple incompatible cpu_enable() methods.

But Suzuki is right that this would be a somewhat invasive change at
this point, and I think it's sufficient to warn about the possibility
of incompatible cpu_enable()s in a comment.

Otherwise, your text sounds good.

Cheers
---Dave