re: clk: lpc32xx: add common clock framework driver

re: clk: lpc32xx: add common clock framework driver

[Dan Carpenter]

Hello Vladimir Zapolskiy (and other clk devs as well),

The patch f7c82a60ba26: "clk: lpc32xx: add common clock framework
driver" from Dec 6, 2015, leads to the following static checker
warning:

	drivers/clk/nxp/clk-lpc32xx.c:1019 clk_mux_get_parent()
	warn: signedness bug returning '(-22)'

drivers/clk/nxp/clk-lpc32xx.c
  1003  static u8 clk_mux_get_parent(struct clk_hw *hw)
               ^^
  1004  {
  1005          struct lpc32xx_clk_mux *mux = to_lpc32xx_mux(hw);
  1006          u32 num_parents = clk_hw_get_num_parents(hw);
  1007          u32 val;
  1008  
  1009          regmap_read(clk_regmap, mux->reg, &val);
  1010          val >>= mux->shift;
  1011          val &= mux->mask;
  1012  
  1013          if (mux->table) {
  1014                  u32 i;
  1015  
  1016                  for (i = 0; i < num_parents; i++)
  1017                          if (mux->table[i] == val)
  1018                                  return i;
  1019                  return -EINVAL;
                        ^^^^^^^^^^^^^^
  1020          }
  1021  
  1022          if (val >= num_parents)
  1023                  return -EINVAL;
                        ^^^^^^^^^^^^^^
This obviously doesn't work since this is a u8 function.

I looked at how this was called to see what we should return on error.
This is called from __clk_init().  It tests for negative error codes.
Ooops.  Also there is no static checker warning for that but I will
create one.  I'm not sure what to do here.  I bet the correct thing is
to convert all the get_parent() function to return int.  Another option
might be to add documentation, but I feel like in the kernel int returns
are self documenting and that's the best option.

  1024  
  1025          return val;
  1026  }

regards,
dan carpenter

Re: clk: lpc32xx: add common clock framework driver

[Vladimir Zapolskiy]

Hello Dan,

On 05.01.2016 13:44, Dan Carpenter wrote:
> Hello Vladimir Zapolskiy (and other clk devs as well),
> 
> The patch f7c82a60ba26: "clk: lpc32xx: add common clock framework
> driver" from Dec 6, 2015, leads to the following static checker
> warning:
> 
> 	drivers/clk/nxp/clk-lpc32xx.c:1019 clk_mux_get_parent()
> 	warn: signedness bug returning '(-22)'

oops, I blindly missed it, which static checker do you use by the way?

GCC and sparse haven't produced this error, may be I have to upgrade
them...

> drivers/clk/nxp/clk-lpc32xx.c
>   1003  static u8 clk_mux_get_parent(struct clk_hw *hw)
>                ^^
>   1004  {
>   1005          struct lpc32xx_clk_mux *mux = to_lpc32xx_mux(hw);
>   1006          u32 num_parents = clk_hw_get_num_parents(hw);
>   1007          u32 val;
>   1008  
>   1009          regmap_read(clk_regmap, mux->reg, &val);
>   1010          val >>= mux->shift;
>   1011          val &= mux->mask;
>   1012  
>   1013          if (mux->table) {
>   1014                  u32 i;
>   1015  
>   1016                  for (i = 0; i < num_parents; i++)
>   1017                          if (mux->table[i] == val)
>   1018                                  return i;
>   1019                  return -EINVAL;
>                         ^^^^^^^^^^^^^^
>   1020          }
>   1021  
>   1022          if (val >= num_parents)
>   1023                  return -EINVAL;
>                         ^^^^^^^^^^^^^^
> This obviously doesn't work since this is a u8 function.

Definitely, thank you for reporting.

"Negative" values may be returned here only if the driver has
another kind of error, namely if description of some mux clock parents
does not correspond to the actual hardware. It may happen due to
a typo in the code or a typo in the spec for example, but if
there is no first class error, then the underlined returns are dead
code. To my best knowledge there is no such problem in this
particular driver code.

However it is worth to mention that this piece of code is practically
a regmap version of common clock framework clk-mux.c clk_mux_get_parent()
function, which is commonly used and has the same problem spread over
multiple drivers.

Moreover talking about CCF this kind of a problem touches other
hardware querying functions:
* .is_prepared (clamped return value to bool in the framework code),
* .is_enabled (clamped return value to bool in the framework code),
* .recalc_rate (fallback to 0 seems to be a valid workaround),
* .get_parent (discussed here),
* .get_phase (this one looks okay).

I've found other potentially faulty examples from this class, e.g.

* clk_is_enabled_regmap() from qcom/clk-regmap.c -- clamped to bool,
* omap2_init_dpll_parent() from ti/clkt_dpll.c,
* mux_get_parent() from clk-qoriq.c -- fallback to index 0,
* wm831x_clkout_get_parent() from clk-wm831x.c -- fallback to index 0,
* kona_peri_clk_get_parent() from bcm/clk-kona.c -- fallback to index 0,
* etc.

> I looked at how this was called to see what we should return on error.
> This is called from __clk_init().  It tests for negative error codes.

Well, not exactly, but more or less. It is important to say that the
function (and the second user of .get_parent() __clk_init_parent) will
be significantly reworked in v4.6 (not in clk tree yet), I have started
to work on the fix on basis of that future version of clk.c.

> Ooops.  Also there is no static checker warning for that but I will
> create one.  

Sounds great, thank you in advance.

> I'm not sure what to do here.  I bet the correct thing is
> to convert all the get_parent() function to return int.

I agree with you, I'm implementing this fix now, when it is ready
I'll share the change with the CCF maintainers and the list for review.

> Another option
> might be to add documentation, but I feel like in the kernel int returns
> are self documenting and that's the best option.

Hopefully Mike and Stephen agree also, let see their comments.

Thank you again for reporting.

--
With best wishes,
Vladimir