kernel-janitors.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mtd: rawnand: Add a check in of_get_nand_secure_regions()
@ 2021-06-17 13:37 Dan Carpenter
  2021-06-25  9:50 ` Manivannan Sadhasivam
  2021-07-15 22:50 ` Miquel Raynal
  0 siblings, 2 replies; 6+ messages in thread
From: Dan Carpenter @ 2021-06-17 13:37 UTC (permalink / raw)
  To: Miquel Raynal, Manivannan Sadhasivam
  Cc: Richard Weinberger, Vignesh Raghavendra, Boris Brezillon,
	Tudor Ambarus, linux-mtd, kernel-janitors

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(-)

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mtd: rawnand: Add a check in of_get_nand_secure_regions()
  2021-06-17 13:37 [PATCH] mtd: rawnand: Add a check in of_get_nand_secure_regions() Dan Carpenter
@ 2021-06-25  9:50 ` Manivannan Sadhasivam
  2021-07-15 22:50 ` Miquel Raynal
  1 sibling, 0 replies; 6+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-25  9:50 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	Boris Brezillon, Tudor Ambarus, linux-mtd, kernel-janitors

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
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mtd: rawnand: Add a check in of_get_nand_secure_regions()
  2021-06-17 13:37 [PATCH] mtd: rawnand: Add a check in of_get_nand_secure_regions() Dan Carpenter
  2021-06-25  9:50 ` Manivannan Sadhasivam
@ 2021-07-15 22:50 ` Miquel Raynal
  2021-07-24 14:27   ` Martin Kaiser
  1 sibling, 1 reply; 6+ messages in thread
From: Miquel Raynal @ 2021-07-15 22:50 UTC (permalink / raw)
  To: Dan Carpenter, Miquel Raynal, Manivannan Sadhasivam
  Cc: Richard Weinberger, Vignesh Raghavendra, Boris Brezillon,
	Tudor Ambarus, linux-mtd, kernel-janitors

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mtd: rawnand: Add a check in of_get_nand_secure_regions()
  2021-07-15 22:50 ` Miquel Raynal
@ 2021-07-24 14:27   ` Martin Kaiser
  2021-07-24 16:03     ` Manivannan Sadhasivam
  0 siblings, 1 reply; 6+ messages in thread
From: Martin Kaiser @ 2021-07-24 14:27 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Dan Carpenter, Manivannan Sadhasivam, Richard Weinberger,
	Vignesh Raghavendra, Boris Brezillon, Tudor Ambarus, linux-mtd,
	kernel-janitors

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mtd: rawnand: Add a check in of_get_nand_secure_regions()
  2021-07-24 14:27   ` Martin Kaiser
@ 2021-07-24 16:03     ` Manivannan Sadhasivam
  2021-07-26  7:58       ` Miquel Raynal
  0 siblings, 1 reply; 6+ messages in thread
From: Manivannan Sadhasivam @ 2021-07-24 16:03 UTC (permalink / raw)
  To: Martin Kaiser
  Cc: Miquel Raynal, Dan Carpenter, Richard Weinberger,
	Vignesh Raghavendra, Boris Brezillon, Tudor Ambarus, linux-mtd,
	kernel-janitors

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mtd: rawnand: Add a check in of_get_nand_secure_regions()
  2021-07-24 16:03     ` Manivannan Sadhasivam
@ 2021-07-26  7:58       ` Miquel Raynal
  0 siblings, 0 replies; 6+ messages in thread
From: Miquel Raynal @ 2021-07-26  7:58 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: Martin Kaiser, Dan Carpenter, Richard Weinberger,
	Vignesh Raghavendra, Boris Brezillon, Tudor Ambarus, linux-mtd,
	kernel-janitors

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-07-26  7:58 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-17 13:37 [PATCH] mtd: rawnand: Add a check in of_get_nand_secure_regions() Dan Carpenter
2021-06-25  9:50 ` Manivannan Sadhasivam
2021-07-15 22:50 ` Miquel Raynal
2021-07-24 14:27   ` Martin Kaiser
2021-07-24 16:03     ` Manivannan Sadhasivam
2021-07-26  7:58       ` Miquel Raynal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).