On 2/23/2020 4:59 PM, Cezary Rojewski wrote:
On 2020-02-21 16:40, Pierre-Louis Bossart wrote:
On 2/21/20 8:41 AM, Joe Perches wrote:
On Fri, 2020-02-21 at 18:11 +0800, Xu Wang wrote:
PTR_ERR should access the value just tested by IS_ERR.
In skl_clk_dev_probe(),it is inconsistent.
Please include all maintainers of given driver when submitting the
patch, thank you.
[]
diff --git a/sound/soc/intel/skylake/skl-ssp-clk.c
b/sound/soc/intel/skylake/skl-ssp-clk.c
[]
@@ -384,7 +384,7 @@ static int skl_clk_dev_probe(struct
platform_device *pdev)
&clks[i], clk_pdata, i);
if (IS_ERR(data->clk[data->avail_clk_cnt])) {
- ret = PTR_ERR(data->clk[data->avail_clk_cnt++]);
+ ret = PTR_ERR(data->clk[data->avail_clk_cnt]);
NAK.
This is not inconsistent and you are removing the ++
which is a post increment. Likely that is necessary.
You could write the access and the increment as two
separate statements if it confuses you.
Well to be fair the code is far from clear.
Thanks for notifying, Pierre.
Although NAK is upheld here. Proposed change is likely to introduce
regression.
the post-increment is likely needed because of the error handling in
unregister_src_clk 1
data->clk[data->avail_clk_cnt] = register_skl_clk(dev,
&clks[i], clk_pdata, i);
if (IS_ERR(data->clk[data->avail_clk_cnt])) {
ret = PTR_ERR(data->clk[data->avail_clk_cnt++]);
goto err_unreg_skl_clk;
}
}
platform_set_drvdata(pdev, data);
return 0;
err_unreg_skl_clk:
unregister_src_clk(data);
static void unregister_src_clk(struct skl_clk_data *dclk)
{
while (dclk->avail_clk_cnt--)
clkdev_drop(dclk->clk[dclk->avail_clk_cnt]->lookup);
}
So the post-increment is cancelled in the while().
That said, the avail_clk_cnt field is never initialized or incremented
in normal usages so the code looks quite suspicious indeed.
As basically entire old Skylake code, so no surprises here : )
struct skl_clk_data::avail_clk_cnt field is initialized with 0 via
devm_kzalloc in skl_clk_dev_probe().
gitk tells me this patch is likely the culprit:
6ee927f2f01466 ('ASoC: Intel: Skylake: Fix NULL ptr dereference when
unloading clk dev')
- data->clk[i] = register_skl_clk(dev, &clks[i], clk_pdata, i);
- if (IS_ERR(data->clk[i])) {
- ret = PTR_ERR(data->clk[i]);
+ data->clk[data->avail_clk_cnt] = register_skl_clk(dev,
+ &clks[i], clk_pdata, i);
+
+ if (IS_ERR(data->clk[data->avail_clk_cnt])) {
+ ret = PTR_ERR(data->clk[data->avail_clk_cnt++]);
goto err_unreg_skl_clk;
}
-
- data->avail_clk_cnt++;
That last removal is probably wrong. Cezary and Amadeusz, you may want
to look at this?
Indeed, code looks wrong. Idk what are we even dropping in
unregister_src_clk() if register_skl_clk() fails and avail_clk_cnt gets
incremented anyway.
In general usage of while(ptr->counter--) (example of which is present
in unregister_src_clk()) is prone to errors. Decrementation happens
regardless of while's check outcome and caller may receive back handle
in invalid state.
Amadeo, your thoughts?
Right, there is a problem with how we do increment available clock
counter. It should be done in success path, sent fix.
Amadeusz
