On Tue, Jul 19, 2016 at 11:44 PM, Boris Brezillon <boris.brezil...@free-electrons.com> wrote: > On Tue, 19 Jul 2016 22:33:57 -0700 > Andrey Smirnov <andrew.smir...@gmail.com> wrote: > >> 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. Same is true for >> chip->cmdfunc(). >> >> To avoid the above scenario change the prototype of nand_set_defaults() >> to return error code, all the callers to check for it and error out if >> cmd_ctrl() is not provided. >> >> Suggested-by: Brian Norris <computersforpe...@gmail.com> >> Signed-off-by: Andrey Smirnov <andrew.smir...@gmail.com> >> --- >> drivers/mtd/nand/nand_base.c | 29 ++++++++++++++++++++++++----- >> 1 file changed, 24 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c >> index ce7b2ca..bee480f 100644 >> --- a/drivers/mtd/nand/nand_base.c >> +++ b/drivers/mtd/nand/nand_base.c >> @@ -3114,22 +3114,30 @@ static void nand_shutdown(struct mtd_info *mtd) >> } >> >> /* Set default functions */ >> -static void nand_set_defaults(struct nand_chip *chip, int busw) >> +static int nand_set_defaults(struct nand_chip *chip, int busw) >> { >> /* check for proper chip_delay setup, set 20us if not */ >> if (!chip->chip_delay) >> chip->chip_delay = 20; >> >> /* check, if a user supplied command function given */ >> - if (chip->cmdfunc == NULL) >> + if (chip->cmdfunc == NULL) { >> + if (!chip->cmd_ctrl) >> + goto no_cmd_ctrl; >> + >> chip->cmdfunc = nand_command; >> + } >> >> /* check, if a user supplied wait function given */ >> if (chip->waitfunc == NULL) >> chip->waitfunc = nand_wait; >> >> - if (!chip->select_chip) >> + if (!chip->select_chip) { >> + if (!chip->cmd_ctrl) >> + goto no_cmd_ctrl; >> + >> chip->select_chip = nand_select_chip; >> + } >> >> /* set for ONFI nand */ >> if (!chip->onfi_set_features) >> @@ -3161,6 +3169,11 @@ static void nand_set_defaults(struct nand_chip *chip, >> int busw) >> init_waitqueue_head(&chip->controller->wq); >> } >> >> + return 0; >> + >> +no_cmd_ctrl: >> + pr_err("chip.cmd_ctrl() callback is not provided\n"); >> + return -EINVAL; >> } >> >> /* Sanitize ONFI strings so we can safely print them */ >> @@ -3878,9 +3891,13 @@ ident_done: >> } >> >> if (chip->options & NAND_BUSWIDTH_AUTO) { >> + int ret; >> + >> WARN_ON(chip->options & NAND_BUSWIDTH_16); >> chip->options |= busw; >> - nand_set_defaults(chip, busw); >> + ret = nand_set_defaults(chip, busw); > > I realize this one will actually never fail because it has already been > checked in nand_scan_ident(). Maybe we should leave the > nand_set_defaults() as a void function and check for missing > ->cmd_ctrl() directly in nand_scan_ident().
Sure, will change in v3, this would also allow me to have only a single check instead of two and those ugly gotos. Andrey