Re: [PATCH v2] media: qcom: camss: fix VFE pm domain off

[barnabas.czeman@mainlining.org]

On 2024-11-29 12:20, Vladimir Zapolskiy wrote:
> On 11/29/24 13:06, Bryan O'Donoghue wrote:
>> On 29/11/2024 08:48, Vladimir Zapolskiy wrote:
>>> On 11/28/24 21:39, Barnabás Czémán wrote:
>>>> Fix NULL pointer check before device_link_del
>>>> is called.
>>>> 
>>>> Unable to handle kernel NULL pointer dereference at virtual address
>>>> 000000000000032c
>>>> Call trace:
>>>>    device_link_put_kref+0xc/0xb8
>>>>    device_link_del+0x30/0x48
>>>>    vfe_pm_domain_off+0x24/0x38 [qcom_camss]
>>>>    vfe_put+0x9c/0xd0 [qcom_camss]
>>>>    vfe_set_power+0x48/0x58 [qcom_camss]
>>>>    pipeline_pm_power_one+0x154/0x158 [videodev]
>>>>    pipeline_pm_power+0x74/0xfc [videodev]
>>>>    v4l2_pipeline_pm_use+0x54/0x90 [videodev]
>>>>    v4l2_pipeline_pm_put+0x14/0x34 [videodev]
>>>>    video_release+0x2c/0x44 [qcom_camss]
>>>>    v4l2_release+0xe4/0xec [videodev]
>>>> 
>>>> Fixes: eb73facec2c2 ("media: qcom: camss: Use common VFE 
>>>> pm_domain_on/
>>>> pm_domain_off where applicable")
>>>> Tested-by: Yassine Oudjana <y.oudjana@protonmail.com>
>>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
>>>> ---
>>>> Changes in v2:
>>>> - Add backtrace to the commit message.
>>>> - Link to v1: https://lore.kernel.org/r/20241122-vfe_pm_domain_off-
>>>> v1-1-81d18f56563d@mainlining.org
>>>> ---
>>>>    drivers/media/platform/qcom/camss/camss-vfe.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>> 
>>>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c 
>>>> b/drivers/
>>>> media/platform/qcom/camss/camss-vfe.c
>>>> index
>>>> 80a62ba11295042802cbaec617fb87c492ea6a55..1bf1473331f63b9ab106d21ea263c84d851c8a31 
>>>> 100644
>>>> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
>>>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
>>>> @@ -595,7 +595,7 @@ void vfe_isr_reset_ack(struct vfe_device *vfe)
>>>>     */
>>>>    void vfe_pm_domain_off(struct vfe_device *vfe)
>>>>    {
>>>> -    if (!vfe->genpd)
>>>> +    if (!vfe->genpd_link)
>>>>            return;
>>>>        device_link_del(vfe->genpd_link);
>>>> 
>>> 
>>> I object to this change, there might be a problem in the code, 
>>> however it
>>> is not yet identified.
>>> 
>>> vfe->genpd is not NULL, if vfe_pm_domain_on()/vfe_pm_domain_off() are
>>> called appropriately, the "fix" does not fix the real problem, it 
>>> veils it.
>>> 
>>> -- Best wishes,
>>> Vladimir
>>> 
>>> 
>> 
>> Let's walk through the logic.
>> 
>> vfe->genpd =
>> 
>> Can happen in vfe_subdev_init();
>> 
>> vfe_pm_domain_on() can fail @ vfe->genpd_link =
>> 
>> If it fails then I _suppose_ we are still calling vfe_pm_domain_off() 
>> at
>> least that's the only logically way I see this error can manifest.
> 
> There should be no room for suppositions, the source code is open.
> 
> If the described by you case is true, and vfe_pm_domain_on() fails,
> then vfe_pm_domain_off() shall not be called, otherwise that's the
> real problem and it shall be fixed instead of being veiled by the
> proposed change.
> 
>> @Barnabás can you confirm that this is the case ?
>> 
>> If not, can you please provide more detail ?
> 
> The change does not describe how to reproduce the problem, which commit
> base is tested, which platform is testes, there is no enough 
> information,
> unfortunately.
I can reproduce the problem with megapixels-sensorprofile on msm8953 and
it can be reproduced with megapixels on msm8996.
The base is the last commit on next.
> 
> --
> Best wishes,
> Vladimir