It has been postulated that the pm_lock is bad for performance because
a CPU currently running rpmh_flush() could block other CPUs from
coming out of idle. Similarly CPUs coming out of / going into idle
all need to contend with each other for the spinlock just to update
the variable tracking who's in PM.
Let's optimize this a bit. Specifically:
- Use a count rather than a bitmask. This is faster to access and
also means we can use the atomic_inc_return() function to really
detect who the last one to enter PM was.
- Accept that it's OK if we race and are doing the flush (because we
think we're last) while another CPU is coming out of idle. As long
as we block that CPU if/when it tries to do an active-only transfer
we're OK.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Link: https://lore.kernel.org/r/20200504104917.v6.5.I295cb72bc5334a2af80313cbe97cb5c9dcb1442c@changeid
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Adding an item into the cache should never be able to make the cache
cleaner. Use "|=" rather than "=" to update the dirty flag.
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Maulik Shah <mkshah@codeaurora.org> Thanks, Maulik
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Fixes: bb7000677a ("soc: qcom: rpmh: Update dirty flag only when data changes")
Reported-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Link: https://lore.kernel.org/r/20200417141531.1.Ia4b74158497213eabad7c3d474c50bfccb3f342e@changeid
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Auditing tcs_invalidate() made me worried. Specifically I saw that it
used spin_lock(), not spin_lock_irqsave(). That always worries me
unless I can trace for sure that I'm in the interrupt handler or that
someone else already disabled interrupts.
Looking more at it, there is actually no reason for these locks
anyway. Specifically the only reason you'd ever call
rpmh_rsc_invalidate() is if you cared that the sleep/wake TCSes were
empty. That means that they need to continue to be empty even after
rpmh_rsc_invalidate() returns. The only way that can happen is if the
caller already has done something to keep all other RPMH users out.
It should be noted that even though the caller is only worried about
making sleep/wake TCSes empty, they also need to worry about stopping
active-only transfers if they need to handle the case where
active-only transfers might borrow the wake TCS.
At the moment rpmh_rsc_invalidate() is only called in PM code from the
last CPU. If that later changes the caller will still need to solve
the above problems themselves, so these locks will never be useful.
Continuing to audit tcs_invalidate(), I found a bug. The function
didn't properly check for a borrowed TCS if we hadn't recently written
anything into the TCS. Specifically, if we've never written to the
WAKE_TCS (or we've flushed it recently) then tcs->slots is empty.
We'll early-out and we'll never call tcs_is_free().
I thought about fixing this bug by either deleting the early check for
bitmap_empty() or possibly only doing it if we knew we weren't on a
TCS that could be borrowed. However, I think it's better to just
delete the checks.
As argued above it's up to the caller to make sure that all other
users of RPMH are quiet before tcs_invalidate() is called. Since
callers need to handle the zero-active-TCS case anyway that means they
need to make sure that the active-only transfers are quiet before
calling too. The one way tcs_invalidate() gets called today is
through rpmh_rsc_cpu_pm_callback() which calls
rpmh_rsc_ctrlr_is_busy() to handle this. When we have another path to
get to tcs_invalidate() it will also need to come up with something
similar and it won't need this extra check either. If we later find
some code path that actually needs this check back in (and somehow
manages to be race free) we can always add it back in.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Maulik Shah <mkshah@codeaurora.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Tested-by: Maulik Shah <mkshah@codeaurora.org>
Link: https://lore.kernel.org/r/20200413100321.v4.9.I07c1f70e0e8f2dc0004bd38970b4e258acdc773e@changeid
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Add changes to invoke rpmh flush() from CPU PM notification.
This is done when the last the cpu is entering deep CPU idle
states and controller is not busy.
Controllers that have 'HW solver' mode like display RSC do not need
to register for CPU PM notification. They may be in autonomous mode
executing low power mode and do not require rpmh_flush() to happen
from CPU PM notification.
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Link: https://lore.kernel.org/r/1586703004-13674-5-git-send-email-mkshah@codeaurora.org
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
TCSes have previously programmed data when rpmh_flush() is called.
This can cause old data to trigger along with newly flushed.
Fix this by cleaning SLEEP and WAKE TCSes before new data is flushed.
With this there is no need to invoke rpmh_rsc_invalidate() call from
rpmh_invalidate().
Simplify rpmh_invalidate() by moving invalidate_batch() inside.
Fixes: 600513dfee ("drivers: qcom: rpmh: cache sleep/wake state requests")
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Link: https://lore.kernel.org/r/1586703004-13674-4-git-send-email-mkshah@codeaurora.org
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Currently rpmh ctrlr dirty flag is set for all cases regardless of data
is really changed or not. Add changes to update dirty flag when data is
changed to newer values. Update dirty flag everytime when data in batch
cache is updated since rpmh_flush() may get invoked from any CPU instead
of only last CPU going to low power mode.
Also move dirty flag updates to happen from within cache_lock and remove
unnecessary INIT_LIST_HEAD() call and a default case from switch.
Fixes: 600513dfee ("drivers: qcom: rpmh: cache sleep/wake state requests")
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
Reviewed-by: Srinivas Rao L <lsrao@codeaurora.org>
Reviewed-by: Evan Green <evgreen@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Link: https://lore.kernel.org/r/1586703004-13674-3-git-send-email-mkshah@codeaurora.org
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
rpmh_flush() was exported with the idea that an external entity
operation during CPU idle would know when to flush the sleep and wake
TCS. Since, this is not the case when defining a power domain for the
RSC. Remove the function export and instead allow the function to be
called internally.
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Link: https://lore.kernel.org/r/1580736940-6985-3-git-send-email-mkshah@codeaurora.org
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Device argument matches with dev variable declared in RPMH message.
Compiler reports error when the argument is NULL since the argument
matches the name of the property. Rename dev argument to device to
fix this.
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Link: https://lore.kernel.org/r/1580736940-6985-2-git-send-email-mkshah@codeaurora.org
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Using the batch API from the interconnect driver sometimes leads to a
KASAN error due to an access to freed memory. This is easier to trigger
with threadirqs on the kernel commandline.
BUG: KASAN: use-after-free in rpmh_tx_done+0x114/0x12c
Read of size 1 at addr fffffff51414ad84 by task irq/110-apps_rs/57
CPU: 0 PID: 57 Comm: irq/110-apps_rs Tainted: G W 4.19.10 #72
Call trace:
dump_backtrace+0x0/0x2f8
show_stack+0x20/0x2c
__dump_stack+0x20/0x28
dump_stack+0xcc/0x10c
print_address_description+0x74/0x240
kasan_report+0x250/0x26c
__asan_report_load1_noabort+0x20/0x2c
rpmh_tx_done+0x114/0x12c
tcs_tx_done+0x450/0x768
irq_forced_thread_fn+0x58/0x9c
irq_thread+0x120/0x1dc
kthread+0x248/0x260
ret_from_fork+0x10/0x18
Allocated by task 385:
kasan_kmalloc+0xac/0x148
__kmalloc+0x170/0x1e4
rpmh_write_batch+0x174/0x540
qcom_icc_set+0x8dc/0x9ac
icc_set+0x288/0x2e8
a6xx_gmu_stop+0x320/0x3c0
a6xx_pm_suspend+0x108/0x124
adreno_suspend+0x50/0x60
pm_generic_runtime_suspend+0x60/0x78
__rpm_callback+0x214/0x32c
rpm_callback+0x54/0x184
rpm_suspend+0x3f8/0xa90
pm_runtime_work+0xb4/0x178
process_one_work+0x544/0xbc0
worker_thread+0x514/0x7d0
kthread+0x248/0x260
ret_from_fork+0x10/0x18
Freed by task 385:
__kasan_slab_free+0x12c/0x1e0
kasan_slab_free+0x10/0x1c
kfree+0x134/0x588
rpmh_write_batch+0x49c/0x540
qcom_icc_set+0x8dc/0x9ac
icc_set+0x288/0x2e8
a6xx_gmu_stop+0x320/0x3c0
a6xx_pm_suspend+0x108/0x124
adreno_suspend+0x50/0x60
cr50_spi spi5.0: SPI transfer timed out
pm_generic_runtime_suspend+0x60/0x78
__rpm_callback+0x214/0x32c
rpm_callback+0x54/0x184
rpm_suspend+0x3f8/0xa90
pm_runtime_work+0xb4/0x178
process_one_work+0x544/0xbc0
worker_thread+0x514/0x7d0
kthread+0x248/0x260
ret_from_fork+0x10/0x18
The buggy address belongs to the object at fffffff51414ac80
which belongs to the cache kmalloc-512 of size 512
The buggy address is located 260 bytes inside of
512-byte region [fffffff51414ac80, fffffff51414ae80)
The buggy address belongs to the page:
page:ffffffbfd4505200 count:1 mapcount:0 mapping:fffffff51e00c680 index:0x0 compound_mapcount: 0
flags: 0x4000000000008100(slab|head)
raw: 4000000000008100 ffffffbfd4529008 ffffffbfd44f9208 fffffff51e00c680
raw: 0000000000000000 0000000000200020 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
fffffff51414ac80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
fffffff51414ad00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>fffffff51414ad80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
fffffff51414ae00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
fffffff51414ae80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
The batch API sets the same completion for each rpmh message that's sent
and then loops through all the messages and waits for that single
completion declared on the stack to be completed before returning from
the function and freeing the message structures. Unfortunately, some
messages may still be in process and 'stuck' in the TCS. At some later
point, the tcs_tx_done() interrupt will run and try to process messages
that have already been freed at the end of rpmh_write_batch(). This will
in turn access the 'needs_free' member of the rpmh_request structure and
cause KASAN to complain. Furthermore, if there's a message that's
completed in rpmh_tx_done() and freed immediately after the complete()
call is made we'll be racing with potentially freed memory when
accessing the 'needs_free' member:
CPU0 CPU1
---- ----
rpmh_tx_done()
complete(&compl)
wait_for_completion(&compl)
kfree(rpm_msg)
if (rpm_msg->needs_free)
<KASAN warning splat>
Let's fix this by allocating a chunk of completions for each message and
waiting for all of them to be completed before returning from the batch
API. Alternatively, we could wait for the last message in the batch, but
that may be a more complicated change because it looks like
tcs_tx_done() just iterates through the indices of the queue and
completes each message instead of tracking the last inserted message and
completing that first.
Fixes: c8790cb6da ("drivers: qcom: rpmh: add support for batch RPMH request")
Cc: Lina Iyer <ilina@codeaurora.org>
Cc: "Raju P.L.S.S.S.N" <rplsssn@codeaurora.org>
Cc: Matthias Kaehlcke <mka@chromium.org>
Cc: Evan Green <evgreen@chromium.org>
Cc: stable@vger.kernel.org
Reviewed-by: Lina Iyer <ilina@codeaurora.org>
Reviewed-by: Evan Green <evgreen@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Andy Gross <andy.gross@linaro.org>
Fix the redundant call being made to send the sleep and wake requests
immediately to the controller.
As per the patch [1], the sleep and wake request votes are cached in
rpmh controller and sent during rpmh_flush(). These requests needs to be
sent only during entry of deeper system low power modes or suspend.
[1] https://patchwork.kernel.org/patch/10477533/
Reviewed-by: Lina Iyer <ilina@codeaurora.org>
Reviewed-by: Evan Green <evgreen@chromium.org>
Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Andy Gross <andy.gross@linaro.org>
Platform drivers need make a lot of resource state requests at the same
time, say, at the start or end of an usecase. It can be quite
inefficient to send each request separately. Instead they can give the
RPMH library a batch of requests to be sent and wait on the whole
transaction to be complete.
rpmh_write_batch() is a blocking call that can be used to send multiple
RPMH command sets. Each RPMH command set is set asynchronously and the
API blocks until all the command sets are complete and receive their
tx_done callbacks.
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Andy Gross <andy.gross@linaro.org>
Platform drivers that want to send a request but do not want to block
until the RPMH request completes have now a new API -
rpmh_write_async().
The API allocates memory and send the requests and returns the control
back to the platform driver. The tx_done callback from the controller is
handled in the context of the controller's thread and frees the
allocated memory. This API allows RPMH requests from atomic contexts as
well.
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
Signed-off-by: Andy Gross <andy.gross@linaro.org>
Active state requests are sent immediately to the RSC controller, while
sleep and wake state requests are cached in this driver to avoid taxing
the RSC controller repeatedly. The cached values will be sent to the
controller when the rpmh_flush() is called.
Generally, flushing is a system PM activity and may be called from the
system PM drivers when the system is entering suspend or deeper sleep
modes during cpuidle.
Also allow invalidating the cached requests, so they may be re-populated
again.
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
[rplsssn: remove unneeded semicolon, address line over 80chars error]
Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
Reviewed-by: Evan Green <evgreen@chromium.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Andy Gross <andy.gross@linaro.org>
Sending RPMH requests and waiting for response from the controller
through a callback is common functionality across all platform drivers.
To simplify drivers, add a library functions to create RPMH client and
send resource state requests.
rpmh_write() is a synchronous blocking call that can be used to send
active state requests.
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
Signed-off-by: Andy Gross <andy.gross@linaro.org>
