On Thu, 3 Jul 2025 13:50:17 -0500 John Groves <[email protected]> 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 <[email protected]> > --- > 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);
