[PATCH v2 0/4] soc: qcom: aoss: Introduce debugfs interface and cleanup things

[PATCH v2 0/4] soc: qcom: aoss: Introduce debugfs interface and cleanup things

[Bjorn Andersson]

The Always On Processor supports a number useful commands for affecting
system resources during in various debug scenarious. Introduce a debugfs
interface for allowing the debugger/tester to send these commands.

While at it, let's make some improvements to the qmp_send() API.


In v1, Andrew pointed out the fact that this interface could be used to
send proprietary commands from a userspace driver. As debugfs isn't
enabled in the vast majority of shipping devices, this wouldn't be very
useful. In the mantime having this interface present is very useful for
members of the upstream Qualcomm community.

It would be even more user (developer) friendly to expose each of these
knobs as individual debugfs files. But the list of commands and
resources looks to be varying from target to target, so this is still
being investigated. The hope is to perhaps capture a set of common
resources.

Changes since v1:
- Reverse xmas tree in qmp_send() in patch 1
- Correct length check of input data in patch 2

Bjorn Andersson (3):
  soc: qcom: aoss: Move length requirements from caller
  soc: qcom: aoss: Format string in qmp_send()
  soc: qcom: aoss: Tidy up qmp_send() callers

Chris Lew (1):
  soc: qcom: aoss: Add debugfs interface for sending messages

 drivers/net/ipa/ipa_power.c        |  5 +-
 drivers/remoteproc/qcom_q6v5.c     |  8 +--
 drivers/soc/qcom/qcom_aoss.c       | 82 +++++++++++++++++++++---------
 include/linux/soc/qcom/qcom_aoss.h |  4 +-
 4 files changed, 61 insertions(+), 38 deletions(-)

-- 
2.25.1

[PATCH v2 1/4] soc: qcom: aoss: Move length requirements from caller

[Bjorn Andersson]

The existing implementation of qmp_send() requires the caller to provide
a buffer which is of word-aligned. The underlying reason for this is
that message ram only supports word accesses, but pushing this
requirement onto the clients results in the same boiler plate code
sprinkled in every call site.

By using a temporary buffer in qmp_send() we can hide the underlying
hardware limitations from the clients and allow them to pass their
NUL-terminates C string directly.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/net/ipa/ipa_power.c        |  2 +-
 drivers/remoteproc/qcom_q6v5.c     |  2 +-
 drivers/soc/qcom/qcom_aoss.c       | 25 ++++++++++++-------------
 include/linux/soc/qcom/qcom_aoss.h |  4 ++--
 4 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c
index 921eecf3eff6..26181eeed975 100644
--- a/drivers/net/ipa/ipa_power.c
+++ b/drivers/net/ipa/ipa_power.c
@@ -332,7 +332,7 @@ void ipa_power_retention(struct ipa *ipa, bool enable)
 
 	(void)snprintf(buf, sizeof(buf), fmt, enable ? '1' : '0');
 
-	ret = qmp_send(power->qmp, buf, sizeof(buf));
+	ret = qmp_send(power->qmp, buf);
 	if (ret)
 		dev_err(power->dev, "error %d sending QMP %sable request\n",
 			ret, enable ? "en" : "dis");
diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
index 192c7aa0e39e..8b41a73fa4d1 100644
--- a/drivers/remoteproc/qcom_q6v5.c
+++ b/drivers/remoteproc/qcom_q6v5.c
@@ -35,7 +35,7 @@ static int q6v5_load_state_toggle(struct qcom_q6v5 *q6v5, bool enable)
 
 	WARN_ON(ret >= Q6V5_LOAD_STATE_MSG_LEN);
 
-	ret = qmp_send(q6v5->qmp, buf, sizeof(buf));
+	ret = qmp_send(q6v5->qmp, buf);
 	if (ret)
 		dev_err(q6v5->dev, "failed to toggle load state\n");
 
diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
index e376c32cc16e..880fe234ca0a 100644
--- a/drivers/soc/qcom/qcom_aoss.c
+++ b/drivers/soc/qcom/qcom_aoss.c
@@ -206,36 +206,35 @@ static bool qmp_message_empty(struct qmp *qmp)
  * qmp_send() - send a message to the AOSS
  * @qmp: qmp context
  * @data: message to be sent
- * @len: length of the message
  *
  * Transmit @data to AOSS and wait for the AOSS to acknowledge the message.
- * @len must be a multiple of 4 and not longer than the mailbox size. Access is
- * synchronized by this implementation.
+ * data must not be longer than the mailbox size. Access is synchronized by
+ * this implementation.
  *
  * Return: 0 on success, negative errno on failure
  */
-int qmp_send(struct qmp *qmp, const void *data, size_t len)
+int qmp_send(struct qmp *qmp, const void *data)
 {
+	char buf[QMP_MSG_LEN];
 	long time_left;
 	int ret;
 
 	if (WARN_ON(IS_ERR_OR_NULL(qmp) || !data))
 		return -EINVAL;
 
-	if (WARN_ON(len + sizeof(u32) > qmp->size))
+	if (WARN_ON(strlen(data) >= sizeof(buf)))
 		return -EINVAL;
 
-	if (WARN_ON(len % sizeof(u32)))
-		return -EINVAL;
+	strscpy_pad(buf, data, sizeof(buf));
 
 	mutex_lock(&qmp->tx_lock);
 
 	/* The message RAM only implements 32-bit accesses */
 	__iowrite32_copy(qmp->msgram + qmp->offset + sizeof(u32),
-			 data, len / sizeof(u32));
-	writel(len, qmp->msgram + qmp->offset);
+			 buf, sizeof(buf) / sizeof(u32));
+	writel(sizeof(buf), qmp->msgram + qmp->offset);
 
-	/* Read back len to confirm data written in message RAM */
+	/* Read back length to confirm data written in message RAM */
 	readl(qmp->msgram + qmp->offset);
 	qmp_kick(qmp);
 
@@ -262,7 +261,7 @@ static int qmp_qdss_clk_prepare(struct clk_hw *hw)
 	static const char buf[QMP_MSG_LEN] = "{class: clock, res: qdss, val: 1}";
 	struct qmp *qmp = container_of(hw, struct qmp, qdss_clk);
 
-	return qmp_send(qmp, buf, sizeof(buf));
+	return qmp_send(qmp, buf);
 }
 
 static void qmp_qdss_clk_unprepare(struct clk_hw *hw)
@@ -270,7 +269,7 @@ static void qmp_qdss_clk_unprepare(struct clk_hw *hw)
 	static const char buf[QMP_MSG_LEN] = "{class: clock, res: qdss, val: 0}";
 	struct qmp *qmp = container_of(hw, struct qmp, qdss_clk);
 
-	qmp_send(qmp, buf, sizeof(buf));
+	qmp_send(qmp, buf);
 }
 
 static const struct clk_ops qmp_qdss_clk_ops = {
@@ -344,7 +343,7 @@ static int qmp_cdev_set_cur_state(struct thermal_cooling_device *cdev,
 			qmp_cdev->name,
 			cdev_state ? "on" : "off");
 
-	ret = qmp_send(qmp_cdev->qmp, buf, sizeof(buf));
+	ret = qmp_send(qmp_cdev->qmp, buf);
 
 	if (!ret)
 		qmp_cdev->state = cdev_state;
diff --git a/include/linux/soc/qcom/qcom_aoss.h b/include/linux/soc/qcom/qcom_aoss.h
index 3c2a82e606f8..7a71406b6050 100644
--- a/include/linux/soc/qcom/qcom_aoss.h
+++ b/include/linux/soc/qcom/qcom_aoss.h
@@ -13,13 +13,13 @@ struct qmp;
 
 #if IS_ENABLED(CONFIG_QCOM_AOSS_QMP)
 
-int qmp_send(struct qmp *qmp, const void *data, size_t len);
+int qmp_send(struct qmp *qmp, const void *data);
 struct qmp *qmp_get(struct device *dev);
 void qmp_put(struct qmp *qmp);
 
 #else
 
-static inline int qmp_send(struct qmp *qmp, const void *data, size_t len)
+static inline int qmp_send(struct qmp *qmp, const void *data)
 {
 	return -ENODEV;
 }
-- 
2.25.1

[PATCH v2 2/4] soc: qcom: aoss: Add debugfs interface for sending messages

[Bjorn Andersson]

From: Chris Lew <clew@codeaurora.org>

In addition to the normal runtime commands, the Always On Processor
(AOP) provides a number of debug commands which can be used during
system debugging for things such as preventing power collapse or placing
floor votes for certain resources. Some of these are documented in the
Robotics RB5 "Debug AOP ADB" linked below.

Provide a debugfs interface for the developer/tester to send these
commands to the AOP.

Link: https://docs.qualcomm.com/bundle/publicresource/topics/80-88500-3/85_Debugging_AOP_ADB.html
Signed-off-by: Chris Lew <clew@codeaurora.org>
[bjorn: Dropped debugfs guards, improve error codes, rewrote commit message]
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/soc/qcom/qcom_aoss.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
index 880fe234ca0a..13bf13ab78d6 100644
--- a/drivers/soc/qcom/qcom_aoss.c
+++ b/drivers/soc/qcom/qcom_aoss.c
@@ -3,6 +3,7 @@
  * Copyright (c) 2019, Linaro Ltd
  */
 #include <linux/clk-provider.h>
+#include <linux/debugfs.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/mailbox_client.h>
@@ -82,6 +83,7 @@ struct qmp {
 
 	struct clk_hw qdss_clk;
 	struct qmp_cooling_device *cooling_devs;
+	struct dentry *debugfs_file;
 };
 
 static void qmp_kick(struct qmp *qmp)
@@ -475,6 +477,32 @@ void qmp_put(struct qmp *qmp)
 }
 EXPORT_SYMBOL(qmp_put);
 
+static ssize_t qmp_debugfs_write(struct file *file, const char __user *userstr,
+				 size_t len, loff_t *pos)
+{
+	struct qmp *qmp = file->private_data;
+	char buf[QMP_MSG_LEN];
+	int ret;
+
+	if (!len || len >= QMP_MSG_LEN)
+		return -EINVAL;
+
+	if (copy_from_user(buf, userstr, len))
+		return -EFAULT;
+	buf[len] = '\0';
+
+	ret = qmp_send(qmp, buf);
+	if (ret < 0)
+		return ret;
+
+	return len;
+}
+
+static const struct file_operations qmp_debugfs_fops = {
+	.open = simple_open,
+	.write = qmp_debugfs_write,
+};
+
 static int qmp_probe(struct platform_device *pdev)
 {
 	struct qmp *qmp;
@@ -523,6 +551,9 @@ static int qmp_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, qmp);
 
+	qmp->debugfs_file = debugfs_create_file("aoss_send_message", 0220, NULL,
+						qmp, &qmp_debugfs_fops);
+
 	return 0;
 
 err_close_qmp:
@@ -537,6 +568,8 @@ static int qmp_remove(struct platform_device *pdev)
 {
 	struct qmp *qmp = platform_get_drvdata(pdev);
 
+	debugfs_remove(qmp->debugfs_file);
+
 	qmp_qdss_clk_remove(qmp);
 	qmp_cooling_devices_remove(qmp);
 
-- 
2.25.1

[PATCH v2 3/4] soc: qcom: aoss: Format string in qmp_send()

[Bjorn Andersson]

The majority of callers to qmp_send() composes the message dynamically
using some form of sprintf(), resulting in unnecessary complication and
stack usage.

By changing the interface of qmp_send() to take a format string and
arguments, the duplicated composition of the commands can be moved to a
single location.

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/soc/qcom/qcom_aoss.c       | 22 ++++++++++++++--------
 include/linux/soc/qcom/qcom_aoss.h |  4 ++--
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
index 13bf13ab78d6..2d8b07b22ebb 100644
--- a/drivers/soc/qcom/qcom_aoss.c
+++ b/drivers/soc/qcom/qcom_aoss.c
@@ -207,27 +207,33 @@ static bool qmp_message_empty(struct qmp *qmp)
 /**
  * qmp_send() - send a message to the AOSS
  * @qmp: qmp context
- * @data: message to be sent
+ * @fmt: format string for message to be sent
+ * @...: arguments for the format string
  *
- * Transmit @data to AOSS and wait for the AOSS to acknowledge the message.
+ * Transmit message to AOSS and wait for the AOSS to acknowledge the message.
  * data must not be longer than the mailbox size. Access is synchronized by
  * this implementation.
  *
  * Return: 0 on success, negative errno on failure
  */
-int qmp_send(struct qmp *qmp, const void *data)
+int qmp_send(struct qmp *qmp, const char *fmt, ...)
 {
 	char buf[QMP_MSG_LEN];
 	long time_left;
+	va_list args;
+	int len;
 	int ret;
 
-	if (WARN_ON(IS_ERR_OR_NULL(qmp) || !data))
+	if (WARN_ON(IS_ERR_OR_NULL(qmp) || !fmt))
 		return -EINVAL;
 
-	if (WARN_ON(strlen(data) >= sizeof(buf)))
-		return -EINVAL;
+	memset(buf, 0, sizeof(buf));
+	va_start(args, fmt);
+	len = vsnprintf(buf, sizeof(buf), fmt, args);
+	va_end(args);
 
-	strscpy_pad(buf, data, sizeof(buf));
+	if (WARN_ON(len >= sizeof(buf)))
+		return -EINVAL;
 
 	mutex_lock(&qmp->tx_lock);
 
@@ -491,7 +497,7 @@ static ssize_t qmp_debugfs_write(struct file *file, const char __user *userstr,
 		return -EFAULT;
 	buf[len] = '\0';
 
-	ret = qmp_send(qmp, buf);
+	ret = qmp_send(qmp, "%s", buf);
 	if (ret < 0)
 		return ret;
 
diff --git a/include/linux/soc/qcom/qcom_aoss.h b/include/linux/soc/qcom/qcom_aoss.h
index 7a71406b6050..7361ca028752 100644
--- a/include/linux/soc/qcom/qcom_aoss.h
+++ b/include/linux/soc/qcom/qcom_aoss.h
@@ -13,13 +13,13 @@ struct qmp;
 
 #if IS_ENABLED(CONFIG_QCOM_AOSS_QMP)
 
-int qmp_send(struct qmp *qmp, const void *data);
+int qmp_send(struct qmp *qmp, const char *fmt, ...);
 struct qmp *qmp_get(struct device *dev);
 void qmp_put(struct qmp *qmp);
 
 #else
 
-static inline int qmp_send(struct qmp *qmp, const void *data)
+static inline int qmp_send(struct qmp *qmp, const char *fmt, ...)
 {
 	return -ENODEV;
 }
-- 
2.25.1

[PATCH v2 4/4] soc: qcom: aoss: Tidy up qmp_send() callers

[Bjorn Andersson]

With qmp_send() handling variable length messages and string formatting
he callers of qmp_send() can be cleaned up to not care about these
things.

Drop the QMP_MSG_LEN sized buffers and use the message formatting, as
appropriate.

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/net/ipa/ipa_power.c    |  5 +----
 drivers/remoteproc/qcom_q6v5.c |  8 +-------
 drivers/soc/qcom/qcom_aoss.c   | 14 ++++----------
 3 files changed, 6 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c
index 26181eeed975..0eaa7a7f3343 100644
--- a/drivers/net/ipa/ipa_power.c
+++ b/drivers/net/ipa/ipa_power.c
@@ -324,15 +324,12 @@ void ipa_power_retention(struct ipa *ipa, bool enable)
 {
 	static const char fmt[] = "{ class: bcm, res: ipa_pc, val: %c }";
 	struct ipa_power *power = ipa->power;
-	char buf[36];	/* Exactly enough for fmt[]; size a multiple of 4 */
 	int ret;
 
 	if (!power->qmp)
 		return;		/* Not needed on this platform */
 
-	(void)snprintf(buf, sizeof(buf), fmt, enable ? '1' : '0');
-
-	ret = qmp_send(power->qmp, buf);
+	ret = qmp_send(power->qmp, fmt, enable ? '1' : '0');
 	if (ret)
 		dev_err(power->dev, "error %d sending QMP %sable request\n",
 			ret, enable ? "en" : "dis");
diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
index 8b41a73fa4d1..4ee5e67a9f03 100644
--- a/drivers/remoteproc/qcom_q6v5.c
+++ b/drivers/remoteproc/qcom_q6v5.c
@@ -23,19 +23,13 @@
 
 static int q6v5_load_state_toggle(struct qcom_q6v5 *q6v5, bool enable)
 {
-	char buf[Q6V5_LOAD_STATE_MSG_LEN];
 	int ret;
 
 	if (!q6v5->qmp)
 		return 0;
 
-	ret = snprintf(buf, sizeof(buf),
-		       "{class: image, res: load_state, name: %s, val: %s}",
+	ret = qmp_send(q6v5->qmp, "{class: image, res: load_state, name: %s, val: %s}",
 		       q6v5->load_state, enable ? "on" : "off");
-
-	WARN_ON(ret >= Q6V5_LOAD_STATE_MSG_LEN);
-
-	ret = qmp_send(q6v5->qmp, buf);
 	if (ret)
 		dev_err(q6v5->dev, "failed to toggle load state\n");
 
diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
index 2d8b07b22ebb..b783ce3f75a4 100644
--- a/drivers/soc/qcom/qcom_aoss.c
+++ b/drivers/soc/qcom/qcom_aoss.c
@@ -266,7 +266,7 @@ EXPORT_SYMBOL(qmp_send);
 
 static int qmp_qdss_clk_prepare(struct clk_hw *hw)
 {
-	static const char buf[QMP_MSG_LEN] = "{class: clock, res: qdss, val: 1}";
+	static const char *buf = "{class: clock, res: qdss, val: 1}";
 	struct qmp *qmp = container_of(hw, struct qmp, qdss_clk);
 
 	return qmp_send(qmp, buf);
@@ -274,7 +274,7 @@ static int qmp_qdss_clk_prepare(struct clk_hw *hw)
 
 static void qmp_qdss_clk_unprepare(struct clk_hw *hw)
 {
-	static const char buf[QMP_MSG_LEN] = "{class: clock, res: qdss, val: 0}";
+	static const char *buf = "{class: clock, res: qdss, val: 0}";
 	struct qmp *qmp = container_of(hw, struct qmp, qdss_clk);
 
 	qmp_send(qmp, buf);
@@ -336,7 +336,6 @@ static int qmp_cdev_set_cur_state(struct thermal_cooling_device *cdev,
 				  unsigned long state)
 {
 	struct qmp_cooling_device *qmp_cdev = cdev->devdata;
-	char buf[QMP_MSG_LEN] = {};
 	bool cdev_state;
 	int ret;
 
@@ -346,13 +345,8 @@ static int qmp_cdev_set_cur_state(struct thermal_cooling_device *cdev,
 	if (qmp_cdev->state == state)
 		return 0;
 
-	snprintf(buf, sizeof(buf),
-		 "{class: volt_flr, event:zero_temp, res:%s, value:%s}",
-			qmp_cdev->name,
-			cdev_state ? "on" : "off");
-
-	ret = qmp_send(qmp_cdev->qmp, buf);
-
+	ret = qmp_send(qmp_cdev->qmp, "{class: volt_flr, event:zero_temp, res:%s, value:%s}",
+		       qmp_cdev->name, cdev_state ? "on" : "off");
 	if (!ret)
 		qmp_cdev->state = cdev_state;
 
-- 
2.25.1

Re: [PATCH v2 2/4] soc: qcom: aoss: Add debugfs interface for sending messages

[Andrew Lunn]

> +static ssize_t qmp_debugfs_write(struct file *file, const char __user *userstr,
> +				 size_t len, loff_t *pos)
> +{
> +	struct qmp *qmp = file->private_data;
> +	char buf[QMP_MSG_LEN];
> +	int ret;
> +
> +	if (!len || len >= QMP_MSG_LEN)
> +		return -EINVAL;
> +
> +	if (copy_from_user(buf, userstr, len))
> +		return -EFAULT;
> +	buf[len] = '\0';
> +
> +	ret = qmp_send(qmp, buf);
> +	if (ret < 0)
> +		return ret;

Sorry, but you still appear to be sending binary blobs from userspace
to the firmware. This is not liked.

The documentation you pointed to has three commands. Please implement
three debugfs files, one per command.

    Andrew

---
pw-bot: cr

Re: [PATCH v2 2/4] soc: qcom: aoss: Add debugfs interface for sending messages

[Bjorn Andersson]

On Fri, Aug 11, 2023 at 11:01:50PM +0200, Andrew Lunn wrote:
> > +static ssize_t qmp_debugfs_write(struct file *file, const char __user *userstr,
> > +				 size_t len, loff_t *pos)
> > +{
> > +	struct qmp *qmp = file->private_data;
> > +	char buf[QMP_MSG_LEN];
> > +	int ret;
> > +
> > +	if (!len || len >= QMP_MSG_LEN)
> > +		return -EINVAL;
> > +
> > +	if (copy_from_user(buf, userstr, len))
> > +		return -EFAULT;
> > +	buf[len] = '\0';
> > +
> > +	ret = qmp_send(qmp, buf);
> > +	if (ret < 0)
> > +		return ret;
> 
> Sorry, but you still appear to be sending binary blobs from userspace
> to the firmware. This is not liked.
> 

As mentioned in the cover letter, I do recognize your concern here. I
don't see it as a realistic way to work around the kernel for reasons of
being proprietary - given that we don't have debugfs mounted in the vast
majority of product.

I do however recognize the benefit of this interface for fellow upstream
engineers.

> The documentation you pointed to has three commands. Please implement
> three debugfs files, one per command.
> 

The documentation pointed to has 4 classes ("class"), but this is not
the full set, each class has N resources ("res") and each resource has a
varying value space - "off", "mol", "enabled", "disabled", "max" to take
the examples from the documentation, other classes takes integers as
argument. Some classes has a fourth key...

Further more, the list of classes, resources and values varies from
target to target.

We're composing the lists of commands, but I'm not sure that it will be
feasible to spell out all the valid commands, on a per-target basis.
It is just a debug feature, I don't want it to take up a significant
portion of the driver.


The alternative is to continue carrying this as an out-of-tree patch,
the only people suffering from that are the ones working exclusively
in on the upstream kernel.

Regards,
Bjorn

Re: [PATCH v2 2/4] soc: qcom: aoss: Add debugfs interface for sending messages

[Bjorn Andersson]

On Fri, Aug 11, 2023 at 01:58:37PM -0700, Bjorn Andersson wrote:
> From: Chris Lew <clew@codeaurora.org>
> 
> In addition to the normal runtime commands, the Always On Processor
> (AOP) provides a number of debug commands which can be used during
> system debugging for things such as preventing power collapse or placing
> floor votes for certain resources. Some of these are documented in the
> Robotics RB5 "Debug AOP ADB" linked below.
> 
> Provide a debugfs interface for the developer/tester to send these
> commands to the AOP.
> 
> Link: https://docs.qualcomm.com/bundle/publicresource/topics/80-88500-3/85_Debugging_AOP_ADB.html
> Signed-off-by: Chris Lew <clew@codeaurora.org>
> [bjorn: Dropped debugfs guards, improve error codes, rewrote commit message]
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>  drivers/soc/qcom/qcom_aoss.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
> index 880fe234ca0a..13bf13ab78d6 100644
> --- a/drivers/soc/qcom/qcom_aoss.c
> +++ b/drivers/soc/qcom/qcom_aoss.c
> @@ -3,6 +3,7 @@
>   * Copyright (c) 2019, Linaro Ltd
>   */
>  #include <linux/clk-provider.h>
> +#include <linux/debugfs.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/mailbox_client.h>
> @@ -82,6 +83,7 @@ struct qmp {
>  
>  	struct clk_hw qdss_clk;
>  	struct qmp_cooling_device *cooling_devs;
> +	struct dentry *debugfs_file;

Simon Horman pointed out in the previous version that this isn't added
to kernel-doc, and I missed correcting this.

Regards,
Bjorn

Re: [PATCH v2 1/4] soc: qcom: aoss: Move length requirements from caller

[Konrad Dybcio]

On 11.08.2023 22:58, Bjorn Andersson wrote:
> The existing implementation of qmp_send() requires the caller to provide
> a buffer which is of word-aligned. The underlying reason for this is
> that message ram only supports word accesses, but pushing this
> requirement onto the clients results in the same boiler plate code
> sprinkled in every call site.
> 
> By using a temporary buffer in qmp_send() we can hide the underlying
> hardware limitations from the clients and allow them to pass their
> NUL-terminates C string directly.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

Re: [PATCH v2 2/4] soc: qcom: aoss: Add debugfs interface for sending messages

[Andrew Lunn]

> As mentioned in the cover letter, I do recognize your concern here. I
> don't see it as a realistic way to work around the kernel for reasons of
> being proprietary - given that we don't have debugfs mounted in the vast
> majority of product.

Look around. How many drivers do you see which allow passing binary
blobs to firmware?

      Andrew

Re: [PATCH v2 2/4] soc: qcom: aoss: Add debugfs interface for sending messages

[Stephen Hemminger]

On Fri, 11 Aug 2023 16:32:28 -0700
Bjorn Andersson <quic_bjorande@quicinc.com> wrote:

> On Fri, Aug 11, 2023 at 11:01:50PM +0200, Andrew Lunn wrote:
> > > +static ssize_t qmp_debugfs_write(struct file *file, const char __user *userstr,
> > > +				 size_t len, loff_t *pos)
> > > +{
> > > +	struct qmp *qmp = file->private_data;
> > > +	char buf[QMP_MSG_LEN];
> > > +	int ret;
> > > +
> > > +	if (!len || len >= QMP_MSG_LEN)
> > > +		return -EINVAL;
> > > +
> > > +	if (copy_from_user(buf, userstr, len))
> > > +		return -EFAULT;
> > > +	buf[len] = '\0';
> > > +
> > > +	ret = qmp_send(qmp, buf);
> > > +	if (ret < 0)
> > > +		return ret;  
> > 
> > Sorry, but you still appear to be sending binary blobs from userspace
> > to the firmware. This is not liked.
> >   
> 
> As mentioned in the cover letter, I do recognize your concern here. I
> don't see it as a realistic way to work around the kernel for reasons of
> being proprietary - given that we don't have debugfs mounted in the vast
> majority of product.

Anyone who cares about security, and has things like kernel lockdown turned on
is going to be scared by this. If you allow API to tell firmware to do any arbitrary thing
it means you could be telling firmware "please read this area of kernel memory for me"

Re: (subset) [PATCH v2 0/4] soc: qcom: aoss: Introduce debugfs interface and cleanup things

[Bjorn Andersson]

On Fri, 11 Aug 2023 13:58:35 -0700, Bjorn Andersson wrote:
> The Always On Processor supports a number useful commands for affecting
> system resources during in various debug scenarious. Introduce a debugfs
> interface for allowing the debugger/tester to send these commands.
> 
> While at it, let's make some improvements to the qmp_send() API.
> 
> 
> [...]

Applied, thanks!

[1/4] soc: qcom: aoss: Move length requirements from caller
      commit: 59e09100836fdb618b107c37189d6001b5825872
[3/4] soc: qcom: aoss: Format string in qmp_send()
      commit: 8873d1e2f88afbe89c99d8f49f88934a2da2991f
[4/4] soc: qcom: aoss: Tidy up qmp_send() callers
      commit: b4f63bbff96e4510676b1e78b00d14baaee9ad29


Please note that I did not pick the debugfs interface (patch 2/4).

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>