On 1/23/2025 10:01 AM, Fontenot, Nathan wrote: > On 1/21/2025 5:14 PM, Ira Weiny wrote: >> Fontenot, Nathan wrote: >>> On 1/16/2025 4:28 PM, Ira Weiny wrote: >>>> Nathan Fontenot wrote: >>>>> In order to handle registering hmem devices for SOFT RESERVE reources >>>> ^^^^^^^^^ >>>> resources >>>> >>>>> that are added late in boot update the hmem_register_resource(), >>>>> hmem_register_device(), and walk_hmem_resources() interfaces. >>>>> >>>>> Remove the target_nid arg to hmem_register_resource(). The target nid >>>>> value is calculated from the resource start address and not used until >>>>> registering a device for the resource. Move the target nid calculation >>>>> to hmem_register_device(). >>>>> >>>>> To allow for registering hmem devices outside of the hmem dax driver >>>>> probe routine save the dax hmem platform driver during probe. The >>>>> hmem_register_device() interface can then drop the host and target >>>>> nid parameters. >>>>> >>>>> There should be no functional changes. >>>>> >>>>> Signed-off-by: Nathan Fontenot <nathan.fontenot@xxxxxxx> [ snip ] >>>>> diff --git a/drivers/dax/hmem/hmem.c b/drivers/dax/hmem/hmem.c >>>>> index 5e7c53f18491..088f4060d4d5 100644 >>>>> --- a/drivers/dax/hmem/hmem.c >>>>> +++ b/drivers/dax/hmem/hmem.c >>>>> @@ -9,6 +9,8 @@ >>>>> static bool region_idle; >>>>> module_param_named(region_idle, region_idle, bool, 0644); >>>>> >>>>> +static struct platform_device *dax_hmem_pdev; >>>> >>>> I don't think you can assume there is only ever 1 hmem platform device. >>>> >>>> hmat_register_target_devices() in particular iterates multiple memory >>>> regions and will create more than one. >>>> >>>> What am I missing? >>> >>> You may be correct that there can be more than one hmem platform device. >>> I was making this change based on a comment from Dan that it may not matter >>> which platform device these are created against. >> >> If that is true I think there should be a big comment around this code >> explaining why it is ok to have the platform device being allocated in >> this call unregistered when a different platform device (host) is >> released. >> >> IOW hmem_register_device() calls two devm_*() functions using host as the >> device used to trigger an action. It is not entirely clear to me why that >> change is safe here. >> >>> >>> I could be wrong in that assumption. If so we'll need to figure lout how to >>> determine which platform device a soft reserve resource would be created >>> against when they are added later in boot from a notification by the >>> srmem notification chain. >> >> I see that it would be more difficult to track. And I'm ok if it really >> does work. But just looking at the commit message and code I don't see >> how this does not at least introduce a functional change. > > I'm going to go back and take a look at this again. I went this direction > using the approach of having the srmem notification chain. The dax driver > then adds soft reserves outside of a probe routine and don't have a > platform device associated with them. > Digging back into this, the dax driver only creates one platform device. During hmem_register_resource (which is what hmat_register_target_devices() calls for each resource) the dax hmem driver will only create a platform device when the first resource is registered. Each resource that is passed to hmem_register_resource() is added to a resource tree internal to the dax/hmem driver that is eventually walked by the dax hmem driver probe routine. Now that I understand this better I am confident that saving a pointer to the dax hmem platform device is safe. I'll include this information in the commit log for the next version of the patch. -Nathan
