Messages in this thread | | | Date | Tue, 19 Jul 2016 20:16:11 +0200 | From | Boris Brezillon <> | Subject | Re: [PATCH 1/2] mtd: nand: BUG_ON in case of no select_chip and cmd_ctrl |
| |
On Tue, 19 Jul 2016 11:11:54 -0700 Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> On Tue, Jul 19, 2016 at 9:22 AM, Richard Weinberger <richard@nod.at> wrote: > > Am 19.07.2016 um 18:12 schrieb Boris Brezillon: > >>>> Not sure a BUG_ON() is worst than a NULL-pointer exception ;-). > >>> > >>> When this really just triggers a NULL-pointer exception, we don't need a BUG_ON or WARN_ON at > >>> all since the kernel can tell anyway what went wrong. > >> > >> Hm, that's not entirely true, depending on your debug options you don't > >> have all the information to guess which line triggered the NULL pointer > >> exception, and this makes it harder to debug. > >> And I agree with Andrey here, it's better to complain at registration > >> time than letting the controller register all its NAND devices and > >> generate exceptions when the NAND is really used. > >> > >> BTW, I don't quite understand the rational behind BUG_ON() eradication. > >> I agree that they should not be used when the driver can recover from a > >> specific failure, but that's not really the case here (some NAND > >> controller drivers don't check nand_scan_tail() or nand_scan() return > >> code). > > > > I've been told that new code (except core code) should not BUG()/_ON(). > > > >> The best solution would probably be to patch all those drivers and then > >> return an error when one of the mandatory hooks is missing, but in the > >> meantime I don't see any problem in adding BUG_ON() calls. > > > > Yes, definitely. > > I don't have any preferences as far BUG_ON/WARN_ON are concerned and > am more than happy to change one for another. > > The reason I came up with that patch is that I stumbled on that > segfault (by not providing custom select_chip() and not setting up > cmd_ctrl()) and it took me good 20 minutes to figure out the nature of > the problem, whereas, IMHO, having a BUG/WARN statement at the would > have been more self-documenting/explanatory. > > What if I modify the patch to change nand_set_default's signature to > return a error code, add corresponding checking in > nand_get_flash_type()/nand_scan_ident() and replace BUG_ON with > WARN_ON? Would it be more agreeable solution?
Agreed.
| |