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

[PATCH 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.

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 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..5e74332515cf 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)
 {
 	long time_left;
+	char buf[QMP_MSG_LEN];
 	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

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

[Simon Horman]

On Sun, Jul 30, 2023 at 09:10:10PM -0700, 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>
> ---
>  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..5e74332515cf 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)
>  {
>  	long time_left;
> +	char buf[QMP_MSG_LEN];
>  	int ret;

Hi Bjorn,

please consider preserving reverse xmas tree - longest line to shortest -
for local variable declarations in this Networking code.

	char buf[QMP_MSG_LEN];
	long time_left;
	int ret;

...

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

[Chris Lew]

On 7/30/2023 9:10 PM, Bjorn Andersson wrote:
> diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
>   	/* 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);
>   

Looks like we are telling the firmware the packet size will always be 
QMP_MSG_LEN?

This should be ok but might be a problem when debugging. The AOSS 
firmware only logs size of the message instead of the full string 
because of memory constraints.

We would normally match the firmware and host logs based on size, but 
won't be able to differentiate this way with a fixed size.

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

[Bjorn Andersson]

On Mon, Jul 31, 2023 at 02:29:44PM -0700, Chris Lew wrote:
> 
> 
> On 7/30/2023 9:10 PM, Bjorn Andersson wrote:
> > diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
> >   	/* 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);
> 
> Looks like we are telling the firmware the packet size will always be
> QMP_MSG_LEN?
> 
> This should be ok but might be a problem when debugging. The AOSS firmware
> only logs size of the message instead of the full string because of memory
> constraints.
> 

Until now ipa_power_retention() has been passing 36 here, everyone else
64, so it is ok.

> We would normally match the firmware and host logs based on size, but won't
> be able to differentiate this way with a fixed size.

I don't mind us changing it to ALIGN(len, 4), but as that would change
the current behavior I'd like to do so in a subsequent patch.

Speaking of behavior, is 64 the max message size? We inherited the 64
from the initial downstream implementation, but qmp->size is quite a bit
bigger.

Regards,
Bjorn

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

[Chris Lew]

On 7/31/2023 4:10 PM, Bjorn Andersson wrote:
> On Mon, Jul 31, 2023 at 02:29:44PM -0700, Chris Lew wrote:
>>
>>
>> On 7/30/2023 9:10 PM, Bjorn Andersson wrote:
>>> diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
>>>    	/* 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);
>>
>> Looks like we are telling the firmware the packet size will always be
>> QMP_MSG_LEN?
>>
>> This should be ok but might be a problem when debugging. The AOSS firmware
>> only logs size of the message instead of the full string because of memory
>> constraints.
>>
> 
> Until now ipa_power_retention() has been passing 36 here, everyone else
> 64, so it is ok.
> 
>> We would normally match the firmware and host logs based on size, but won't
>> be able to differentiate this way with a fixed size.
> 
> I don't mind us changing it to ALIGN(len, 4), but as that would change
> the current behavior I'd like to do so in a subsequent patch.
> 
> Speaking of behavior, is 64 the max message size? We inherited the 64
> from the initial downstream implementation, but qmp->size is quite a bit
> bigger.
> 

The max message size the firmware can handle is 0x64, so 100 bytes, but 
I haven't seen any messages go above 64 bytes.

> Regards,
> Bjorn

[PATCH 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 5e74332515cf..4c5bb7034fff 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

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

[Konrad Dybcio]

On 31.07.2023 06:10, Bjorn Andersson wrote:
> From: Chris Lew <clew@codeaurora.org>
No QUIC email?

[...]


> +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)
>=? Otherwise the last char may be overwritten by the NULL termination
couple lines below

> +		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;
> +}
Konrad

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

[Bjorn Andersson]

On Mon, Jul 31, 2023 at 10:15:34AM +0200, Konrad Dybcio wrote:
> On 31.07.2023 06:10, Bjorn Andersson wrote:
> > From: Chris Lew <clew@codeaurora.org>
> No QUIC email?
> 

That's the author and s-o-b address of the patch. mailmap will help you
if you want to reach him.

> [...]
> 
> 
> > +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)
> >=? Otherwise the last char may be overwritten by the NULL termination
> couple lines below
> 

My mind had a '\0' accounted for in len as well, but you're right.

Thanks,
Bjorn

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

[Andrew Lunn]

On Sun, Jul 30, 2023 at 09:10:11PM -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.

This sort of sending arbitrary binary blob commands is not liked,
since it allow user space closed source drivers. At minimum, please
provide a file per command, with the kernel marshalling parameters
into the binary format, and decoding any returned values.

       Andrew

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

[Bjorn Andersson]

On Mon, Jul 31, 2023 at 10:21:31AM +0200, Andrew Lunn wrote:
> On Sun, Jul 30, 2023 at 09:10:11PM -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.
> 
> This sort of sending arbitrary binary blob commands is not liked,
> since it allow user space closed source drivers. At minimum, please
> provide a file per command, with the kernel marshalling parameters
> into the binary format, and decoding any returned values.
> 

Thanks for your input Andrew, that is a valid concern.

The interface is in debugfs and as such wouldn't be suitable for closed
source drivers, as in the majority of our shipping software debugfs
isn't enabled.

Regards,
Bjorn

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

[Andrew Lunn]

On Mon, Jul 31, 2023 at 08:39:38AM -0700, Bjorn Andersson wrote:
> On Mon, Jul 31, 2023 at 10:21:31AM +0200, Andrew Lunn wrote:
> > On Sun, Jul 30, 2023 at 09:10:11PM -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.
> > 
> > This sort of sending arbitrary binary blob commands is not liked,
> > since it allow user space closed source drivers. At minimum, please
> > provide a file per command, with the kernel marshalling parameters
> > into the binary format, and decoding any returned values.
> > 
> 
> Thanks for your input Andrew, that is a valid concern.
> 
> The interface is in debugfs and as such wouldn't be suitable for closed
> source drivers, as in the majority of our shipping software debugfs
> isn't enabled.

There only appears to be 3 commands, so it is now too much of a burden
to do it properly, and not have a binary blob API.

And most distros do have debugfs at least built and available, but
maybe not mounted.

      Andrew

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

[Simon Horman]

On Sun, Jul 30, 2023 at 09:10:11PM -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 5e74332515cf..4c5bb7034fff 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;

Hi Bjorn,

Please consider adding debugfs_file to the kernel doc for struct qmp.

>  };
>  
>  static void qmp_kick(struct qmp *qmp)

...

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

[Pavan Kondeti]

On Sun, Jul 30, 2023 at 09:10:11PM -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>

Thanks Bjorn and Chris for enabling this interface. It will be very useful. 
We use this interface  in downstream kernel during throughput/suspend issues debug. 
I have tested your series with v6.4 on SM8550 and it works as expected.

Thanks,
Pavan

[PATCH 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.

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 4c5bb7034fff..b59c8681bcfe 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, ...)
 {
 	long time_left;
+	va_list args;
 	char buf[QMP_MSG_LEN];
+	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

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

[Konrad Dybcio]

On 31.07.2023 06:10, Bjorn Andersson wrote:
> 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.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
[...]


> +int qmp_send(struct qmp *qmp, const char *fmt, ...)
>  {
>  	long time_left;
> +	va_list args;
>  	char buf[QMP_MSG_LEN];
> +	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));
Wouldn't initializing the array with = { 0 } be faster?

Otherwise, this looks very cool

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

[PATCH 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.

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 b59c8681bcfe..4a4e12c7439a 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 4/4] soc: qcom: aoss: Tidy up qmp_send() callers

[Konrad Dybcio]

On 31.07.2023 06:10, Bjorn Andersson wrote:
> 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.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
If Alex is okay with that going through qcom:

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

One nit below.

[...]

> -	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}",
There's a space after the first colon but not after the subsequent
ones.

Konrad