On Thu, Feb 08, 2018 at 12:32:56PM +0000, Robin Murphy wrote: > On 08/02/18 12:01, Dave Martin wrote: > >On Thu, Feb 08, 2018 at 10:53:52AM +0000, Suzuki K Poulose wrote: > >>On 07/02/18 10:39, Dave Martin wrote: > >>>On Wed, Jan 31, 2018 at 06:28:03PM +0000, Suzuki K Poulose wrote: > >>>>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. > >> > >>> > >>>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