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

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.


In v1, Andrew pointed out the fact that this interface could be used to
send proprietary commands from a userspace driver. As debugfs isn't
enabled in the vast majority of shipping devices, this wouldn't be very
useful. In the mantime having this interface present is very useful for
members of the upstream Qualcomm community.

It would be even more user (developer) friendly to expose each of these
knobs as individual debugfs files. But the list of commands and
resources looks to be varying from target to target, so this is still
being investigated. The hope is to perhaps capture a set of common
resources.

Changes since v1:
- Reverse xmas tree in qmp_send() in patch 1
- Correct length check of input data in patch 2

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

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

> 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"

On Fri, 11 Aug 2023 13:58:35 -0700, Bjorn Andersson wrote:
> 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.
> 
> 
> [...]

Applied, thanks!

[1/4] soc: qcom: aoss: Move length requirements from caller
      commit: 59e09100836fdb618b107c37189d6001b5825872
[3/4] soc: qcom: aoss: Format string in qmp_send()
      commit: 8873d1e2f88afbe89c99d8f49f88934a2da2991f
[4/4] soc: qcom: aoss: Tidy up qmp_send() callers
      commit: b4f63bbff96e4510676b1e78b00d14baaee9ad29


Please note that I did not pick the debugfs interface (patch 2/4).

Best regards,