| Message ID | 20230811205839.727373-3-quic_bjorande@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | soc: qcom: aoss: Introduce debugfs interface and cleanup things | expand |
> +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
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
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
> 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
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 --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);