Messages in this thread | | | Subject | Re: [PATCH] Intel: Skylake: Fix inconsistent IS_ERR and PTR_ERR | From | Amadeusz Sławiński <> | Date | Mon, 24 Feb 2020 11:42:59 +0100 |
| |
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
| |