Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756707AbcK2QxQ (ORCPT ); Tue, 29 Nov 2016 11:53:16 -0500 Received: from mail-pg0-f66.google.com ([74.125.83.66]:32791 "EHLO mail-pg0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754646AbcK2QxB (ORCPT ); Tue, 29 Nov 2016 11:53:01 -0500 Subject: Re: [PATCH] of: Fix issue where code would fall through to error case. To: Rob Herring 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> Cc: Moritz Fischer , Moritz Fischer , "linux-kernel@vger.kernel.org" , Pantelis Antoniou , moritz@pure-entropy.org, "devicetree@vger.kernel.org" From: Frank Rowand Message-ID: <583DB269.9070208@gmail.com> Date: Tue, 29 Nov 2016 08:52:57 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3389 Lines: 101 On 11/29/16 07:06, Rob Herring wrote: > 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 I misread your original suggestion to mean to put the "if (err) pr_err(...)" inside the for loop, where Moritz had made his changes. Now I understand what you really meant, to put the "if (err) pr_err(...)" after the for loop. Yes, that is a good way to do it. Sorry for the extra noise.... -Frank