PTR_ERR should access the value just tested by IS_ERR.
In skl_clk_dev_probe(),it is inconsistent.
Signed-off-by: Xu Wang <[email protected]>
---
sound/soc/intel/skylake/skl-ssp-clk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/intel/skylake/skl-ssp-clk.c b/sound/soc/intel/skylake/skl-ssp-clk.c
index 1c0e5226cb5b..f7aae10b629b 100644
--- 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]);
goto err_unreg_skl_clk;
}
}
--
2.17.1
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.
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?
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
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