Re: [PATCH] mtd: rawnand: Add a check in of_get_nand_secure_regions()

From: Manivannan Sadhasivam <mani@kernel.org>
To: Martin Kaiser <martin@kaiser.cx>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>,
	Dan Carpenter <dan.carpenter@oracle.com>,
	Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Boris Brezillon <boris.brezillon@collabora.com>,
	Tudor Ambarus <tudor.ambarus@microchip.com>,
	linux-mtd@lists.infradead.org, kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] mtd: rawnand: Add a check in of_get_nand_secure_regions()
Date: Sat, 24 Jul 2021 21:33:08 +0530	[thread overview]
Message-ID: <20210724160308.GA62321@thinkpad> (raw)
In-Reply-To: <20210724142712.GA24036@martin-debian-1.paytec.ch>

On Sat, Jul 24, 2021 at 04:27:30PM +0200, Martin Kaiser wrote:
> Hi all,
> 
> Thus wrote Miquel Raynal (miquel.raynal@bootlin.com):
> 
> > On Thu, 2021-06-17 at 13:37:25 UTC, Dan Carpenter wrote:
> > > Check for whether of_property_count_elems_of_size() returns a negative
> > > error code.
> 
> > > Fixes: 13b89768275d ("mtd: rawnand: Add support for secure regions in NAND memory")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
> 
> > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/fixes, thanks.
> 
> I'm running linux-next on an imx25 system with the following flash chip
> 
> [    1.997539] nand: device found, Manufacturer ID: 0x98, Chip ID: 0xaa
> [    2.004134] nand: Toshiba NAND 256MiB 1,8V 8-bit
> [    2.008917] nand: 256 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 128
> 
> The system is using the drivers/mtd/nand/raw/mxc_nand.c driver.
> 
> Since this commit appeared in linux-next, mxc_nand's probe function fails
> with -EINVAL, taking this path
> 
> mxcnd_probe
>    nand_scan
>       nand_scan_with_ids
>          nand_scan_tail
>             of_get_nand_secure_regions
> 
> nr_elem = of_property_count_elems_of_size(dn, "secure-regions", sizeof(u64));
> returns -EINVAL as there's no secure-regions property in my device tree.
> 

Doh! Sorry for missing this.

> We should certainly handle negative error codes before we calculate
> chip->nr_secure_regions = nr_elem / 2
> but a missing secure-regions property is a valid case and should not make
> the probe fail.
> 

Absolutely!

> If the property exists, but the device-tree entry is incorrect
> and of_property_count_elems_of_size returns -ENODATA, we might print a
> warning and ignore the entry.
> 

Hmm, I think it is best to error out in this case as the user has got DT wrong.

> What do you think?
> 

Since of_property_count_elems_of_size() returns -EINVAL if the length is not
a multiple of sizeof(u64), we can't just ignore -EINVAL.

So I think we can just check for the existence of the property before invoking
of_get_nand_secure_regions(). Miquel, what do you think?

Thanks,
Mani

> Thanks,
> 
>    Martin