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

[Hannes Reinecke <hare@suse.de>]

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
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                               +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org