Dan Williams wrote: > Ira Weiny wrote: > > Dan Williams wrote: > > > The reference counting of dax_region objects is needlessly complicated, > > > has lead to confusion [1], and has hidden a bug [2]. Towards cleaning up > > > that mess introduce alloc_dev_dax_id() to minimize the holding of a > > > dax_region reference to only what dev_dax_release() needs, the > > > dax_region->ida. > > > > > > Part of the reason for the mess was the design to dereference a > > > dax_region in all cases in free_dev_dax_id() even if the id was > > > statically assigned by the upper level dax_region driver. Remove the > > > need to call "is_static(dax_region)" by tracking whether the id is > > > dynamic directly in the dev_dax instance itself. > > > > > > With that flag the dax_region pinning and release per dev_dax instance > > > can move to alloc_dev_dax_id() and free_dev_dax_id() respectively. > > > > > > A follow-on cleanup address the unnecessary references in the dax_region > > > setup and drivers. > > > > > > Fixes: 0f3da14a4f05 ("device-dax: introduce 'seed' devices") > > > Link: > > > http://lore.kernel.org/r/20221203095858.612027-1-liuyongqian...@huawei.com > > > [1] > > > Link: > > > http://lore.kernel.org/r/3cf0890b-4eb0-e70e-cd9c-2ecc3d496...@hpe.com [2] > > > Reported-by: Yongqiang Liu <liuyongqian...@huawei.com> > > > Reported-by: Paul Cassella <casse...@hpe.com> > > > Reported-by: Ira Weiny <ira.we...@intel.com> > > > Signed-off-by: Dan Williams <dan.j.willi...@intel.com> > [..] > > > @@ -1326,6 +1340,7 @@ struct dev_dax *devm_create_dev_dax(struct > > > dev_dax_data *data) > > > if (!dev_dax) > > > return ERR_PTR(-ENOMEM); > > > > > > + dev_dax->region = dax_region; > > > > Overall I like that this reference is not needed to be carried and/or > > managed by the callers. > > > > However, here you are referencing the dax_region from the dev_dax in an > > unrelated place to where the reference matters (in id management). > > > > Could alloc_dev_dax_id() change to: > > > > static int alloc_dev_dax_id(struct dev_dax *dev_dax, struct dax_region > > *dax_region) > > { > > ... > > } > > > > Then make this assignment next to where the kref is taken so it is clear > > that this is the only user of the reference? > > > > I did not pick up on the fact this reference was only needed to free the > > id at all in reviewing the code and I think this would make it even more > > clear. > > I hesitate only for symmetry reasons. I.e. that there are many interfaces in > this file, in addition to free_dev_dax_id(), where @dax_region is > implicitly retrieved from the @dev_dax.
Ok but the reason we need this extra reference and for the dax_region to live this long is because the ida within the dax_region. Otherwise the normal device references would be enough, right? Regardless, I've convinced myself this is ok. Reviewed-by: Ira Weiny <ira.we...@intel.com> Ira