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

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

[Miquel Raynal]

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
Signed-off-by: Miquel Raynal <miquel.raynal@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
-- 
2.14.1

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

[Boris Brezillon]

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

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

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@bootlin.com> wrote on Thu, 19 Jul 2018
00:31:34 +0200:

> 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 :-).

Ok, I'll remove the Cc: stable tag but I guess I can keep the Fixes
one.

Miquèl

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

[Boris Brezillon]

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

> Hi Boris,
> 
> Boris Brezillon <boris.brezillon@bootlin.com> wrote on Thu, 19 Jul 2018
> 00:31:34 +0200:
> 
> > 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 :-).  
> 
> Ok, I'll remove the Cc: stable tag but I guess I can keep the Fixes
> one.

Sure.

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

[Miquel Raynal]

> > > > 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 :-).    
> > 
> > Ok, I'll remove the Cc: stable tag but I guess I can keep the Fixes
> > one.  
> 
> Sure.

Applied to nand/next without the cc:stable tag.
 

-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com