Messages in this thread | | | Date | Tue, 19 Jul 2016 12:39:18 -0700 | From | Brian Norris <> | Subject | Re: [PATCH 1/2] mtd: nand: BUG_ON in case of no select_chip and cmd_ctrl |
| |
Hi,
On Tue, Jul 19, 2016 at 08:47:03PM +0200, Boris Brezillon wrote: > On Tue, 19 Jul 2016 11:19:16 -0700 > Brian Norris <computersforpeace@gmail.com> wrote: > > > On Tue, Jul 19, 2016 at 06:12:48PM +0200, Boris Brezillon wrote: > > > On Tue, 19 Jul 2016 18:02:27 +0200 > > > Richard Weinberger <richard@nod.at> wrote: > > > > Am 19.07.2016 um 17:59 schrieb Boris Brezillon: > > > > > On Tue, 19 Jul 2016 17:44:48 +0200 > > > > > Richard Weinberger <richard@nod.at> wrote: > > > > >> Am 19.07.2016 um 17:41 schrieb Andrey Smirnov: > > > > >>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > > > > >>> index ce7b2ca..57043a6 100644 > > > > >>> --- a/drivers/mtd/nand/nand_base.c > > > > >>> +++ b/drivers/mtd/nand/nand_base.c > > > > >>> @@ -3128,8 +3128,10 @@ static void nand_set_defaults(struct nand_chip *chip, int busw) > > > > >>> if (chip->waitfunc == NULL) > > > > >>> chip->waitfunc = nand_wait; > > > > >>> > > > > >>> - if (!chip->select_chip) > > > > >>> + if (!chip->select_chip) { > > > > >>> + BUG_ON(!chip->cmd_ctrl); > > > > >> > > > > >> Please don't add new BUG_ON() calls. WARN_ON() is good enough to raise the driver developer's > > > > >> attention and won't kill the machine. > > > > > > > > > > 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. > > > > Yes, definitely better to complain at registration. But complaining > > doesn't have to be BUG_ON(). > > > > > 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). > > > > It's really not helpful to anyone to have a single picky/buggy/whatever > > driver crash the entire system (e.g., on an early prototype board; or > > while somebody is tinkering and forgets something) when we could > > perfectly easily just fail to register the driver. There are plenty of > > other subsystems that do this, and the world hasn't caught fire yet. > > Hey, I'm already convinced that properly handling error codes in all > drivers and core code is the best approach, but we should also patch > all the code that does not follow this rule and not only framework code.
OK.
> > And regarding the "drivers don't check ... return code": I'm pretty > > tired of that excuse. I don't want to gate any more correct error > > handling on the fact that drivers are s**t. > > That's a bit unfair. I'm trying to improve things in the NAND framework > (and will keep doing so as much as I can), but last time I suggested to
Sorry if I was unfair there. Wasn't intending to blame you (you didn't write 90%+ of the code); just suggesting different priorities.
> patch a driver to properly handle nand_scan_tail() return code instead > of ignoring it you said it was not required [1].
I believe I suggested that it was not required as a dependency for returning errors in the core code. Not that such patches to fix drivers were unwanted.
> You'll say that these drivers have already been used/tested by the > people who submitted them, and that they're known to work fine as is, > but keeping these old drivers in an unclean state just encourages new > comers to submit code reproducing the same mistake, so let's tackle the > problem and patch all offenders.
Patching all offenders is a great goal. Reviewing new drivers to avoid repeating mistakes is good too. And the former can affect the latter, certainly. I just don't want the poor drivers to be an excuse for doing things poorly in the core.
> > > 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. > > > > I do. > > I'm not against dropping all BUG_ON()/BUG() usage in the NAND/MTD > framework, but we should also patch all the offending drivers.
Yes, we're in agreement on that point then.
Brian
| |