| Message ID | 20151102043009.6610.83896.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Sun, Nov 01, 2015 at 11:30:10PM -0500, Dan Williams wrote: > This masking prevents access to the end of the device via dax_do_io(), > and is unnecessary as arch_add_memory() would have rejected an unaligned > allocation. > > Cc: <stable@vger.kernel.org> > Cc: Ross Zwisler <ross.zwisler@linux.intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/nvdimm/pmem.c | 17 +++-------------- > 1 file changed, 3 insertions(+), 14 deletions(-) > > diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c > index e46988fbdee5..93472953e231 100644 > --- a/drivers/nvdimm/pmem.c > +++ b/drivers/nvdimm/pmem.c > @@ -100,26 +100,15 @@ static int pmem_rw_page(struct block_device *bdev, sector_t sector, > } > > static long pmem_direct_access(struct block_device *bdev, sector_t sector, > - void __pmem **kaddr, unsigned long *pfn) > + void __pmem **kaddr, pfn_t *pfn) It seems kind of weird to change only this instance of direct_access() to have the last argument as a pfn_t instead of an unsigned long? If pfn_t is more descriptive (I think it is) should we update the definition in struct block_device_operations and all the other implementors of direct_access as well? If that's touching too much, let's do them all together later, but let's not change one now and have them be inconsistent. > { > struct pmem_device *pmem = bdev->bd_disk->private_data; > resource_size_t offset = sector * 512 + pmem->data_offset; > - resource_size_t size; > > - if (pmem->data_offset) { > - /* > - * Limit the direct_access() size to what is covered by > - * the memmap > - */ > - size = (pmem->size - offset) & ~ND_PFN_MASK; > - } else > - size = pmem->size - offset; > - > - /* FIXME convert DAX to comprehend that this mapping has a lifetime */ > *kaddr = pmem->virt_addr + offset; > - *pfn = (pmem->phys_addr + offset) >> PAGE_SHIFT; > + *pfn = __phys_to_pfn(pmem->phys_addr + offset, pmem->pfn_flags); __phys_to_pfn() only takes a single argument (the paddr) in v4.3, jens/for-4.4/integrity and in nvdimm/libnvdimm-for-next. Is this second argument of pfn_flags actually correct?
On Tue, Nov 3, 2015 at 11:32 AM, Ross Zwisler <ross.zwisler@linux.intel.com> wrote: > On Sun, Nov 01, 2015 at 11:30:10PM -0500, Dan Williams wrote: >> This masking prevents access to the end of the device via dax_do_io(), >> and is unnecessary as arch_add_memory() would have rejected an unaligned >> allocation. >> >> Cc: <stable@vger.kernel.org> >> Cc: Ross Zwisler <ross.zwisler@linux.intel.com> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> >> --- >> drivers/nvdimm/pmem.c | 17 +++-------------- >> 1 file changed, 3 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c >> index e46988fbdee5..93472953e231 100644 >> --- a/drivers/nvdimm/pmem.c >> +++ b/drivers/nvdimm/pmem.c >> @@ -100,26 +100,15 @@ static int pmem_rw_page(struct block_device *bdev, sector_t sector, >> } >> >> static long pmem_direct_access(struct block_device *bdev, sector_t sector, >> - void __pmem **kaddr, unsigned long *pfn) >> + void __pmem **kaddr, pfn_t *pfn) > > It seems kind of weird to change only this instance of direct_access() to have > the last argument as a pfn_t instead of an unsigned long? If pfn_t is more > descriptive (I think it is) should we update the definition in struct > block_device_operations and all the other implementors of direct_access as > well? If that's touching too much, let's do them all together later, but > let's not change one now and have them be inconsistent. > Oh, nice catch, that's just a mistake when I moved this patch earlier in the series... and I wonder why 0day didn't complain about it? In any event, will fix. >> { >> struct pmem_device *pmem = bdev->bd_disk->private_data; >> resource_size_t offset = sector * 512 + pmem->data_offset; >> - resource_size_t size; >> >> - if (pmem->data_offset) { >> - /* >> - * Limit the direct_access() size to what is covered by >> - * the memmap >> - */ >> - size = (pmem->size - offset) & ~ND_PFN_MASK; >> - } else >> - size = pmem->size - offset; >> - >> - /* FIXME convert DAX to comprehend that this mapping has a lifetime */ >> *kaddr = pmem->virt_addr + offset; >> - *pfn = (pmem->phys_addr + offset) >> PAGE_SHIFT; >> + *pfn = __phys_to_pfn(pmem->phys_addr + offset, pmem->pfn_flags); > > __phys_to_pfn() only takes a single argument (the paddr) in v4.3, > jens/for-4.4/integrity and in nvdimm/libnvdimm-for-next. Is this second > argument of pfn_flags actually correct? Yeah, this shouldn't even compile. Thanks Ross!
diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c index e46988fbdee5..93472953e231 100644 --- a/drivers/nvdimm/pmem.c +++ b/drivers/nvdimm/pmem.c @@ -100,26 +100,15 @@ static int pmem_rw_page(struct block_device *bdev, sector_t sector, } static long pmem_direct_access(struct block_device *bdev, sector_t sector, - void __pmem **kaddr, unsigned long *pfn) + void __pmem **kaddr, pfn_t *pfn) { struct pmem_device *pmem = bdev->bd_disk->private_data; resource_size_t offset = sector * 512 + pmem->data_offset; - resource_size_t size; - if (pmem->data_offset) { - /* - * Limit the direct_access() size to what is covered by - * the memmap - */ - size = (pmem->size - offset) & ~ND_PFN_MASK; - } else - size = pmem->size - offset; - - /* FIXME convert DAX to comprehend that this mapping has a lifetime */ *kaddr = pmem->virt_addr + offset; - *pfn = (pmem->phys_addr + offset) >> PAGE_SHIFT; + *pfn = __phys_to_pfn(pmem->phys_addr + offset, pmem->pfn_flags); - return size; + return pmem->size - offset; } static const struct block_device_operations pmem_fops = {
This masking prevents access to the end of the device via dax_do_io(), and is unnecessary as arch_add_memory() would have rejected an unaligned allocation. Cc: <stable@vger.kernel.org> Cc: Ross Zwisler <ross.zwisler@linux.intel.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/nvdimm/pmem.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-)