On Thu, 3 Jul 2025 13:50:17 -0500
John Groves <John@Groves.net> wrote:
> Save the kva from memremap because we need it for iomap rw support.
>
> Prior to famfs, there were no iomap users of /dev/dax - so the virtual
> address from memremap was not needed.
>
> Also: in some cases dev_dax_probe() is called with the first
> dev_dax->range offset past the start of pgmap[0].range. In those cases
> we need to add the difference to virt_addr in order to have the physaddr's
> in dev_dax->ranges match dev_dax->virt_addr.
>
> This happens with devdax devices that started as pmem and got converted
> to devdax. I'm not sure whether the offset is due to label storage, or
> page tables, but this works in all known cases.
Clearly a question we need to resolve to understand if this is correct
handling.
>
> Signed-off-by: John Groves <john@groves.net>
> ---
> drivers/dax/dax-private.h | 1 +
> drivers/dax/device.c | 15 +++++++++++++++
> 2 files changed, 16 insertions(+)
>
> diff --git a/drivers/dax/dax-private.h b/drivers/dax/dax-private.h
> index 0867115aeef2..2a6b07813f9f 100644
> --- a/drivers/dax/dax-private.h
> +++ b/drivers/dax/dax-private.h
> @@ -81,6 +81,7 @@ struct dev_dax_range {
> struct dev_dax {
> struct dax_region *region;
> struct dax_device *dax_dev;
> + void *virt_addr;
> unsigned int align;
> int target_node;
> bool dyn_id;
> diff --git a/drivers/dax/device.c b/drivers/dax/device.c
> index 29f61771fef0..583150478dcc 100644
> --- a/drivers/dax/device.c
> +++ b/drivers/dax/device.c
> @@ -372,6 +372,7 @@ static int dev_dax_probe(struct dev_dax *dev_dax)
> struct dax_device *dax_dev = dev_dax->dax_dev;
> struct device *dev = &dev_dax->dev;
> struct dev_pagemap *pgmap;
> + u64 data_offset = 0;
> struct inode *inode;
> struct cdev *cdev;
> void *addr;
> @@ -426,6 +427,20 @@ static int dev_dax_probe(struct dev_dax *dev_dax)
> if (IS_ERR(addr))
> return PTR_ERR(addr);
>
> + /* Detect whether the data is at a non-zero offset into the memory */
> + if (pgmap->range.start != dev_dax->ranges[0].range.start) {
Using pgmap->range.start here but then getting to the same (I think)
with dev_dax->pgmap[0].range.start is rather inconsistent.
Also, perhaps drag the assignment of phys and pgmap_phys out of this
scope so that you can use them for the condition check above and
then reuse the same in here.
> + u64 phys = dev_dax->ranges[0].range.start;
> + u64 pgmap_phys = dev_dax->pgmap[0].range.start;
> + u64 vmemmap_shift = dev_dax->pgmap[0].vmemmap_shift;
> +
> + if (!WARN_ON(pgmap_phys > phys))
> + data_offset = phys - pgmap_phys;
In the event of the condition above being false.
phys == pgmap_phys and data_offset == 0.
So why not do this unconditionally replacing this block with something like
/* Apply necessary offset */
dev_dax->virt_addr = addr +
(dev_dax->ranges[0].range.start - pgmap->range.start);
> +
> + pr_debug("%s: offset detected phys=%llx pgmap_phys=%llx offset=%llx shift=%llx\n",
> + __func__, phys, pgmap_phys, data_offset, vmemmap_shift);
If it's only used in the print, I'd just put the path to vmemmap_shift directly in here
and probably get to it via pgmap->vmemmap_shift
> + }
> + dev_dax->virt_addr = addr + data_offset;
> +
> inode = dax_inode(dax_dev);
> cdev = inode->i_cdev;
> cdev_init(cdev, &dax_fops);