Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1502140 > unrolled thread

[PATCH v2 0/4] remoteproc: Add sysfs interface

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/4] remoteproc: Add sysfs interface Matt Redfearn <matt.redfearn@imgtec.com> - 2016-10-17 18:00 +0200
    [PATCH v2 3/4] remoteproc: Add a sysfs interface for firmware and state Matt Redfearn <matt.redfearn@imgtec.com> - 2016-10-17 18:00 +0200
      Re: [PATCH v2 3/4] remoteproc: Add a sysfs interface for firmware  and state Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-10-19 00:20 +0200
        Re: [PATCH v2 3/4] remoteproc: Add a sysfs interface for firmware and  state Matt Redfearn <matt.redfearn@imgtec.com> - 2016-10-19 17:00 +0200
    [PATCH v2 2/4] remoteproc: Make rproc_add_virtio_devices non-static Matt Redfearn <matt.redfearn@imgtec.com> - 2016-10-17 18:00 +0200

#1502140 — [PATCH v2 0/4] remoteproc: Add sysfs interface

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, then that is
wired up to the new sysfs interface.

This series is based on v4.9-rc1


Changes in v2:
Use a kmalloc'd copy of the firmware name instead of having a fixed
length array in struct rproc
New patch to allow access to firmware loading from remoteproc_sysfs.c
Have firmware_store perform the necessary steps inline.
Use sysfs_streq when dealing with writes to sysfs files

Matt Redfearn (4):
  remoteproc: Keep local copy of firmware name
  remoteproc: Make rproc_add_virtio_devices non-static
  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             |  36 +++---
 drivers/remoteproc/remoteproc_debugfs.c          |  71 -----------
 drivers/remoteproc/remoteproc_internal.h         |   6 +
 drivers/remoteproc/remoteproc_sysfs.c            | 155 +++++++++++++++++++++++
 include/linux/remoteproc.h                       |   2 +-
 7 files changed, 233 insertions(+), 88 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-remoteproc
 create mode 100644 drivers/remoteproc/remoteproc_sysfs.c

-- 
2.7.4

[toc] | [next] | [standalone]

#1502146 — [PATCH v2 3/4] remoteproc: Add a sysfs interface for firmware and state

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@imgtec.com>

---

Changes in v2:
Have firmware_store perform the necessary steps inline.
Use sysfs_streq when dealing with writes to sysfs files

 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            | 155 +++++++++++++++++++++++
 5 files changed, 214 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@imgtec.com>
+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@imgtec.com>
+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 6dfb62ed643f..6da9b12f9798 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 d86b4be956a9..e302036d29e7 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1347,6 +1347,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);
@@ -1485,6 +1486,7 @@ EXPORT_SYMBOL(rproc_report_crash);
 
 static int __init remoteproc_init(void)
 {
+	rproc_init_sysfs();
 	rproc_init_debugfs();
 
 	return 0;
@@ -1496,6 +1498,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 82345930ecbd..fce736251a7f 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..afa9ca040a7e
--- /dev/null
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -0,0 +1,155 @@
+/*
+ * 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);
+
+	return sprintf(buf, "%s\n", rproc->firmware);
+}
+
+/* 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);
+	char *p;
+	int err, len = count;
+
+	err = mutex_lock_interruptible(&rproc->lock);
+	if (err) {
+		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
+		return -EINVAL;
+	}
+
+	if (rproc->state != RPROC_OFFLINE) {
+		dev_err(dev, "can't change firmware while running\n");
+		err = -EBUSY;
+		goto out;
+	}
+
+	p = memchr(buf, '\n', count);
+	if (p)
+		len = p - buf;
+
+	p = kstrndup(buf, len, GFP_KERNEL);
+	if (!p) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	kfree(rproc->firmware);
+	rproc->firmware = p;
+
+	err = rproc_add_virtio_devices(rproc);
+out:
+	mutex_unlock(&rproc->lock);
+
+	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 sprintf(buf, "%s\n", rproc_state_string[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;
+
+	if (sysfs_streq(buf, "start")) {
+		if (rproc->state == RPROC_RUNNING)
+			return -EBUSY;
+
+		ret = rproc_boot(rproc);
+		if (ret)
+			dev_err(&rproc->dev, "Boot failed: %d\n", ret);
+	} else if (sysfs_streq(buf, "stop")) {
+		if (rproc->state != RPROC_RUNNING)
+			return -EINVAL;
+
+		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 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

[toc] | [prev] | [next] | [standalone]

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

On Mon 17 Oct 08:49 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 <matt.redfearn@imgtec.com>
> 
> ---
> 
> Changes in v2:
> Have firmware_store perform the necessary steps inline.
> Use sysfs_streq when dealing with writes to sysfs files
> 
[..]
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> new file mode 100644
> index 000000000000..afa9ca040a7e
> --- /dev/null
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -0,0 +1,155 @@
> +/*
> + * 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);
> +
> +	return sprintf(buf, "%s\n", rproc->firmware);
> +}
> +
> +/* 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);
> +	char *p;
> +	int err, len = count;
> +
> +	err = mutex_lock_interruptible(&rproc->lock);
> +	if (err) {
> +		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
> +		return -EINVAL;
> +	}
> +
> +	if (rproc->state != RPROC_OFFLINE) {
> +		dev_err(dev, "can't change firmware while running\n");
> +		err = -EBUSY;
> +		goto out;
> +	}
> +
> +	p = memchr(buf, '\n', count);
> +	if (p)
> +		len = p - buf;

I prefer the following variant:

len = strcspn(buf, '\n');

> +
> +	p = kstrndup(buf, len, GFP_KERNEL);
> +	if (!p) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
> +
> +	kfree(rproc->firmware);
> +	rproc->firmware = p;
> +
> +	err = rproc_add_virtio_devices(rproc);

As of v4.9 rproc_add_virtio_devices() will only check to see if the
rproc driver has auto_boot set and if so boot the core. (Yes it needs a
new name)

Also I'm not sure if it's valid to provide a sysfs API like this and
sometimes setting firmware results in rproc_boot() and sometimes not...

So, just drop patch 2 in this series and drop the call to
rproc_add_virtio_devices() here.

> +out:
> +	mutex_unlock(&rproc->lock);
> +
> +	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",

Please be overly explicit here and make these:

	[RPROC_OFFLINE] = "offline",

> +	"suspended",
> +	"running",
> +	"crashed",
> +	"invalid",
> +};
> +

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

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

Hi Bjorn,


On 18/10/16 23:16, Bjorn Andersson wrote:
> On Mon 17 Oct 08:49 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 <matt.redfearn@imgtec.com>
>>
>> ---
>>
>> Changes in v2:
>> Have firmware_store perform the necessary steps inline.
>> Use sysfs_streq when dealing with writes to sysfs files
>>
> [..]
>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
>> new file mode 100644
>> index 000000000000..afa9ca040a7e
>> --- /dev/null
>> +++ b/drivers/remoteproc/remoteproc_sysfs.c
>> @@ -0,0 +1,155 @@
>> +/*
>> + * 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);
>> +
>> +	return sprintf(buf, "%s\n", rproc->firmware);
>> +}
>> +
>> +/* 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);
>> +	char *p;
>> +	int err, len = count;
>> +
>> +	err = mutex_lock_interruptible(&rproc->lock);
>> +	if (err) {
>> +		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (rproc->state != RPROC_OFFLINE) {
>> +		dev_err(dev, "can't change firmware while running\n");
>> +		err = -EBUSY;
>> +		goto out;
>> +	}
>> +
>> +	p = memchr(buf, '\n', count);
>> +	if (p)
>> +		len = p - buf;
> I prefer the following variant:
>
> len = strcspn(buf, '\n');

Yes - that's neater.

>
>> +
>> +	p = kstrndup(buf, len, GFP_KERNEL);
>> +	if (!p) {
>> +		err = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	kfree(rproc->firmware);
>> +	rproc->firmware = p;
>> +
>> +	err = rproc_add_virtio_devices(rproc);
> As of v4.9 rproc_add_virtio_devices() will only check to see if the
> rproc driver has auto_boot set and if so boot the core. (Yes it needs a
> new name)
>
> Also I'm not sure if it's valid to provide a sysfs API like this and
> sometimes setting firmware results in rproc_boot() and sometimes not...
>
> So, just drop patch 2 in this series and drop the call to
> rproc_add_virtio_devices() here.

Makes sense, and it's more consistent behavior with that change.

>
>> +out:
>> +	mutex_unlock(&rproc->lock);
>> +
>> +	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",
> Please be overly explicit here and make these:
>
> 	[RPROC_OFFLINE] = "offline",

No problem.

Thanks,
Matt

>
>> +	"suspended",
>> +	"running",
>> +	"crashed",
>> +	"invalid",
>> +};
>> +
> Regards,
> Bjorn

[toc] | [prev] | [next] | [standalone]

#1502148 — [PATCH v2 2/4] remoteproc: Make rproc_add_virtio_devices non-static

This function will be required in pther files by the next patch in the
series.

Signed-off-by: Matt Redfearn <matt.redfearn@imgtec.com>
---

Changes in v2:
New patch to allow access to firmware loading from remoteproc_sysfs.c

 drivers/remoteproc/remoteproc_core.c     | 2 +-
 drivers/remoteproc/remoteproc_internal.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index ccc2a73e94dd..d86b4be956a9 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -916,7 +916,7 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 	complete_all(&rproc->firmware_loading_complete);
 }
 
-static int rproc_add_virtio_devices(struct rproc *rproc)
+int rproc_add_virtio_devices(struct rproc *rproc)
 {
 	int ret;
 
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 4cf93ca2816e..82345930ecbd 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_add_virtio_devices(struct rproc *rproc);
 
 /* from remoteproc_virtio.c */
 int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id);
-- 
2.7.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web