On Thu, Oct 24, 2019 at 2:07 AM Aneesh Kumar K.V <[email protected]> wrote: > > On 10/24/19 7:36 AM, Dan Williams wrote: > > On Thu, Oct 17, 2019 at 12:33 AM Aneesh Kumar K.V > > <[email protected]> wrote: > >> > >> nvdimm core currently maps the full namespace to an ioremap range > >> while probing the namespace mode. This can result in probe failures > >> on architectures that have limited ioremap space. > >> > >> For example, with a large btt namespace that consumes most of I/O remap > >> range, depending on the sequence of namespace initialization, the user can > >> find > >> a pfn namespace initialization failure due to unavailable I/O remap space > >> which nvdimm core uses for temporary mapping. > >> > >> nvdimm core can avoid this failure by only mapping the reserver block area > >> to > > > > s/reserver/reserved/ > > Will fix > > > > > >> check for pfn superblock type and map the full namespace resource only > >> before > >> using the namespace. > > > > Are you going to follow up with the BTT patch that uses this new facility? > > > > I am not yet sure about that. ioremap/vmap area is the way we get a > kernel mapping without struct page backing. What we are suggesting hee > is the ability to have a direct mapped mapping without struct page. I > need to look at closely about what that imply. > > >> > >> Signed-off-by: Aneesh Kumar K.V <[email protected]> > >> --- > >> Changes from v2: > >> * update changelog > >> > >> Changes from V1: > >> * update changelog > >> * update patch based on review feedback. > >> > >> drivers/dax/pmem/core.c | 2 +- > >> drivers/nvdimm/claim.c | 7 +++---- > >> drivers/nvdimm/nd.h | 4 ++-- > >> drivers/nvdimm/pfn.h | 6 ++++++ > >> drivers/nvdimm/pfn_devs.c | 5 ----- > >> drivers/nvdimm/pmem.c | 15 ++++++++++++--- > >> 6 files changed, 24 insertions(+), 15 deletions(-) > >> > >> diff --git a/drivers/dax/pmem/core.c b/drivers/dax/pmem/core.c > >> index 6eb6dfdf19bf..f174dbfbe1c4 100644 > >> --- a/drivers/dax/pmem/core.c > >> +++ b/drivers/dax/pmem/core.c > >> @@ -28,7 +28,7 @@ struct dev_dax *__dax_pmem_probe(struct device *dev, > >> enum dev_dax_subsys subsys) > >> nsio = to_nd_namespace_io(&ndns->dev); > >> > >> /* parse the 'pfn' info block via ->rw_bytes */ > >> - rc = devm_nsio_enable(dev, nsio); > >> + rc = devm_nsio_enable(dev, nsio, info_block_reserve()); > >> if (rc) > >> return ERR_PTR(rc); > >> rc = nvdimm_setup_pfn(nd_pfn, &pgmap); > >> diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c > >> index 2985ca949912..d89d2c039e25 100644 > >> --- a/drivers/nvdimm/claim.c > >> +++ b/drivers/nvdimm/claim.c > >> @@ -300,12 +300,12 @@ static int nsio_rw_bytes(struct nd_namespace_common > >> *ndns, > >> return rc; > >> } > >> > >> -int devm_nsio_enable(struct device *dev, struct nd_namespace_io *nsio) > >> +int devm_nsio_enable(struct device *dev, struct nd_namespace_io *nsio, > >> unsigned long size) > >> { > >> struct resource *res = &nsio->res; > >> struct nd_namespace_common *ndns = &nsio->common; > >> > >> - nsio->size = resource_size(res); > >> + nsio->size = size; > > > > This needs a: > > > > if (nsio->size) > > devm_memunmap(dev, nsio->addr); > > > Why ? we should not be calling devm_nsio_enable twice now. > > > > > > >> if (!devm_request_mem_region(dev, res->start, resource_size(res), > >> dev_name(&ndns->dev))) { > > > > Also don't repeat the devm_request_mem_region() if one was already done. > > > > Another thing to check is if nsio->size gets cleared when the > > namespace is disabled, if not that well need to be added in the > > shutdown path. > > > > > Not sure I understand that. So with this patch when we probe we always > use info_block_reserve() size irrespective of the device. This probe > will result in us adding a btt/pfn/dax device and we return -ENXIO after > this probe. This return value will cause us to destroy the I/O remap > mmapping we did with info_block_reserve() size. Also the nsio data > structure is also freed. > > nd_pmem_probe is then again called with btt device type and in that case > we do devm_memremap again. > > if (btt) > remap the full namespace size. > else > remap the info_block_reserve_size. > > > This infor block reserve mapping is unmapped in pmem_attach_disk() > > > Anything i am missing in the above flow?
Oh no, you're right, this does make the implementation only call devm_nsio_enable() once. However, now that I look I want to suggest some small reorganizations of where devm_nsio_enable() is called. I'll take a stab at folding some changes on top of your patch. _______________________________________________ Linux-nvdimm mailing list -- [email protected] To unsubscribe send an email to [email protected]
