On Mon, Nov 28, 2016 at 9:30 AM, Frank Rowand <[email protected]> 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 >>> <[email protected]> wrote: >>>> On Thu, Nov 17, 2016 at 4:02 PM, Frank Rowand <[email protected]> >>>> 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 <[email protected]> >>>>>>> --- >>>>>>> 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 <[email protected]> >>>>>> >>>>>> -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
