* Re: [PATCH] Intel: Skylake: Fix inconsistent IS_ERR and PTR_ERR
[not found] <20200221101112.3104-1-vulab@iscas.ac.cn>
@ 2020-02-21 14:41 ` Joe Perches
2020-02-21 15:40 ` Pierre-Louis Bossart
0 siblings, 1 reply; 4+ messages in thread
From: Joe Perches @ 2020-02-21 14:41 UTC (permalink / raw)
To: Xu Wang, perex, tiwai, alsa-devel; +Cc: linux-kernel
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Intel: Skylake: Fix inconsistent IS_ERR and PTR_ERR
2020-02-21 14:41 ` [PATCH] Intel: Skylake: Fix inconsistent IS_ERR and PTR_ERR Joe Perches
@ 2020-02-21 15:40 ` Pierre-Louis Bossart
2020-02-23 15:59 ` Cezary Rojewski
0 siblings, 1 reply; 4+ messages in thread
From: Pierre-Louis Bossart @ 2020-02-21 15:40 UTC (permalink / raw)
To: Joe Perches, Xu Wang, perex, tiwai, alsa-devel
Cc: Rojewski, Cezary, linux-kernel, Slawinski, AmadeuszX
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?
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Intel: Skylake: Fix inconsistent IS_ERR and PTR_ERR
2020-02-21 15:40 ` Pierre-Louis Bossart
@ 2020-02-23 15:59 ` Cezary Rojewski
2020-02-24 10:42 ` Amadeusz Sławiński
0 siblings, 1 reply; 4+ messages in thread
From: Cezary Rojewski @ 2020-02-23 15:59 UTC (permalink / raw)
To: Pierre-Louis Bossart, Joe Perches, Xu Wang, Slawinski, AmadeuszX
Cc: linux-kernel, alsa-devel, tiwai
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Intel: Skylake: Fix inconsistent IS_ERR and PTR_ERR
2020-02-23 15:59 ` Cezary Rojewski
@ 2020-02-24 10:42 ` Amadeusz Sławiński
0 siblings, 0 replies; 4+ messages in thread
From: Amadeusz Sławiński @ 2020-02-24 10:42 UTC (permalink / raw)
To: Cezary Rojewski, Pierre-Louis Bossart, Joe Perches, Xu Wang,
Slawinski, AmadeuszX
Cc: alsa-devel, linux-kernel, tiwai
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-02-24 10:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200221101112.3104-1-vulab@iscas.ac.cn>
2020-02-21 14:41 ` [PATCH] Intel: Skylake: Fix inconsistent IS_ERR and PTR_ERR Joe Perches
2020-02-21 15:40 ` Pierre-Louis Bossart
2020-02-23 15:59 ` Cezary Rojewski
2020-02-24 10:42 ` Amadeusz Sławiński
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).