media: qcom: camss: fix error path on configuration of power domains

  There is a chance to meet runtime issues during configuration of CAMSS
power domains, because on the error path dev_pm_domain_detach() is
unexpectedly called with NULL or error pointer.

Fixes: 23aa4f0cd327 ("media: qcom: camss: Move VFE power-domain specifics into vfe.c")
Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/media/platform/qcom/camss/camss.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)
  

On 07/08/2024 09:52, Vladimir Zapolskiy wrote:
> On 8/7/24 11:39, Bryan O'Donoghue wrote:
>> On 07/08/2024 00:37, Vladimir Zapolskiy wrote:
>>> On 8/7/24 02:30, Bryan O'Donoghue wrote:
>>>> On 07/08/2024 00:27, Vladimir Zapolskiy wrote:
>>>>> Hi Bryan.
>>>>>
>>>>> On 8/7/24 02:15, Bryan O'Donoghue wrote:
>>>>>> On 06/08/2024 23:12, Vladimir Zapolskiy wrote:
>>>>>>> There is a chance to meet runtime issues during configuration of 
>>>>>>> CAMSS
>>>>>>> power domains, because on the error path dev_pm_domain_detach() is
>>>>>>> unexpectedly called with NULL or error pointer.
>>>>>>>
>>>>>>> Fixes: 23aa4f0cd327 ("media: qcom: camss: Move VFE power-domain
>>>>>>> specifics into vfe.c")
>>>>>>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>>>>>>
>>>>>> Have you tested this with and without named power domains in your 
>>>>>> dts ?
>>>>>> The logic here is complex to support both the legacy non-named 
>>>>>> case and
>>>>>> the updated named required case.
>>>>>
>>>>> The problem and the fix are pretty straightforward, if you notice any
>>>>> issues
>>>>> with it, please let me know.
>>>>>
>>>>> As it's said in the commit description the problem is unrelated to
>>>>> named/not named
>>>>> power domains, I tested the fix only on a platform without
>>>>> "power-domain-names"
>>>>> property in camss device tree node.
>>>>>
>>>>>> Could you also provide a backtrace of a failing camss_configure_pd()
>>>>>> for
>>>>>> the commit log.
>>>>>
>>>>> Sure, I believe anyone can get a backtrace simply by disabling 
>>>>> camcc at
>>>>> build time,
>>>>> so that camss power domain supplies disappear:
>>>>
>>>> Ah OK, that's how, proof positive if its not tested, its not working,
>>>> I've extensively tested both named and non-named pds but, yep never 
>>>> with
>>>> camcc switched off.
>>>>
>>>>>
>>>>> [   13.541205] Unable to handle kernel NULL pointer dereference at
>>>>> virtual address 00000000000001a2
>>>>> [   13.550224] Mem abort info:
>>>>> [   13.553110]   ESR = 0x0000000096000004
>>>>> [   13.556975]   EC = 0x25: DABT (current EL), IL = 32 bits
>>>>> [   13.562438]   SET = 0, FnV = 0
>>>>> [   13.565580]   EA = 0, S1PTW = 0
>>>>> [   13.568813]   FSC = 0x04: level 0 translation fault
>>>>> [   13.573824] Data abort info:
>>>>> [   13.576787]   ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
>>>>> [   13.582424]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
>>>>> [   13.587614]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
>>>>> [   13.593074] user pgtable: 4k pages, 48-bit VAs, 
>>>>> pgdp=000000088a55a000
>>>>> [   13.599693] [00000000000001a2] pgd=0000000000000000,
>>>>> p4d=0000000000000000
>>>>> [   13.606666] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
>>>>> [   13.613104] Modules linked in:
>>>>>
>>>>> <snip>
>>>>>
>>>>> [   13.632753] Workqueue: events_unbound deferred_probe_work_func
>>>>> [   13.638776] pstate: 23400009 (nzCv daif +PAN -UAO +TCO +DIT -SSBS
>>>>> BTYPE=--)
>>>>> [   13.645926] pc : dev_pm_domain_detach+0x8/0x48
>>>>> [   13.650521] lr : camss_probe+0x374/0x9c0
>>>>> [   13.654577] sp : ffff800086ec3ab0
>>>>> [   13.657985] x29: ffff800086ec3ab0 x28: ffff8000855079c0 x27:
>>>>> ffff800085507000
>>>>> [   13.665329] x26: ffff000800c4b040 x25: 0000000000000000 x24:
>>>>> ffff00080aa72c20
>>>>> [   13.672659] x23: ffff800083060588 x22: ffff000801397010 x21:
>>>>> ffff800083060588
>>>>> [   13.679989] x20: 00000000ffffff92 x19: ffff00080aa72880 x18:
>>>>> ffffffffffffffff
>>>>> [   13.687318] x17: 6e6570656420676e x16: 69726f6e6769202c x15:
>>>>> 0000000000000000
>>>>> [   13.694648] x14: 000000000000003e x13: 0000000000000000 x12:
>>>>> 0000000000000000
>>>>> [   13.701988] x11: ffff00080b350460 x10: ffff00080b350248 x9 :
>>>>> ffff800081ddcde4
>>>>> [   13.709318] x8 : ffff00080b350270 x7 : 0000000000000001 x6 :
>>>>> 8000003ff0000000
>>>>> [   13.716658] x5 : ffff00080149a300 x4 : ffff000a72592b70 x3 :
>>>>> 0000000000076404
>>>>> [   13.723998] x2 : 0000000000000000 x1 : 0000000000000001 x0 :
>>>>> ffffffffffffff92
>>>>> [   13.731338] Call trace:
>>>>> [   13.733865]  dev_pm_domain_detach+0x8/0x48
>>>>> [   13.738081]  platform_probe+0x70/0xf0
>>>>> [   13.741864]  really_probe+0xc4/0x2a8
>>>>> [   13.745556]  __driver_probe_device+0x80/0x140
>>>>> [   13.750045]  driver_probe_device+0x48/0x170
>>>>> [   13.754355]  __device_attach_driver+0xc0/0x148
>>>>> [   13.758937]  bus_for_each_drv+0x88/0xf0
>>>>> [   13.762894]  __device_attach+0xb0/0x1c0
>>>>> [   13.766852]  device_initial_probe+0x1c/0x30
>>>>> [   13.771165]  bus_probe_device+0xb4/0xc0
>>>>> [   13.775124]  deferred_probe_work_func+0x90/0xd0
>>>>> [   13.779787]  process_one_work+0x164/0x3e0
>>>>> [   13.783920]  worker_thread+0x310/0x420
>>>>> [   13.787777]  kthread+0x120/0x130
>>>>> [   13.791123]  ret_from_fork+0x10/0x20
>>>>> [   13.794821] Code: 828a2cb8 ffff8000 aa1e03e9 d503201f (f9410802)
>>>>> [   13.801088] ---[ end trace 0000000000000000 ]---
>>>>
>>>> I'd be obliged if you could add to your commit log and verify 
>>>> everything
>>>> works for you with both named and unnamed power-domains.
>>>>
>>>
>>> No objections to resend the change with an updated commit message, since
>>> it raised a question, I can add information about a method how to 
>>> reproduce
>>> the bug.
>>>
>>> However I would like to know your opinion about the change itself, are
>>> there
>>> any noticeable issues? Thank you in advance!
>>>
>>> -- 
>>> Best wishes,
>>> Vladimir
>>
>> Why not just
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss.c
>> b/drivers/media/platform/qcom/camss/camss.c
>> index 51b1d3550421a..9990af675190c 100644
>> --- a/drivers/media/platform/qcom/camss/camss.c
>> +++ b/drivers/media/platform/qcom/camss/camss.c
>> @@ -2162,7 +2162,8 @@ static int camss_configure_pd(struct camss *camss)
>>           return 0;
>>
>>    fail_pm:
>> -       dev_pm_domain_detach(camss->genpd, true);
>> +       if (camss->genpd)
>> +               dev_pm_domain_detach(camss->genpd, true);
>>
>>
>> ?
>>
> 
> Because your change is invalid strictly speaking, again you've missed
> an error pointer case, but that's secondary, since it could be improved.
> 
> However your change brings more of unnecessary complexity, because it
> increases both cyclomatic complexity and increases LoC, when my change
> reduces the values in both these metrics.
> 
> My change makes the code way simpler, hopefully I managed to explain it.

Cyclomatic complexity you know, I checked

pmccabe drivers/media/platform/qcom/camss/camss.c

base:
12	12	37	2088	81	drivers/media/platform/qcom/camss/camss.c(2088): 
camss_configure_pd

vlad:
12	12	35	2088	78	drivers/media/platform/qcom/camss/camss.c(2088): 
camss_configure_pd

bod:
13	13	38	2088	82	drivers/media/platform/qcom/camss/camss.c(2088): 
camss_configure_pd

That's convincing enough for me, please add the kernel backtrace on your V2.

---
bod