* [bug report] clk: vc5: Enable addition output configurations of the Versaclock
@ 2020-06-26 10:32 Dan Carpenter
2020-06-26 22:11 ` Adam Ford
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2020-06-26 10:32 UTC (permalink / raw)
To: aford173; +Cc: linux-clk
Hello Adam Ford,
The patch 260249f929e8: "clk: vc5: Enable addition output
configurations of the Versaclock" from Jun 3, 2020, leads to the
following static checker warning:
drivers/clk/clk-versaclock5.c:795 vc5_get_output_config()
warn: missing error code here? 'of_get_child_by_name()' failed. 'ret' = '0'
drivers/clk/clk-versaclock5.c
784 static int vc5_get_output_config(struct i2c_client *client,
785 struct vc5_hw_data *clk_out)
786 {
787 struct device_node *np_output;
788 char *child_name;
789 int ret = 0;
790
791 child_name = kasprintf(GFP_KERNEL, "OUT%d", clk_out->num + 1);
^^^^^^^^^^
No check for NULL which will lead to an Oops on the next line.
792 np_output = of_get_child_by_name(client->dev.of_node, child_name);
^^^^^^^^^^
Dereferenced without checking inside function.
793 kfree(child_name);
794 if (!np_output)
795 goto output_done;
^^^^^^^^^^^^^^^^^
Why not just "return 0;" so that it's obvious this path is a success
path. :/ Do nothing gotos have a typical forgot to set the error
code bug but direct returns don't suffer from this problem.
People think that do nothing gotos will force future developers think
about error handling but this is not supported by data. My experience
is that nothing can force people to think about error handling.
796
797 ret = vc5_update_mode(np_output, clk_out);
798 if (ret)
799 goto output_error;
800
801 ret = vc5_update_power(np_output, clk_out);
802 if (ret)
803 goto output_error;
804
805 ret = vc5_update_slew(np_output, clk_out);
806
807 output_error:
808 if (ret) {
809 dev_err(&client->dev,
810 "Invalid clock output configuration OUT%d\n",
811 clk_out->num + 1);
812 }
813
814 of_node_put(np_output);
815
816 output_done:
817 return ret;
818 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] clk: vc5: Enable addition output configurations of the Versaclock
2020-06-26 10:32 [bug report] clk: vc5: Enable addition output configurations of the Versaclock Dan Carpenter
@ 2020-06-26 22:11 ` Adam Ford
0 siblings, 0 replies; 2+ messages in thread
From: Adam Ford @ 2020-06-26 22:11 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-clk
On Fri, Jun 26, 2020 at 5:32 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hello Adam Ford,
>
> The patch 260249f929e8: "clk: vc5: Enable addition output
> configurations of the Versaclock" from Jun 3, 2020, leads to the
> following static checker warning:
>
> drivers/clk/clk-versaclock5.c:795 vc5_get_output_config()
> warn: missing error code here? 'of_get_child_by_name()' failed. 'ret' = '0'
>
> drivers/clk/clk-versaclock5.c
> 784 static int vc5_get_output_config(struct i2c_client *client,
> 785 struct vc5_hw_data *clk_out)
> 786 {
> 787 struct device_node *np_output;
> 788 char *child_name;
> 789 int ret = 0;
> 790
> 791 child_name = kasprintf(GFP_KERNEL, "OUT%d", clk_out->num + 1);
> ^^^^^^^^^^
> No check for NULL which will lead to an Oops on the next line.
Thanks for your feedback.
I've drafted a patch that I want to run by a colleague who wrote much
of the patch I submitted. I'd like him to review it.
I have also applied another patch [1] which corrects some other
issues. The patch I am drafting goes after [1].
>
> 792 np_output = of_get_child_by_name(client->dev.of_node, child_name);
> ^^^^^^^^^^
> Dereferenced without checking inside function.
>
> 793 kfree(child_name);
> 794 if (!np_output)
> 795 goto output_done;
> ^^^^^^^^^^^^^^^^^
> Why not just "return 0;" so that it's obvious this path is a success
> path. :/ Do nothing gotos have a typical forgot to set the error
> code bug but direct returns don't suffer from this problem.
>
> People think that do nothing gotos will force future developers think
> about error handling but this is not supported by data. My experience
> is that nothing can force people to think about error handling.
That makes sense to me.
I've added 'suggested-by' with your name on it, so you'll get CC'd
when I submit the patch. I'd welcome any feedback you have to offer.
This driver series is important to me, because I need it in order to
support a new board I am trying to get into the kernel.
thanks,
adam
>
> 796
> 797 ret = vc5_update_mode(np_output, clk_out);
> 798 if (ret)
> 799 goto output_error;
> 800
> 801 ret = vc5_update_power(np_output, clk_out);
> 802 if (ret)
> 803 goto output_error;
> 804
> 805 ret = vc5_update_slew(np_output, clk_out);
> 806
> 807 output_error:
> 808 if (ret) {
> 809 dev_err(&client->dev,
> 810 "Invalid clock output configuration OUT%d\n",
> 811 clk_out->num + 1);
> 812 }
> 813
> 814 of_node_put(np_output);
> 815
> 816 output_done:
> 817 return ret;
> 818 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-06-26 22:11 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-26 10:32 [bug report] clk: vc5: Enable addition output configurations of the Versaclock Dan Carpenter
2020-06-26 22:11 ` Adam Ford
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).