diff mbox series

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

Message ID 20230731041013.2950307-3-quic_bjorande@quicinc.com
State New
Headers
Series soc: qcom: aoss: Introduce debugfs interface and cleanup things |

Commit Message

Bjorn Andersson July 31, 2023, 4:10 a.m. UTC 
  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(+)

Comments

Konrad Dybcio July 31, 2023, 8:15 a.m. UTC | #1 
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
Andrew Lunn July 31, 2023, 8:21 a.m. UTC | #2 
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
Simon Horman July 31, 2023, 3:08 p.m. UTC | #3 
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)

...
Bjorn Andersson July 31, 2023, 3:39 p.m. UTC | #4 
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
Bjorn Andersson July 31, 2023, 4:01 p.m. UTC | #5 
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
Pavan Kondeti Aug. 1, 2023, 4:41 a.m. UTC | #6 
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
Andrew Lunn Aug. 1, 2023, 8:45 a.m. UTC | #7 
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
diff mbox series

Patch

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);