On Wed, Oct 30, 2019 at 4:33 PM Dan Williams <[email protected]> wrote: > > 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.
This change is causing the "pfn-meta-errors.sh" test to fail. It is due to the fact the info_block_reserve() is not a sufficient reservation for errors in the page metadata area. _______________________________________________ Linux-nvdimm mailing list -- [email protected] To unsubscribe send an email to [email protected]
