lkml.org 
[lkml]   [2015]   [Nov]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 05/15] libnvdimm, pmem: fix size trim in pmem_direct_access()
    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?


    \
     
     \ /
      Last update: 2015-11-03 20:41    [W:4.028 / U:0.708 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site