On 3/6/24 7:53 AM, Jonathan Cameron wrote:
> On Tue, 20 Feb 2024 16:12:40 -0700
> Dave Jiang <dave.jiang@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Rafael J. Wysocki <rafael@xxxxxxxxxx>
>> Reviewed-by: "Huang, Ying" <ying.huang@xxxxxxxxx>
>> Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
>
> One missing error check and another trivial comment.
> With error check handled.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>
>
>
>> +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.
>
>
