Re: [PATCH v2 3/5] mtd: rawnand: sunxi: clean sunxi_nand_chip_init()

[Andre Przywara <andre.przywara@arm.com>]

On Mon, 27 Apr 2026 14:35:08 +0200
Andre Przywara <andre.przywara@arm.com> wrote:

> Hi,
> 
> On 3/27/26 15:42, Michael Nazzareno Trimarchi wrote:
> > Hi Richard
> > 
> > On Fri, Mar 27, 2026 at 3:05 PM Richard Genoud
> > <richard.genoud@bootlin.com> wrote:  
> >>
> >> In sunxi_nand_chip_init there's quite a lot of kfree/return, it's easy
> >> to forget a kfree(), so use a goto/kfree instead.
> >>
> >> Signed-off-by: Richard Genoud <richard.genoud@bootlin.com>
> >> ---
> >>   drivers/mtd/nand/raw/sunxi_nand.c | 41 ++++++++++++++-----------------
> >>   1 file changed, 18 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
> >> index 9fc9bc5e0198..ecab9ebc9576 100644
> >> --- a/drivers/mtd/nand/raw/sunxi_nand.c
> >> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
> >> @@ -1600,21 +1600,20 @@ static int sunxi_nand_chip_init(struct udevice *dev, struct sunxi_nfc *nfc,
> >>                  if (ret) {
> >>                          dev_err(dev, "could not retrieve reg property: %d\n",
> >>                                  ret);
> >> -                       kfree(chip);
> >> -                       return ret;
> >> +                       goto out;
> >>                  }
> >>
> >>                  if (tmp > NFC_MAX_CS) {
> >>                          dev_err(dev,
> >>                                  "invalid reg value: %u (max CS = 7)\n", tmp);
> >> -                       kfree(chip);
> >> -                       return -EINVAL;
> >> +                       ret = -EINVAL;
> >> +                       goto out;
> >>                  }
> >>
> >>                  if (test_and_set_bit(tmp, &nfc->assigned_cs)) {
> >>                          dev_err(dev, "CS %d already assigned\n", tmp);
> >> -                       kfree(chip);
> >> -                       return -EINVAL;
> >> +                       ret = -EINVAL;
> >> +                       goto out;
> >>                  }
> >>
> >>                  chip->sels[i].cs = tmp;
> >> @@ -1640,15 +1639,13 @@ static int sunxi_nand_chip_init(struct udevice *dev, struct sunxi_nfc *nfc,
> >>                  dev_err(dev,
> >>                          "could not retrieve timings for ONFI mode 0: %d\n",
> >>                          ret);
> >> -               kfree(chip);
> >> -               return ret;
> >> +               goto out;
> >>          }
> >>
> >>          ret = sunxi_nand_chip_set_timings(nfc, chip, timings);
> >>          if (ret) {
> >>                  dev_err(dev, "could not configure chip timings: %d\n", ret);
> >> -               kfree(chip);
> >> -               return ret;
> >> +               goto out;
> >>          }
> >>
> >>          nand = &chip->nand;
> >> @@ -1669,10 +1666,8 @@ static int sunxi_nand_chip_init(struct udevice *dev, struct sunxi_nfc *nfc,
> >>
> >>          mtd = nand_to_mtd(nand);
> >>          ret = nand_scan_ident(mtd, nsels, NULL);
> >> -       if (ret) {
> >> -               kfree(chip);
> >> -               return ret;
> >> -       }
> >> +       if (ret)
> >> +               goto out;
> >>
> >>          if (nand->bbt_options & NAND_BBT_USE_FLASH)
> >>                  nand->bbt_options |= NAND_BBT_NO_OOB;
> >> @@ -1685,34 +1680,34 @@ static int sunxi_nand_chip_init(struct udevice *dev, struct sunxi_nfc *nfc,
> >>          ret = sunxi_nand_chip_init_timings(nfc, chip);
> >>          if (ret) {
> >>                  dev_err(dev, "could not configure chip timings: %d\n", ret);
> >> -               kfree(chip);
> >> -               return ret;
> >> +               goto out;
> >>          }
> >>
> >>          ret = sunxi_nand_ecc_init(mtd, &nand->ecc);
> >>          if (ret) {
> >>                  dev_err(dev, "ECC init failed: %d\n", ret);
> >> -               kfree(chip);
> >> -               return ret;
> >> +               goto out;
> >>          }
> >>
> >>          ret = nand_scan_tail(mtd);
> >>          if (ret) {
> >>                  dev_err(dev, "nand_scan_tail failed: %d\n", ret);
> >> -               kfree(chip);
> >> -               return ret;
> >> +               goto out;
> >>          }
> >>
> >>          ret = nand_register(devnum, mtd);
> >>          if (ret) {
> >>                  dev_err(dev, "failed to register mtd device: %d\n", ret);
> >> -               kfree(chip);
> >> -               return ret;
> >> +               goto out;
> >>          }
> >>
> >>          list_add_tail(&chip->node, &nfc->chips);
> >>
> >> -       return 0;
> >> +out:
> >> +       if (ret)
> >> +               kfree(chip);
> >> +
> >> +       return ret;
> >>   }
> >>
> >>   static int sunxi_nand_chips_init(struct udevice *dev, struct sunxi_nfc *nfc)  
> > 
> > I need to go a bit in the code but seems that out can happen even if
> > there is no error. It's
> > not so common to have if (ret) if exit condition in case of errot  
> 
> So I think it's fine in this case, ret is always set before we "goto", 
> and in the success case "ret" must be 0 from the last check. But I agree 
> that the construct is a bit uncommon and fragile, so what about keeping 
> the "return 0;" (to use the common pattern and help readability), and 
> rename the label to "out_err" or "out_free", to make it more obvious 
> that this the error path only, and we expect ret to be set?

So FYI, in the interest of moving this patch set forward, I took the
freedom of changing this as I suggested above. That's gonna be part of
the PR now.

Cheers,
Andre

> 
> Cheers,
> Andre
> 
> > 
> > Michael  
>