Just curious how does lockdep do with this? Doesn't OPP call into
interconnect code, so lockdep will complain about ABBA?
Unfortunately it does. It seems, the icc-clk is not a proper way to go here. I will take a look at reusing set_required_opps for this case.
--
With best wishes
Dmitry
Just curious how does lockdep do with this? Doesn't OPP call into
interconnect code, so lockdep will complain about ABBA?
Unfortunately it does. It seems, the icc-clk is not a proper way to go here.
I will take a look at reusing set_required_opps for this case.
Could you elaborate a bit which locks exactly cause trouble here?
I'm probably missing something here.
From a quick look at the OPP code I don't see a global lock taken there
for the entire OPP switch sequence, so I'm not sure how this could cause
an ABBA deadlock.
Thanks,
Stephan
Just curious how does lockdep do with this? Doesn't OPP call into
interconnect code, so lockdep will complain about ABBA?
Unfortunately it does. It seems, the icc-clk is not a proper way to go here.
I will take a look at reusing set_required_opps for this case.
Could you elaborate a bit which locks exactly cause trouble here?
I'm probably missing something here.
From a quick look at the OPP code I don't see a global lock taken there
for the entire OPP switch sequence, so I'm not sure how this could cause
an ABBA deadlock.
Just curious how does lockdep do with this? Doesn't OPP call into
interconnect code, so lockdep will complain about ABBA?
Unfortunately it does. It seems, the icc-clk is not a proper way to go here.
I will take a look at reusing set_required_opps for this case.
Could you elaborate a bit which locks exactly cause trouble here?
I'm probably missing something here.
From a quick look at the OPP code I don't see a global lock taken there
for the entire OPP switch sequence, so I'm not sure how this could cause
an ABBA deadlock.
Hm, I'm not entirely sure how to interpret this. Is there really a
connection between OPP and ICC here? It looks more like ICC <->
regulator and somehow related to memory allocations (the fs_reclaim).
Was there more output around this?
In general, I would expect that adjusting a regulator from an
interconnect driver should be made possible somehow. Just because the
RPM firmware or similar typically handles this internally on Qualcomm
platforms doesn't mean no one else will ever need to do this. If you
have a higher bandwidth requests, need to increase the clock, which
again depends on a higher voltage, then you need to be able to do the
regulator_set_voltage() from the ICC driver somehow.
Thanks,
Stephan
Just curious how does lockdep do with this? Doesn't OPP call into
interconnect code, so lockdep will complain about ABBA?
Unfortunately it does. It seems, the icc-clk is not a proper way to go here.
I will take a look at reusing set_required_opps for this case.
Could you elaborate a bit which locks exactly cause trouble here?
I'm probably missing something here.
From a quick look at the OPP code I don't see a global lock taken there
for the entire OPP switch sequence, so I'm not sure how this could cause
an ABBA deadlock.
Hm, I'm not entirely sure how to interpret this. Is there really a
connection between OPP and ICC here? It looks more like ICC <->
regulator and somehow related to memory allocations (the fs_reclaim).
Was there more output around this?
In general, I would expect that adjusting a regulator from an
interconnect driver should be made possible somehow. Just because the
RPM firmware or similar typically handles this internally on Qualcomm
platforms doesn't mean no one else will ever need to do this. If you
have a higher bandwidth requests, need to increase the clock, which
again depends on a higher voltage, then you need to be able to do the
regulator_set_voltage() from the ICC driver somehow.
This kind of dependency is handled by the consumer, not by the ICC
driver. Usually we explicitly put them to the opp tables, matching
rates and volage.
I think I'll still check the required-opps approach. It will require
manually aggregating the L2 rate requirements, but then we will be
safe from such kinds of interdependencies. And maybe then we can also
rewrite msm8996 to use required-opps too.
--
With best wishes
Dmitry
Just curious how does lockdep do with this? Doesn't OPP call into
interconnect code, so lockdep will complain about ABBA?
Unfortunately it does. It seems, the icc-clk is not a proper way to go here.
I will take a look at reusing set_required_opps for this case.
Could you elaborate a bit which locks exactly cause trouble here?
I'm probably missing something here.
From a quick look at the OPP code I don't see a global lock taken there
for the entire OPP switch sequence, so I'm not sure how this could cause
an ABBA deadlock.
Hm, I'm not entirely sure how to interpret this. Is there really a
connection between OPP and ICC here? It looks more like ICC <->
regulator and somehow related to memory allocations (the fs_reclaim).
Was there more output around this?
Thanks! I assume you haven't noticed an actual deadlock (i.e. cpufreq
not progressing) without lockdep?
FWIW I'm not fully convinced that this deadlock can actually happen in
practice for your particular limited use case (i.e. that it is not a
false positive).
quoted
In general, I would expect that adjusting a regulator from an
interconnect driver should be made possible somehow. Just because the
RPM firmware or similar typically handles this internally on Qualcomm
platforms doesn't mean no one else will ever need to do this. If you
have a higher bandwidth requests, need to increase the clock, which
again depends on a higher voltage, then you need to be able to do the
regulator_set_voltage() from the ICC driver somehow.
This kind of dependency is handled by the consumer, not by the ICC
driver. Usually we explicitly put them to the opp tables, matching
rates and volage.
Right, but I'm talking about the voltage requirements of the aggregated
bus clock. The rate for that is calculated from the different bandwidth
requirements from all the consumers and not just a single one. From the
kernel perspective, the interconnect driver is the consumer of that
clock. On Qualcomm SMD RPM platforms the voltage adjustment for those is
handled behind the scenes by the RPM firmware (no idea about RPMH and
RPM-old).
But one could easily think of a (non-Qualcomm) SoC without RPM, where
the interconnect driver calculates a particular bus rate for multiple
consumers and then needs to vote for a power domain or regulator to be
able to generate that clock safely.
I think I'll still check the required-opps approach. It will require
manually aggregating the L2 rate requirements, but then we will be
safe from such kinds of interdependencies. And maybe then we can also
rewrite msm8996 to use required-opps too.
Sure, thanks! I think at the moment the required-opps implementation is
somewhat entangled with power domains/genpd but perhaps it's easy enough
to refactor that to be more general.
Thanks,
Stephan
Just curious how does lockdep do with this? Doesn't OPP call into
interconnect code, so lockdep will complain about ABBA?
Unfortunately it does. It seems, the icc-clk is not a proper way to go here.
I will take a look at reusing set_required_opps for this case.
Could you elaborate a bit which locks exactly cause trouble here?
I'm probably missing something here.
From a quick look at the OPP code I don't see a global lock taken there
for the entire OPP switch sequence, so I'm not sure how this could cause
an ABBA deadlock.
Hm, I'm not entirely sure how to interpret this. Is there really a
connection between OPP and ICC here? It looks more like ICC <->
regulator and somehow related to memory allocations (the fs_reclaim).
Was there more output around this?
[ 7.362902] ======================================================
[ 7.363447] WARNING: possible circular locking dependency detected
[ 7.369434] 6.6.0-rc3-next-20230929-08870-g49503b4b9f55 #129 Not tainted
[ 7.375604] ------------------------------------------------------
[ 7.382453] swapper/0/1 is trying to acquire lock:
[ 7.388437] c143445c (icc_bw_lock){+.+.}-{3:3}, at: icc_init+0x0/0x104
[ 7.393225]
[ 7.393225] but task is already holding lock:
[ 7.399730] c1397444 (fs_reclaim){+.+.}-{0:0}, at: icc_init+0x18/0x104
[ 7.405547]
[ 7.405547] which lock already depends on the new lock.
[ 7.405547]
[ 7.412077]
[ 7.412077] the existing dependency chain (in reverse order) is:
[ 7.420310]
[ 7.420310] -> #2 (fs_reclaim){+.+.}-{0:0}:
[ 7.427767] fs_reclaim_acquire+0x60/0x94
[ 7.433492] __kmem_cache_alloc_node+0x30/0x2b4
[ 7.437742] __kmalloc_node_track_caller+0x48/0x244
[ 7.442954] kstrdup+0x30/0x58
[ 7.448325] create_regulator+0x70/0x2b8
The regulator core should avoid holding the lock while calling into
kstrdup() and also debugfs file creation APIs.
-Stephen