Re: [PATCH v2] mtd: rawnand: make subop helpers return unsigned values

[Boris Brezillon <boris.brezillon@bootlin.com>]

On Thu, 19 Jul 2018 00:09:12 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> A report from Colin Ian King pointed a CoverityScan issue where error
> values on these helpers where not checked in the drivers. These
> helpers can error out only in case of a software bug in driver code,
> not because of a runtime/hardware error. Hence, let's WARN_ON() in this
> case and return 0 which is harmless anyway.
> 
> Fixes: 8878b126df76 ("mtd: nand: add ->exec_op() implementation")
> Cc: stable@vger.kernel.org

Is it really worth backporting this patch? I mean, the bug does not
exist, it's just a potential problem that can only arise when
drivers/core are buggy, which AFAICT is not the case yet :-).

> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>

> ---
> 
> Changes since v1:
> =================
> * At first I decided to continue returning negative errors and
>   handling these cases in the drivers. Not sure this was the right thing
>   to do as reported by Boris so now the core WARN_ON() on error (only
>   due to some bug in a controller driver) and return an harmless
>   value. The drivers are not touched anymore, hence this patch is alone
>   now. 
> 
>  drivers/mtd/nand/raw/nand_base.c | 44 ++++++++++++++++++++--------------------
>  include/linux/mtd/rawnand.h      | 16 +++++++--------
>  2 files changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 4fa5e20d9690..9bb76ddff4be 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -2668,8 +2668,8 @@ static bool nand_subop_instr_is_valid(const struct nand_subop *subop,
>  	return subop && instr_idx < subop->ninstrs;
>  }
>  
> -static int nand_subop_get_start_off(const struct nand_subop *subop,
> -				    unsigned int instr_idx)
> +static unsigned int nand_subop_get_start_off(const struct nand_subop *subop,
> +					     unsigned int instr_idx)
>  {
>  	if (instr_idx)
>  		return 0;
> @@ -2688,12 +2688,12 @@ static int nand_subop_get_start_off(const struct nand_subop *subop,
>   *
>   * Given an address instruction, returns the offset of the first cycle to issue.
>   */
> -int nand_subop_get_addr_start_off(const struct nand_subop *subop,
> -				  unsigned int instr_idx)
> +unsigned int nand_subop_get_addr_start_off(const struct nand_subop *subop,
> +					   unsigned int instr_idx)
>  {
> -	if (!nand_subop_instr_is_valid(subop, instr_idx) ||
> -	    subop->instrs[instr_idx].type != NAND_OP_ADDR_INSTR)
> -		return -EINVAL;
> +	if (WARN_ON(!nand_subop_instr_is_valid(subop, instr_idx) ||
> +		    subop->instrs[instr_idx].type != NAND_OP_ADDR_INSTR))
> +		return 0;
>  
>  	return nand_subop_get_start_off(subop, instr_idx);
>  }
> @@ -2710,14 +2710,14 @@ EXPORT_SYMBOL_GPL(nand_subop_get_addr_start_off);
>   *
>   * Given an address instruction, returns the number of address cycle to issue.
>   */
> -int nand_subop_get_num_addr_cyc(const struct nand_subop *subop,
> -				unsigned int instr_idx)
> +unsigned int nand_subop_get_num_addr_cyc(const struct nand_subop *subop,
> +					 unsigned int instr_idx)
>  {
>  	int start_off, end_off;
>  
> -	if (!nand_subop_instr_is_valid(subop, instr_idx) ||
> -	    subop->instrs[instr_idx].type != NAND_OP_ADDR_INSTR)
> -		return -EINVAL;
> +	if (WARN_ON(!nand_subop_instr_is_valid(subop, instr_idx) ||
> +		    subop->instrs[instr_idx].type != NAND_OP_ADDR_INSTR))
> +		return 0;
>  
>  	start_off = nand_subop_get_addr_start_off(subop, instr_idx);
>  
> @@ -2742,12 +2742,12 @@ EXPORT_SYMBOL_GPL(nand_subop_get_num_addr_cyc);
>   *
>   * Given a data instruction, returns the offset to start from.
>   */
> -int nand_subop_get_data_start_off(const struct nand_subop *subop,
> -				  unsigned int instr_idx)
> +unsigned int nand_subop_get_data_start_off(const struct nand_subop *subop,
> +					   unsigned int instr_idx)
>  {
> -	if (!nand_subop_instr_is_valid(subop, instr_idx) ||
> -	    !nand_instr_is_data(&subop->instrs[instr_idx]))
> -		return -EINVAL;
> +	if (WARN_ON(!nand_subop_instr_is_valid(subop, instr_idx) ||
> +		    !nand_instr_is_data(&subop->instrs[instr_idx])))
> +		return 0;
>  
>  	return nand_subop_get_start_off(subop, instr_idx);
>  }
> @@ -2764,14 +2764,14 @@ EXPORT_SYMBOL_GPL(nand_subop_get_data_start_off);
>   *
>   * Returns the length of the chunk of data to send/receive.
>   */
> -int nand_subop_get_data_len(const struct nand_subop *subop,
> -			    unsigned int instr_idx)
> +unsigned int nand_subop_get_data_len(const struct nand_subop *subop,
> +				     unsigned int instr_idx)
>  {
>  	int start_off = 0, end_off;
>  
> -	if (!nand_subop_instr_is_valid(subop, instr_idx) ||
> -	    !nand_instr_is_data(&subop->instrs[instr_idx]))
> -		return -EINVAL;
> +	if (WARN_ON(!nand_subop_instr_is_valid(subop, instr_idx) ||
> +		    !nand_instr_is_data(&subop->instrs[instr_idx])))
> +		return 0;
>  
>  	start_off = nand_subop_get_data_start_off(subop, instr_idx);
>  
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index e383c7f32574..876a9dd47e74 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -1007,14 +1007,14 @@ struct nand_subop {
>  	unsigned int last_instr_end_off;
>  };
>  
> -int nand_subop_get_addr_start_off(const struct nand_subop *subop,
> -				  unsigned int op_id);
> -int nand_subop_get_num_addr_cyc(const struct nand_subop *subop,
> -				unsigned int op_id);
> -int nand_subop_get_data_start_off(const struct nand_subop *subop,
> -				  unsigned int op_id);
> -int nand_subop_get_data_len(const struct nand_subop *subop,
> -			    unsigned int op_id);
> +unsigned int nand_subop_get_addr_start_off(const struct nand_subop *subop,
> +					   unsigned int op_id);
> +unsigned int nand_subop_get_num_addr_cyc(const struct nand_subop *subop,
> +					 unsigned int op_id);
> +unsigned int nand_subop_get_data_start_off(const struct nand_subop *subop,
> +					   unsigned int op_id);
> +unsigned int nand_subop_get_data_len(const struct nand_subop *subop,
> +				     unsigned int op_id);
>  
>  /**
>   * struct nand_op_parser_addr_constraints - Constraints for address instructions