On 24/02/26 12:32PM, Jonathan Cameron wrote: > On Fri, 23 Feb 2024 11:41:49 -0600 > John Groves <John@xxxxxxxxxx> wrote: > > > Notes about this commit: > > > > * These methods are based somewhat loosely on pmem_dax_ops from > > drivers/nvdimm/pmem.c > > > > * dev_dax_direct_access() is returns the hpa, pfn and kva. The kva was > > newly stored as dev_dax->virt_addr by dev_dax_probe(). > > > > * The hpa/pfn are used for mmap (dax_iomap_fault()), and the kva is used > > for read/write (dax_iomap_rw()) > > > > * dev_dax_recovery_write() and dev_dax_zero_page_range() have not been > > tested yet. I'm looking for suggestions as to how to test those. > > > > Signed-off-by: John Groves <john@xxxxxxxxxx> > > --- > > drivers/dax/bus.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 107 insertions(+) > > > > diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c > > index 664e8c1b9930..06fcda810674 100644 > > --- a/drivers/dax/bus.c > > +++ b/drivers/dax/bus.c > > @@ -10,6 +10,12 @@ > > #include "dax-private.h" > > #include "bus.h" > > > > +#if IS_ENABLED(CONFIG_DEV_DAX_IOMAP) > > +#include <linux/backing-dev.h> > > +#include <linux/pfn_t.h> > > +#include <linux/range.h> > > +#endif > > + > > Is it worth avoiding includes based on config? Probably not. Just trying to demonstrate that I can be tedious :D I'll drop the #if unless somebody disagrees. > > > static DEFINE_MUTEX(dax_bus_lock); > > > > #define DAX_NAME_LEN 30 > > @@ -1349,6 +1355,101 @@ __weak phys_addr_t dax_pgoff_to_phys(struct dev_dax *dev_dax, pgoff_t pgoff, > > } > > EXPORT_SYMBOL_GPL(dax_pgoff_to_phys); > > > > +#if IS_ENABLED(CONFIG_DEV_DAX_IOMAP) > > + > > > + > > +static long __dev_dax_direct_access(struct dax_device *dax_dev, pgoff_t pgoff, > > + long nr_pages, enum dax_access_mode mode, void **kaddr, > > + pfn_t *pfn) > > +{ > > + struct dev_dax *dev_dax = dax_get_private(dax_dev); > > + size_t dax_size = dev_dax_size(dev_dax); > > + size_t size = nr_pages << PAGE_SHIFT; > > + size_t offset = pgoff << PAGE_SHIFT; > > + phys_addr_t phys; > > + u64 virt_addr = dev_dax->virt_addr + offset; > > + pfn_t local_pfn; > > + u64 flags = PFN_DEV|PFN_MAP; > > + > > + WARN_ON(!dev_dax->virt_addr); /* virt_addr must be saved for direct_access */ > Fair enough, but from local code point of view, does it make sense to check this > if !kaddr as we won't use this. Hmm. This gets called with kaddr=NULL for mmap faults, and with non-NULL kaddr for read/write (which need the virt_addr to do a memcpy variant). If dev_dax->virt-addr is NULL, mmap will work but read/write will hork. I lean toward keeping the warning. With these updates, it's broken if read/write are broken. > > + > > + phys = dax_pgoff_to_phys(dev_dax, pgoff, nr_pages << PAGE_SHIFT); > > + > > + if (kaddr) > > + *kaddr = (void *)virt_addr; > > Back to earlier comment on virt_addr as a void *. Definitely looking like > that would be more accurate and simpler! Also not much point in computing > virt_addr unless kaddr is good. Yes, done (the void *). the computation is copied directly from drivers/nvdimm/__pmem_direct_access() - which does not warn if virt_addr is null. Actually I suppose this code should just trust that dev_dax_probe sets virt_addr, and not warn? So I'm now contradicting myself above... > > > + > > + local_pfn = phys_to_pfn_t(phys, flags); /* are flags correct? */ > If you aren't going to do anything with it for !pfn, move it under the if (pfn). > > > + if (pfn) > > + *pfn = local_pfn; > > + > > + /* This the valid size at the specified address */ > > + return PHYS_PFN(min_t(size_t, size, dax_size - offset)); > > +} > > + > > > + > > +static const struct dax_operations dev_dax_ops = { > > + .direct_access = dev_dax_direct_access, > > + .zero_page_range = dev_dax_zero_page_range, > > + .recovery_write = dev_dax_recovery_write, > > +}; > > + > > +#endif /* IS_ENABLED(CONFIG_DEV_DAX_IOMAP) */ > > + > > struct dev_dax *devm_create_dev_dax(struct dev_dax_data *data) > > { > > struct dax_region *dax_region = data->dax_region; > > @@ -1404,11 +1505,17 @@ struct dev_dax *devm_create_dev_dax(struct dev_dax_data *data) > > } > > } > > > > If we were to make this > > if (IS_ENABLED(CONFIG_DEV_DAX_IOMAP)) > > etc can we avoid the ifdef stuff above and let dead code removal deal with it? > Might need a few stubs - I haven't tried. Better, thanks. No stubs needed. > > > +#if IS_ENABLED(CONFIG_DEV_DAX_IOMAP) > > + /* holder_ops currently populated separately in a slightly hacky way */ > > + dax_dev = alloc_dax(dev_dax, &dev_dax_ops); > > +#else > > /* > > * No dax_operations since there is no access to this device outside of > > * mmap of the resulting character device. > > */ > > dax_dev = alloc_dax(dev_dax, NULL); > > +#endif Thanks! John