* [bug report] mtd: plat_nand: request memory resource before doing ioremap
@ 2018-02-20 8:41 Dan Carpenter
2018-02-20 9:07 ` Boris Brezillon
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2018-02-20 8:41 UTC (permalink / raw)
To: hartleys; +Cc: Richard Weinberger, linux-mtd
[ This is really weird that Smatch is complaining about ancient code
today. No idea why. - dan ]
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'
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;
^^^^^^^^^^^^^^^^^^^
Ugh... Success handling. There seems to be a lot of it in this
subsystem. :(
101
102 nand_release(mtd);
^^^
This call to nand_release() makes no sense. It calls unregister but
mtd_device_parse_register() failed.
103 out:
104 if (pdata->ctrl.remove)
105 pdata->ctrl.remove(pdev);
106 return err;
107 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] mtd: plat_nand: request memory resource before doing ioremap
2018-02-20 8:41 [bug report] mtd: plat_nand: request memory resource before doing ioremap Dan Carpenter
@ 2018-02-20 9:07 ` Boris Brezillon
2018-02-20 9:36 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Boris Brezillon @ 2018-02-20 9:07 UTC (permalink / raw)
To: Dan Carpenter; +Cc: hartleys, Richard Weinberger, linux-mtd
Hi Dan,
On Tue, 20 Feb 2018 11:41:15 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:
> [ This is really weird that Smatch is complaining about ancient code
> today. No idea why. - dan ]
Probably because I moved NAND code around recently.
>
> 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?
>
> 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;
> ^^^^^^^^^^^^^^^^^^^
>
> 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.
>
> 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
--
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] mtd: plat_nand: request memory resource before doing ioremap
2018-02-20 9:07 ` Boris Brezillon
@ 2018-02-20 9:36 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2018-02-20 9:36 UTC (permalink / raw)
To: Boris Brezillon; +Cc: hartleys, Richard Weinberger, linux-mtd
On Tue, Feb 20, 2018 at 10:07:59AM +0100, Boris Brezillon wrote:
> Hi Dan,
>
> On Tue, 20 Feb 2018 11:41:15 +0300
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> > [ This is really weird that Smatch is complaining about ancient code
> > today. No idea why. - dan ]
>
> Probably because I moved NAND code around recently.
>
> >
> > 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?
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.
>
> >
> > 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;
> > ^^^^^^^^^^^^^^^^^^^
> >
> > 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.
> >
> > 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.
>
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-02-20 9:36 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-20 8:41 [bug report] mtd: plat_nand: request memory resource before doing ioremap Dan Carpenter
2018-02-20 9:07 ` Boris Brezillon
2018-02-20 9:36 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.