| Message ID | YMtQFXE0F1w7mUh+@mwanda |
|---|---|
| State | Accepted |
| Headers | show |
| Series | mtd: rawnand: Add a check in of_get_nand_secure_regions() | expand |
On Thu, Jun 17, 2021 at 04:37:25PM +0300, 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> Thanks, Mani > --- > drivers/mtd/nand/raw/nand_base.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c > index 57a583149cc0..cbba46432e39 100644 > --- a/drivers/mtd/nand/raw/nand_base.c > +++ b/drivers/mtd/nand/raw/nand_base.c > @@ -5231,8 +5231,8 @@ static int of_get_nand_secure_regions(struct nand_chip *chip) > int nr_elem, i, j; > > nr_elem = of_property_count_elems_of_size(dn, "secure-regions", sizeof(u64)); > - if (!nr_elem) > - return 0; > + if (nr_elem <= 0) > + return nr_elem; > > chip->nr_secure_regions = nr_elem / 2; > chip->secure_regions = kcalloc(chip->nr_secure_regions, sizeof(*chip->secure_regions), > -- > 2.30.2 >
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. Miquel
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. 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. 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. What do you think? Thanks, Martin
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
Hi Mani, Manivannan Sadhasivam <mani@kernel.org> wrote on Sat, 24 Jul 2021 21:33:08 +0530: > 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? Yes please add this check and we should be good. Thanks, Miquèl
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index 57a583149cc0..cbba46432e39 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -5231,8 +5231,8 @@ static int of_get_nand_secure_regions(struct nand_chip *chip) int nr_elem, i, j; nr_elem = of_property_count_elems_of_size(dn, "secure-regions", sizeof(u64)); - if (!nr_elem) - return 0; + if (nr_elem <= 0) + return nr_elem; chip->nr_secure_regions = nr_elem / 2; chip->secure_regions = kcalloc(chip->nr_secure_regions, sizeof(*chip->secure_regions),
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> --- drivers/mtd/nand/raw/nand_base.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)