[bug report] clk: vc5: Enable addition output configurations of the Versaclock

[bug report] clk: vc5: Enable addition output configurations of the Versaclock

[Dan Carpenter]

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

Re: [bug report] clk: vc5: Enable addition output configurations of the Versaclock

[Adam Ford]

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