Message ID | CAPgLHd8V06KzBcPmFywSfNxM0q6=s86xn-AdLKeY9h5JZ_nP7g@mail.gmail.com |
---|---|
State | Accepted |
Commit | 9211439b8a933a624d770113d0e0aa742db2ec67 |
Headers | show |
On Friday, November 01, 2013 9:16 AM, Wei Yongjun wrote: > > From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > > Fix to return a negative error code from the error handling > case instead of 0, as done elsewhere in this function. Commit message is right? :-( Best regards, Jingoo Han > > Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > --- > drivers/mtd/nand/omap2.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c > index ec40b8d..f777250 100644 > --- a/drivers/mtd/nand/omap2.c > +++ b/drivers/mtd/nand/omap2.c > @@ -1953,7 +1953,8 @@ static int omap_nand_probe(struct platform_device *pdev) > nand_chip->ecc.read_page = omap_read_page_bch; > nand_chip->ecc.write_page = omap_write_page_bch; > /* This ECC scheme requires ELM H/W block */ > - if (is_elm_present(info, pdata->elm_of_node, BCH8_ECC) < 0) { > + err = is_elm_present(info, pdata->elm_of_node, BCH8_ECC); > + if (err < 0) { > pr_err("nand: error: could not initialize ELM\n"); > goto return_error; > }
On Thu, Oct 31, 2013 at 7:18 PM, Jingoo Han <jg1.han@samsung.com> wrote: > On Friday, November 01, 2013 9:16 AM, Wei Yongjun wrote: >> >> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> >> >> Fix to return a negative error code from the error handling >> case instead of 0, as done elsewhere in this function. > > Commit message is right? :-( It sounds OK by my reading. Unless you're having trouble parsing what "as done elsewhere in this function" is being applied to. (IOW, is the rest of the function returning a negative error code on the error paths, or is it returning 0? Of course the answer is the former, but it's possible to misread it.) If it helps, I can try to tweak the wording a bit when applying this patch. Pekon, can I get an Acked-by? Thanks, Brian
On Tue, Nov 05, 2013 at 01:59:25PM -0800, Brian Norris wrote: > On Thu, Oct 31, 2013 at 7:18 PM, Jingoo Han <jg1.han@samsung.com> wrote: > > On Friday, November 01, 2013 9:16 AM, Wei Yongjun wrote: > >> > >> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > >> > >> Fix to return a negative error code from the error handling > >> case instead of 0, as done elsewhere in this function. > > > > Commit message is right? :-( > > It sounds OK by my reading. Unless you're having trouble parsing what > "as done elsewhere in this function" is being applied to. (IOW, is the > rest of the function returning a negative error code on the error > paths, or is it returning 0? Of course the answer is the former, but > it's possible to misread it.) If it helps, I can try to tweak the > wording a bit when applying this patch. > > Pekon, can I get an Acked-by? > I guess you'd prefer Pekon's ack than mine, but anyway: Acked-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> I'd like to point out this driver has other "mis-behaviors" in returning codes in some other places. In particular, this pattern can be found repeatedly: if (do_something()) { err = -ENXIO; goto some_other_place; } Which should probably be: err = do_something(); if (err) goto some_other_place; Wei: maybe you'd like to prepare some more patches?
> From: Brian Norris [mailto:computersforpeace@gmail.com] > > On Thu, Oct 31, 2013 at 7:18 PM, Jingoo Han <jg1.han@samsung.com> > >> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > >> > >> Fix to return a negative error code from the error handling > >> case instead of 0, as done elsewhere in this function. > > > > Commit message is right? :-( > > It sounds OK by my reading. Unless you're having trouble parsing what > "as done elsewhere in this function" is being applied to. (IOW, is the > rest of the function returning a negative error code on the error > paths, or is it returning 0? Of course the answer is the former, but > it's possible to misread it.) If it helps, I can try to tweak the > wording a bit when applying this patch. > > Pekon, can I get an Acked-by? > Yes sure .. Sorry I was away from both mailbox and boards. Acked-by: Pekon Gupta <pekon@ti.com> And thanks much Wei Yongjun for fixing this . with regards, pekon
On Tue, Nov 05, 2013 at 07:45:27PM -0300, Ezequiel Garcia wrote: > On Tue, Nov 05, 2013 at 01:59:25PM -0800, Brian Norris wrote: > > On Thu, Oct 31, 2013 at 7:18 PM, Jingoo Han <jg1.han@samsung.com> wrote: > > > On Friday, November 01, 2013 9:16 AM, Wei Yongjun wrote: > > >> > > >> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > > >> > > >> Fix to return a negative error code from the error handling > > >> case instead of 0, as done elsewhere in this function. > > > > > > Commit message is right? :-( > > > > It sounds OK by my reading. Unless you're having trouble parsing what > > "as done elsewhere in this function" is being applied to. (IOW, is the > > rest of the function returning a negative error code on the error > > paths, or is it returning 0? Of course the answer is the former, but > > it's possible to misread it.) If it helps, I can try to tweak the > > wording a bit when applying this patch. > > > > Pekon, can I get an Acked-by? > > > > I guess you'd prefer Pekon's ack than mine, but anyway: Eh, this patch was pretty small anyway. But extra eyes are good. > Acked-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> Thanks. > I'd like to point out this driver has other "mis-behaviors" in returning codes > in some other places. > > In particular, this pattern can be found repeatedly: > > if (do_something()) { > err = -ENXIO; > goto some_other_place; > } > > Which should probably be: > > err = do_something(); > if (err) > goto some_other_place; Yeah, these could be made more consistent. If the callee is choosing good error codes, then we can just return them. But this is mostly cosmetic. > Wei: maybe you'd like to prepare some more patches? Brian
On Wed, Nov 06, 2013 at 06:06:25PM +0000, Pekon Gupta wrote: > > From: Brian Norris [mailto:computersforpeace@gmail.com] > > > On Thu, Oct 31, 2013 at 7:18 PM, Jingoo Han <jg1.han@samsung.com> > > >> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > > >> > > >> Fix to return a negative error code from the error handling > > >> case instead of 0, as done elsewhere in this function. > > > > > > Commit message is right? :-( > > > > It sounds OK by my reading. Unless you're having trouble parsing what > > "as done elsewhere in this function" is being applied to. (IOW, is the > > rest of the function returning a negative error code on the error > > paths, or is it returning 0? Of course the answer is the former, but > > it's possible to misread it.) If it helps, I can try to tweak the > > wording a bit when applying this patch. > > > > Pekon, can I get an Acked-by? > > > Yes sure .. Sorry I was away from both mailbox and boards. No problem. > Acked-by: Pekon Gupta <pekon@ti.com> > > And thanks much Wei Yongjun for fixing this . Thanks. Pushed to l2-mtd.git with a slight tweak in the description. Brian
On Thursday, November 07, 2013 4:19 PM, Brian Norris wrote: > On Wed, Nov 06, 2013 at 06:06:25PM +0000, Pekon Gupta wrote: > > > From: Brian Norris [mailto:computersforpeace@gmail.com] > > > > On Thu, Oct 31, 2013 at 7:18 PM, Jingoo Han <jg1.han@samsung.com> > > > >> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > > > >> > > > >> Fix to return a negative error code from the error handling > > > >> case instead of 0, as done elsewhere in this function. > > > > > > > > Commit message is right? :-( > > > > > > It sounds OK by my reading. Unless you're having trouble parsing what > > > "as done elsewhere in this function" is being applied to. (IOW, is the > > > rest of the function returning a negative error code on the error > > > paths, or is it returning 0? Of course the answer is the former, but > > > it's possible to misread it.) If it helps, I can try to tweak the > > > wording a bit when applying this patch. > > > > > > Pekon, can I get an Acked-by? > > > > > Yes sure .. Sorry I was away from both mailbox and boards. > > No problem. > > > Acked-by: Pekon Gupta <pekon@ti.com> > > > > And thanks much Wei Yongjun for fixing this . > > Thanks. Pushed to l2-mtd.git with a slight tweak in the description. Hi Norris, I checked the patch in l2-mtd.git. The commit message looks good. I understand what the patch means. Thank you. Best regards, Jingoo Han
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c index ec40b8d..f777250 100644 --- a/drivers/mtd/nand/omap2.c +++ b/drivers/mtd/nand/omap2.c @@ -1953,7 +1953,8 @@ static int omap_nand_probe(struct platform_device *pdev) nand_chip->ecc.read_page = omap_read_page_bch; nand_chip->ecc.write_page = omap_write_page_bch; /* This ECC scheme requires ELM H/W block */ - if (is_elm_present(info, pdata->elm_of_node, BCH8_ECC) < 0) { + err = is_elm_present(info, pdata->elm_of_node, BCH8_ECC); + if (err < 0) { pr_err("nand: error: could not initialize ELM\n"); goto return_error; }