| Message ID | 20200703111856.40280-1-hare@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libnvdimm: call devm_namespace_disable() on error | expand |
Hi Hannes, Hannes Reinecke <hare@suse.de> writes: > Once devm_namespace_enable() has been called the error path in the > calling function will not call devm_namespace_disable(), leaving the > namespace enabled on error. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > drivers/dax/pmem/core.c | 2 +- > drivers/nvdimm/btt.c | 5 ++++- > drivers/nvdimm/claim.c | 8 +++++++- > drivers/nvdimm/pfn_devs.c | 1 + > drivers/nvdimm/pmem.c | 20 ++++++++++---------- > 5 files changed, 23 insertions(+), 13 deletions(-) > > diff --git a/drivers/dax/pmem/core.c b/drivers/dax/pmem/core.c > index 2bedf8414fff..4b26434f0aca 100644 > --- a/drivers/dax/pmem/core.c > +++ b/drivers/dax/pmem/core.c > @@ -31,9 +31,9 @@ struct dev_dax *__dax_pmem_probe(struct device *dev, enum dev_dax_subsys subsys) > if (rc) > return ERR_PTR(rc); > rc = nvdimm_setup_pfn(nd_pfn, &pgmap); > + devm_namespace_disable(dev, ndns); > if (rc) > return ERR_PTR(rc); > - devm_namespace_disable(dev, ndns); > > /* reserve the metadata area, device-dax will reserve the data */ > pfn_sb = nd_pfn->pfn_sb; > diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c > index 48e9d169b6f9..bd4747f2c99b 100644 > --- a/drivers/nvdimm/btt.c > +++ b/drivers/nvdimm/btt.c > @@ -1704,13 +1704,16 @@ int nvdimm_namespace_attach_btt(struct nd_namespace_common *ndns) > dev_dbg(&nd_btt->dev, "%s must be at least %ld bytes\n", > dev_name(&ndns->dev), > ARENA_MIN_SIZE + nd_btt->initial_offset); > + devm_namespace_disable(&nd_btt->dev, ndns); > return -ENXIO; > } > nd_region = to_nd_region(nd_btt->dev.parent); > btt = btt_init(nd_btt, rawsize, nd_btt->lbasize, nd_btt->uuid, > nd_region); > - if (!btt) > + if (!btt) { > + devm_namespace_disable(&nd_btt->dev, ndns); > return -ENOMEM; > + } > nd_btt->btt = btt; > > return 0; > diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c > index 45964acba944..15fd1b92d32f 100644 > --- a/drivers/nvdimm/claim.c > +++ b/drivers/nvdimm/claim.c > @@ -314,12 +314,18 @@ int devm_nsio_enable(struct device *dev, struct nd_namespace_io *nsio, > } > > ndns->rw_bytes = nsio_rw_bytes; > - if (devm_init_badblocks(dev, &nsio->bb)) > + if (devm_init_badblocks(dev, &nsio->bb)) { > + devm_release_mem_region(dev, res->start, size); > return -ENOMEM; > + } > nvdimm_badblocks_populate(to_nd_region(ndns->dev.parent), &nsio->bb, > &nsio->res); > > nsio->addr = devm_memremap(dev, res->start, size, ARCH_MEMREMAP_PMEM); > + if (IS_ERR(nsio->addr)) { > + devm_exit_badblocks(dev, &nsio->bb); > + devm_release_mem_region(dev, res->start, size); > + } > > return PTR_ERR_OR_ZERO(nsio->addr); > } > diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c > index 34db557dbad1..9faa92662643 100644 > --- a/drivers/nvdimm/pfn_devs.c > +++ b/drivers/nvdimm/pfn_devs.c > @@ -408,6 +408,7 @@ static int nd_pfn_clear_memmap_errors(struct nd_pfn *nd_pfn) > nsoff += chunk; > } > if (rc) { > + devm_namespace_disable(&nd_pfn->dev, ndns); The disable here seems to be wrong. This function called from the pmem_attach_disk path, where its expected the namespace is enabled after the setup_pfn. Also in case of an error, we do another disable in pmem_attach_disk. Thanks, Santosh > dev_err(&nd_pfn->dev, > "error clearing %x badblocks at %llx\n", > num_bad, first_bad); > diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c > index d25e66fd942d..4f667fe6ef72 100644 > --- a/drivers/nvdimm/pmem.c > +++ b/drivers/nvdimm/pmem.c > @@ -401,8 +401,10 @@ static int pmem_attach_disk(struct device *dev, > if (is_nd_pfn(dev)) { > nd_pfn = to_nd_pfn(dev); > rc = nvdimm_setup_pfn(nd_pfn, &pmem->pgmap); > - if (rc) > + if (rc) { > + devm_namespace_disable(dev, ndns); > return rc; > + } > } > > /* we're attaching a block device, disable raw namespace access */ > @@ -549,17 +551,15 @@ static int nd_pmem_probe(struct device *dev) > ret = nd_pfn_probe(dev, ndns); > if (ret == 0) > return -ENXIO; > - else if (ret == -EOPNOTSUPP) > - return ret; > - > - ret = nd_dax_probe(dev, ndns); > - if (ret == 0) > - return -ENXIO; > - else if (ret == -EOPNOTSUPP) > - return ret; > - > + else if (ret != EOPNOTSUPP) { > + ret = nd_dax_probe(dev, ndns); > + if (ret == 0) > + return -ENXIO; > + } > /* probe complete, attach handles namespace enabling */ > devm_namespace_disable(dev, ndns); > + if (ret == -EOPNOTSUPP) > + return ret; > > return pmem_attach_disk(dev, ndns); > } > -- > 2.16.4 > _______________________________________________ > Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org > To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
Hi! Hannes, Hannes Reinecke <hare@suse.de> writes: > Once devm_namespace_enable() has been called the error path in the > calling function will not call devm_namespace_disable(), leaving the > namespace enabled on error. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > drivers/dax/pmem/core.c | 2 +- > drivers/nvdimm/btt.c | 5 ++++- > drivers/nvdimm/claim.c | 8 +++++++- > drivers/nvdimm/pfn_devs.c | 1 + > drivers/nvdimm/pmem.c | 20 ++++++++++---------- > 5 files changed, 23 insertions(+), 13 deletions(-) > [snip] > diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c > index d25e66fd942d..4f667fe6ef72 100644 > --- a/drivers/nvdimm/pmem.c > +++ b/drivers/nvdimm/pmem.c > @@ -401,8 +401,10 @@ static int pmem_attach_disk(struct device *dev, > if (is_nd_pfn(dev)) { > nd_pfn = to_nd_pfn(dev); > rc = nvdimm_setup_pfn(nd_pfn, &pmem->pgmap); > - if (rc) > + if (rc) { > + devm_namespace_disable(dev, ndns); > return rc; > + } As mentioned in the previous mail, when rc != 0, disable is called again here. > } > > /* we're attaching a block device, disable raw namespace access */ > @@ -549,17 +551,15 @@ static int nd_pmem_probe(struct device *dev) > ret = nd_pfn_probe(dev, ndns); > if (ret == 0) > return -ENXIO; > - else if (ret == -EOPNOTSUPP) > - return ret; > - > - ret = nd_dax_probe(dev, ndns); > - if (ret == 0) > - return -ENXIO; > - else if (ret == -EOPNOTSUPP) > - return ret; > - > + else if (ret != EOPNOTSUPP) { ^ -EOPNOTSUPP Thanks, Santosh > + ret = nd_dax_probe(dev, ndns); > + if (ret == 0) > + return -ENXIO; > + } > /* probe complete, attach handles namespace enabling */ > devm_namespace_disable(dev, ndns); > + if (ret == -EOPNOTSUPP) > + return ret; > > return pmem_attach_disk(dev, ndns); > } > -- > 2.16.4 > _______________________________________________ > Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org > To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
On 7/9/20 7:39 AM, Santosh Sivaraj wrote: > Hi Hannes, > > Hannes Reinecke <hare@suse.de> writes: > >> Once devm_namespace_enable() has been called the error path in the >> calling function will not call devm_namespace_disable(), leaving the >> namespace enabled on error. >> >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> --- >> drivers/dax/pmem/core.c | 2 +- >> drivers/nvdimm/btt.c | 5 ++++- >> drivers/nvdimm/claim.c | 8 +++++++- >> drivers/nvdimm/pfn_devs.c | 1 + >> drivers/nvdimm/pmem.c | 20 ++++++++++---------- >> 5 files changed, 23 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/dax/pmem/core.c b/drivers/dax/pmem/core.c >> index 2bedf8414fff..4b26434f0aca 100644 >> --- a/drivers/dax/pmem/core.c >> +++ b/drivers/dax/pmem/core.c >> @@ -31,9 +31,9 @@ struct dev_dax *__dax_pmem_probe(struct device *dev, enum dev_dax_subsys subsys) >> if (rc) >> return ERR_PTR(rc); >> rc = nvdimm_setup_pfn(nd_pfn, &pgmap); >> + devm_namespace_disable(dev, ndns); >> if (rc) >> return ERR_PTR(rc); >> - devm_namespace_disable(dev, ndns); >> >> /* reserve the metadata area, device-dax will reserve the data */ >> pfn_sb = nd_pfn->pfn_sb; >> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c >> index 48e9d169b6f9..bd4747f2c99b 100644 >> --- a/drivers/nvdimm/btt.c >> +++ b/drivers/nvdimm/btt.c >> @@ -1704,13 +1704,16 @@ int nvdimm_namespace_attach_btt(struct nd_namespace_common *ndns) >> dev_dbg(&nd_btt->dev, "%s must be at least %ld bytes\n", >> dev_name(&ndns->dev), >> ARENA_MIN_SIZE + nd_btt->initial_offset); >> + devm_namespace_disable(&nd_btt->dev, ndns); >> return -ENXIO; >> } >> nd_region = to_nd_region(nd_btt->dev.parent); >> btt = btt_init(nd_btt, rawsize, nd_btt->lbasize, nd_btt->uuid, >> nd_region); >> - if (!btt) >> + if (!btt) { >> + devm_namespace_disable(&nd_btt->dev, ndns); >> return -ENOMEM; >> + } >> nd_btt->btt = btt; >> >> return 0; >> diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c >> index 45964acba944..15fd1b92d32f 100644 >> --- a/drivers/nvdimm/claim.c >> +++ b/drivers/nvdimm/claim.c >> @@ -314,12 +314,18 @@ int devm_nsio_enable(struct device *dev, struct nd_namespace_io *nsio, >> } >> >> ndns->rw_bytes = nsio_rw_bytes; >> - if (devm_init_badblocks(dev, &nsio->bb)) >> + if (devm_init_badblocks(dev, &nsio->bb)) { >> + devm_release_mem_region(dev, res->start, size); >> return -ENOMEM; >> + } >> nvdimm_badblocks_populate(to_nd_region(ndns->dev.parent), &nsio->bb, >> &nsio->res); >> >> nsio->addr = devm_memremap(dev, res->start, size, ARCH_MEMREMAP_PMEM); >> + if (IS_ERR(nsio->addr)) { >> + devm_exit_badblocks(dev, &nsio->bb); >> + devm_release_mem_region(dev, res->start, size); >> + } >> >> return PTR_ERR_OR_ZERO(nsio->addr); >> } >> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c >> index 34db557dbad1..9faa92662643 100644 >> --- a/drivers/nvdimm/pfn_devs.c >> +++ b/drivers/nvdimm/pfn_devs.c >> @@ -408,6 +408,7 @@ static int nd_pfn_clear_memmap_errors(struct nd_pfn *nd_pfn) >> nsoff += chunk; >> } >> if (rc) { >> + devm_namespace_disable(&nd_pfn->dev, ndns); > > The disable here seems to be wrong. > > This function called from the pmem_attach_disk path, where its expected the > namespace is enabled after the setup_pfn. Also in case of an error, we do > another disable in pmem_attach_disk. Oh. Correct. Will be fixing it up. Cheers, Hannes
diff --git a/drivers/dax/pmem/core.c b/drivers/dax/pmem/core.c index 2bedf8414fff..4b26434f0aca 100644 --- a/drivers/dax/pmem/core.c +++ b/drivers/dax/pmem/core.c @@ -31,9 +31,9 @@ struct dev_dax *__dax_pmem_probe(struct device *dev, enum dev_dax_subsys subsys) if (rc) return ERR_PTR(rc); rc = nvdimm_setup_pfn(nd_pfn, &pgmap); + devm_namespace_disable(dev, ndns); if (rc) return ERR_PTR(rc); - devm_namespace_disable(dev, ndns); /* reserve the metadata area, device-dax will reserve the data */ pfn_sb = nd_pfn->pfn_sb; diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c index 48e9d169b6f9..bd4747f2c99b 100644 --- a/drivers/nvdimm/btt.c +++ b/drivers/nvdimm/btt.c @@ -1704,13 +1704,16 @@ int nvdimm_namespace_attach_btt(struct nd_namespace_common *ndns) dev_dbg(&nd_btt->dev, "%s must be at least %ld bytes\n", dev_name(&ndns->dev), ARENA_MIN_SIZE + nd_btt->initial_offset); + devm_namespace_disable(&nd_btt->dev, ndns); return -ENXIO; } nd_region = to_nd_region(nd_btt->dev.parent); btt = btt_init(nd_btt, rawsize, nd_btt->lbasize, nd_btt->uuid, nd_region); - if (!btt) + if (!btt) { + devm_namespace_disable(&nd_btt->dev, ndns); return -ENOMEM; + } nd_btt->btt = btt; return 0; diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c index 45964acba944..15fd1b92d32f 100644 --- a/drivers/nvdimm/claim.c +++ b/drivers/nvdimm/claim.c @@ -314,12 +314,18 @@ int devm_nsio_enable(struct device *dev, struct nd_namespace_io *nsio, } ndns->rw_bytes = nsio_rw_bytes; - if (devm_init_badblocks(dev, &nsio->bb)) + if (devm_init_badblocks(dev, &nsio->bb)) { + devm_release_mem_region(dev, res->start, size); return -ENOMEM; + } nvdimm_badblocks_populate(to_nd_region(ndns->dev.parent), &nsio->bb, &nsio->res); nsio->addr = devm_memremap(dev, res->start, size, ARCH_MEMREMAP_PMEM); + if (IS_ERR(nsio->addr)) { + devm_exit_badblocks(dev, &nsio->bb); + devm_release_mem_region(dev, res->start, size); + } return PTR_ERR_OR_ZERO(nsio->addr); } diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c index 34db557dbad1..9faa92662643 100644 --- a/drivers/nvdimm/pfn_devs.c +++ b/drivers/nvdimm/pfn_devs.c @@ -408,6 +408,7 @@ static int nd_pfn_clear_memmap_errors(struct nd_pfn *nd_pfn) nsoff += chunk; } if (rc) { + devm_namespace_disable(&nd_pfn->dev, ndns); dev_err(&nd_pfn->dev, "error clearing %x badblocks at %llx\n", num_bad, first_bad); diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c index d25e66fd942d..4f667fe6ef72 100644 --- a/drivers/nvdimm/pmem.c +++ b/drivers/nvdimm/pmem.c @@ -401,8 +401,10 @@ static int pmem_attach_disk(struct device *dev, if (is_nd_pfn(dev)) { nd_pfn = to_nd_pfn(dev); rc = nvdimm_setup_pfn(nd_pfn, &pmem->pgmap); - if (rc) + if (rc) { + devm_namespace_disable(dev, ndns); return rc; + } } /* we're attaching a block device, disable raw namespace access */ @@ -549,17 +551,15 @@ static int nd_pmem_probe(struct device *dev) ret = nd_pfn_probe(dev, ndns); if (ret == 0) return -ENXIO; - else if (ret == -EOPNOTSUPP) - return ret; - - ret = nd_dax_probe(dev, ndns); - if (ret == 0) - return -ENXIO; - else if (ret == -EOPNOTSUPP) - return ret; - + else if (ret != EOPNOTSUPP) { + ret = nd_dax_probe(dev, ndns); + if (ret == 0) + return -ENXIO; + } /* probe complete, attach handles namespace enabling */ devm_namespace_disable(dev, ndns); + if (ret == -EOPNOTSUPP) + return ret; return pmem_attach_disk(dev, ndns); }
Once devm_namespace_enable() has been called the error path in the calling function will not call devm_namespace_disable(), leaving the namespace enabled on error. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/dax/pmem/core.c | 2 +- drivers/nvdimm/btt.c | 5 ++++- drivers/nvdimm/claim.c | 8 +++++++- drivers/nvdimm/pfn_devs.c | 1 + drivers/nvdimm/pmem.c | 20 ++++++++++---------- 5 files changed, 23 insertions(+), 13 deletions(-)