Re: [PATCH v6 11/12] cxl/region: Add memory hotplug notifier for cxl region

[Dave Jiang <dave.jiang@intel.com>]

On 3/6/24 7:53 AM, Jonathan Cameron wrote:
> On Tue, 20 Feb 2024 16:12:40 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> When the CXL region is formed, the driver would computed the performance
>> data for the region. However this data is not available at the node data
>> collection that has been populated by the HMAT during kernel
>> initialization. Add a memory hotplug notifier to update the access
>> coordinates to the 'struct memory_target' context kept by the
>> HMAT_REPORTING code.
>>
>> Add CXL_CALLBACK_PRI for a memory hotplug callback priority. Set the
>> priority number to be called before HMAT_CALLBACK_PRI. The CXL update must
>> happen before hmat_callback().
>>
>> A new HMAT_REPORING helper hmat_update_target_coordinates() is added in
> 
> REPORTING
> 
>> order to allow CXL to update the memory_target access coordinates.
>>
>> A new ext_updated member is added to the memory_target to indicate that
>> the access coordinates within the memory_target has been updated by an
>> external agent such as CXL. This prevents data being overwritten by the
>> hmat_update_target_attrs() triggered by hmat_callback().
>>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Rafael J. Wysocki <rafael@kernel.org>
>> Reviewed-by: "Huang, Ying" <ying.huang@intel.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> One missing error check and another trivial comment.
> With error check handled.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> 
> 
>> +static void remove_coord_notifier(void *data)
>> +{
>> +	struct notifier_block *memory_notifier = data;
>> +
>> +	unregister_memory_notifier(memory_notifier);
> 
> Trivial but no real loss of info if you do

ok will change

> 
> 	unregister_memory_notifier(data);
> 
>> +}
>> +
>>  static int cxl_region_probe(struct device *dev)
>>  {
>>  	struct cxl_region *cxlr = to_cxl_region(dev);
>> @@ -3081,6 +3151,12 @@ static int cxl_region_probe(struct device *dev)
>>  		goto out;
>>  	}
>>  
>> +	cxlr->memory_notifier.notifier_call = cxl_region_perf_attrs_callback;
>> +	cxlr->memory_notifier.priority = CXL_CALLBACK_PRI;
>> +	register_memory_notifier(&cxlr->memory_notifier);
>> +	rc = devm_add_action_or_reset(&cxlr->dev, remove_coord_notifier,
>> +				      &cxlr->memory_notifier);
>> +
> 
> Check rc?  Very unlikely to fail, but you never know..

It's actually checked a few lines down after cxl_region_sem gets released. Probably too far away for diff to include it.

DJ
> 
>>  	/*
>>  	 * From this point on any path that changes the region's state away from
>>  	 * CXL_CONFIG_COMMIT is also responsible for releasing the driver.
> 
>