| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAL_JsqKJqakXGWDEKPAoJ9P5myYXnodcYCdvowV6MPuZXgwVmw@mail.gmail.com>
Date: Fri, 21 Mar 2014 08:39:29 -0500
From: Rob Herring <robherring2@...il.com>
To: Pantelis Antoniou <pantelis.antoniou@...sulko.com>
Cc: Grant Likely <grant.likely@...retlab.ca>,
Stephen Warren <swarren@...dotorg.org>,
Matt Porter <matt.porter@...aro.org>,
Koen Kooi <koen@...inion.thruhere.net>,
Alison Chaiken <Alison_Chaiken@...tor.com>,
Dinh Nguyen <dinh.linux@...il.com>,
Jan Lubbe <jluebbe@...net.de>,
Alexander Sverdlin <alexander.sverdlin@....com>,
Michael Stickel <ms@...able.de>,
Guenter Roeck <linux@...ck-us.net>,
Dirk Behme <dirk.behme@...il.com>,
Alan Tull <delicious.quinoa@...il.com>,
Sascha Hauer <s.hauer@...gutronix.de>,
Michael Bohan <mbohan@...eaurora.org>,
Ionut Nicu <ioan.nicu.ext@....com>,
Michal Simek <monstr@...str.eu>,
Matt Ranostay <mranostay@...il.com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Pete Popov <pete.popov@...sulko.com>,
Dan Malek <dan.malek@...sulko.com>,
Georgi Vlaev <georgi.vlaev@...sulko.com>
Subject: Re: [PATCH v3 3/7] OF: DT-Overlay configfs interface
On Tue, Mar 18, 2014 at 4:56 PM, Pantelis Antoniou
<pantelis.antoniou@...sulko.com> wrote:
> Add a runtime interface to using configfs for generic device tree overlay
> usage.
>
> A device-tree configfs entry is created in /config/device-tree/overlays
>
> To create an overlay you mkdir the directory and then echo the overlay
> firmware file to the path property file.
>
> # mkdir /config/device-tree/overlays/foo
> # echo foo.dtbo >/config/device-tree/overlays/foo/path
The purpose of 'path' and what determines its name is not really
clear. The documentation should be clear enough that a user knowing no
details of overlays can be handed a dtbo file and pointer to the
documentation and they can apply it without further information.
> The overlay file will be loaded using the standard firmware loader
> and will be applied.
>
> To remove it simply rmdir the directory.
>
> # rmdir /config/device-tree/overlays/foo
This description should be put into a file in Documentation/. Probably
Documentation/devicetree/ since it is a kernel interface and not a
bindings.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@...sulko.com>
> ---
> drivers/of/Kconfig | 5 +
> drivers/of/Makefile | 1 +
> drivers/of/configfs.c | 272 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 278 insertions(+)
> create mode 100644 drivers/of/configfs.c
>
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 7517be2..fc0e3ec 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -77,6 +77,10 @@ config OF_RESERVED_MEM
> help
> Helpers to allow for reservation of memory regions
>
> +config OF_CONFIGFS
> + select CONFIGFS_FS
> + def_bool n
Is it feasible to make this a module (perhaps all of the overlay support)?
> +
> config OF_RESOLVE
> bool "OF Dynamic resolution support"
> depends on OF
> @@ -92,6 +96,7 @@ config OF_OVERLAY
> select OF_DYNAMIC
> select OF_DEVICE
> select OF_RESOLVE
> + select OF_CONFIGFS
> help
> OpenFirmware overlay support. Allows you to modify on runtime the
> live tree using overlays.
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index d2a6e0d..4efa17b 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -12,3 +12,4 @@ obj-$(CONFIG_OF_MTD) += of_mtd.o
> obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
> obj-$(CONFIG_OF_RESOLVE) += resolver.o
> obj-$(CONFIG_OF_OVERLAY) += overlay.o
> +obj-$(CONFIG_OF_CONFIGFS) += configfs.o
> diff --git a/drivers/of/configfs.c b/drivers/of/configfs.c
> new file mode 100644
> index 0000000..a494643
> --- /dev/null
> +++ b/drivers/of/configfs.c
> @@ -0,0 +1,272 @@
> +/*
> + * Configfs entries for device-tree
> + *
> + * Copyright (C) 2013 - Pantelis Antoniou <panto@...oniou-consulting.com>
It's 2014 now. I assume you have made some changes in 2014.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +#include <linux/ctype.h>
> +#include <linux/cpu.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/spinlock.h>
> +#include <linux/slab.h>
> +#include <linux/proc_fs.h>
> +#include <linux/configfs.h>
> +#include <linux/types.h>
> +#include <linux/stat.h>
> +#include <linux/limits.h>
> +#include <linux/file.h>
> +#include <linux/vmalloc.h>
> +#include <linux/firmware.h>
> +
> +#include "of_private.h"
> +
> +#ifdef CONFIG_OF_OVERLAY
This file is only compiled with this defined, so it isn't needed.
> +
> +struct cfs_overlay_item {
> + struct config_item item;
> +
> + char path[PATH_MAX];
> +
Remove the blank lines.
> + const struct firmware *fw;
> + struct device_node *overlay;
> + int ovinfo_cnt;
> + struct of_overlay_info *ovinfo;
> + unsigned int applied : 1;
Do you plan to expand this? If not, just use bool.
> +};
> +
> +static inline struct cfs_overlay_item *to_cfs_overlay_item(struct config_item *item)
> +{
> + return item ? container_of(item, struct cfs_overlay_item, item) : NULL;
> +}
> +
> +CONFIGFS_ATTR_STRUCT(cfs_overlay_item);
> +#define CFS_OVERLAY_ITEM_ATTR(_name, _mode, _show, _store) \
> +struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
> + __CONFIGFS_ATTR(_name, _mode, _show, _store)
> +#define CFS_OVERLAY_ITEM_ATTR_RO(_name, _show) \
> +struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
> + __CONFIGFS_ATTR_RO(_name, _show)
> +
> +static ssize_t cfs_overlay_item_path_show(struct cfs_overlay_item *overlay,
> + char *page)
> +{
> + return sprintf(page, "%s\n", overlay->path);
> +}
> +
> +static ssize_t cfs_overlay_item_path_store(struct cfs_overlay_item *overlay,
> + const char *page, size_t count)
> +{
> + const char *p = page;
> + char *s;
> + int err;
> +
> + /* if it's set do not allow changes */
> + if (overlay->path[0] != '\0')
> + return -EPERM;
> +
> + /* copy to path buffer (and make sure it's always zero terminated */
> + count = snprintf(overlay->path, sizeof(overlay->path) - 1, "%s", p);
> + overlay->path[sizeof(overlay->path) - 1] = '\0';
> +
> + /* strip trailing newlines */
> + s = overlay->path + strlen(overlay->path);
> + while (s > overlay->path && *--s == '\n')
> + *s = '\0';
Seems like this would be common pattern. There are no helpers to do this?
> +
> + pr_debug("%s: path is '%s'\n", __func__, overlay->path);
> +
> + err = request_firmware(&overlay->fw, overlay->path, NULL);
> + if (err != 0)
> + goto out_err;
> +
> + /* unflatten the tree */
> + of_fdt_unflatten_tree((void *)overlay->fw->data, &overlay->overlay);
> + if (overlay->overlay == NULL) {
"if (!overlay->overlay)" is preferred.
overlay->node would be better naming.
> + pr_err("%s: failed to unflatten tree\n", __func__);
> + err = -EINVAL;
> + goto out_err;
> + }
> + pr_debug("%s: unflattened OK\n", __func__);
> +
> + /* mark it as detached */
> + of_node_set_flag(overlay->overlay, OF_DETACHED);
> +
> + /* perform resolution */
> + err = of_resolve(overlay->overlay);
> + if (err != 0) {
> + pr_err("%s: Failed to resolve tree\n", __func__);
> + goto out_err;
> + }
> + pr_debug("%s: resolved OK\n", __func__);
> +
> + /* now build an overlay info array */
> + err = of_build_overlay_info(overlay->overlay,
> + &overlay->ovinfo_cnt, &overlay->ovinfo);
> + if (err != 0) {
> + pr_err("%s: Failed to build overlay info\n", __func__);
> + goto out_err;
> + }
> +
> + pr_debug("%s: built %d overlay segments\n", __func__,
> + overlay->ovinfo_cnt);
> +
> + err = of_overlay(overlay->ovinfo_cnt, overlay->ovinfo);
This really applies to the previous patches, but there's not much
consistency in the function naming. It is not clear what of_overlay
does.
I would prefix all funcitons of_overlay_* and call this one
of_overlay_apply (or finalize?).
> + if (err != 0) {
> + pr_err("%s: Failed to apply overlay\n", __func__);
> + goto out_err;
> + }
> +
> + overlay->applied = 1;
> +
> + pr_debug("%s: Applied #%d overlay segments\n", __func__,
> + overlay->ovinfo_cnt);
> +
> + return count;
> +
> +out_err:
> + if (overlay->applied)
> + of_overlay_revert(overlay->ovinfo_cnt, overlay->ovinfo);
> + overlay->applied = 0;
> +
> + if (overlay->ovinfo)
> + of_free_overlay_info(overlay->ovinfo_cnt, overlay->ovinfo);
> + overlay->ovinfo = NULL;
> + overlay->ovinfo_cnt = 0;
> +
> + release_firmware(overlay->fw);
This needs some clean-up with multiple jump labels. You shouldn't need
the if statements and you cal release_firmware on request_firmware
error.
> + overlay->fw = NULL;
> +
> + overlay->path[0] = '\0';
> + return err;
> +}
> +
> +static ssize_t cfs_overlay_item_status_show(struct cfs_overlay_item *overlay,
> + char *page)
> +{
> + return sprintf(page, "%s\n",
> + overlay->applied ? "applied" : "unapplied");
> +}
This needs to be added to the above mentioned documentation along with
any other files.
> +
> +CFS_OVERLAY_ITEM_ATTR(path, S_IRUGO | S_IWUSR, \
> + cfs_overlay_item_path_show, cfs_overlay_item_path_store);
> +CFS_OVERLAY_ITEM_ATTR_RO(status, cfs_overlay_item_status_show);
> +
> +static struct configfs_attribute *cfs_overlay_attrs[] = {
> + &cfs_overlay_item_attr_path.attr,
> + &cfs_overlay_item_attr_status.attr,
> + NULL,
> +};
> +
> +static void cfs_overlay_release(struct config_item *item)
> +{
> + struct cfs_overlay_item *overlay = to_cfs_overlay_item(item);
> +
> + if (overlay->applied)
> + of_overlay_revert(overlay->ovinfo_cnt, overlay->ovinfo);
> + if (overlay->ovinfo)
> + of_free_overlay_info(overlay->ovinfo_cnt, overlay->ovinfo);
> + if (overlay->fw)
> + release_firmware(overlay->fw);
> + kfree(overlay);
> +}
> +
> +CONFIGFS_ATTR_OPS(cfs_overlay_item);
> +static struct configfs_item_operations cfs_overlay_item_ops = {
> + .release = cfs_overlay_release,
> + .show_attribute = cfs_overlay_item_attr_show,
> + .store_attribute = cfs_overlay_item_attr_store,
> +};
> +
> +static struct config_item_type cfs_overlay_type = {
> + .ct_item_ops = &cfs_overlay_item_ops,
> + .ct_attrs = cfs_overlay_attrs,
> + .ct_owner = THIS_MODULE,
> +};
> +
> +static struct config_item *cfs_overlay_group_make_item(struct config_group *group, const char *name)
> +{
> + struct cfs_overlay_item *overlay;
> +
> + overlay = kzalloc(sizeof(*overlay), GFP_KERNEL);
> + if (!overlay)
> + return ERR_PTR(-ENOMEM);
> +
> + config_item_init_type_name(&overlay->item, name, &cfs_overlay_type);
> + return &overlay->item;
> +}
> +
> +static void cfs_overlay_group_drop_item(struct config_group *group, struct config_item *item)
> +{
> + struct cfs_overlay_item *overlay = to_cfs_overlay_item(item);
> +
> + config_item_put(&overlay->item);
> +}
> +
> +static struct configfs_group_operations overlays_ops = {
> + .make_item = cfs_overlay_group_make_item,
> + .drop_item = cfs_overlay_group_drop_item,
> +};
> +
> +static struct config_item_type overlays_type = {
> + .ct_group_ops = &overlays_ops,
> + .ct_owner = THIS_MODULE,
> +};
> +
> +#endif /* CONFIG_OF_OVERLAY */
> +
> +static struct configfs_group_operations of_cfs_ops = {
> + /* empty - we don't allow anything to be created */
> +};
> +
> +static struct config_item_type of_cfs_type = {
> + .ct_group_ops = &of_cfs_ops,
> + .ct_owner = THIS_MODULE,
> +};
> +
> +struct config_group of_cfs_overlay_group;
> +
> +struct config_group *of_cfs_def_groups[] = {
> +#ifdef CONFIG_OF_OVERLAY
> + &of_cfs_overlay_group,
> +#endif
> + NULL
> +};
> +
> +static struct configfs_subsystem of_cfs_subsys = {
> + .su_group = {
> + .cg_item = {
> + .ci_namebuf = "device-tree",
> + .ci_type = &of_cfs_type,
> + },
> + .default_groups = of_cfs_def_groups,
> + },
> + .su_mutex = __MUTEX_INITIALIZER(of_cfs_subsys.su_mutex),
> +};
> +
> +static int __init of_cfs_init(void)
> +{
> + int ret;
> +
> + pr_info("%s\n", __func__);
> +
> + config_group_init(&of_cfs_subsys.su_group);
> +#ifdef CONFIG_OF_OVERLAY
> + config_group_init_type_name(&of_cfs_overlay_group, "overlays", &overlays_type);
> +#endif
> +
> + ret = configfs_register_subsystem(&of_cfs_subsys);
> + if (ret != 0) {
> + pr_err("%s: failed to register subsys\n", __func__);
> + goto out;
> + }
> + pr_info("%s: OK\n", __func__);
These pr_info prints are not necessary you can use initcall_debug.
> +out:
> + return ret;
> +}
> +late_initcall(of_cfs_init);
Does this really need to be late_initcall?
Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists