Re: [PATCH 2/3] nvdimm/pmem: move dax_attribute_group from dax to pmem

On Mon, Sep 20, 2021 at 3:51 PM Ira Weiny [off-list ref] wrote:

On Mon, Sep 20, 2021 at 09:27:25AM +0200, Christoph Hellwig wrote:

...

quoted

diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index ef4950f808326..bbeb3f46db157 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c

@@ -328,6 +328,49 @@ static const struct dax_operations pmem_dax_ops = {
      .zero_page_range = pmem_dax_zero_page_range,
 };

+static ssize_t write_cache_show(struct device *dev,
+             struct device_attribute *attr, char *buf)
+{
+     struct pmem_device *pmem = dev_to_disk(dev)->private_data;

I want to say this should be dax_get_private()...  However, looking at the use

No, this wants to do from @dev to @dax_dev. dax_get_private() assumes
that @dax_dev is already known. Also, in this case @dev is the gendisk
device, so this is a gendisk-to-dax-device with special knowledge that
the gendisk is for a pmem device.

of dax_get_private() not a single caller checks for NULL!  :-(

All the callers are correctly assuming that their usage is before kill_dax().

So now I wonder why dax_get_private() exists...  :-/

It exists so that the definition of 'struct dax_device' can remain
private, as no one should be directly mucking with dax_device
internals outside of the provided APIs.

A quick history search does not make anything apparent.  When the DAXDEV_ALIVE
check was added to dax_get_private() no callers were changed to account for a
potential NULL return.

Dan?

I double checked, but this all looks ok to me.