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@xxxxxxxxxx>
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.
--
Best wishes,
Vladimir
