[bug report] mtd: spi-nor: Introduce 'struct spi_nor_controller_ops'

[bug report] mtd: spi-nor: Introduce 'struct spi_nor_controller_ops'

[Dan Carpenter]

Hello Tudor Ambarus,

This is a semi-automatic email about new static checker warnings.

The patch 453977875364: "mtd: spi-nor: Introduce 'struct
spi_nor_controller_ops'" from Sep 24, 2019, leads to the following
Smatch complaint:

    drivers/mtd/spi-nor/spi-nor.c:967 spi_nor_erase_sector()
    error: we previously assumed 'nor->controller_ops' could be null (see line 945)

drivers/mtd/spi-nor/spi-nor.c
   944	
   945		if (nor->controller_ops && nor->controller_ops->erase)
                    ^^^^^^^^^^^^^^^^^^^
Can this really be NULL?  Probably this check can be removed...

   946			return nor->controller_ops->erase(nor, addr);
   947	
   948		if (nor->spimem) {
   949			struct spi_mem_op op =
   950				SPI_MEM_OP(SPI_MEM_OP_CMD(nor->erase_opcode, 1),
   951					   SPI_MEM_OP_ADDR(nor->addr_width, addr, 1),
   952					   SPI_MEM_OP_NO_DUMMY,
   953					   SPI_MEM_OP_NO_DATA);
   954	
   955			return spi_mem_exec_op(nor->spimem, &op);
   956		}
   957	
   958		/*
   959		 * Default implementation, if driver doesn't have a specialized HW
   960		 * control
   961		 */
   962		for (i = nor->addr_width - 1; i >= 0; i--) {
   963			nor->bouncebuf[i] = addr & 0xff;
   964			addr >>= 8;
   965		}
   966	
   967		return nor->controller_ops->write_reg(nor, nor->erase_opcode,
                       ^^^^^^^^^^^^^^^^^^^^^
The patch adds a new unchecked dereference.

   968						      nor->bouncebuf, nor->addr_width);
   969	}

regards,
dan carpenter

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [bug report] mtd: spi-nor: Introduce 'struct spi_nor_controller_ops'

[Tudor.Ambarus]

On 10/25/2019 12:03 PM, Dan Carpenter wrote:
> External E-Mail
> 
> 
> Hello Tudor Ambarus,

Hi,

The code should be fine, see below.

> 
> This is a semi-automatic email about new static checker warnings.
> 
> The patch 453977875364: "mtd: spi-nor: Introduce 'struct
> spi_nor_controller_ops'" from Sep 24, 2019, leads to the following
> Smatch complaint:
> 
>     drivers/mtd/spi-nor/spi-nor.c:967 spi_nor_erase_sector()
>     error: we previously assumed 'nor->controller_ops' could be null (see line 945)
> 
> drivers/mtd/spi-nor/spi-nor.c
>    944	
>    945		if (nor->controller_ops && nor->controller_ops->erase)
>                     ^^^^^^^^^^^^^^^^^^^
> Can this really be NULL?  Probably this check can be removed...

nor->controller_ops can be NULL when the controller is under the SPI-MEM
interface, i.e. nor->spimem != NULL.

If not under SPI-MEM, the controller implements its own operations.
controller_ops->erase is optional, can be NULL. This check verifies if the
controller implements its own operations and if the optional one (erase) is
present or not.

> 
>    946			return nor->controller_ops->erase(nor, addr);
>    947	
>    948		if (nor->spimem) {
>    949			struct spi_mem_op op =
>    950				SPI_MEM_OP(SPI_MEM_OP_CMD(nor->erase_opcode, 1),
>    951					   SPI_MEM_OP_ADDR(nor->addr_width, addr, 1),
>    952					   SPI_MEM_OP_NO_DUMMY,
>    953					   SPI_MEM_OP_NO_DATA);
>    954	
>    955			return spi_mem_exec_op(nor->spimem, &op);
>    956		}
>    957	
>    958		/*
>    959		 * Default implementation, if driver doesn't have a specialized HW
>    960		 * control
>    961		 */
>    962		for (i = nor->addr_width - 1; i >= 0; i--) {
>    963			nor->bouncebuf[i] = addr & 0xff;
>    964			addr >>= 8;
>    965		}
>    966	
>    967		return nor->controller_ops->write_reg(nor, nor->erase_opcode,>                        ^^^^^^^^^^^^^^^^^^^^^
> The patch adds a new unchecked dereference.

When the controller implements its own ops, nor->controller_ops->write_reg is
mandatory and backed-up by the:

static int spi_nor_check(struct spi_nor *nor)
{
        if (!nor->dev ||
            (!nor->spimem && nor->controller_ops &&
            (!nor->controller_ops->read ||
             !nor->controller_ops->write ||
             !nor->controller_ops->read_reg ||
             !nor->controller_ops->write_reg))) {
                pr_err("spi-nor: please fill all the necessary fields!\n");
                return -EINVAL;
        }

Cheers,
ta
> 
>    968						      nor->bouncebuf, nor->addr_width);
>    969	}
> 
> regards,
> dan carpenter
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 
> 
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH] mtd: spi-nor: Move condition to avoid a NULL check

[Tudor.Ambarus]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

When the controller is not under the SPI-MEM interface it may implement
the optional controller_ops->erase() method.

nor->spimem and nor->controller_ops are disjunctive. Move the
nor->controller_ops->erase != NULL check as an 'else if' case to
nor->spimem, in order to avoid the nor->controller_ops != NULL
check.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
Intended for better readability.

 drivers/mtd/spi-nor/spi-nor.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 07ec7841ec32..d0e1502d21ce 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1118,9 +1118,6 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
 
 	addr = spi_nor_convert_addr(nor, addr);
 
-	if (nor->controller_ops && nor->controller_ops->erase)
-		return nor->controller_ops->erase(nor, addr);
-
 	if (nor->spimem) {
 		struct spi_mem_op op =
 			SPI_MEM_OP(SPI_MEM_OP_CMD(nor->erase_opcode, 1),
@@ -1129,6 +1126,8 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
 				   SPI_MEM_OP_NO_DATA);
 
 		return spi_mem_exec_op(nor->spimem, &op);
+	} else if (nor->controller_ops->erase) {
+		return nor->controller_ops->erase(nor, addr);
 	}
 
 	/*
-- 
2.9.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: Move condition to avoid a NULL check

[Dan Carpenter]

On Fri, Oct 25, 2019 at 12:32:25PM +0000, Tudor.Ambarus@microchip.com wrote:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> When the controller is not under the SPI-MEM interface it may implement
> the optional controller_ops->erase() method.
> 
> nor->spimem and nor->controller_ops are disjunctive. Move the
> nor->controller_ops->erase != NULL check as an 'else if' case to
> nor->spimem, in order to avoid the nor->controller_ops != NULL
> check.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> Intended for better readability.

Thanks!

regards,
dan carpenter


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 1/2] mtd: spi-nor: Make sure nor->spimem and nor->controller_ops are mutual exclusive

[Tudor.Ambarus]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

Expand the spi_nor_check() to make sure that nor->spimem and
nor->controller_ops are mutual exclusive.

Fixes: b35b9a10362d ("mtd: spi-nor: Move m25p80 code in spi-nor.c")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
v2: new patch

 drivers/mtd/spi-nor/spi-nor.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index a6f9f833c862..b452d3d0de28 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2741,6 +2741,12 @@ static int spi_nor_check(struct spi_nor *nor)
 		return -EINVAL;
 	}
 
+	if ((nor->spimem && nor->controller_ops) ||
+	    (!nor->spimem && !nor->controller_ops)) {
+		dev_err(nor->dev, "nor->spimem and nor->controller_ops are mutual exclusive\n");
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
-- 
2.9.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 2/2] mtd: spi-nor: Move condition to avoid a NULL check

[Tudor.Ambarus]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

When the controller is not under the SPI-MEM interface it may implement
the optional controller_ops->erase() method.

nor->spimem and nor->controller_ops are mutual exclusive. Move the
nor->controller_ops->erase != NULL check as an 'else if' case to
nor->spimem, in order to avoid the nor->controller_ops != NULL
check.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
v2: add Reported-by tag, amend commit description.

 drivers/mtd/spi-nor/spi-nor.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index b452d3d0de28..8eaf097098d9 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -942,9 +942,6 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
 
 	addr = spi_nor_convert_addr(nor, addr);
 
-	if (nor->controller_ops && nor->controller_ops->erase)
-		return nor->controller_ops->erase(nor, addr);
-
 	if (nor->spimem) {
 		struct spi_mem_op op =
 			SPI_MEM_OP(SPI_MEM_OP_CMD(nor->erase_opcode, 1),
@@ -953,6 +950,8 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
 				   SPI_MEM_OP_NO_DATA);
 
 		return spi_mem_exec_op(nor->spimem, &op);
+	} else if (nor->controller_ops->erase) {
+		return nor->controller_ops->erase(nor, addr);
 	}
 
 	/*
-- 
2.9.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 2/2] mtd: spi-nor: Move condition to avoid a NULL check

[Boris Brezillon]

On Fri, 25 Oct 2019 14:28:36 +0000
<Tudor.Ambarus@microchip.com> wrote:

> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> When the controller is not under the SPI-MEM interface it may implement
> the optional controller_ops->erase() method.
> 
> nor->spimem and nor->controller_ops are mutual exclusive. Move the

					  ^mutually

> nor->controller_ops->erase != NULL check as an 'else if' case to
> nor->spimem, in order to avoid the nor->controller_ops != NULL
> check.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

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

> ---
> v2: add Reported-by tag, amend commit description.
> 
>  drivers/mtd/spi-nor/spi-nor.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index b452d3d0de28..8eaf097098d9 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -942,9 +942,6 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>  
>  	addr = spi_nor_convert_addr(nor, addr);
>  
> -	if (nor->controller_ops && nor->controller_ops->erase)
> -		return nor->controller_ops->erase(nor, addr);
> -
>  	if (nor->spimem) {
>  		struct spi_mem_op op =
>  			SPI_MEM_OP(SPI_MEM_OP_CMD(nor->erase_opcode, 1),
> @@ -953,6 +950,8 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>  				   SPI_MEM_OP_NO_DATA);
>  
>  		return spi_mem_exec_op(nor->spimem, &op);
> +	} else if (nor->controller_ops->erase) {
> +		return nor->controller_ops->erase(nor, addr);
>  	}
>  
>  	/*




______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 2/2] mtd: spi-nor: Move condition to avoid a NULL check

[Tudor.Ambarus]

On 10/25/2019 05:28 PM, Tudor Ambarus - M18064 wrote:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> When the controller is not under the SPI-MEM interface it may implement
> the optional controller_ops->erase() method.
> 
> nor->spimem and nor->controller_ops are mutual exclusive. Move the
> nor->controller_ops->erase != NULL check as an 'else if' case to
> nor->spimem, in order to avoid the nor->controller_ops != NULL
> check.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> v2: add Reported-by tag, amend commit description.
> 
>  drivers/mtd/spi-nor/spi-nor.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Amended commit description, s/mutual/mutually. Patch applied to spi-nor/next.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 1/2] mtd: spi-nor: Make sure nor->spimem and nor->controller_ops are mutual exclusive

[Boris Brezillon]

On Fri, 25 Oct 2019 14:28:34 +0000
<Tudor.Ambarus@microchip.com> wrote:

> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> Expand the spi_nor_check() to make sure that nor->spimem and
> nor->controller_ops are mutual exclusive.

			  ^mutually

> 
> Fixes: b35b9a10362d ("mtd: spi-nor: Move m25p80 code in spi-nor.c")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> v2: new patch
> 
>  drivers/mtd/spi-nor/spi-nor.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index a6f9f833c862..b452d3d0de28 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2741,6 +2741,12 @@ static int spi_nor_check(struct spi_nor *nor)
>  		return -EINVAL;
>  	}
>  
> +	if ((nor->spimem && nor->controller_ops) ||
> +	    (!nor->spimem && !nor->controller_ops)) {
> +		dev_err(nor->dev, "nor->spimem and nor->controller_ops are mutual exclusive\n");

Hm, the test does more than checking that only one of nor->spimem
nor->controller_ops is set, it also checks that at least one of them is
provided. Maybe you should split those tests and have a different error
message for each, or make the error message more generic.

> +		return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH] mtd: spi-nor: Make sure nor->spimem and nor->controller_ops are mutually exclusive

[Tudor.Ambarus]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

Expand the spi_nor_check() to make sure that nor->spimem and
nor->controller_ops are mutually exclusive.

Fixes: b35b9a10362d ("mtd: spi-nor: Move m25p80 code in spi-nor.c")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
v3:
- split checks for better error accuracy
- s/mutual/mutually

 drivers/mtd/spi-nor/spi-nor.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 824649eecd59..f5d24ccf5108 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2878,6 +2878,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 static int spi_nor_check(struct spi_nor *nor)
 {
 	if (!nor->dev ||
+	    (!nor->spimem && !nor->controller_ops) ||
 	    (!nor->spimem && nor->controller_ops &&
 	    (!nor->controller_ops->read ||
 	     !nor->controller_ops->write ||
@@ -2887,6 +2888,11 @@ static int spi_nor_check(struct spi_nor *nor)
 		return -EINVAL;
 	}
 
+	if (nor->spimem && nor->controller_ops) {
+		dev_err(nor->dev, "nor->spimem and nor->controller_ops are mutually exclusive, please set just one of them.\n");
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
-- 
2.9.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: Make sure nor->spimem and nor->controller_ops are mutually exclusive

[Boris Brezillon]

On Mon, 11 Nov 2019 09:44:08 +0000
<Tudor.Ambarus@microchip.com> wrote:

> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> Expand the spi_nor_check() to make sure that nor->spimem and
> nor->controller_ops are mutually exclusive.
> 
> Fixes: b35b9a10362d ("mtd: spi-nor: Move m25p80 code in spi-nor.c")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

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

> ---
> v3:
> - split checks for better error accuracy
> - s/mutual/mutually
> 
>  drivers/mtd/spi-nor/spi-nor.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 824649eecd59..f5d24ccf5108 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2878,6 +2878,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>  static int spi_nor_check(struct spi_nor *nor)
>  {
>  	if (!nor->dev ||
> +	    (!nor->spimem && !nor->controller_ops) ||
>  	    (!nor->spimem && nor->controller_ops &&
>  	    (!nor->controller_ops->read ||
>  	     !nor->controller_ops->write ||
> @@ -2887,6 +2888,11 @@ static int spi_nor_check(struct spi_nor *nor)
>  		return -EINVAL;
>  	}
>  
> +	if (nor->spimem && nor->controller_ops) {
> +		dev_err(nor->dev, "nor->spimem and nor->controller_ops are mutually exclusive, please set just one of them.\n");
> +		return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: Make sure nor->spimem and nor->controller_ops are mutually exclusive

[Tudor.Ambarus]

On 11/11/2019 11:44 AM, Tudor Ambarus - M18064 wrote:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> Expand the spi_nor_check() to make sure that nor->spimem and
> nor->controller_ops are mutually exclusive.
> 
> Fixes: b35b9a10362d ("mtd: spi-nor: Move m25p80 code in spi-nor.c")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> v3:
> - split checks for better error accuracy
> - s/mutual/mutually
> 
>  drivers/mtd/spi-nor/spi-nor.c | 6 ++++++
>  1 file changed, 6 insertions(+)

Applied to spi-nor/next.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/