Re: [PATCH] libnvdimm: call devm_namespace_disable() on error

[Santosh Sivaraj <santosh@fossix.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(-)
>
> 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
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org