Re: [PATCH 1/2] mtd: nand: BUG_ON in case of no select_chip and cmd_ctrl

[Boris Brezillon]

On Tue, 19 Jul 2016 18:02:27 +0200
Richard Weinberger <richard@xxxxxx> wrote:

> Am 19.07.2016 um 17:59 schrieb Boris Brezillon:
> > On Tue, 19 Jul 2016 17:44:48 +0200
> > Richard Weinberger <richard@xxxxxx> wrote:
> >   
> >> Am 19.07.2016 um 17:41 schrieb Andrey Smirnov:  
> >>> If no user specified chip->select_chip() function is provided, code in
> >>> nand_base.c will automatically set this hook to nand_select_chip(),
> >>> which in turn depends on chip->cmd_ctrl() hook being valid. Not
> >>> providing both of those functions in NAND controller driver (for example
> >>> by mistake) will result in a bit cryptic segfault. Replace it with
> >>> explicit BUG_ON statement so it would be obvious what went wrong.
> >>>
> >>> Signed-off-by: Andrey Smirnov <andrew.smirnov@xxxxxxxxx>
> >>> ---
> >>>  drivers/mtd/nand/nand_base.c | 4 +++-
> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> 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.

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).

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.