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.
[]
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.
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.
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?