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