It is often desireable to boot a remote processor with different
firmware files, depending on the needs of the system at a particular
time. This series adds a sysfs interface to the remoteproc core,
exposing interfaces to manipulate the remote processor. One interface is
the "state" file which performs the same function as the one in debugfs
(which is removed later in the series). The other is a "firmware" file
which allows retrieval of the name of the running firmware, and allows a
new firmware to be loaded when written, as long as the remote processor
is currently stopped.
Some groundwork must be laid first, changing the storage mechanism of
the firmware name such that it can be rewritten easily and then
providing an accessor function to facilitate changing the loaded
firmware. That is then wired up to the new sysfs interface.
This series is based on v4.8
Matt Redfearn (4):
remoteproc: Use fixed length field for firmware name
remoteproc: Introduce rproc_change_firmware
remoteproc: Add a sysfs interface for firmware and state
remoteproc: debugfs: Remove state entry which is duplicated is sysfs
Documentation/ABI/testing/sysfs-class-remoteproc | 50 +++++++++
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/remoteproc_core.c | 129 ++++++++++++++++++----
drivers/remoteproc/remoteproc_debugfs.c | 71 ------------
drivers/remoteproc/remoteproc_internal.h | 6 ++
drivers/remoteproc/remoteproc_sysfs.c | 132 +++++++++++++++++++++++
include/linux/remoteproc.h | 4 +-
7 files changed, 299 insertions(+), 94 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-class-remoteproc
create mode 100644 drivers/remoteproc/remoteproc_sysfs.c
--
2.7.4
Since there is now an always available state file in sysfs with the same
function as this one in debugfs, remove the redundant entry.
Signed-off-by: Matt Redfearn <[email protected]>
---
drivers/remoteproc/remoteproc_debugfs.c | 71 ---------------------------------
1 file changed, 71 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
index 74a120b6e206..7a3ca3059892 100644
--- a/drivers/remoteproc/remoteproc_debugfs.c
+++ b/drivers/remoteproc/remoteproc_debugfs.c
@@ -59,75 +59,6 @@ static const struct file_operations trace_rproc_ops = {
.llseek = generic_file_llseek,
};
-/*
- * A state-to-string lookup table, for exposing a human readable state
- * via debugfs. 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 debugfs */
-static ssize_t rproc_state_read(struct file *filp, char __user *userbuf,
- size_t count, loff_t *ppos)
-{
- struct rproc *rproc = filp->private_data;
- unsigned int state;
- char buf[30];
- int i;
-
- state = rproc->state > RPROC_LAST ? RPROC_LAST : rproc->state;
-
- i = scnprintf(buf, 30, "%.28s (%d)\n", rproc_state_string[state],
- rproc->state);
-
- 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[10];
- int ret;
-
- if (count > sizeof(buf) || count <= 0)
- return -EINVAL;
-
- ret = copy_from_user(buf, userbuf, count);
- if (ret)
- return -EFAULT;
-
- if (buf[count - 1] == '\n')
- buf[count - 1] = '\0';
-
- if (!strncmp(buf, "start", count)) {
- ret = rproc_boot(rproc);
- if (ret) {
- dev_err(&rproc->dev, "Boot failed: %d\n", ret);
- return ret;
- }
- } else if (!strncmp(buf, "stop", count)) {
- rproc_shutdown(rproc);
- } else {
- dev_err(&rproc->dev, "Unrecognised option: %s\n", buf);
- 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,
-};
-
/* expose the name of the remote processor via debugfs */
static ssize_t rproc_name_read(struct file *filp, char __user *userbuf,
size_t count, loff_t *ppos)
@@ -265,8 +196,6 @@ void rproc_create_debug_dir(struct rproc *rproc)
debugfs_create_file("name", 0400, rproc->dbg_dir,
rproc, &rproc_name_ops);
- debugfs_create_file("state", 0400, rproc->dbg_dir,
- rproc, &rproc_state_ops);
debugfs_create_file("recovery", 0400, rproc->dbg_dir,
rproc, &rproc_recovery_ops);
}
--
2.7.4
Storage of the firmware name was inconsistent, either storing a pointer
to a name stored with unknown ownership, or a variable length tacked
onto the end of the struct proc allocated in rproc_alloc.
In preparation for allowing the firmware of an already allocated struct
rproc to be changed, the easiest way to allow reallocation of the name
is to switch to a fixed length buffer held as part of the struct rproc.
That way we can either copy the provided firmware name into it, or print
into it based on a name template. A new function,
rproc_set_firmware_name() is introduced for this purpose, and that logic
removed from rproc_alloc().
Signed-off-by: Matt Redfearn <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 64 +++++++++++++++++++++++-------------
include/linux/remoteproc.h | 4 ++-
2 files changed, 45 insertions(+), 23 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index fe0539ed9cb5..48cd9d5afb69 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1309,6 +1309,42 @@ static void rproc_type_release(struct device *dev)
kfree(rproc);
}
+/**
+ * rproc_set_firmware_name() - helper to create a valid firmare name
+ * @rproc: remote processor
+ * @firmware: name of firmware file, can be NULL
+ *
+ * If the caller didn't pass in a firmware name then construct a default name,
+ * otherwise the provided name is copied into the firmware field of struct
+ * rproc. If the name is too long to fit, -EINVAL is returned.
+ *
+ * Returns 0 on success and an appropriate error code otherwise.
+ */
+static int rproc_set_firmware_name(struct rproc *rproc, const char *firmware)
+{
+ char *cp, *template = "rproc-%s-fw";
+ int name_len;
+
+ if (firmware) {
+ name_len = strlen(firmware);
+ cp = memchr(firmware, '\n', name_len);
+ if (cp)
+ name_len = cp - firmware;
+
+ if (name_len > RPROC_MAX_FIRMWARE_NAME_LEN)
+ return -EINVAL;
+
+ strncpy(rproc->firmware, firmware, name_len);
+ rproc->firmware[name_len] = '\0';
+ } else {
+ snprintf(rproc->firmware, RPROC_MAX_FIRMWARE_NAME_LEN,
+ template, rproc->name);
+ }
+
+ dev_dbg(&rproc->dev, "Using firmware %s\n", rproc->firmware);
+ return 0;
+}
+
static struct device_type rproc_type = {
.name = "remoteproc",
.release = rproc_type_release,
@@ -1342,35 +1378,14 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
const char *firmware, int len)
{
struct rproc *rproc;
- char *p, *template = "rproc-%s-fw";
- int name_len = 0;
if (!dev || !name || !ops)
return NULL;
- if (!firmware)
- /*
- * Make room for default firmware name (minus %s plus '\0').
- * If the caller didn't pass in a firmware name then
- * construct a default name. We're already glomming 'len'
- * bytes onto the end of the struct rproc allocation, so do
- * a few more for the default firmware name (but only if
- * the caller doesn't pass one).
- */
- name_len = strlen(name) + strlen(template) - 2 + 1;
-
- rproc = kzalloc(sizeof(struct rproc) + len + name_len, GFP_KERNEL);
+ rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL);
if (!rproc)
return NULL;
- if (!firmware) {
- p = (char *)rproc + sizeof(struct rproc) + len;
- snprintf(p, name_len, template, name);
- } else {
- p = (char *)firmware;
- }
-
- rproc->firmware = p;
rproc->name = name;
rproc->ops = ops;
rproc->priv = &rproc[1];
@@ -1389,6 +1404,11 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
dev_set_name(&rproc->dev, "remoteproc%d", rproc->index);
+ if (rproc_set_firmware_name(rproc, firmware)) {
+ put_device(&rproc->dev);
+ return NULL;
+ }
+
atomic_set(&rproc->power, 0);
/* Set ELF as the default fw_ops handler */
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 1c457a8dd5a6..7a6f9ad55011 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -42,6 +42,8 @@
#include <linux/idr.h>
#include <linux/of.h>
+#define RPROC_MAX_FIRMWARE_NAME_LEN 128
+
/**
* struct resource_table - firmware resource table header
* @ver: version number
@@ -416,7 +418,7 @@ struct rproc {
struct list_head node;
struct iommu_domain *domain;
const char *name;
- const char *firmware;
+ char firmware[RPROC_MAX_FIRMWARE_NAME_LEN];
void *priv;
const struct rproc_ops *ops;
struct device dev;
--
2.7.4
It is often desirable to be able to change the running firmware on a
remote processor dynamically. This used to require a complete
destruction and readdition of the struct rproc, but now that the
firmware name is fixed length, it can be updated freely.
So long as the remote processor is in RPROC_OFFLINE state,
rproc_change_firmware() will free resources from the previous firmware
and request a new one be loaded.
Signed-off-by: Matt Redfearn <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 62 ++++++++++++++++++++++++++++++++
drivers/remoteproc/remoteproc_internal.h | 1 +
2 files changed, 63 insertions(+)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 48cd9d5afb69..2152b484f314 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1345,6 +1345,68 @@ static int rproc_set_firmware_name(struct rproc *rproc, const char *firmware)
return 0;
}
+/**
+ * rproc_change_firmware() - change the loaded firmware on a remote processor
+ * @rproc: remote processor
+ * @firmware: name of firmware file to load, can be NULL
+ *
+ * Attempts to change the firmware loaded for a remote processor. The processor
+ * must be in RPROC_OFFLINE state.
+ *
+ * Any allocated resources for the exiting firmware are released, the new
+ * firmware name is set and then any virtio devices probed.
+ *
+ * Returns 0 on success and an appropriate error code otherwise.
+ *
+ * Note: this function initiates an asynchronous firmware loading
+ * context, which will look for virtio devices supported by the rproc's
+ * firmware.
+ *
+ * If found, those virtio devices will be created and added, so as a result
+ * of registering this remote processor, additional virtio drivers might be
+ * probed.
+ */
+int rproc_change_firmware(struct rproc *rproc, const char *firmware)
+{
+ struct device *dev = &rproc->dev;
+ struct rproc_vdev *rvdev, *rvtmp;
+ int ret;
+
+ ret = mutex_lock_interruptible(&rproc->lock);
+ if (ret) {
+ dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
+ return -EINVAL;
+ }
+
+ if (rproc->state != RPROC_OFFLINE) {
+ dev_err(dev, "can't change firmware while running\n");
+ ret = -EBUSY;
+ goto out;
+ }
+
+ /* Wait for any pending firmware load */
+ wait_for_completion(&rproc->firmware_loading_complete);
+
+ /* clean up all acquired resources */
+ rproc_resource_cleanup(rproc);
+
+ /* clean up remote vdev entries */
+ list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node)
+ rproc_remove_virtio_dev(rvdev);
+
+ /* Free the copy of the resource table */
+ kfree(rproc->cached_table);
+
+ ret = rproc_set_firmware_name(rproc, firmware);
+ if (ret)
+ goto out;
+
+ ret = rproc_add_virtio_devices(rproc);
+out:
+ mutex_unlock(&rproc->lock);
+ return ret;
+}
+
static struct device_type rproc_type = {
.name = "remoteproc",
.release = rproc_type_release,
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 57e1de59bec8..837faf2677a6 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -49,6 +49,7 @@ struct rproc_fw_ops {
void rproc_release(struct kref *kref);
irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
int rproc_boot_nowait(struct rproc *rproc);
+int rproc_change_firmware(struct rproc *rproc, const char *firmware);
/* from remoteproc_virtio.c */
int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id);
--
2.7.4
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 <[email protected]>
---
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 <[email protected]>
+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 <[email protected]>
+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;
+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 = {
+ .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);
+}
--
2.7.4
On 10/11/2016 03:39 PM, Matt Redfearn wrote:
> Storage of the firmware name was inconsistent, either storing a pointer
> to a name stored with unknown ownership, or a variable length tacked
> onto the end of the struct proc allocated in rproc_alloc.
>
> In preparation for allowing the firmware of an already allocated struct
> rproc to be changed, the easiest way to allow reallocation of the name
> is to switch to a fixed length buffer held as part of the struct rproc.
> That way we can either copy the provided firmware name into it, or print
> into it based on a name template. A new function,
> rproc_set_firmware_name() is introduced for this purpose, and that logic
> removed from rproc_alloc().
>
> Signed-off-by: Matt Redfearn <[email protected]>
> ---
>
> drivers/remoteproc/remoteproc_core.c | 64 +++++++++++++++++++++++-------------
> include/linux/remoteproc.h | 4 ++-
> 2 files changed, 45 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index fe0539ed9cb5..48cd9d5afb69 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1309,6 +1309,42 @@ static void rproc_type_release(struct device *dev)
> kfree(rproc);
> }
>
> +/**
> + * rproc_set_firmware_name() - helper to create a valid firmare name
> + * @rproc: remote processor
> + * @firmware: name of firmware file, can be NULL
> + *
> + * If the caller didn't pass in a firmware name then construct a default name,
> + * otherwise the provided name is copied into the firmware field of struct
> + * rproc. If the name is too long to fit, -EINVAL is returned.
> + *
> + * Returns 0 on success and an appropriate error code otherwise.
> + */
> +static int rproc_set_firmware_name(struct rproc *rproc, const char *firmware)
> +{
> + char *cp, *template = "rproc-%s-fw";
> + int name_len;
> +
> + if (firmware) {
> + name_len = strlen(firmware);
> + cp = memchr(firmware, '\n', name_len);
> + if (cp)
> + name_len = cp - firmware;
> +
> + if (name_len > RPROC_MAX_FIRMWARE_NAME_LEN)
Hi Matt,
As you are added '\0' at offset name_len, name_len should be < to
RPROC_MAX_FIRMWARE_NAME_LEN.
Test should be if (name_len >= RPROC_MAX_FIRMWARE_NAME_LEN)
Regards,
Loic
> + return -EINVAL;
> +
> + strncpy(rproc->firmware, firmware, name_len);
> + rproc->firmware[name_len] = '\0';
> + } else {
> + snprintf(rproc->firmware, RPROC_MAX_FIRMWARE_NAME_LEN,
> + template, rproc->name);
> + }
> +
> + dev_dbg(&rproc->dev, "Using firmware %s\n", rproc->firmware);
> + return 0;
> +}
> +
> static struct device_type rproc_type = {
> .name = "remoteproc",
> .release = rproc_type_release,
> @@ -1342,35 +1378,14 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
> const char *firmware, int len)
> {
> struct rproc *rproc;
> - char *p, *template = "rproc-%s-fw";
> - int name_len = 0;
>
> if (!dev || !name || !ops)
> return NULL;
>
> - if (!firmware)
> - /*
> - * Make room for default firmware name (minus %s plus '\0').
> - * If the caller didn't pass in a firmware name then
> - * construct a default name. We're already glomming 'len'
> - * bytes onto the end of the struct rproc allocation, so do
> - * a few more for the default firmware name (but only if
> - * the caller doesn't pass one).
> - */
> - name_len = strlen(name) + strlen(template) - 2 + 1;
> -
> - rproc = kzalloc(sizeof(struct rproc) + len + name_len, GFP_KERNEL);
> + rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL);
> if (!rproc)
> return NULL;
>
> - if (!firmware) {
> - p = (char *)rproc + sizeof(struct rproc) + len;
> - snprintf(p, name_len, template, name);
> - } else {
> - p = (char *)firmware;
> - }
> -
> - rproc->firmware = p;
> rproc->name = name;
> rproc->ops = ops;
> rproc->priv = &rproc[1];
> @@ -1389,6 +1404,11 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>
> dev_set_name(&rproc->dev, "remoteproc%d", rproc->index);
>
> + if (rproc_set_firmware_name(rproc, firmware)) {
> + put_device(&rproc->dev);
> + return NULL;
> + }
> +
> atomic_set(&rproc->power, 0);
>
> /* Set ELF as the default fw_ops handler */
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 1c457a8dd5a6..7a6f9ad55011 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -42,6 +42,8 @@
> #include <linux/idr.h>
> #include <linux/of.h>
>
> +#define RPROC_MAX_FIRMWARE_NAME_LEN 128
> +
> /**
> * struct resource_table - firmware resource table header
> * @ver: version number
> @@ -416,7 +418,7 @@ struct rproc {
> struct list_head node;
> struct iommu_domain *domain;
> const char *name;
> - const char *firmware;
> + char firmware[RPROC_MAX_FIRMWARE_NAME_LEN];
> void *priv;
> const struct rproc_ops *ops;
> struct device dev;
>
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 <[email protected]>
>
> ---
>
> 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 <[email protected]>
> +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 <[email protected]>
> +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.
> +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);
> +}
>
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 <[email protected]>
>>
>> ---
>>
>> 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 <[email protected]>
>> +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 <[email protected]>
>> +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.
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);
>> +}
>>
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 <[email protected]>
>>>
>>> ---
>>>
>>> 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 <[email protected]>
>>> +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 <[email protected]>
>>> +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);
>>> +}
>>>
>
Hi Loic
On 13/10/16 15:39, loic pallardy wrote:
>
>
> 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 <[email protected]>
>>>>
>>>> ---
>>>>
>>>> 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 <[email protected]>
>>>> +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 <[email protected]>
>>>> +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
OK, no problem - that is probably neater :-)
Thanks,
Matt
>>
>> 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);
>>>> +}
>>>>
>>
Hi Loic,
On 13/10/16 14:22, loic pallardy wrote:
>
>
> On 10/11/2016 03:39 PM, Matt Redfearn wrote:
>> Storage of the firmware name was inconsistent, either storing a pointer
>> to a name stored with unknown ownership, or a variable length tacked
>> onto the end of the struct proc allocated in rproc_alloc.
>>
>> In preparation for allowing the firmware of an already allocated struct
>> rproc to be changed, the easiest way to allow reallocation of the name
>> is to switch to a fixed length buffer held as part of the struct rproc.
>> That way we can either copy the provided firmware name into it, or print
>> into it based on a name template. A new function,
>> rproc_set_firmware_name() is introduced for this purpose, and that logic
>> removed from rproc_alloc().
>>
>> Signed-off-by: Matt Redfearn <[email protected]>
>> ---
>>
>> drivers/remoteproc/remoteproc_core.c | 64
>> +++++++++++++++++++++++-------------
>> include/linux/remoteproc.h | 4 ++-
>> 2 files changed, 45 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c
>> b/drivers/remoteproc/remoteproc_core.c
>> index fe0539ed9cb5..48cd9d5afb69 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1309,6 +1309,42 @@ static void rproc_type_release(struct device
>> *dev)
>> kfree(rproc);
>> }
>>
>> +/**
>> + * rproc_set_firmware_name() - helper to create a valid firmare name
>> + * @rproc: remote processor
>> + * @firmware: name of firmware file, can be NULL
>> + *
>> + * If the caller didn't pass in a firmware name then construct a
>> default name,
>> + * otherwise the provided name is copied into the firmware field of
>> struct
>> + * rproc. If the name is too long to fit, -EINVAL is returned.
>> + *
>> + * Returns 0 on success and an appropriate error code otherwise.
>> + */
>> +static int rproc_set_firmware_name(struct rproc *rproc, const char
>> *firmware)
>> +{
>> + char *cp, *template = "rproc-%s-fw";
>> + int name_len;
>> +
>> + if (firmware) {
>> + name_len = strlen(firmware);
>> + cp = memchr(firmware, '\n', name_len);
>> + if (cp)
>> + name_len = cp - firmware;
>> +
>> + if (name_len > RPROC_MAX_FIRMWARE_NAME_LEN)
> Hi Matt,
>
> As you are added '\0' at offset name_len, name_len should be < to
> RPROC_MAX_FIRMWARE_NAME_LEN.
>
> Test should be if (name_len >= RPROC_MAX_FIRMWARE_NAME_LEN)
Yes - good spot, thanks :-)
Matt
>
> Regards,
> Loic
>> + return -EINVAL;
>> +
>> + strncpy(rproc->firmware, firmware, name_len);
>> + rproc->firmware[name_len] = '\0';
>> + } else {
>> + snprintf(rproc->firmware, RPROC_MAX_FIRMWARE_NAME_LEN,
>> + template, rproc->name);
>> + }
>> +
>> + dev_dbg(&rproc->dev, "Using firmware %s\n", rproc->firmware);
>> + return 0;
>> +}
>> +
>> static struct device_type rproc_type = {
>> .name = "remoteproc",
>> .release = rproc_type_release,
>> @@ -1342,35 +1378,14 @@ struct rproc *rproc_alloc(struct device *dev,
>> const char *name,
>> const char *firmware, int len)
>> {
>> struct rproc *rproc;
>> - char *p, *template = "rproc-%s-fw";
>> - int name_len = 0;
>>
>> if (!dev || !name || !ops)
>> return NULL;
>>
>> - if (!firmware)
>> - /*
>> - * Make room for default firmware name (minus %s plus '\0').
>> - * If the caller didn't pass in a firmware name then
>> - * construct a default name. We're already glomming 'len'
>> - * bytes onto the end of the struct rproc allocation, so do
>> - * a few more for the default firmware name (but only if
>> - * the caller doesn't pass one).
>> - */
>> - name_len = strlen(name) + strlen(template) - 2 + 1;
>> -
>> - rproc = kzalloc(sizeof(struct rproc) + len + name_len, GFP_KERNEL);
>> + rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL);
>> if (!rproc)
>> return NULL;
>>
>> - if (!firmware) {
>> - p = (char *)rproc + sizeof(struct rproc) + len;
>> - snprintf(p, name_len, template, name);
>> - } else {
>> - p = (char *)firmware;
>> - }
>> -
>> - rproc->firmware = p;
>> rproc->name = name;
>> rproc->ops = ops;
>> rproc->priv = &rproc[1];
>> @@ -1389,6 +1404,11 @@ struct rproc *rproc_alloc(struct device *dev,
>> const char *name,
>>
>> dev_set_name(&rproc->dev, "remoteproc%d", rproc->index);
>>
>> + if (rproc_set_firmware_name(rproc, firmware)) {
>> + put_device(&rproc->dev);
>> + return NULL;
>> + }
>> +
>> atomic_set(&rproc->power, 0);
>>
>> /* Set ELF as the default fw_ops handler */
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 1c457a8dd5a6..7a6f9ad55011 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -42,6 +42,8 @@
>> #include <linux/idr.h>
>> #include <linux/of.h>
>>
>> +#define RPROC_MAX_FIRMWARE_NAME_LEN 128
>> +
>> /**
>> * struct resource_table - firmware resource table header
>> * @ver: version number
>> @@ -416,7 +418,7 @@ struct rproc {
>> struct list_head node;
>> struct iommu_domain *domain;
>> const char *name;
>> - const char *firmware;
>> + char firmware[RPROC_MAX_FIRMWARE_NAME_LEN];
>> void *priv;
>> const struct rproc_ops *ops;
>> struct device dev;
>>
On Tue 11 Oct 06:39 PDT 2016, Matt Redfearn wrote:
> Storage of the firmware name was inconsistent, either storing a pointer
> to a name stored with unknown ownership, or a variable length tacked
> onto the end of the struct proc allocated in rproc_alloc.
>
Instead of using a statically sized array for "firmware", just keep it
a pointer to a local copy of the firmware name.
> In preparation for allowing the firmware of an already allocated struct
> rproc to be changed, the easiest way to allow reallocation of the name
> is to switch to a fixed length buffer held as part of the struct rproc.
> That way we can either copy the provided firmware name into it, or print
> into it based on a name template. A new function,
> rproc_set_firmware_name() is introduced for this purpose, and that logic
> removed from rproc_alloc().
>
> Signed-off-by: Matt Redfearn <[email protected]>
> ---
>
> drivers/remoteproc/remoteproc_core.c | 64 +++++++++++++++++++++++-------------
> include/linux/remoteproc.h | 4 ++-
> 2 files changed, 45 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index fe0539ed9cb5..48cd9d5afb69 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1309,6 +1309,42 @@ static void rproc_type_release(struct device *dev)
> kfree(rproc);
> }
>
> +/**
> + * rproc_set_firmware_name() - helper to create a valid firmare name
> + * @rproc: remote processor
> + * @firmware: name of firmware file, can be NULL
> + *
> + * If the caller didn't pass in a firmware name then construct a default name,
> + * otherwise the provided name is copied into the firmware field of struct
> + * rproc. If the name is too long to fit, -EINVAL is returned.
> + *
> + * Returns 0 on success and an appropriate error code otherwise.
> + */
> +static int rproc_set_firmware_name(struct rproc *rproc, const char *firmware)
That would turn this function into something like:
char *p, *template = "rproc-%s-fw";
if (!firmware) {
name_len = strlen(name) + strlen(template) - 2 + 1;
p = kmalloc(name_len, GFP_KERNEL);
if (!p)
return -ENOMEM;
sprintf(p, template, firmware);
} else {
p = kstrdup(firmware, GFP_KERNEL);
if (!p)
return -ENOMEM;
}
kfree(rproc->firmware);
rproc->firmware = p;
Like your version this leaves the firmware name unchanged in the case of
an error.
> +{
> + char *cp, *template = "rproc-%s-fw";
> + int name_len;
> +
> + if (firmware) {
> + name_len = strlen(firmware);
> + cp = memchr(firmware, '\n', name_len);
Let's just require the caller to pass us a valid name.
> + if (cp)
> + name_len = cp - firmware;
> +
> + if (name_len > RPROC_MAX_FIRMWARE_NAME_LEN)
> + return -EINVAL;
> +
> + strncpy(rproc->firmware, firmware, name_len);
> + rproc->firmware[name_len] = '\0';
> + } else {
> + snprintf(rproc->firmware, RPROC_MAX_FIRMWARE_NAME_LEN,
> + template, rproc->name);
> + }
> +
> + dev_dbg(&rproc->dev, "Using firmware %s\n", rproc->firmware);
> + return 0;
> +}
PS. Don't forget to free the string in rproc_type_release()
Regards,
Bjorn
> +
> static struct device_type rproc_type = {
> .name = "remoteproc",
> .release = rproc_type_release,
> @@ -1342,35 +1378,14 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
> const char *firmware, int len)
> {
> struct rproc *rproc;
> - char *p, *template = "rproc-%s-fw";
> - int name_len = 0;
>
> if (!dev || !name || !ops)
> return NULL;
>
> - if (!firmware)
> - /*
> - * Make room for default firmware name (minus %s plus '\0').
> - * If the caller didn't pass in a firmware name then
> - * construct a default name. We're already glomming 'len'
> - * bytes onto the end of the struct rproc allocation, so do
> - * a few more for the default firmware name (but only if
> - * the caller doesn't pass one).
> - */
> - name_len = strlen(name) + strlen(template) - 2 + 1;
> -
> - rproc = kzalloc(sizeof(struct rproc) + len + name_len, GFP_KERNEL);
> + rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL);
> if (!rproc)
> return NULL;
>
> - if (!firmware) {
> - p = (char *)rproc + sizeof(struct rproc) + len;
> - snprintf(p, name_len, template, name);
> - } else {
> - p = (char *)firmware;
> - }
> -
> - rproc->firmware = p;
> rproc->name = name;
> rproc->ops = ops;
> rproc->priv = &rproc[1];
> @@ -1389,6 +1404,11 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>
> dev_set_name(&rproc->dev, "remoteproc%d", rproc->index);
>
> + if (rproc_set_firmware_name(rproc, firmware)) {
> + put_device(&rproc->dev);
> + return NULL;
> + }
> +
> atomic_set(&rproc->power, 0);
>
> /* Set ELF as the default fw_ops handler */
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 1c457a8dd5a6..7a6f9ad55011 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -42,6 +42,8 @@
> #include <linux/idr.h>
> #include <linux/of.h>
>
> +#define RPROC_MAX_FIRMWARE_NAME_LEN 128
> +
> /**
> * struct resource_table - firmware resource table header
> * @ver: version number
> @@ -416,7 +418,7 @@ struct rproc {
> struct list_head node;
> struct iommu_domain *domain;
> const char *name;
> - const char *firmware;
> + char firmware[RPROC_MAX_FIRMWARE_NAME_LEN];
> void *priv;
> const struct rproc_ops *ops;
> struct device dev;
> --
> 2.7.4
>
On Tue 11 Oct 06:39 PDT 2016, Matt Redfearn wrote:
> It is often desirable to be able to change the running firmware on a
> remote processor dynamically. This used to require a complete
> destruction and readdition of the struct rproc, but now that the
> firmware name is fixed length, it can be updated freely.
>
> So long as the remote processor is in RPROC_OFFLINE state,
> rproc_change_firmware() will free resources from the previous firmware
> and request a new one be loaded.
>
> Signed-off-by: Matt Redfearn <[email protected]>
> ---
>
> drivers/remoteproc/remoteproc_core.c | 62 ++++++++++++++++++++++++++++++++
> drivers/remoteproc/remoteproc_internal.h | 1 +
> 2 files changed, 63 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 48cd9d5afb69..2152b484f314 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1345,6 +1345,68 @@ static int rproc_set_firmware_name(struct rproc *rproc, const char *firmware)
> return 0;
> }
>
> +/**
> + * rproc_change_firmware() - change the loaded firmware on a remote processor
> + * @rproc: remote processor
> + * @firmware: name of firmware file to load, can be NULL
> + *
> + * Attempts to change the firmware loaded for a remote processor. The processor
> + * must be in RPROC_OFFLINE state.
> + *
> + * Any allocated resources for the exiting firmware are released, the new
> + * firmware name is set and then any virtio devices probed.
> + *
> + * Returns 0 on success and an appropriate error code otherwise.
> + *
> + * Note: this function initiates an asynchronous firmware loading
> + * context, which will look for virtio devices supported by the rproc's
> + * firmware.
> + *
> + * If found, those virtio devices will be created and added, so as a result
> + * of registering this remote processor, additional virtio drivers might be
> + * probed.
> + */
> +int rproc_change_firmware(struct rproc *rproc, const char *firmware)
> +{
> + struct device *dev = &rproc->dev;
> + struct rproc_vdev *rvdev, *rvtmp;
> + int ret;
> +
> + ret = mutex_lock_interruptible(&rproc->lock);
> + if (ret) {
> + dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
> + return -EINVAL;
> + }
> +
> + if (rproc->state != RPROC_OFFLINE) {
> + dev_err(dev, "can't change firmware while running\n");
> + ret = -EBUSY;
> + goto out;
> + }
> +
> + /* Wait for any pending firmware load */
> + wait_for_completion(&rproc->firmware_loading_complete);
> +
> + /* clean up all acquired resources */
> + rproc_resource_cleanup(rproc);
> +
> + /* clean up remote vdev entries */
> + list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node)
> + rproc_remove_virtio_dev(rvdev);
> +
> + /* Free the copy of the resource table */
> + kfree(rproc->cached_table);
As of v4.9-rc1 these resources will not be allocated if state ==
RPROC_OFFLINE, so you can reduce this to take the lock, check the state
and set the firmware name.
> +
> + ret = rproc_set_firmware_name(rproc, firmware);
> + if (ret)
> + goto out;
> +
> + ret = rproc_add_virtio_devices(rproc);
> +out:
> + mutex_unlock(&rproc->lock);
> + return ret;
> +}
> +
> static struct device_type rproc_type = {
> .name = "remoteproc",
> .release = rproc_type_release,
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 57e1de59bec8..837faf2677a6 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -49,6 +49,7 @@ struct rproc_fw_ops {
> void rproc_release(struct kref *kref);
> irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
> int rproc_boot_nowait(struct rproc *rproc);
> +int rproc_change_firmware(struct rproc *rproc, const char *firmware);
>
> /* from remoteproc_virtio.c */
> int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id);
> --
> 2.7.4
>
On Tue 11 Oct 06:39 PDT 2016, 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 <[email protected]>
>
> ---
>
> 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 <[email protected]>
> +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 <[email protected]>
> +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;
> +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");
Per current and future logic, rproc->firmware can't be NULL.
> +}
> +
> +/* 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);
Rather than introducing rproc_set_firmware_name() and
rproc_change_firmware() - which does not have a use case beyond this
caller you should be able to keep rproc_alloc() more or less as is, and
then inline a simplified version (as we don't have to deal with the
templated case here).
> + return err ? err : count;
> +}
> +static DEVICE_ATTR_RW(firmware);
> +
Extra newline...
> +
> +/*
> + * 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);
The documentation above does not describe the "($state)" part, which I'm
fine leaving out. We know the strings are below PAGE_SIZE, so you can
use sprintf().
> +}
> +
> +/* 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)) {
Please use sysfs_streq() instead, it will handle the \n for you.
> + 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 = {
> + .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);
> +}
Besides these nits, I think this looks good.
Thanks,
Bjorn
On Thu 13 Oct 07:39 PDT 2016, loic pallardy wrote:
>
>
> 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:
[..]
> >>>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
>
Compare:
rproc->dev.class = &rproc_class;
to:
rproc_register_sysfs(rproc);
The meaning of the prior is obvious, the latter require the jump to a
different file to follow. And we would only be trading one global
variable for a global function.
A viable alternative would be to keep a local pointer to the class in
core.c, that we assign during the call to rproc_init_sysfs().
But I'm fine with the way it's written.
Regards,
Bjorn