* re: clk: ti: Add support for dm814x ADPLL
@ 2016-03-02 11:11 Dan Carpenter
2016-03-02 17:05 ` Tony Lindgren
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2016-03-02 11:11 UTC (permalink / raw)
To: tony; +Cc: linux-clk
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)
drivers/clk/ti/adpll.c
450 static unsigned long ti_adpll_recalc_rate(struct clk_hw *hw,
451 unsigned long parent_rate)
452 {
453 struct ti_adpll_dco_data *dco = to_dco(hw);
454 struct ti_adpll_data *d = to_adpll(dco);
455 u32 frac_m, divider, v;
456 u64 rate;
457 unsigned long flags;
458
459 if (ti_adpll_clock_is_bypass(d))
460 return 0;
461
462 spin_lock_irqsave(&d->lock, flags);
463 frac_m = readl_relaxed(d->regs + ADPLL_FRACDIV_OFFSET);
464 frac_m &= ADPLL_FRACDIV_FRACTIONALM_MASK;
465 rate = readw_relaxed(d->regs + ADPLL_MN2DIV_OFFSET) << 18;
This should probably be:
rate = (u64)readw_relaxed(d->regs + ADPLL_MN2DIV_OFFSET) << 18;
466 rate += frac_m;
467 rate *= parent_rate;
[ snip ]
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.
781 continue;
782 if (ac->cl)
783 clkdev_drop(ac->cl);
784 if (ac->unregister)
785 ac->unregister(ac->clk);
786 }
787 }
[ snip ]
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.
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.
923
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.
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.
946
947 return err;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: clk: ti: Add support for dm814x ADPLL
2016-03-02 11:11 clk: ti: Add support for dm814x ADPLL Dan Carpenter
@ 2016-03-02 17:05 ` Tony Lindgren
2016-03-02 18:09 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Tony Lindgren @ 2016-03-02 17:05 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-clk, linux-omap, Stephen Boyd
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: clk: ti: Add support for dm814x ADPLL
2016-03-02 17:05 ` Tony Lindgren
@ 2016-03-02 18:09 ` Dan Carpenter
2016-03-02 18:19 ` Tony Lindgren
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2016-03-02 18:09 UTC (permalink / raw)
To: Tony Lindgren; +Cc: linux-clk, linux-omap, Stephen Boyd
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: clk: ti: Add support for dm814x ADPLL
2016-03-02 18:09 ` Dan Carpenter
@ 2016-03-02 18:19 ` Tony Lindgren
0 siblings, 0 replies; 4+ messages in thread
From: Tony Lindgren @ 2016-03-02 18:19 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-clk, linux-omap, Stephen Boyd
* 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-03-02 18:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-02 11:11 clk: ti: Add support for dm814x ADPLL Dan Carpenter
2016-03-02 17:05 ` Tony Lindgren
2016-03-02 18:09 ` Dan Carpenter
2016-03-02 18:19 ` Tony Lindgren
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.