Re: [PATCH v2 6/11] Update CPU Maps

[Nathan Fontenot <nfont@linux.vnet.ibm.com>]

On 04/03/2013 11:42 PM, Paul Mackerras wrote:
> On Mon, Mar 25, 2013 at 01:57:08PM -0500, Nathan Fontenot wrote:
>> From: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
>>
>> Platform events such as partition migration or the new PRRN firmware
>> feature can cause the NUMA characteristics of a CPU to change, and these
>> changes will be reflected in the device tree nodes for the affected
>> CPUs.
>>
>> This patch registers a handler for Open Firmware device tree updates
>> and reconfigures the CPU and node maps whenever the associativity
>> changes. Currently, this is accomplished by marking the affected CPUs in
>> the cpu_associativity_changes_mask and allowing
>> arch_update_cpu_topology() to retrieve the new associativity information
>> using hcall_vphn().
>>
>> Protecting the NUMA cpu maps from concurrent access during an update
>> operation will be addressed in a subsequent patch in this series.
>>
>> Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
> 
> [snip]
> 
>> +	if (firmware_has_feature(OV5_PRRN)) {
> 
> Shouldn't this be FW_FEATURE_PRRN?  How well has this patch been
> tested? :-/

Yes this should have been FW_FEATURE_PRRN.

I know I tested this and it took some digging to find out why my test succeeded
even though I used the wrong value in the call to firmware_has_feature. The value
for OV5_PRRN (0x0540) just happens to match some of he bits that are set in
powerpc_firmware_features bit field and cause the check to return true. My test
worked out of sheer luck. I'll update this patch and re-test to ensure it works
with the real value.

This does make me think, should we update firmware_has_feature() to avoid this
kind of false positive in the future. something like

#define firmware_has_feature(feature)                                              \
        ((FW_FEATURE_ALWAYS & (feature)) == (feature) ||                           \
         (FW_FEATURE_POSSIBLE & powerpc_firmware_features & (feature)) == (feature)

-Nathan