Re: [PATCH] PM / devfreq: mediatek: Fix possible dereference of null pointer

[Matthias Brugger <matthias.bgg@gmail.com>]

On 20/06/2022 16:21, AngeloGioacchino Del Regno wrote:
> Il 17/06/22 15:33, matthias.bgg@kernel.org ha scritto:
>> From: Matthias Brugger <matthias.bgg@gmail.com>
>>
>> We dereference the driver data before checking of it's valid.
>> This patch fixes this, by accessing the PLL data struct after cheching
>> the pointer
>>
> 
> Hello Matthias,
> 
> honestly, I don't think that this commit is right: mtk_ccifreq_target() is the
> devfreq_dev_profile's .target() callback!
> 
> Checking mtk_ccifreq_probe(), we are setting drvdata long before adding the
> devfreq device so, actually, it's impossible for dev_get_drvdata(dev) to return
> NULL, or whatever invalid pointer.
> 
> This means that the right thing to do in mtk_ccifreq_target() is to simply remove
> the `drv` NULL check, as this can never happen! :-)
> 

Yes you are right. Probe will error out if drv can't be allocated or cci_clk 
isn't present. I'll send a new patch.

Thanks,
Matthias

> Cheers,
> Angelo
> 
>> Fixes: 07dc787be231 ("Add linux-next specific files for 20220617")
>> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
>> ---
>>   drivers/devfreq/mtk-cci-devfreq.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/devfreq/mtk-cci-devfreq.c 
>> b/drivers/devfreq/mtk-cci-devfreq.c
>> index 71abb3fbd042..77522f16c878 100644
>> --- a/drivers/devfreq/mtk-cci-devfreq.c
>> +++ b/drivers/devfreq/mtk-cci-devfreq.c
>> @@ -127,7 +127,7 @@ static int mtk_ccifreq_target(struct device *dev, unsigned 
>> long *freq,
>>                     u32 flags)
>>   {
>>       struct mtk_ccifreq_drv *drv = dev_get_drvdata(dev);
>> -    struct clk *cci_pll = clk_get_parent(drv->cci_clk);
>> +    struct clk *cci_pll;
>>       struct dev_pm_opp *opp;
>>       unsigned long opp_rate;
>>       int voltage, pre_voltage, inter_voltage, target_voltage, ret;
>> @@ -138,6 +138,7 @@ static int mtk_ccifreq_target(struct device *dev, unsigned 
>> long *freq,
>>       if (drv->pre_freq == *freq)
>>           return 0;
>> +    cci_pll = clk_get_parent(drv->cci_clk);
>>       inter_voltage = drv->inter_voltage;
>>       opp_rate = *freq;
>>
> 
>