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

[<Tudor.Ambarus@xxxxxxxxxxxxx>]

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/