Re: clk: ti: Add support for dm814x ADPLL

Re: clk: ti: Add support for dm814x ADPLL

[Tony Lindgren]

Hi

* Dan Carpenter <dan.carpenter@oracle.com> [160302 03:12]:
> Hello Tony Lindgren,
> 
> The patch e6ab1637c643: "clk: ti: Add support for dm814x ADPLL" from
> Feb 26, 2016, leads to the following static checker warnings:
> 
> 	drivers/clk/ti/adpll.c:465 ti_adpll_recalc_rate()
> 	warn: should '__readw(d->regs + 20) << 18' be a 64 bit type?
> 
> 	drivers/clk/ti/adpll.c:945 ti_adpll_probe()
> 	error: we previously assumed 'd->clocks' could be null (see line 921)

Stephen already patched most of this with "clk: ti: Fix some errors
found by static checkers", some mysteries remain below though.

> This should probably be:
> 		rate = (u64)readw_relaxed(d->regs + ADPLL_MN2DIV_OFFSET) << 18;

Already fixed by Stephen.

>    773  static void ti_adpll_free_resources(struct ti_adpll_data *d)
>    774  {
>    775          int i;
>    776  
>    777          for (i = TI_ADPLL_M3; i >= 0; i--) {
>    778                  struct ti_adpll_clock *ac = &d->clocks[i];
>    779  
>    780                  if (!ac || IS_ERR_OR_NULL(ac->clk))
> 
> ac can't possibly be NULL here.

Yeah looks like we can remove ac check here.

>    910          err = ti_adpll_init_registers(d);
>    911          if (err)
>    912                  return err;
>    913  
>    914          err = ti_adpll_init_inputs(d);
>    915          if (err)
>    916                  return err;
>                         ^^^^^^^^^^
> This is the last direct return in the function, meaning that
> ti_adpll_init_inputs() must allocate something but I can't see what.
> It should match clkdev_drop() and ac->unregister()?  I don't understand.

Hmm I don't get this one. How did you get a warning here, is this a
warning from sparse also?

>    917  
>    918          d->clocks = devm_kzalloc(d->dev, sizeof(struct ti_adpll_clock) *
>    919                                   TI_ADPLL_NR_CLOCKS,
>    920                                   GFP_KERNEL);
>    921          if (!d->clocks)
>    922                  goto free;
> 
> We don't set the error code here.

Fixed by Stephen.

>    924          err = ti_adpll_init_dco(d);
>    925          if (err) {
>    926                  dev_err(dev, "could not register dco: %i\n", err);
>    927                  goto free;
>    928          }
>    929  
>    930          err = ti_adpll_init_children_adpll_s(d);
>    931          if (err)
>    932                  goto free;
>    933          err = ti_adpll_init_children_adpll_lj(d);
>    934          if (err)
>    935                  goto free;
>    936  
>    937          err = of_clk_add_provider(d->np, of_clk_src_onecell_get, &d->outputs);
>    938          if (err)
>    939                  goto free;
>    940  
>    941          return 0;
>    942  
>    943  free:
>    944          WARN_ON(1);
>    945          ti_adpll_free_resources(d);
> 
> This is a classic one err bug, where we have a single exit point but
> it's too complicated and subtle to handle all errors so we mess up.  The
> label doesn't indicate what we are freeing.

Well there's really not much to free with devm.. We could rename
the label to unregister to avoid confusion?

> Ideally, the allocate and the free functions mirror each other but I
> don't see a ti_adpll_(alloc|register|whatever)_resources().  Possibly
> this is a mirror of ti_adpll_init_dco() so it could be called
> ti_adpll_free_dco()?  I'm not sure.

Or it could be just be renamed to ti_adpll_unregister() as
we're not freeing any memory there.

Regards,

Tony

Re: clk: ti: Add support for dm814x ADPLL

[Dan Carpenter]

On Wed, Mar 02, 2016 at 09:05:03AM -0800, Tony Lindgren wrote:
> >    910          err = ti_adpll_init_registers(d);
> >    911          if (err)
> >    912                  return err;
> >    913  
> >    914          err = ti_adpll_init_inputs(d);
> >    915          if (err)
> >    916                  return err;
> >                         ^^^^^^^^^^
> > This is the last direct return in the function, meaning that
> > ti_adpll_init_inputs() must allocate something but I can't see what.
> > It should match clkdev_drop() and ac->unregister()?  I don't understand.
> 
> Hmm I don't get this one. How did you get a warning here, is this a
> warning from sparse also?

There isn't a warning here.  I'm just saying that when I'm reading this
code I assume that ti_adpll_free_resources() is supposed to undo
ti_adpll_init_inputs().

I looked at Steven's patch and now I see what's going on here.  The
error handling is fine when that's applied.  Thanks.

regards,
dan carpenter

Re: clk: ti: Add support for dm814x ADPLL

[Tony Lindgren]

* Dan Carpenter <dan.carpenter@oracle.com> [160302 10:09]:
> On Wed, Mar 02, 2016 at 09:05:03AM -0800, Tony Lindgren wrote:
> > >    910          err = ti_adpll_init_registers(d);
> > >    911          if (err)
> > >    912                  return err;
> > >    913  
> > >    914          err = ti_adpll_init_inputs(d);
> > >    915          if (err)
> > >    916                  return err;
> > >                         ^^^^^^^^^^
> > > This is the last direct return in the function, meaning that
> > > ti_adpll_init_inputs() must allocate something but I can't see what.
> > > It should match clkdev_drop() and ac->unregister()?  I don't understand.
> > 
> > Hmm I don't get this one. How did you get a warning here, is this a
> > warning from sparse also?
> 
> There isn't a warning here.  I'm just saying that when I'm reading this
> code I assume that ti_adpll_free_resources() is supposed to undo
> ti_adpll_init_inputs().
>
> I looked at Steven's patch and now I see what's going on here.  The
> error handling is fine when that's applied.  Thanks.

OK thanks for checking.

Regards,

Tony