mtd: spi-nor: Introduce 'struct spi_nor_controller_ops'
Subsystems (2)
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/
Quoted 4 lines · Dan Carpenter
External E-Mail
Hello Tudor Ambarus,
Hi,
The code should be fine, see below.
Quoted 7 lines
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:
Quoted 8 lines
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.
Quoted 23 lines
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,> ^^^^^^^^^^^^^^^^^^^^^Quoted 1 line
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
Quoted 4 lines
968 nor->bouncebuf, nor->addr_width); 969 }
Quoted 8 lines
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 1/1 mtd: spi-nor: Move condition to avoid a NULL check
From: Tudor Ambarus <[email protected]>
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.
---
Intended for better readability.
drivers/mtd/spi-nor/spi-nor.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/mtd/spi-nor/spi-nor.c +2 -3
@@ -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); }/*
Signature · 6 lines
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
Quoted 5 lines · On Fri, Oct 25, 2019 at 12:32:25PM +0000, [email protected] wrote:
From: Tudor Ambarus <[email protected]>
When the controller is not under the SPI-MEM interface it may implement the optional
controller_ops->erase()method.
Quoted 8 lines
nor->spimemandnor->controller_opsare disjunctive. Move thenor->controller_ops->erase!= NULL check as an 'else if' case tonor->spimem, in order to avoid thenor->controller_ops!= NULL check.Signed-off-by Tudor Ambarus <[email protected]>---
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
In reply to [email protected]From: Tudor Ambarus <[email protected]>
Expand the spi_nor_check() to make sure that nor->spimem and nor->controller_ops are mutual exclusive.
---
v2: new patch
drivers/mtd/spi-nor/spi-nor.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/mtd/spi-nor/spi-nor.c +6 -0
@@ -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 ; }
Signature · 6 lines
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
From: Tudor Ambarus <[email protected]>
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.
---
v2: add Reported-by tag, amend commit description.
drivers/mtd/spi-nor/spi-nor.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/mtd/spi-nor/spi-nor.c +2 -3
@@ -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); }/*
Signature · 6 lines
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
Quoted 6 lines · On Fri, 25 Oct 2019 14:28:36 +0000 <[email protected]> wrote:
From: Tudor Ambarus <[email protected]>
When the controller is not under the SPI-MEM interface it may implement the optional
controller_ops->erase()method.
nor->spimemandnor->controller_opsare mutual exclusive. Move the
^mutually
Quoted 6 lines
nor->controller_ops->erase!= NULL check as an 'else if' case tonor->spimem, in order to avoid thenor->controller_ops!= NULL check.Reported-by Dan Carpenter <[email protected]>Signed-off-by Tudor Ambarus <[email protected]>
Quoted 21 lines
---
v2: add Reported-by tag, amend commit description.drivers/mtd/spi-nor/spi-nor.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)drivers/mtd/spi-nor/spi-nor.c +0
-3@@ -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),
drivers/mtd/spi-nor/spi-nor.c static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr) · line 957
Quoted 8 lines
drivers/mtd/spi-nor/spi-nor.c +2
-0@@ -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
In reply to [email protected]Quoted 10 lines · On 10/25/2019 05:28 PM, Tudor Ambarus - M18064 wrote:
From: Tudor Ambarus <[email protected]>
When the controller is not under the SPI-MEM interface it may implement the optional
controller_ops->erase()method.
nor->spimemandnor->controller_opsare mutual exclusive. Move thenor->controller_ops->erase!= NULL check as an 'else if' case tonor->spimem, in order to avoid thenor->controller_ops!= NULL check.
Quoted 7 lines
Reported-by Dan Carpenter <[email protected]>Signed-off-by Tudor Ambarus <[email protected]>---
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
In reply to [email protected]Quoted 4 lines · On Fri, 25 Oct 2019 14:28:34 +0000 <[email protected]> wrote:
From: Tudor Ambarus <[email protected]>
Expand the
spi_nor_check()to make sure thatnor->spimemandnor->controller_opsare mutual exclusive.
^mutually
Quoted 15 lines
Fixes b35b9a10362d ("mtd: spi-nor: Move m25p80 code in spi-nor.c")Reported-by Dan Carpenter <[email protected]>Signed-off-by Tudor Ambarus <[email protected]>---
v2: new patchdrivers/mtd/spi-nor/spi-nor.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/mtd/spi-nor/spi-nor.c +0
-0
drivers/mtd/spi-nor/spi-nor.c static int spi_nor_check(struct spi_nor *nor) · line 2746
Quoted 6 lines
drivers/mtd/spi-nor/spi-nor.c +3
-0@@ -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.
drivers/mtd/spi-nor/spi-nor.c static int spi_nor_check(struct spi_nor *nor) · line 2752
Quoted 6 lines
drivers/mtd/spi-nor/spi-nor.c +3
-0+return -EINVAL ; + } +return 0 ; }
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
PATCH 1/1 mtd: spi-nor: Make sure nor->spimem and nor->controller_ops are mutually exclusive
From: Tudor Ambarus <[email protected]>
Expand the spi_nor_check() to make sure that nor->spimem and nor->controller_ops are mutually exclusive.
---
v3:
- split checks for better error accuracy
- s/mutual/mutually
drivers/mtd/spi-nor/spi-nor.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/mtd/spi-nor/spi-nor.c +6 -0
@@ -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 ; }
Signature · 6 lines
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
Quoted 8 lines · On Mon, 11 Nov 2019 09:44:08 +0000 <[email protected]> wrote:
From: Tudor Ambarus <[email protected]>
Expand the
spi_nor_check()to make sure thatnor->spimemandnor->controller_opsare mutually exclusive.Fixes b35b9a10362d ("mtd: spi-nor: Move m25p80 code in spi-nor.c")Reported-by Dan Carpenter <[email protected]>Signed-off-by Tudor Ambarus <[email protected]>
Quoted 22 lines
---
v3:
- split checks for better error accuracy
- s/mutual/mutuallydrivers/mtd/spi-nor/spi-nor.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/mtd/spi-nor/spi-nor.c +1
-0@@ -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 ;
drivers/mtd/spi-nor/spi-nor.c static int spi_nor_check(struct spi_nor *nor) · line 2898
Quoted 10 lines
drivers/mtd/spi-nor/spi-nor.c +5
-0} +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
In reply to [email protected]Quoted 8 lines · On 11/11/2019 11:44 AM, Tudor Ambarus - M18064 wrote:
From: Tudor Ambarus <[email protected]>
Expand the
spi_nor_check()to make sure thatnor->spimemandnor->controller_opsare mutually exclusive.Fixes b35b9a10362d ("mtd: spi-nor: Move m25p80 code in spi-nor.c")Reported-by Dan Carpenter <[email protected]>Signed-off-by Tudor Ambarus <[email protected]>
Quoted 7 lines
---
v3:
- split checks for better error accuracy
- s/mutual/mutuallydrivers/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/