Re: [PATCH 4/5] acpi_nfit, libnvdimm: Add support for clear poison list and bad blocks

[Johannes Thumshirn <jthumshirn@suse.de>]

On 03/06/2017 09:32 PM, Dave Jiang wrote:
> Providing mechanism to clear poison list via the ndctl ND_CMD_CLEAR_ERROR
> call. We will update the poison list and also the badblocks at region level
> if the region is in dax mode or in pmem mode and not active.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---

[...]

> +	if ((cmd != ND_CMD_CLEAR_ERROR) || !nvdimm_bus || !clear_err->cleared)
             ^~ Unnecessary parenthesis?

[...]

> +
> +		/* make sure clear_err range is within a SPA range */
> +		if (((clear_begin >= spa_begin) &&
> +					(clear_begin < (spa_end))) &&
> +				((clear_end > spa_begin) &&
> +				 (clear_end <= spa_end))) {

Indentation looks a bit odd here and the superfluous parenthesis aren't
improving the situation.

[...]


> +	if (nd_btt || nd_pfn || nd_dax) {
> +		if (nd_btt)
> +			ndns = nd_btt->ndns;
> +		else if (nd_pfn)
> +			ndns = nd_pfn->ndns;
> +		else if (nd_dax)
> +			ndns = nd_dax->nd_pfn.ndns;
> +
> +		if (!ndns)
> +			return 0;

How can this (the !ndns case) ever happen?

And anyways isn't this sufficient, or am I missing something:

	if (nd_btt)
		ndns = nd_btt->ndns;
	else if (nd_pfn)
		ndns = nd_pfn->ndns;
	else if (nd_dax)
		ndns = nd_dax->nd_pfn.ndns;
	else
		ndns = to_ndns(dev);

> +	} else
> +		ndns = to_ndns(dev);
> +

Thanks,
	Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm