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

From: Dan Williams <dan.j.williams@intel.com>
To: Ira Weiny <ira.weiny@intel.com>
Cc: Christoph Hellwig <hch@lst.de>, Jens Axboe <axboe@kernel.dk>,
	Vishal Verma <vishal.l.verma@intel.com>,
	 Dave Jiang <dave.jiang@intel.com>,
	linux-block@vger.kernel.org,
	 Linux NVDIMM <nvdimm@lists.linux.dev>
Subject: Re: [PATCH 2/3] nvdimm/pmem: move dax_attribute_group from dax to pmem
Date: Mon, 20 Sep 2021 16:36:05 -0700	[thread overview]
Message-ID: <CAPcyv4jn=HJRSMKPCFZzHmMoWD2x2EGjWr0O8mB63RFHj_jDvg@mail.gmail.com> (raw)
In-Reply-To: <20210920225125.GY3169279@iweiny-DESK2.sc.intel.com>

On Mon, Sep 20, 2021 at 3:51 PM Ira Weiny <ira.weiny@intel.com> wrote:
>
> On Mon, Sep 20, 2021 at 09:27:25AM +0200, Christoph Hellwig wrote:
>
> ...
>
> > 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.