Re: [PATCH 3/4] remoteproc: Add a sysfs interface for firmware and state

[loic pallardy]

On 10/13/2016 04:25 PM, Matt Redfearn wrote:
> Hi Loic,
>
>
> On 13/10/16 14:56, loic pallardy wrote:
> > On 10/11/2016 03:39 PM, Matt Redfearn wrote:
> > > This patch adds a sysfs interface to rproc allowing the firmware name
> > > and processor state to be changed dynamically.
> > >
> > > State was previously available in debugfs, and is replicated here. The
> > > firmware file allows retrieval of the running firmware name, and a new
> > > one to be specified at run time, so long as the remote processor has
> > > been stopped.
> > >
> > > Signed-off-by: Matt Redfearn <matt.redfearn@xxxxxxxxxx>
> > >
> > > ---
> > >
> > >  Documentation/ABI/testing/sysfs-class-remoteproc |  50 +++++++++
> > >  drivers/remoteproc/Makefile                      |   1 +
> > >  drivers/remoteproc/remoteproc_core.c             |   3 +
> > >  drivers/remoteproc/remoteproc_internal.h         |   5 +
> > >  drivers/remoteproc/remoteproc_sysfs.c            | 132
> > > +++++++++++++++++++++++
> > >  5 files changed, 191 insertions(+)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-class-remoteproc
> > >  create mode 100644 drivers/remoteproc/remoteproc_sysfs.c
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-class-remoteproc
> > > b/Documentation/ABI/testing/sysfs-class-remoteproc
> > > new file mode 100644
> > > index 000000000000..d188afebc8ba
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-class-remoteproc
> > > @@ -0,0 +1,50 @@
> > > +What:        /sys/class/remoteproc/.../firmware
> > > +Date:        October 2016
> > > +Contact:    Matt Redfearn <matt.redfearn@xxxxxxxxxx>
> > > +Description:    Remote processor firmware
> > > +
> > > +        Reports the name of the firmware currently loaded to the
> > > +        remote processor.
> > > +
> > > +        To change the running firmware, ensure the remote processor is
> > > +        stopped (using /sys/class/remoteproc/.../state) and write a
> > > new filename.
> > > +
> > > +What:        /sys/class/remoteproc/.../state
> > > +Date:        October 2016
> > > +Contact:    Matt Redfearn <matt.redfearn@xxxxxxxxxx>
> > > +Description:    Remote processor state
> > > +
> > > +        Reports the state of the remote processor, which will be one
> > > of:
> > > +
> > > +        "offline"
> > > +        "suspended"
> > > +        "running"
> > > +        "crashed"
> > > +        "invalid"
> > > +
> > > +        "offline" means the remote processor is powered off.
> > > +
> > > +        "suspended" means that the remote processor is suspended and
> > > +        must be woken to receive messages.
> > > +
> > > +        "running" is the normal state of an available remote processor
> > > +
> > > +        "crashed" indicates that a problem/crash has been detected on
> > > +        the remote processor.
> > > +
> > > +        "invalid" is returned if the remote processor is in an
> > > +        unknown state.
> > > +
> > > +        Writing this file controls the state of the remote processor.
> > > +        The following states can be written:
> > > +
> > > +        "start"
> > > +        "stop"
> > > +
> > > +        Writing "start" will attempt to start the processor running the
> > > +        firmware indicated by, or written to,
> > > +        /sys/class/remoteproc/.../firmware. The remote processor should
> > > +        transition to "running" state.
> > > +
> > > +        Writing "stop" will attempt to halt the remote processor and
> > > +        return it to the "offline" state.
> > > diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> > > index 92d3758bd15c..666258bd80aa 100644
> > > --- a/drivers/remoteproc/Makefile
> > > +++ b/drivers/remoteproc/Makefile
> > > @@ -5,6 +5,7 @@
> > >  obj-$(CONFIG_REMOTEPROC)        += remoteproc.o
> > >  remoteproc-y                := remoteproc_core.o
> > >  remoteproc-y                += remoteproc_debugfs.o
> > > +remoteproc-y                += remoteproc_sysfs.o
> > >  remoteproc-y                += remoteproc_virtio.o
> > >  remoteproc-y                += remoteproc_elf_loader.o
> > >  obj-$(CONFIG_OMAP_REMOTEPROC)        += omap_remoteproc.o
> > > diff --git a/drivers/remoteproc/remoteproc_core.c
> > > b/drivers/remoteproc/remoteproc_core.c
> > > index 2152b484f314..642c5b58fec5 100644
> > > --- a/drivers/remoteproc/remoteproc_core.c
> > > +++ b/drivers/remoteproc/remoteproc_core.c
> > > @@ -1455,6 +1455,7 @@ struct rproc *rproc_alloc(struct device *dev,
> > > const char *name,
> > >      device_initialize(&rproc->dev);
> > >      rproc->dev.parent = dev;
> > >      rproc->dev.type = &rproc_type;
> > > +    rproc->dev.class = &rproc_class;
> > >
> > >      /* Assign a unique device index and name */
> > >      rproc->index = ida_simple_get(&rproc_dev_index, 0, 0, GFP_KERNEL);
> > > @@ -1580,6 +1581,7 @@ EXPORT_SYMBOL(rproc_report_crash);
> > >
> > >  static int __init remoteproc_init(void)
> > >  {
> > > +    rproc_init_sysfs();
> > >      rproc_init_debugfs();
> > >
> > >      return 0;
> > > @@ -1591,6 +1593,7 @@ static void __exit remoteproc_exit(void)
> > >      ida_destroy(&rproc_dev_index);
> > >
> > >      rproc_exit_debugfs();
> > > +    rproc_exit_sysfs();
> > >  }
> > >  module_exit(remoteproc_exit);
> > >
> > > diff --git a/drivers/remoteproc/remoteproc_internal.h
> > > b/drivers/remoteproc/remoteproc_internal.h
> > > index 837faf2677a6..2beb86ddfacc 100644
> > > --- a/drivers/remoteproc/remoteproc_internal.h
> > > +++ b/drivers/remoteproc/remoteproc_internal.h
> > > @@ -64,6 +64,11 @@ void rproc_create_debug_dir(struct rproc *rproc);
> > >  void rproc_init_debugfs(void);
> > >  void rproc_exit_debugfs(void);
> > >
> > > +/* from remoteproc_sysfs.c */
> > > +extern struct class rproc_class;
> > struct class rproc_class should be static to remoteproc_sysfs.c file.
> > It will be better to create a new interface like rproc_register_sysfs
> > to associate rproc_class to rproc device.
>
> It would be nice if that were possible, but since the class has to
> associated with the devices created within remoteproc_core, as it stands
> we must have visibility of this symbol there, see above the change to
> drivers/remoteproc/remoteproc_core.c line 1455.
> The alternative would be a utility function in remoteproc_sysfs.c to
> associate the class with an rproc device, it depends what the preference
> would be.

Yes it will be my preference. remoteproc_sysfs.c file to provide a new 
service like:
void rproc_register_sysfs(struct rproc *rproc) {
	rproc->dev.class = &rproc_class;
}

and to call this function from rproc_alloc

/Loic
> Thanks,
> Matt
>
> > > +int rproc_init_sysfs(void);
> > > +void rproc_exit_sysfs(void);
> > > +
> > >  void rproc_free_vring(struct rproc_vring *rvring);
> > >  int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
> > >
> > > diff --git a/drivers/remoteproc/remoteproc_sysfs.c
> > > b/drivers/remoteproc/remoteproc_sysfs.c
> > > new file mode 100644
> > > index 000000000000..892599863ca6
> > > --- /dev/null
> > > +++ b/drivers/remoteproc/remoteproc_sysfs.c
> > > @@ -0,0 +1,132 @@
> > > +/*
> > > + * Remote Processor Framework
> > > + *
> > > + * This program is free software; you can redistribute it and/or
> > > + * modify it under the terms of the GNU General Public License
> > > + * version 2 as published by the Free Software Foundation.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + */
> > > +
> > > +#include <linux/remoteproc.h>
> > > +
> > > +#include "remoteproc_internal.h"
> > > +
> > > +#define to_rproc(d) container_of(d, struct rproc, dev)
> > > +
> > > +/* Expose the loaded / running firmware name via sysfs */
> > > +static ssize_t firmware_show(struct device *dev, struct
> > > device_attribute *attr,
> > > +              char *buf)
> > > +{
> > > +    struct rproc *rproc = to_rproc(dev);
> > > +
> > > +    if (rproc->firmware)
> > > +        return sprintf(buf, "%s\n", rproc->firmware);
> > > +    else
> > > +        return sprintf(buf, "unknown");
> > > +}
> > > +
> > > +/* Change firmware name via sysfs */
> > > +static ssize_t firmware_store(struct device *dev,
> > > +                  struct device_attribute *attr,
> > > +                  const char *buf, size_t count)
> > > +{
> > > +    struct rproc *rproc = to_rproc(dev);
> > > +    int err;
> > > +
> > > +    err = rproc_change_firmware(rproc, buf);
> > > +    return err ? err : count;
> > > +}
> > > +static DEVICE_ATTR_RW(firmware);
> > > +
> > > +
> > > +/*
> > > + * A state-to-string lookup table, for exposing a human readable state
> > > + * via sysfs. Always keep in sync with enum rproc_state
> > > + */
> > > +static const char * const rproc_state_string[] = {
> > > +    "offline",
> > > +    "suspended",
> > > +    "running",
> > > +    "crashed",
> > > +    "invalid",
> > > +};
> > > +
> > > +/* Expose the state of the remote processor via sysfs */
> > > +static ssize_t state_show(struct device *dev, struct
> > > device_attribute *attr,
> > > +              char *buf)
> > > +{
> > > +    struct rproc *rproc = to_rproc(dev);
> > > +    unsigned int state;
> > > +
> > > +    state = rproc->state > RPROC_LAST ? RPROC_LAST : rproc->state;
> > > +    return scnprintf(buf, 30, "%.28s (%d)\n",
> > > +             rproc_state_string[state], rproc->state);
> > > +}
> > > +
> > > +/* Change remote processor state via sysfs */
> > > +static ssize_t state_store(struct device *dev,
> > > +                  struct device_attribute *attr,
> > > +                  const char *buf, size_t count)
> > > +{
> > > +    struct rproc *rproc = to_rproc(dev);
> > > +    int ret = 0;
> > > +    int len = count;
> > > +
> > > +    if (buf[len - 1] == '\n')
> > > +        len--;
> > > +
> > > +    if (!strncmp(buf, "start", len)) {
> > > +        if (rproc->state == RPROC_RUNNING)
> > > +            return -EBUSY;
> > > +
> > > +        ret = rproc_boot(rproc);
> > > +        if (ret)
> > > +            dev_err(&rproc->dev, "Boot failed: %d\n", ret);
> > > +    } else if (!strncmp(buf, "stop", len)) {
> > > +        rproc_shutdown(rproc);
> > > +    } else {
> > > +        dev_err(&rproc->dev, "Unrecognised option: %s\n", buf);
> > > +        ret = -EINVAL;
> > > +    }
> > > +    return ret ? ret : count;
> > > +}
> > > +static DEVICE_ATTR_RW(state);
> > > +
> > > +static const struct attribute *rproc_attrs[] = {
> > > +    &dev_attr_firmware.attr,
> > > +    &dev_attr_state.attr,
> > > +    NULL
> > > +};
> > > +
> > > +static const struct attribute_group rproc_devgroup = {
> > > +    .attrs = rproc_attrs
> > > +};
> > > +
> > > +static const struct attribute_group *rproc_devgroups[] = {
> > > +    &rproc_devgroup,
> > > +    NULL
> > > +};
> > > +
> > > +struct class rproc_class = {
> > static ?
> >
> > Regards,
> > Loic
> > > +    .name        = "remoteproc",
> > > +    .dev_groups    = rproc_devgroups,
> > > +};
> > > +
> > > +int __init rproc_init_sysfs(void)
> > > +{
> > > +    /* create remoteproc device class for sysfs */
> > > +    int err = class_register(&rproc_class);
> > > +
> > > +    if (err)
> > > +        pr_err("remoteproc: unable to register class\n");
> > > +    return err;
> > > +}
> > > +
> > > +void __exit rproc_exit_sysfs(void)
> > > +{
> > > +    class_unregister(&rproc_class);
> > > +}