Messages in this thread | | | Subject | Re: [PATCH] Intel: Skylake: Fix inconsistent IS_ERR and PTR_ERR | From | Cezary Rojewski <> | Date | Sun, 23 Feb 2020 16:59:30 +0100 |
| |
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?
Czarek
| |