Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933817AbcK2PGr (ORCPT ); Tue, 29 Nov 2016 10:06:47 -0500 Received: from mail.kernel.org ([198.145.29.136]:49008 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933512AbcK2PGi (ORCPT ); Tue, 29 Nov 2016 10:06:38 -0500 MIME-Version: 1.0 In-Reply-To: <583C4D83.6040800@gmail.com> References: <1479425157-6235-1-git-send-email-moritz.fischer@ettus.com> <582E3FFC.80305@gmail.com> <582E452E.3080909@gmail.com> <583A0110.4090603@gmail.com> <583C4D83.6040800@gmail.com> From: Rob Herring Date: Tue, 29 Nov 2016 09:06:08 -0600 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH] of: Fix issue where code would fall through to error case. To: Frank Rowand Cc: Moritz Fischer , Moritz Fischer , "linux-kernel@vger.kernel.org" , Pantelis Antoniou , moritz@pure-entropy.org, "devicetree@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2953 Lines: 88 On Mon, Nov 28, 2016 at 9:30 AM, Frank Rowand wrote: > On 11/26/16 13:39, Frank Rowand wrote: >> On 11/23/16 13:58, Rob Herring wrote: >>> On Thu, Nov 17, 2016 at 6:10 PM, Moritz Fischer >>> wrote: >>>> On Thu, Nov 17, 2016 at 4:02 PM, Frank Rowand wrote: >>>>> On 11/17/16 15:40, Frank Rowand wrote: >>>>>> On 11/17/16 15:25, Moritz Fischer wrote: >>>>>>> No longer fall through into the error case that prints out >>>>>>> an error if no error (err = 0) occurred. >>>>>>> >>>>>>> Fixes d9181b20a83(of: Add back an error message, restructured) >>>>>>> Signed-off-by: Moritz Fischer >>>>>>> --- >>>>>>> drivers/of/resolver.c | 6 +++++- >>>>>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c >>>>>>> index 783bd09..785076d 100644 >>>>>>> --- a/drivers/of/resolver.c >>>>>>> +++ b/drivers/of/resolver.c >>>>>>> @@ -358,9 +358,13 @@ int of_resolve_phandles(struct device_node *overlay) >>>>>>> >>>>>>> err = update_usages_of_a_phandle_reference(overlay, prop, phandle); >>>>>>> if (err) >>>>>>> - break; >>>>>>> + goto err_out; >>>>>>> } >>>>>>> >>>>>>> + of_node_put(tree_symbols); >>>>>>> + >>>>>>> + return 0; >>>>>>> + >>>>>>> err_out: >>>>>>> pr_err("overlay phandle fixup failed: %d\n", err); >>>>>>> out: >>>>>> >>>>>> Thanks for catching that. >>>>>> >>>>>> Rob, please apply. >>>>>> >>>>>> Reviewed-by: Frank Rowand >>>>>> >>>>>> -Frank >>>>> >>>>> On second thought, isn't the common pattern when clean up is needed for >>>>> both the no-error path and the error path something like: >>>>> >>>>> >>>>> out: >>>>> of_node_put(tree_symbols); >>>>> return err; >>>>> >>>>> err_out: >>>>> pr_err("overlay phandle fixup failed: %d\n", err); >>>>> goto out; >>>>> } >>>>> >>>>> >>>>> I don't have a strong opinion, whatever Rob wants to take is fine with me. >>>> >>>> Same here. I tried to avoid the jumping back part, but if that's the >>>> common pattern, >>>> I can submit a v2 doing that instead. >>> >>> Both are ugly. Just do: >>> >>> if (err) >>> pr_err(...); >>> >>> Rob >> >> Agreed. Thanks for the touch of sanity Rob. >> >> -Frank > > I succumbed to looking only at the few lines of code above and not the > fuller context of the file that the patch applies to. > > The proposed patch was fixing the problem that a normal completion > of the for loop was falling through into the err_out label. So what > looks cleaner ("if (err) pr_err(...)") is actually not correct. What!? The *only* problem was printing the error message in the err=0 case. All that needs to be fixed is not doing that. If we do that, then we really only need 1 goto label. Rob