Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932915AbcJMO0Y (ORCPT ); Thu, 13 Oct 2016 10:26:24 -0400 Received: from mailapp02.imgtec.com ([217.156.133.132]:39391 "EHLO mailapp01.imgtec.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932631AbcJMO0F (ORCPT ); Thu, 13 Oct 2016 10:26:05 -0400 Subject: Re: [PATCH 3/4] remoteproc: Add a sysfs interface for firmware and state To: loic pallardy , Bjorn Andersson , Ohad Ben-Cohen References: <1476193185-32107-1-git-send-email-matt.redfearn@imgtec.com> <1476193185-32107-4-git-send-email-matt.redfearn@imgtec.com> CC: , From: Matt Redfearn Message-ID: Date: Thu, 13 Oct 2016 15:25:54 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.150.130.83] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10336 Lines: 309 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 >> >> --- >> >> 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 >> +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 >> +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 >> + >> +#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); >> +} >>