Re: [PATCH] Intel: Skylake: Fix inconsistent IS_ERR and PTR_ERR

[Amadeusz SÅawiÅski]

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