diff mbox series

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

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

Commit Message

Bjorn Andersson Aug. 11, 2023, 8:58 p.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

Bjorn Andersson Aug. 11, 2023, 11:32 p.m. UTC | #1 
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
Bjorn Andersson Aug. 11, 2023, 11:36 p.m. UTC | #2 
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
Andrew Lunn Aug. 12, 2023, 5:11 p.m. UTC | #3 
> 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
Stephen Hemminger Aug. 13, 2023, 1:59 a.m. UTC | #4 
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"
diff mbox series

Patch

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