clk: ti: Add support for dm814x ADPLL
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
Hi
Quoted 10 lines · Dan Carpenter
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.
Quoted 2 lines
This should probably be:
rate = (u64)readw_relaxed(d->regs + ADPLL_MN2DIV_OFFSET) << 18;
Already fixed by Stephen.
Quoted 10 lines
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.
Quoted 11 lines
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 matchclkdev_drop()andac->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?
Quoted 8 lines
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.
Quoted 23 lines
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);Quoted 3 lines
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?
Quoted 4 lines
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 calledti_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
Quoted 12 lines · Tony Lindgren
Quoted 11 lines
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 matchclkdev_drop()andac->unregister()? I don't understand.
Quoted 2 lines
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
Quoted 16 lines · Dan Carpenter
Quoted 12 lines · Tony Lindgren
Quoted 11 lines
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 matchclkdev_drop()andac->unregister()? I don't understand.Quoted 2 lines
Hmm I don't get this one. How did you get a warning here, is this a warning from sparse also?
Quoted 6 lines
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 undoti_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