On 5/17/2024 12:21 AM, Mukesh Ojha wrote:
On 5/17/2024 4:28 AM, 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;
+
const struct firmware *firmware;
const struct firmware *dtb_firmware;
@@ -399,6 +404,12 @@ static int adsp_stop(struct rproc *rproc)
if (handover)
qcom_pas_handover(&adsp->q6v5);
+ if (adsp->hwlock) {
+ ret = hwspin_lock_bust(adsp->hwlock, adsp->hwlock_id);
+ if (ret)
+ dev_info(adsp->dev, "failed to bust hwspinlock\n");
+ }
+
As you said above, you seem to cover only wdog case and fatal cases
are already handled at remote;
You are clearing here for both ? is this right understanding ?
Yes that is correct. While the firmware is able to handle error fatal
cases, I think it is still the responsibility of the remoteproc driver
to try and bust the lock on behalf of the rproc in both cases.
The bust will only clear the spinlock if it has the token (specific id
for that rproc) that is passed into hwspin_lock_bust, so it is safe to
attempt this even if the value has been cleared by the rproc in the
error fatal case.
-Mukesh
