On 5/21/2024 10:38 AM, Konrad Dybcio wrote:
On 5/17/24 00:58, Chris Lew wrote:
From: Richard Maina <quic_rmaina@xxxxxxxxxxx>
When remoteproc goes down unexpectedly this results in a state where any
acquired hwspinlocks will remain locked possibly resulting in deadlock.
In order to ensure all locks are freed we include a call to
hwspin_lock_bust() during remoteproc shutdown.
For qcom_q6v5_pas remoteprocs, each remoteproc has an assigned id that
is used to take the hwspinlock. Remoteproc should use this id to try and
bust the lock on remoteproc stop.
This edge case only occurs with q6v5_pas watchdog crashes. The error
fatal case has handling to clear the hwspinlock before the error fatal
interrupt is triggered.
Signed-off-by: Richard Maina <quic_rmaina@xxxxxxxxxxx>
Signed-off-by: Chris Lew <quic_clew@xxxxxxxxxxx>
---
drivers/remoteproc/qcom_q6v5_pas.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c
b/drivers/remoteproc/qcom_q6v5_pas.c
index 54d8005d40a3..57178fcb9aa3 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -10,6 +10,7 @@
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/firmware.h>
+#include <linux/hwspinlock.h>
#include <linux/interrupt.h>
#include <linux/kernel.h>
#include <linux/module.h>
@@ -52,6 +53,7 @@ struct adsp_data {
const char *ssr_name;
const char *sysmon_name;
int ssctl_id;
+ int hwlock_id;
int region_assign_idx;
int region_assign_count;
@@ -84,6 +86,9 @@ struct qcom_adsp {
bool decrypt_shutdown;
const char *info_name;
+ struct hwspinlock *hwlock;
+ int hwlock_id;
IIRC, this is the same one that is passed in the DT.
Can we get it dynamically from there?
The argument passed in DT is the index of the hwlock in the TCSR mutex
region. The index determines use of hwlock[0..n]
This id is supposed to be the identifier that is passed into
hwspin_lock_bust(). The actual value that we would read from
hwlock[0..n] to see if we need to bust the lock.
Maybe the naming of this variable is confusing. Do you have any
suggestions to make it clearer? Could call it hwlock_bust_id.
We could also try increasing the #hwlock-cells to 2 and have something
like <&phandle index bust_id>. To me this seemed odd for clients that
weren't planning on using the bust_id.
Konrad
