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