Hi Miquel,
On Thu, Feb 25, 2021 at 08:47:02AM +0100, Miquel Raynal wrote:
> Hi Manivannan,
>
> Manivannan Sadhasivam <[email protected]> wrote on Thu,
> 25 Feb 2021 09:41:29 +0530:
>
> > On a typical end product, a vendor may choose to secure some regions in
> > the NAND memory which are supposed to stay intact between FW upgrades.
> > The access to those regions will be blocked by a secure element like
> > Trustzone. So the normal world software like Linux kernel should not
> > touch these regions (including reading).
> >
> > The regions are declared using a NAND chip DT property,
> > "nand-secure-regions". So let's make use of this property and skip
> > access to the secure regions present in a system.
> >
> > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > ---
>
> [...]
>
> > config_nand_page_write(nandc);
> > @@ -2830,7 +2865,8 @@ static int qcom_nand_host_init_and_register(struct
> > qcom_nand_controller *nandc,
> > struct nand_chip *chip = &host->chip;
> > struct mtd_info *mtd = nand_to_mtd(chip);
> > struct device *dev = nandc->dev;
> > - int ret;
> > + struct property *prop;
> > + int ret, length, nr_elem;
> >
> > ret = of_property_read_u32(dn, "reg", &host->cs);
> > if (ret) {
> > @@ -2886,6 +2922,24 @@ static int qcom_nand_host_init_and_register(struct
> > qcom_nand_controller *nandc,
> > }
> > }
> >
> > + /*
> > + * Look for secure regions in the NAND chip. These regions are supposed
> > + * to be protected by a secure element like Trustzone. So the read/write
> > + * accesses to these regions will be blocked in the runtime by this
> > + * driver.
> > + */
> > + prop = of_find_property(dn, "nand-secure-regions", &length);
>
> I'm not sure the nand- prefix on this property is needed here, but
> whatever.
>
I was not sure either but added it since most of the other properties
had it. But I can remove it.
> > + if (prop) {
> > + nr_elem = length / sizeof(u32);
> > + host->nr_sec_regions = nr_elem / 2;
> > +
> > + host->sec_regions = devm_kcalloc(dev, nr_elem, sizeof(u32),
> > GFP_KERNEL);
> > + if (!host->sec_regions)
> > + return -ENOMEM;
> > +
> > + of_property_read_u32_array(dn, "nand-secure-regions",
> > host->sec_regions, nr_elem);
> > + }
> > +
>
> I would move this before nand_scan().
>
Okay, I'll do it.
Thanks,
Mani
> If you don't, you should bail out with a nand_cleanup() upon error.
>
> > ret = mtd_device_parse_register(mtd, probes, NULL, NULL, 0);
> > if (ret)
> > nand_cleanup(chip);
>
>
> Otherwise lgtm.
>
> Thanks,
> Miquèl
