Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756935AbcK2Pzb (ORCPT ); Tue, 29 Nov 2016 10:55:31 -0500 Received: from mail-pg0-f54.google.com ([74.125.83.54]:35872 "EHLO mail-pg0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756324AbcK2PzU (ORCPT ); Tue, 29 Nov 2016 10:55:20 -0500 Date: Tue, 29 Nov 2016 07:55:17 -0800 From: Moritz Fischer To: Rob Herring Cc: Frank Rowand , Moritz Fischer , "linux-kernel@vger.kernel.org" , Pantelis Antoniou , moritz@pure-entropy.org, "devicetree@vger.kernel.org" Subject: Re: [PATCH] of: Fix issue where code would fall through to error case. Message-ID: <20161129155517.GA23667@live.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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.7.0 (2016-08-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3416 Lines: 109 Hi Rob, On Tue, Nov 29, 2016 at 09:06:08AM -0600, 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. I think you're right. Can you look at my v3 that I sent. I also tried to fix cases where we can just do return 0; vs. err = 0; goto err ... err: of_node_put(NULL /*tree_symbols is NULL*/); return err; Thanks, Moritz