[v2,4/4] remoteproc: debugfs: Add ability to boot remote processor using debugfs

This functionality is especially useful during the testing phase.  When
used in conjunction with Mailbox's Test Framework we can trivially conduct
end-to-end testing i.e. boot co-processor, send and receive messages to
the co-processor, then shut it down again (repeat as required).

Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/remoteproc_debugfs.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

On 28/08/15 03:31, Lee Jones wrote:
> This functionality is especially useful during the testing phase.  When
> used in conjunction with Mailbox's Test Framework we can trivially conduct
> end-to-end testing i.e. boot co-processor, send and receive messages to
> the co-processor, then shut it down again (repeat as required).

This does not strike me as a particularly well defined nor suitable
interface for controlling a remote processor's state. I know you are
just extending the existing debugfs interface here, but someone ought to
remove that piece of code and make it a proper character device or
netlink or whatever that allows someone to get proper signaling of
what's going on with the remote processor state by polling or listening
to a socket.

What's the intended use case behind debugfs for this after all? Is your
application expected to keep reading the state file in a loop until it
is happy and reads "running", how is that not racy by nature?

> 
> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_debugfs.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
> index 9d30809..464470d 100644
> --- a/drivers/remoteproc/remoteproc_debugfs.c
> +++ b/drivers/remoteproc/remoteproc_debugfs.c
> @@ -88,8 +88,37 @@ static ssize_t rproc_state_read(struct file *filp, char __user *userbuf,
>  	return simple_read_from_buffer(userbuf, count, ppos, buf, i);
>  }
>  
> +static ssize_t rproc_state_write(struct file *filp, const char __user *userbuf,
> +				 size_t count, loff_t *ppos)
> +{
> +	struct rproc *rproc = filp->private_data;
> +	char buf[2];
> +	int ret;
> +
> +	ret = copy_from_user(buf, userbuf, 1);
> +	if (ret)
> +		return -EFAULT;
> +
> +	switch (buf[0]) {
> +	case '0':
> +		rproc_shutdown(rproc);
> +		break;
> +	case '1':
> +		ret = rproc_boot(rproc);
> +		if (ret)
> +			dev_warn(&rproc->dev, "Boot failed: %d\n", ret);
> +		break;
> +	default:
> +		dev_err(&rproc->dev, "Unrecognised option: %x\n", buf[1]);
> +		return -EINVAL;
> +	}
> +
> +	return count;
> +}
> +
>  static const struct file_operations rproc_state_ops = {
>  	.read = rproc_state_read,
> +	.write = rproc_state_write,
>  	.open = simple_open,
>  	.llseek	= generic_file_llseek,
>  };
>

On Fri, 28 Aug 2015, Florian Fainelli wrote:
> On 28/08/15 03:31, Lee Jones wrote:
> > This functionality is especially useful during the testing phase.  When
> > used in conjunction with Mailbox's Test Framework we can trivially conduct
> > end-to-end testing i.e. boot co-processor, send and receive messages to
> > the co-processor, then shut it down again (repeat as required).
> 
> This does not strike me as a particularly well defined nor suitable
> interface for controlling a remote processor's state. I know you are
> just extending the existing debugfs interface here, but someone ought to
> remove that piece of code and make it a proper character device or
> netlink or whatever that allows someone to get proper signaling of
> what's going on with the remote processor state by polling or listening
> to a socket.

Please don't confuse this debugging interface as a solid means by
which to control co-processors.  It's in DebugFS for a reason.  The
idea of this feature is for driver writers to control *easily*
control co-processors for testing purposes only.

DebugFS will be disabled in production images.

> What's the intended use case behind debugfs for this after all? Is your
> application expected to keep reading the state file in a loop until it
> is happy and reads "running", how is that not racy by nature?

There is no 'application'.  One of the key wins for DebugFS is that
driver writers don't have to write applications during the testing
phase.  Using a character device instead would be a mistake IMO.

> > Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/remoteproc/remoteproc_debugfs.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
> > index 9d30809..464470d 100644
> > --- a/drivers/remoteproc/remoteproc_debugfs.c
> > +++ b/drivers/remoteproc/remoteproc_debugfs.c
> > @@ -88,8 +88,37 @@ static ssize_t rproc_state_read(struct file *filp, char __user *userbuf,
> >  	return simple_read_from_buffer(userbuf, count, ppos, buf, i);
> >  }
> >  
> > +static ssize_t rproc_state_write(struct file *filp, const char __user *userbuf,
> > +				 size_t count, loff_t *ppos)
> > +{
> > +	struct rproc *rproc = filp->private_data;
> > +	char buf[2];
> > +	int ret;
> > +
> > +	ret = copy_from_user(buf, userbuf, 1);
> > +	if (ret)
> > +		return -EFAULT;
> > +
> > +	switch (buf[0]) {
> > +	case '0':
> > +		rproc_shutdown(rproc);
> > +		break;
> > +	case '1':
> > +		ret = rproc_boot(rproc);
> > +		if (ret)
> > +			dev_warn(&rproc->dev, "Boot failed: %d\n", ret);
> > +		break;
> > +	default:
> > +		dev_err(&rproc->dev, "Unrecognised option: %x\n", buf[1]);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return count;
> > +}
> > +
> >  static const struct file_operations rproc_state_ops = {
> >  	.read = rproc_state_read,
> > +	.write = rproc_state_write,
> >  	.open = simple_open,
> >  	.llseek	= generic_file_llseek,
> >  };
> > 
> 
>

On Fri, 28 Aug 2015, Nathan Lynch wrote:

> On 08/28/2015 05:31 AM, Lee Jones wrote:
> > +static ssize_t rproc_state_write(struct file *filp, const char __user *userbuf,
> > +				 size_t count, loff_t *ppos)
> > +{
> > +	struct rproc *rproc = filp->private_data;
> > +	char buf[2];
> > +	int ret;
> > +
> > +	ret = copy_from_user(buf, userbuf, 1);
> > +	if (ret)
> > +		return -EFAULT;
> > +
> > +	switch (buf[0]) {
> > +	case '0':
> > +		rproc_shutdown(rproc);
> > +		break;
> > +	case '1':
> > +		ret = rproc_boot(rproc);
> > +		if (ret)
> > +			dev_warn(&rproc->dev, "Boot failed: %d\n", ret);
> > +		break;
> > +	default:
> > +		dev_err(&rproc->dev, "Unrecognised option: %x\n", buf[1]);
> > +		return -EINVAL;
> 
> This prints uninitialized kernel stack contents instead of what was
> copied from user space.

Yes, you're right.  I will conduct a better test of the failure path
here. 

> Is the dev_err statement really necessary anyway?

Yes.  As I described in my other mail, this interface is for Kernel
Engineers, who read the kernel log.

> > +	}
> > +
> > +	return count;
> > +}
> 
> If rproc_boot fails, that should be reflected in the syscall result.
> 
> This interface is essentially exposing the remoteproc->power refcount to
> user space; is that okay?  Seems like it makes it easy to underflow
> remoteproc->power through successive shutdown calls.

If the subsystem is suseptable to underruns someone should think about
adding protection against imbalances in the 'core'.

> The other debugfs interface in remoteproc that has a write method
> (recovery) accepts more expressive string commands as opposed to 0/1.
> It would be more consistent for this interface to take commands such as
> "boot" and "shutdown" IMO.

Agreed.