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

[Andre Przywara]

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 at 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 at 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?

Cheers,
Andre

> 
> Michael