mtd: plat_nand: request memory resource before doing ioremap

bug report Dan Carpenter 2 participants 3 messages Feb 20, 2018 Memory technology devices (MTD)

lore

Share & tools

lore

Boris Brezillon

2018-02-20 09:08 UTC #1 lore raw

Hi Dan,

Quoted 2 lines · On Tue, 20 Feb 2018 11:41:15 +0300 Dan Carpenter <[email protected]>…

[ This is really weird that Smatch is complaining about ancient code
today. No idea why. - dan ]

Probably because I moved NAND code around recently.

Quoted 9 lines

Hello H Hartley Sweeten,
The patch 2d098a725333: "mtd: plat_nand: request memory resource before doing ioremap" from Oct 19, 2009, leads to the following static checker warning:
	drivers/mtd/nand/raw/plat_nand.c:100 plat_nand_probe()
	info: return a literal instead of 'err'

Hm, I don't really get this error message? What's wrong with returning err when it's 0? It's true that we could return 0 directly, but I don't see it as a real issue. Am I missing something?

Quoted 27 lines

drivers/mtd/nand/raw/plat_nand.c

    78  
    79          platform_set_drvdata(pdev, data);
    80  
    81          /* Handle any platform specific setup */
    82          if (pdata->ctrl.probe) {
    83                  err = pdata->ctrl.probe(pdev);
    84                  if (err)
    85                          goto out;
    86          }
    87  
    88          /* Scan to find existence of the device */
    89          err = nand_scan(mtd, pdata->chip.nr_chips);
    90          if (err)
    91                  goto out;
    92  
    93          part_types = pdata->chip.part_probe_types;
    94  
    95          err = mtd_device_parse_register(mtd, part_types, NULL,
    96                                          pdata->chip.partitions,
    97                                          pdata->chip.nr_partitions);
    98  
    99          if (!err)
   100                  return err;
                ^^^^^^^^^^^^^^^^^^^

Quoted 2 lines

Ugh... Success handling. There seems to be a lot of it in this subsystem. :(

Yep, there's a lot of ancient code that no one dares to touch in the fear that it may break things. Also, there's so many old drivers that cleaning up everything would take a lot of time.

Quoted 6 lines

   101  
   102          nand_release(mtd);
                             ^^^
This call to nand_release() makes no sense. It calls unregister but mtd_device_parse_register() failed.

You're right, we should call nand_cleanup() here. This being said, calling mtd_device_unregister() should be harmless because of this test [1].

To sum-up, this code is definitely not meeting the kernel standards in term of code quality, but it doesn't seem to be buggy either.

Regards,

Boris

[1]https://elixir.bootlin.com/linux/v4.8/source/drivers/mtd/mtdcore.c#L664

Signature · 3 lines

Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Dan Carpenter

author

2018-02-20 09:36 UTC #2 lore raw

Quoted 17 lines · Boris Brezillon

Hi Dan,
Quoted 2 lines · On Tue, 20 Feb 2018 11:41:15 +0300 Dan Carpenter <[email protected]>…
[ This is really weird that Smatch is complaining about ancient code
today. No idea why. - dan ]
Probably because I moved NAND code around recently.
Quoted 7 lines
Hello H Hartley Sweeten,
The patch 2d098a725333: "mtd: plat_nand: request memory resource before doing ioremap" from Oct 19, 2009, leads to the following static checker warning:

Quoted 6 lines

Quoted 2 lines
	drivers/mtd/nand/raw/plat_nand.c:100 plat_nand_probe()
	info: return a literal instead of 'err'
Hm, I don't really get this error message? What's wrong with returning err when it's 0? It's true that we could return 0 directly, but I don't see it as a real issue. Am I missing something?

There are a couple reasons. Literals are more readable:

if (!ret)
- return ret;
+ return 0;

But the main reason is that most of the time in the kernel we do error handling instead of success handling. So sometimes the '!' character is just a typo. This kind of bug would normally be caught in testing unless we weren't able to test it or it's in a permission check where we normally expect the function to just return zero.

Quoted 28 lines

drivers/mtd/nand/raw/plat_nand.c

    78  
    79          platform_set_drvdata(pdev, data);
    80  
    81          /* Handle any platform specific setup */
    82          if (pdata->ctrl.probe) {
    83                  err = pdata->ctrl.probe(pdev);
    84                  if (err)
    85                          goto out;
    86          }
    87  
    88          /* Scan to find existence of the device */
    89          err = nand_scan(mtd, pdata->chip.nr_chips);
    90          if (err)
    91                  goto out;
    92  
    93          part_types = pdata->chip.part_probe_types;
    94  
    95          err = mtd_device_parse_register(mtd, part_types, NULL,
    96                                          pdata->chip.partitions,
    97                                          pdata->chip.nr_partitions);
    98  
    99          if (!err)
   100                  return err;
                ^^^^^^^^^^^^^^^^^^^

Quoted 7 lines

Quoted 2 lines
Ugh... Success handling. There seems to be a lot of it in this subsystem. :(
Yep, there's a lot of ancient code that no one dares to touch in the fear that it may break things. Also, there's so many old drivers that cleaning up everything would take a lot of time.

Yes. Of course. I understand.

Quoted 7 lines

Quoted 6 lines
   101  
   102          nand_release(mtd);
                             ^^^
This call to nand_release() makes no sense. It calls unregister but mtd_device_parse_register() failed.

Quoted 7 lines

You're right, we should call nand_cleanup() here. This being said, calling mtd_device_unregister() should be harmless because of this test [1].
To sum-up, this code is definitely not meeting the kernel standards in term of code quality, but it doesn't seem to be buggy either.

Thanks for taking a look at this code. I have looked at it some more and you're obviously right that it works. Part of what puzzled me, is that I had no clue that nand_cleanup() went with nand_scan(). I sort of figured from the context that we must be freeing something that nand_scan() allocated, but it wasn't obvious.

regards,
dan carpenter