[PATCH v5 0/3] Venus dynamic debug

[PATCH v5 0/3] Venus dynamic debug

[Stanimir Varbanov]

Hello,

Changes in v5:
 * 1/3 - dropped dev_warn when set FW debug level - Greg KH
 * 3/3 - dropped pr_debug, and now group levels by prefix in dev_dbg

v4 can be fount at [1].

regards,
Stan

[1] https://www.spinics.net/lists/kernel/msg3550106.html

Stanimir Varbanov (3):
  venus: Add debugfs interface to set firmware log level
  venus: Add a debugfs file for SSR trigger
  venus: Make debug infrastructure more flexible

 drivers/media/platform/qcom/venus/Makefile    |  2 +-
 drivers/media/platform/qcom/venus/core.c      |  3 ++
 drivers/media/platform/qcom/venus/core.h      |  8 +++
 drivers/media/platform/qcom/venus/dbgfs.c     | 51 +++++++++++++++++++
 drivers/media/platform/qcom/venus/dbgfs.h     | 12 +++++
 drivers/media/platform/qcom/venus/helpers.c   |  2 +-
 drivers/media/platform/qcom/venus/hfi_msgs.c  | 18 +++----
 drivers/media/platform/qcom/venus/hfi_venus.c | 10 ++--
 .../media/platform/qcom/venus/pm_helpers.c    |  2 +-
 drivers/media/platform/qcom/venus/vdec.c      |  6 +--
 10 files changed, 96 insertions(+), 18 deletions(-)
 create mode 100644 drivers/media/platform/qcom/venus/dbgfs.c
 create mode 100644 drivers/media/platform/qcom/venus/dbgfs.h

-- 
2.17.1

[PATCH v5 1/3] venus: Add debugfs interface to set firmware log level

[Stanimir Varbanov]

This will be useful when debugging specific issues related to
firmware HFI interface.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/Makefile    |  2 +-
 drivers/media/platform/qcom/venus/core.c      |  3 +++
 drivers/media/platform/qcom/venus/core.h      |  3 +++
 drivers/media/platform/qcom/venus/dbgfs.c     | 21 +++++++++++++++++++
 drivers/media/platform/qcom/venus/dbgfs.h     | 12 +++++++++++
 drivers/media/platform/qcom/venus/hfi_venus.c |  6 +++++-
 6 files changed, 45 insertions(+), 2 deletions(-)
 create mode 100644 drivers/media/platform/qcom/venus/dbgfs.c
 create mode 100644 drivers/media/platform/qcom/venus/dbgfs.h

diff --git a/drivers/media/platform/qcom/venus/Makefile b/drivers/media/platform/qcom/venus/Makefile
index 64af0bc1edae..dfc636865709 100644
--- a/drivers/media/platform/qcom/venus/Makefile
+++ b/drivers/media/platform/qcom/venus/Makefile
@@ -3,7 +3,7 @@
 
 venus-core-objs += core.o helpers.o firmware.o \
 		   hfi_venus.o hfi_msgs.o hfi_cmds.o hfi.o \
-		   hfi_parser.o pm_helpers.o
+		   hfi_parser.o pm_helpers.o dbgfs.o
 
 venus-dec-objs += vdec.o vdec_ctrls.o
 venus-enc-objs += venc.o venc_ctrls.o
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 203c6538044f..249141e27fea 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -290,6 +290,8 @@ static int venus_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_dev_unregister;
 
+	venus_dbgfs_init(core);
+
 	return 0;
 
 err_dev_unregister:
@@ -337,6 +339,7 @@ static int venus_remove(struct platform_device *pdev)
 	v4l2_device_unregister(&core->v4l2_dev);
 	mutex_destroy(&core->pm_lock);
 	mutex_destroy(&core->lock);
+	venus_dbgfs_deinit(core);
 
 	return ret;
 }
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 7118612673c9..b48782f9aa95 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -12,6 +12,7 @@
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 
+#include "dbgfs.h"
 #include "hfi.h"
 
 #define VIDC_CLKS_NUM_MAX		4
@@ -136,6 +137,7 @@ struct venus_caps {
  * @priv:	a private filed for HFI operations
  * @ops:		the core HFI operations
  * @work:	a delayed work for handling system fatal error
+ * @root:	debugfs root directory
  */
 struct venus_core {
 	void __iomem *base;
@@ -185,6 +187,7 @@ struct venus_core {
 	unsigned int codecs_count;
 	unsigned int core0_usage_count;
 	unsigned int core1_usage_count;
+	struct dentry *root;
 };
 
 struct vdec_controls {
diff --git a/drivers/media/platform/qcom/venus/dbgfs.c b/drivers/media/platform/qcom/venus/dbgfs.c
new file mode 100644
index 000000000000..782d54ac1b8f
--- /dev/null
+++ b/drivers/media/platform/qcom/venus/dbgfs.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2020 Linaro Ltd.
+ */
+
+#include <linux/debugfs.h>
+
+#include "core.h"
+
+extern int venus_fw_debug;
+
+void venus_dbgfs_init(struct venus_core *core)
+{
+	core->root = debugfs_create_dir("venus", NULL);
+	debugfs_create_x32("fw_level", 0644, core->root, &venus_fw_debug);
+}
+
+void venus_dbgfs_deinit(struct venus_core *core)
+{
+	debugfs_remove_recursive(core->root);
+}
diff --git a/drivers/media/platform/qcom/venus/dbgfs.h b/drivers/media/platform/qcom/venus/dbgfs.h
new file mode 100644
index 000000000000..b7b621a8472f
--- /dev/null
+++ b/drivers/media/platform/qcom/venus/dbgfs.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2020 Linaro Ltd. */
+
+#ifndef __VENUS_DBGFS_H__
+#define __VENUS_DBGFS_H__
+
+struct venus_core;
+
+void venus_dbgfs_init(struct venus_core *core);
+void venus_dbgfs_deinit(struct venus_core *core);
+
+#endif
diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index 0d8855014ab3..450c4800c12e 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -130,7 +130,7 @@ struct venus_hfi_device {
 };
 
 static bool venus_pkt_debug;
-static int venus_fw_debug = HFI_DEBUG_MSG_ERROR | HFI_DEBUG_MSG_FATAL;
+int venus_fw_debug = HFI_DEBUG_MSG_ERROR | HFI_DEBUG_MSG_FATAL;
 static bool venus_sys_idle_indicator;
 static bool venus_fw_low_power_mode = true;
 static int venus_hw_rsp_timeout = 1000;
@@ -1133,6 +1133,10 @@ static int venus_session_init(struct venus_inst *inst, u32 session_type,
 	struct hfi_session_init_pkt pkt;
 	int ret;
 
+	ret = venus_sys_set_debug(hdev, venus_fw_debug);
+	if (ret)
+		goto err;
+
 	ret = pkt_session_init(&pkt, inst, session_type, codec);
 	if (ret)
 		goto err;
-- 
2.17.1

[PATCH v5 2/3] venus: Add a debugfs file for SSR trigger

[Stanimir Varbanov]

The SSR (SubSystem Restart) is used to simulate an error on FW
side of Venus. We support following type of triggers - fatal error,
div by zero and watchdog IRQ.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/dbgfs.c | 30 +++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/dbgfs.c b/drivers/media/platform/qcom/venus/dbgfs.c
index 782d54ac1b8f..f95b7b1febe5 100644
--- a/drivers/media/platform/qcom/venus/dbgfs.c
+++ b/drivers/media/platform/qcom/venus/dbgfs.c
@@ -9,10 +9,40 @@
 
 extern int venus_fw_debug;
 
+static int trigger_ssr_open(struct inode *inode, struct file *file)
+{
+	file->private_data = inode->i_private;
+	return 0;
+}
+
+static ssize_t trigger_ssr_write(struct file *filp, const char __user *buf,
+				 size_t count, loff_t *ppos)
+{
+	struct venus_core *core = filp->private_data;
+	u32 ssr_type;
+	int ret;
+
+	ret = kstrtou32_from_user(buf, count, 4, &ssr_type);
+	if (ret)
+		return ret;
+
+	ret = hfi_core_trigger_ssr(core, ssr_type);
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
+static const struct file_operations ssr_fops = {
+	.open = trigger_ssr_open,
+	.write = trigger_ssr_write,
+};
+
 void venus_dbgfs_init(struct venus_core *core)
 {
 	core->root = debugfs_create_dir("venus", NULL);
 	debugfs_create_x32("fw_level", 0644, core->root, &venus_fw_debug);
+	debugfs_create_file("trigger_ssr", 0200, core->root, core, &ssr_fops);
 }
 
 void venus_dbgfs_deinit(struct venus_core *core)
-- 
2.17.1

[PATCH v5 3/3] venus: Make debug infrastructure more flexible

[Stanimir Varbanov]

Here we introduce debug prefixes for dev_dbg groups by level of
importance - Venus{Low,Med,High,FW} Enabling the particular level
will be done by dynamic debug.

For example to enable debug messages with low level:
echo 'format "VenusLow" +p' > debugfs/dynamic_debug/control

If you want to enable all levels:
echo 'format "Venus" +p' > debugfs/dynamic_debug/control

All the features which dynamic debugging provide are preserved.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/core.h       |  5 +++++
 drivers/media/platform/qcom/venus/helpers.c    |  2 +-
 drivers/media/platform/qcom/venus/hfi_msgs.c   | 18 +++++++++---------
 drivers/media/platform/qcom/venus/hfi_venus.c  |  4 ++--
 drivers/media/platform/qcom/venus/pm_helpers.c |  2 +-
 drivers/media/platform/qcom/venus/vdec.c       |  6 +++---
 6 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index b48782f9aa95..b1c94316a553 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -15,6 +15,11 @@
 #include "dbgfs.h"
 #include "hfi.h"
 
+#define VDBGL	"VenusLow : "
+#define VDBGM	"VenusMed : "
+#define VDBGH	"VenusHigh: "
+#define VDBGFW	"VenusFW  : "
+
 #define VIDC_CLKS_NUM_MAX		4
 #define VIDC_VCODEC_CLKS_NUM_MAX	2
 #define VIDC_PMDOMAINS_NUM_MAX		3
diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index 0143af7822b2..7147871d9dc1 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -396,7 +396,7 @@ put_ts_metadata(struct venus_inst *inst, struct vb2_v4l2_buffer *vbuf)
 	}
 
 	if (slot == -1) {
-		dev_dbg(inst->core->dev, "%s: no free slot\n", __func__);
+		dev_dbg(inst->core->dev, VDBGL "no free slot\n");
 		return;
 	}
 
diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
index 279a9d6fe737..06a1908ca225 100644
--- a/drivers/media/platform/qcom/venus/hfi_msgs.c
+++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
@@ -138,7 +138,7 @@ static void event_sys_error(struct venus_core *core, u32 event,
 			    struct hfi_msg_event_notify_pkt *pkt)
 {
 	if (pkt)
-		dev_dbg(core->dev,
+		dev_dbg(core->dev, VDBGH
 			"sys error (session id:%x, data1:%x, data2:%x)\n",
 			pkt->shdr.session_id, pkt->event_data1,
 			pkt->event_data2);
@@ -152,7 +152,7 @@ event_session_error(struct venus_core *core, struct venus_inst *inst,
 {
 	struct device *dev = core->dev;
 
-	dev_dbg(dev, "session error: event id:%x, session id:%x\n",
+	dev_dbg(dev, VDBGH "session error: event id:%x, session id:%x\n",
 		pkt->event_data1, pkt->shdr.session_id);
 
 	if (!inst)
@@ -247,7 +247,7 @@ sys_get_prop_image_version(struct device *dev,
 		/* bad packet */
 		return;
 
-	dev_dbg(dev, "F/W version: %s\n", (u8 *)&pkt->data[1]);
+	dev_dbg(dev, VDBGL "F/W version: %s\n", (u8 *)&pkt->data[1]);
 }
 
 static void hfi_sys_property_info(struct venus_core *core,
@@ -257,7 +257,7 @@ static void hfi_sys_property_info(struct venus_core *core,
 	struct device *dev = core->dev;
 
 	if (!pkt->num_properties) {
-		dev_dbg(dev, "%s: no properties\n", __func__);
+		dev_dbg(dev, VDBGL "no properties\n");
 		return;
 	}
 
@@ -266,7 +266,7 @@ static void hfi_sys_property_info(struct venus_core *core,
 		sys_get_prop_image_version(dev, pkt);
 		break;
 	default:
-		dev_dbg(dev, "%s: unknown property data\n", __func__);
+		dev_dbg(dev, VDBGL "unknown property data\n");
 		break;
 	}
 }
@@ -297,7 +297,7 @@ static void hfi_sys_ping_done(struct venus_core *core, struct venus_inst *inst,
 static void hfi_sys_idle_done(struct venus_core *core, struct venus_inst *inst,
 			      void *packet)
 {
-	dev_dbg(core->dev, "sys idle\n");
+	dev_dbg(core->dev, VDBGL "sys idle\n");
 }
 
 static void hfi_sys_pc_prepare_done(struct venus_core *core,
@@ -305,7 +305,8 @@ static void hfi_sys_pc_prepare_done(struct venus_core *core,
 {
 	struct hfi_msg_sys_pc_prep_done_pkt *pkt = packet;
 
-	dev_dbg(core->dev, "pc prepare done (error %x)\n", pkt->error_type);
+	dev_dbg(core->dev, VDBGL "pc prepare done (error %x)\n",
+		pkt->error_type);
 }
 
 static unsigned int
@@ -387,8 +388,7 @@ static void hfi_session_prop_info(struct venus_core *core,
 	case HFI_PROPERTY_CONFIG_VDEC_ENTROPY:
 		break;
 	default:
-		dev_dbg(dev, "%s: unknown property id:%x\n", __func__,
-			pkt->data[0]);
+		dev_dbg(dev, VDBGM "unknown property id:%x\n", pkt->data[0]);
 		return;
 	}
 
diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index 450c4800c12e..cb9367254c4e 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -477,7 +477,7 @@ static u32 venus_hwversion(struct venus_hfi_device *hdev)
 	minor = minor >> WRAPPER_HW_VERSION_MINOR_VERSION_SHIFT;
 	step = ver & WRAPPER_HW_VERSION_STEP_VERSION_MASK;
 
-	dev_dbg(dev, "venus hw version %x.%x.%x\n", major, minor, step);
+	dev_dbg(dev, VDBGL "venus hw version %x.%x.%x\n", major, minor, step);
 
 	return major;
 }
@@ -906,7 +906,7 @@ static void venus_flush_debug_queue(struct venus_hfi_device *hdev)
 		if (pkt->hdr.pkt_type != HFI_MSG_SYS_COV) {
 			struct hfi_msg_sys_debug_pkt *pkt = packet;
 
-			dev_dbg(dev, "%s", pkt->msg_data);
+			dev_dbg(dev, VDBGFW "%s", pkt->msg_data);
 		}
 	}
 }
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 531e7a41658f..cecf079cf0b8 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -212,7 +212,7 @@ static int load_scale_bw(struct venus_core *core)
 	}
 	mutex_unlock(&core->lock);
 
-	dev_dbg(core->dev, "total: avg_bw: %u, peak_bw: %u\n",
+	dev_dbg(core->dev, VDBGL "total: avg_bw: %u, peak_bw: %u\n",
 		total_avg, total_peak);
 
 	return icc_set_bw(core->video_path, total_avg, total_peak);
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 7c4c483d5438..6a71fc47273e 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -225,7 +225,7 @@ static int vdec_check_src_change(struct venus_inst *inst)
 
 	if (!(inst->codec_state == VENUS_DEC_STATE_CAPTURE_SETUP) ||
 	    !inst->reconfig)
-		dev_dbg(inst->core->dev, "%s: wrong state\n", __func__);
+		dev_dbg(inst->core->dev, VDBGH "wrong state\n");
 
 done:
 	return 0;
@@ -1310,7 +1310,7 @@ static void vdec_event_change(struct venus_inst *inst,
 	if (inst->bit_depth != ev_data->bit_depth)
 		inst->bit_depth = ev_data->bit_depth;
 
-	dev_dbg(dev, "event %s sufficient resources (%ux%u)\n",
+	dev_dbg(dev, VDBGM "event %s sufficient resources (%ux%u)\n",
 		sufficient ? "" : "not", ev_data->width, ev_data->height);
 
 	if (sufficient) {
@@ -1344,7 +1344,7 @@ static void vdec_event_change(struct venus_inst *inst,
 
 		ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, false);
 		if (ret)
-			dev_dbg(dev, "flush output error %d\n", ret);
+			dev_dbg(dev, VDBGH "flush output error %d\n", ret);
 	}
 
 	inst->reconfig = true;
-- 
2.17.1

Re: [PATCH v5 2/3] venus: Add a debugfs file for SSR trigger

[Stephen Boyd]

Quoting Stanimir Varbanov (2020-07-30 02:53:49)
> The SSR (SubSystem Restart) is used to simulate an error on FW
> side of Venus. We support following type of triggers - fatal error,
> div by zero and watchdog IRQ.

Can this use the fault injection framework instead of custom debugfs?
See Documentation/fault-injection/.

Re: [PATCH v5 0/3] Venus dynamic debug

[Stanimir Varbanov]

Hi all,

If no objections I'm going to queue 1/3 and 3/3 for v5.10. 2/3 will be
postponed because of Stephen's comment.

On 7/30/20 12:53 PM, Stanimir Varbanov wrote:
> Hello,
> 
> Changes in v5:
>  * 1/3 - dropped dev_warn when set FW debug level - Greg KH
>  * 3/3 - dropped pr_debug, and now group levels by prefix in dev_dbg
> 
> v4 can be fount at [1].
> 
> regards,
> Stan
> 
> [1] https://www.spinics.net/lists/kernel/msg3550106.html
> 
> Stanimir Varbanov (3):
>   venus: Add debugfs interface to set firmware log level
>   venus: Add a debugfs file for SSR trigger
>   venus: Make debug infrastructure more flexible
> 
>  drivers/media/platform/qcom/venus/Makefile    |  2 +-
>  drivers/media/platform/qcom/venus/core.c      |  3 ++
>  drivers/media/platform/qcom/venus/core.h      |  8 +++
>  drivers/media/platform/qcom/venus/dbgfs.c     | 51 +++++++++++++++++++
>  drivers/media/platform/qcom/venus/dbgfs.h     | 12 +++++
>  drivers/media/platform/qcom/venus/helpers.c   |  2 +-
>  drivers/media/platform/qcom/venus/hfi_msgs.c  | 18 +++----
>  drivers/media/platform/qcom/venus/hfi_venus.c | 10 ++--
>  .../media/platform/qcom/venus/pm_helpers.c    |  2 +-
>  drivers/media/platform/qcom/venus/vdec.c      |  6 +--
>  10 files changed, 96 insertions(+), 18 deletions(-)
>  create mode 100644 drivers/media/platform/qcom/venus/dbgfs.c
>  create mode 100644 drivers/media/platform/qcom/venus/dbgfs.h
> 

-- 
regards,
Stan

Re: [PATCH v5 2/3] venus: Add a debugfs file for SSR trigger

[dikshita]

Hi Stephen,

Reviving the discussion on this change as we need to pull this in.

As per your suggestion, I explored the fault injection framework to 
implement this functionality.
But I don't think that meets our requirements.

We need a way to trigger subsystem restart from the client-side, it's 
not derived from the driver.

while fault injection framework enables the driver to trigger an 
injection
when a specific event occurs for eg: page allocation failure or memory 
access failure.

So, IMO, we will have to use custom debugfs only.

Please feel free to correct me in case my understanding of the framework 
is wrong.

Thanks,
Dikshita

On 2020-08-12 03:19, Stephen Boyd wrote:
> Quoting Stanimir Varbanov (2020-07-30 02:53:49)
>> The SSR (SubSystem Restart) is used to simulate an error on FW
>> side of Venus. We support following type of triggers - fatal error,
>> div by zero and watchdog IRQ.
> 
> Can this use the fault injection framework instead of custom debugfs?
> See Documentation/fault-injection/.

Re: [PATCH v5 2/3] venus: Add a debugfs file for SSR trigger

[Stephen Boyd]

Quoting dikshita@codeaurora.org (2021-09-15 02:13:09)
> Hi Stephen,
>
> Reviving the discussion on this change as we need to pull this in.
>
> As per your suggestion, I explored the fault injection framework to
> implement this functionality.
> But I don't think that meets our requirements.
>
> We need a way to trigger subsystem restart from the client-side, it's
> not derived from the driver.

Just to confirm, this is all for debugging purposes right?

>
> while fault injection framework enables the driver to trigger an
> injection
> when a specific event occurs for eg: page allocation failure or memory
> access failure.
>
> So, IMO, we will have to use custom debugfs only.

Can you use DECLARE_FAULT_ATTR()? Or you need it to be active instead of
passive, i.e. it shouldn't wait for should_fail() to return true, but
actively trigger something on the remoteproc?

>
> Please feel free to correct me in case my understanding of the framework
> is wrong.
>

I presume the fault injection framework could get a new feature that
lets the fault be injected immediately upon writing the debugfs file.
My goal is to consolidate this sort of logic into one place and then put
it behind some config option that distros can disable so the kernel
isn't bloated with debug features that end users will never care about.

Re: [PATCH v5 2/3] venus: Add a debugfs file for SSR trigger

[dikshita]

On 2021-09-16 01:09, Stephen Boyd wrote:
> Quoting dikshita@codeaurora.org (2021-09-15 02:13:09)
>> Hi Stephen,
>> 
>> Reviving the discussion on this change as we need to pull this in.
>> 
>> As per your suggestion, I explored the fault injection framework to
>> implement this functionality.
>> But I don't think that meets our requirements.
>> 
>> We need a way to trigger subsystem restart from the client-side, it's
>> not derived from the driver.
> 
> Just to confirm, this is all for debugging purposes right?
> 
yes, correct. this is for debugging purposes. We need this to simulate 
an error on FW side.
In a normal scenario, when FW runs into error, sys error is triggered 
from FW as result of which
a sequence of commands are followed for restarting the system.
using this feature, we are trying to simulate this error on FW i.e we 
are forcing the FW to run into an error.
>> 
>> while fault injection framework enables the driver to trigger an
>> injection
>> when a specific event occurs for eg: page allocation failure or memory
>> access failure.
>> 
>> So, IMO, we will have to use custom debugfs only.
> 
> Can you use DECLARE_FAULT_ATTR()? Or you need it to be active instead 
> of
> passive, i.e. it shouldn't wait for should_fail() to return true, but
> actively trigger something on the remoteproc?
> 

yes, it doesn't need to wait for should_fail() to return true.
the client/user should be able to trigger this subsystem restart(SSR) at 
any point of time
when a session is running. It's totally client-driven.

>> 
>> Please feel free to correct me in case my understanding of the 
>> framework
>> is wrong.
>> 
> 
> I presume the fault injection framework could get a new feature that
> lets the fault be injected immediately upon writing the debugfs file.
> My goal is to consolidate this sort of logic into one place and then 
> put
> it behind some config option that distros can disable so the kernel
> isn't bloated with debug features that end users will never care about.

Re: [PATCH v5 2/3] venus: Add a debugfs file for SSR trigger

[Stephen Boyd]

Quoting dikshita@codeaurora.org (2021-09-15 23:29:36)
> On 2021-09-16 01:09, Stephen Boyd wrote:
> > Quoting dikshita@codeaurora.org (2021-09-15 02:13:09)
> >>
> >> So, IMO, we will have to use custom debugfs only.
> >
> > Can you use DECLARE_FAULT_ATTR()? Or you need it to be active instead
> > of
> > passive, i.e. it shouldn't wait for should_fail() to return true, but
> > actively trigger something on the remoteproc?
> >
>
> yes, it doesn't need to wait for should_fail() to return true.
> the client/user should be able to trigger this subsystem restart(SSR) at
> any point of time
> when a session is running. It's totally client-driven.
>
> >>
> >> Please feel free to correct me in case my understanding of the
> >> framework
> >> is wrong.
> >>
> >
> > I presume the fault injection framework could get a new feature that
> > lets the fault be injected immediately upon writing the debugfs file.
> > My goal is to consolidate this sort of logic into one place and then
> > put
> > it behind some config option that distros can disable so the kernel
> > isn't bloated with debug features that end users will never care about.

So you can modify fault injection framework to support direct injection
instead of statistical failures?

Re: [PATCH v5 2/3] venus: Add a debugfs file for SSR trigger

[dikshita]

On 2021-09-17 11:48, Stephen Boyd wrote:
> Quoting dikshita@codeaurora.org (2021-09-15 23:29:36)
>> On 2021-09-16 01:09, Stephen Boyd wrote:
>> > Quoting dikshita@codeaurora.org (2021-09-15 02:13:09)
>> >>
>> >> So, IMO, we will have to use custom debugfs only.
>> >
>> > Can you use DECLARE_FAULT_ATTR()? Or you need it to be active instead
>> > of
>> > passive, i.e. it shouldn't wait for should_fail() to return true, but
>> > actively trigger something on the remoteproc?
>> >
>> 
>> yes, it doesn't need to wait for should_fail() to return true.
>> the client/user should be able to trigger this subsystem restart(SSR) 
>> at
>> any point of time
>> when a session is running. It's totally client-driven.
>> 
>> >>
>> >> Please feel free to correct me in case my understanding of the
>> >> framework
>> >> is wrong.
>> >>
>> >
>> > I presume the fault injection framework could get a new feature that
>> > lets the fault be injected immediately upon writing the debugfs file.
>> > My goal is to consolidate this sort of logic into one place and then
>> > put
>> > it behind some config option that distros can disable so the kernel
>> > isn't bloated with debug features that end users will never care about.
> 
> So you can modify fault injection framework to support direct injection
> instead of statistical failures?

I am not sure how to do that. Could you pls give me more info?
Also, how is this beneficial than using debugfs?