All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] RFC: Device overlay manager (PCI/USB + DT)
@ 2015-06-12 20:04 Pantelis Antoniou
  2015-06-15 14:43   ` Paul Bolle
  0 siblings, 1 reply; 4+ messages in thread
From: Pantelis Antoniou @ 2015-06-12 20:04 UTC (permalink / raw)
  To: Rob Herring
  Cc: Grant Likely, Matt Porter, Koen Kooi, Guenter Roeck, devicetree,
	linux-kernel, Pantelis Antoniou, Pantelis Antoniou

This probably misplaced (in drivers/misc) patch allows use
of device tree overlays on the two kinds of probeable busses
that count nowadays, PCI & USB.

It does so by dynamically creating device nodes for the busses &
devices that are probed and according to user-configuration applying
an overlay when they appear.

It is still a WIP but it's coming along nicely.

Issues: Only PCI works for now, the generated bindings are not
correct according to the openfirmware spec.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/misc/Kconfig    |    9 +
 drivers/misc/Makefile   |    1 +
 drivers/misc/devovmgr.c | 1305 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 1315 insertions(+)
 create mode 100644 drivers/misc/devovmgr.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index f9e09e1..c9577d6 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -525,6 +525,15 @@ config BONE_CAPEMGR
 	  Say Y here to include support for automatic loading of
 	  beaglebone capes.
 
+config DEV_OVERLAYMGR
+	tristate "Device overlay manager"
+	depends on OF
+	select OF_OVERLAY
+	default n
+	help
+	  Say Y here to include support for the automagical dev
+	  overlay manager.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 659b78b..7009f26 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -57,3 +57,4 @@ obj-$(CONFIG_ECHO)		+= echo/
 obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE)		+= cxl/
 obj-$(CONFIG_BONE_CAPEMGR)	+= bone_capemgr.o
+obj-$(CONFIG_DEV_OVERLAYMGR)	+= devovmgr.o
diff --git a/drivers/misc/devovmgr.c b/drivers/misc/devovmgr.c
new file mode 100644
index 0000000..82d2b07
--- /dev/null
+++ b/drivers/misc/devovmgr.c
@@ -0,0 +1,1305 @@
+/*
+ * Device overlay manager
+ *
+ * Copyright (C) 2015 Konsulko Group
+ * Pantelis Antoniou <pantelis.antoniou@konsulko.com>
+ *
+ * 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/sizes.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 <linux/pci.h>
+#include <linux/usb.h>
+#include <linux/mod_devicetable.h>
+#include <linux/workqueue.h>
+#include <linux/firmware.h>
+
+enum dovmgr_type {
+	ITEM_PCI,
+	ITEM_USB
+};
+
+struct dovmgr_item;
+
+struct dovmgr_dev_item {
+	struct dovmgr_item *item;
+	struct list_head node;
+	struct device *dev;
+	const struct firmware *fw;
+	struct device_node *overlay;
+	int overlay_id;
+	struct work_struct work;
+};
+
+struct dovmgr_item {
+	struct config_item item;
+	char *path;
+	bool enable;
+	char *overlay_name;
+	struct mutex dev_item_mutex;
+	struct list_head dev_item_list;
+	enum dovmgr_type type;
+	union {
+		struct pci_device_id pci;
+		struct usb_device_id usb;
+	};
+};
+
+struct config_group dovmgr_pci_group;
+struct config_group dovmgr_usb_group;
+
+static inline struct dovmgr_item *to_dovmgr_item(struct config_item *cfsitem)
+{
+	if (!cfsitem)
+		return NULL;
+
+	return container_of(cfsitem, struct dovmgr_item, item);
+}
+
+static int dovmgr_notifier_action(struct config_group *group,
+		unsigned long action, struct device *dev,
+		int (*do_match)(struct dovmgr_item *item, struct device *dev),
+		int (*do_action)(struct dovmgr_item *item, unsigned long action,
+			struct device *dev))
+{
+	struct config_item *cfsitem;
+	struct dovmgr_item *item;
+	int ret;
+
+	/* only handle device notifiers */
+	if (action != BUS_NOTIFY_ADD_DEVICE &&
+		action != BUS_NOTIFY_DEL_DEVICE &&
+		action != BUS_NOTIFY_REMOVED_DEVICE)
+		return 0;
+
+	ret = 0;
+	mutex_lock(&group->cg_subsys->su_mutex);
+	list_for_each_entry(cfsitem, &group->cg_children, ci_entry) {
+		item = to_dovmgr_item(cfsitem);
+		if (!item->enable || !(*do_match)(item, dev))
+			continue;
+
+		ret = (*do_action)(item, action, dev);
+		if (ret != 0)
+			break;
+	}
+	mutex_unlock(&group->cg_subsys->su_mutex);
+	return ret;
+}
+
+#if IS_ENABLED(CONFIG_PCI)
+/* copy of drivers/pci/pci.h */
+static inline const struct pci_device_id *
+pci_match_one_device(const struct pci_device_id *id, const struct pci_dev *dev)
+{
+	if ((id->vendor == PCI_ANY_ID || id->vendor == dev->vendor) &&
+	    (id->device == PCI_ANY_ID || id->device == dev->device) &&
+	    (id->subvendor == PCI_ANY_ID ||
+		id->subvendor == dev->subsystem_vendor) &&
+	    (id->subdevice == PCI_ANY_ID ||
+		id->subdevice == dev->subsystem_device) &&
+	    !((id->class ^ dev->class) & id->class_mask))
+		return id;
+	return NULL;
+}
+
+static int dovmgr_pci_item_match(struct dovmgr_item *item, struct device *dev)
+{
+	struct pci_dev *pdev;
+
+	BUG_ON(item->type != ITEM_PCI);
+	pdev = to_pci_dev(dev);
+
+	return pci_match_one_device(&item->pci, pdev) != NULL;
+}
+#endif
+
+#if IS_ENABLED(CONFIG_USB)
+/* in drivers/usb/core/driver.c */
+extern int usb_match_device(struct usb_device *dev,
+		const struct usb_device_id *id);
+
+static int dovmgr_usb_item_match(struct dovmgr_item *item, struct device *dev)
+{
+	struct usb_device *udev;
+
+	BUG_ON(item->type != ITEM_USB);
+	udev = to_usb_device(dev);
+
+	return usb_match_device(udev, &item->usb);
+}
+#endif
+
+static struct dovmgr_dev_item *dovmgr_lookup_dev_item(struct dovmgr_item *item,
+		struct device *dev)
+{
+	struct dovmgr_dev_item *ditem;
+
+	list_for_each_entry(ditem, &item->dev_item_list, node)
+		if (ditem->dev == dev)
+			return ditem;
+	return NULL;
+}
+
+static void dovmgr_item_work_func(struct work_struct *work)
+{
+	struct dovmgr_dev_item *ditem = container_of(work,
+			struct dovmgr_dev_item, work);
+	struct dovmgr_item *item = ditem->item;
+	struct device *dev;
+	struct device_node *np;
+	int err;
+
+	mutex_lock(&item->dev_item_mutex);
+
+	dev = ditem->dev;
+	np = dev->of_node;
+	if (!dev || !np || !item->overlay_name || ditem->overlay_id >= 0)
+		goto out_unlock;
+
+	pr_info("%s: %s %s\n", __func__,
+		kobject_name(&dev->kobj), of_node_full_name(np));
+
+	err = request_firmware_direct(&ditem->fw, item->overlay_name, dev);
+	if (err != 0) {
+		pr_err("%s: %s failed to load firmware '%s'\n", __func__,
+			kobject_name(&dev->kobj), item->overlay_name);
+		goto out_unlock;
+	}
+
+	of_fdt_unflatten_tree((void *)ditem->fw->data, &ditem->overlay);
+	if (ditem->overlay == NULL) {
+		pr_err("%s: %s failed to load firmware '%s'\n", __func__,
+			kobject_name(&dev->kobj), item->overlay_name);
+		goto out_release_fw;
+	}
+
+	/* mark it as detached */
+	of_node_set_flag(ditem->overlay, OF_DETACHED);
+
+	/* perform resolution */
+	err = of_resolve_phandles(ditem->overlay);
+	if (err != 0) {
+		pr_err("%s: %s failed to resolve tree\n", __func__,
+			kobject_name(&dev->kobj));
+		goto out_release_overlay;
+	}
+
+	err = of_overlay_create_target_root(ditem->overlay, np);
+	if (err < 0) {
+		pr_err("%s: %s failed to create overlay\n", __func__,
+			kobject_name(&dev->kobj));
+		goto out_release_overlay;
+	}
+	ditem->overlay_id = err;
+
+out_unlock:
+	mutex_unlock(&item->dev_item_mutex);
+	return;
+
+out_release_overlay:
+	/* TODO: free the overlay, we can't right now cause
+	 * the unflatten method does not track it */
+	ditem->overlay = NULL;
+out_release_fw:
+	release_firmware(ditem->fw);
+	ditem->fw = NULL;
+	goto out_unlock;
+}
+
+/* dev item list mutex lock must be held */
+static int dovmgr_add_dev_item(struct dovmgr_item *item, struct device *dev)
+{
+	struct dovmgr_dev_item *ditem;
+
+	/* first make sure there's no duplicate */
+	if (dovmgr_lookup_dev_item(item, dev))
+		return -EEXIST;
+
+	/* add the device item */
+	ditem = kzalloc(sizeof(*ditem), GFP_KERNEL);
+	if (!ditem)
+		return -ENOMEM;
+	ditem->overlay_id = -1;
+	ditem->dev = get_device(dev);
+	INIT_WORK(&ditem->work, dovmgr_item_work_func);
+	ditem->item = item;
+
+	list_add_tail(&ditem->node, &item->dev_item_list);
+
+	pr_info("%s: added device %s from item's %s list\n", __func__,
+			kobject_name(&dev->kobj),
+			config_item_name(&item->item));
+
+	/* now schedule the overlay application */
+	if (item->overlay_name)
+		schedule_work(&ditem->work);
+
+	return 0;
+}
+
+static int dovmgr_remove_dev_item(struct dovmgr_item *item, struct device *dev)
+{
+	struct dovmgr_dev_item *ditem;
+
+	/* find it */
+	ditem = dovmgr_lookup_dev_item(item, dev);
+	if (!ditem)
+		return -ENODEV;
+
+	if (work_pending(&ditem->work))
+		cancel_work_sync(&ditem->work);
+
+	if (ditem->overlay_id >= 0) {
+		of_overlay_destroy(ditem->overlay_id);
+		ditem->overlay_id = -1;
+
+	}
+
+	if (ditem->overlay) {
+		/* TODO: free the overlay, we can't right now cause
+		* the unflatten method does not track it */
+		ditem->overlay = NULL;
+	}
+
+	if (ditem->fw) {
+		/* TODO release_firmware(ditem->fw); */
+		release_firmware(ditem->fw);
+		ditem->fw = NULL;
+	}
+
+	put_device(ditem->dev);
+	list_del(&ditem->node);
+
+	kfree(ditem);
+
+	pr_info("%s: removed device %s from item's %s list\n", __func__,
+			kobject_name(&dev->kobj),
+			config_item_name(&item->item));
+
+	return 0;
+}
+
+static int dovmgr_item_notify(struct dovmgr_item *item,
+		unsigned long action, struct device *dev)
+{
+	int ret;
+
+	ret = 0;
+	mutex_lock(&item->dev_item_mutex);
+
+	switch (action) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		pr_info("%s: BUS_NOTIFY_ADD_DEVICE for %s\n", __func__,
+				kobject_name(&dev->kobj));
+
+		ret = dovmgr_add_dev_item(item, dev);
+		if (ret != 0)
+			goto out_unlock;
+
+		break;
+
+	case BUS_NOTIFY_DEL_DEVICE:
+		pr_info("%s: BUS_NOTIFY_DEL_DEVICE for %s\n", __func__,
+				kobject_name(&dev->kobj));
+		break;
+
+	case BUS_NOTIFY_REMOVED_DEVICE:
+		pr_info("%s: BUS_NOTIFY_REMOVE_DEVICE for %s\n", __func__,
+				kobject_name(&dev->kobj));
+
+		ret = dovmgr_remove_dev_item(item, dev);
+		if (ret != 0)
+			goto out_unlock;
+
+		break;
+	}
+
+out_unlock:
+	mutex_unlock(&item->dev_item_mutex);
+
+	return ret;
+}
+
+#if IS_ENABLED(CONFIG_PCI)
+static int dovmgr_pci_add_iterator(struct device *dev, void *data)
+{
+	struct dovmgr_item *item = data;
+
+	/* do add match */
+	if (!item->enable || !dovmgr_pci_item_match(item, dev))
+		return 0;
+
+	pr_info("%s: dev=%s\n", __func__, kobject_name(&dev->kobj));
+
+	return dovmgr_item_notify(item, BUS_NOTIFY_ADD_DEVICE, dev);
+}
+
+static int dovmgr_pci_removed_iterator(struct device *dev, void *data)
+{
+	struct dovmgr_item *item = data;
+
+	/* do add match */
+	if (item->enable || !dovmgr_pci_item_match(item, dev))
+		return 0;
+
+	pr_info("%s: dev=%s\n", __func__, kobject_name(&dev->kobj));
+
+	return dovmgr_item_notify(item, BUS_NOTIFY_REMOVED_DEVICE, dev);
+}
+#endif
+
+#if IS_ENABLED(CONFIG_USB)
+static int dovmgr_usb_add_iterator(struct device *dev, void *data)
+{
+	struct dovmgr_item *item = data;
+
+	/* do add match */
+	if (item->enable || !dovmgr_usb_item_match(item, dev))
+		return 0;
+
+	pr_info("%s: dev=%s\n", __func__, kobject_name(&dev->kobj));
+
+	return dovmgr_item_notify(item, BUS_NOTIFY_ADD_DEVICE, dev);
+}
+
+static int dovmgr_usb_removed_iterator(struct device *dev, void *data)
+{
+	struct dovmgr_item *item = data;
+
+	/* do add match */
+	if (!item->enable || !dovmgr_usb_item_match(item, dev))
+		return 0;
+
+	pr_info("%s: dev=%s\n", __func__, kobject_name(&dev->kobj));
+
+	return dovmgr_item_notify(item, BUS_NOTIFY_REMOVED_DEVICE, dev);
+}
+#endif
+
+static int dovmgr_item_set_enable(struct dovmgr_item *item, bool new_enable)
+{
+	int ret;
+
+	if (new_enable == item->enable)
+		return 0;
+
+	item->enable = new_enable;
+	switch (item->type) {
+#if IS_ENABLED(CONFIG_PCI)
+	case ITEM_PCI:
+		ret = bus_for_each_dev(&pci_bus_type, NULL, item,
+			new_enable ? dovmgr_pci_add_iterator :
+					dovmgr_pci_removed_iterator);
+		if (ret != 0)
+			return ret;
+		break;
+#endif
+#if IS_ENABLED(CONFIG_USB)
+	case ITEM_USB:
+		ret = bus_for_each_dev(&usb_bus_type, NULL, item,
+			new_enable ? dovmgr_usb_add_iterator :
+					dovmgr_usb_removed_iterator);
+		if (ret != 0)
+			return ret;
+		break;
+#endif
+	default:
+		break;
+	}
+	return 0;
+}
+
+
+CONFIGFS_ATTR_STRUCT(dovmgr_item);
+#define DOVMGR_ITEM_ATTR(_name, _mode, _show, _store)	\
+struct dovmgr_item_attribute dovmgr_item_attr_##_name = \
+	__CONFIGFS_ATTR(_name, _mode, _show, _store)
+#define DOVMGR_ITEM_ATTR_RO(_name, _show)	\
+struct dovmgr_item_attribute dovmgr_item_attr_##_name = \
+	__CONFIGFS_ATTR_RO(_name, _show)
+
+CONFIGFS_BIN_ATTR_STRUCT(dovmgr_item);
+#define DOVMGR_ITEM_BIN_ATTR(_name, _mode, _read, _write, _priv, _max) \
+struct dovmgr_item_bin_attribute dovmgr_item_bin_attr_##_name = \
+	__CONFIGFS_BIN_ATTR(_name, _mode, _read, _write, _priv, _max)
+#define DOVMGR_ITEM_BIN_ATTR_RO(_name, _read, _priv, _max)	\
+struct dovmgr_item_bin_attribute dovmgr_item_bin_attr_##_name = \
+	__CONFIGFS_BIN_ATTR_RO(_name, _read, _priv, _max)
+
+static ssize_t dovmgr_item_str_show(struct dovmgr_item *item,
+		char *page, char **strp)
+{
+	return snprintf(page, PAGE_SIZE, "%s\n",
+			*strp ? *strp : "");
+}
+
+static ssize_t dovmgr_item_str_store(struct dovmgr_item *item,
+		const char *page, size_t count, char **strp)
+{
+	const char *s;
+	int len;
+
+	/* copy to path buffer (and make sure it's always zero terminated */
+	len = strnlen(page, PAGE_SIZE);
+	if (len >= PAGE_SIZE)
+		return -EINVAL;
+	s = page + len;
+	while (len > 0 && *--s == '\n')
+		len--;
+	if (len == 0)
+		return -EINVAL;
+
+	if (*strp)
+		kfree(*strp);
+	*strp = kmalloc(len + 1, GFP_KERNEL);
+	if (!*strp)
+		return -ENOMEM;
+	memcpy(*strp, page, len);
+	(*strp)[len + 1] = '\0';
+
+	return count;
+}
+
+static ssize_t dovmgr_item_path_show(struct dovmgr_item *item, char *page)
+{
+	return dovmgr_item_str_show(item, page, &item->path);
+}
+
+static ssize_t dovmgr_item_path_store(struct dovmgr_item *item,
+		const char *page, size_t count)
+{
+	return dovmgr_item_str_store(item, page, count, &item->path);
+}
+
+static ssize_t dovmgr_item_enable_show(struct dovmgr_item *item, char *page)
+{
+	return snprintf(page, PAGE_SIZE, "%u\n", !!item->enable);
+}
+
+static ssize_t dovmgr_item_enable_store(struct dovmgr_item *item,
+		const char *page, size_t count)
+{
+	int ret;
+	unsigned int val;
+
+	ret = kstrtouint(page, 0, &val);
+	if (ret != 0)
+		return ret;
+
+	ret = dovmgr_item_set_enable(item, !!val);
+	if (ret != 0)
+		return ret;
+
+	return count;
+}
+
+static ssize_t dovmgr_item_overlay_show(struct dovmgr_item *item, char *page)
+{
+	ssize_t ret;
+
+	mutex_lock(&item->dev_item_mutex);
+	ret = snprintf(page, PAGE_SIZE, "%s\n", item->overlay_name ?
+			item->overlay_name : "");
+	mutex_unlock(&item->dev_item_mutex);
+	return ret;
+};
+
+
+static ssize_t dovmgr_item_overlay_store(struct dovmgr_item *item,
+		const char *page, size_t count)
+{
+	ssize_t ret;
+
+	mutex_lock(&item->dev_item_mutex);
+	kfree(item->overlay_name);
+	item->overlay_name = kstrndup(page, PAGE_SIZE, GFP_KERNEL);
+	if (!item->overlay_name)
+		ret = -ENOMEM;
+	else
+		ret = count;
+	mutex_unlock(&item->dev_item_mutex);
+	return ret;
+}
+
+static ssize_t dovmgr_item_status_show(struct dovmgr_item *item, char *page)
+{
+	struct dovmgr_dev_item *ditem;
+	char *p, *e;
+	int len;
+
+	p = page;
+	e = page + PAGE_SIZE;
+
+	mutex_lock(&item->dev_item_mutex);
+	list_for_each_entry(ditem, &item->dev_item_list, node) {
+		len = snprintf(p, e - p, "%s:%s:%d\n",
+			kobject_name(&ditem->dev->kobj),
+			of_node_full_name(ditem->dev->of_node),
+			ditem->overlay_id);
+		p += len;
+		if (p >= e - 1)
+			break;
+	}
+	mutex_unlock(&item->dev_item_mutex);
+
+	return p - page;
+}
+
+DOVMGR_ITEM_ATTR(path, S_IRUGO | S_IWUSR,
+		dovmgr_item_path_show, dovmgr_item_path_store);
+DOVMGR_ITEM_ATTR_RO(status, dovmgr_item_status_show);
+DOVMGR_ITEM_ATTR(enable, S_IRUGO | S_IWUSR,
+		dovmgr_item_enable_show, dovmgr_item_enable_store);
+DOVMGR_ITEM_ATTR(overlay, S_IRUGO | S_IWUSR,
+		dovmgr_item_overlay_show, dovmgr_item_overlay_store);
+
+#if IS_ENABLED(CONFIG_PCI)
+static ssize_t dovmgr_item_pci_device_show(struct dovmgr_item *item, char *page)
+{
+	return snprintf(page, PAGE_SIZE, "0x%04x\n", item->pci.device);
+}
+
+static ssize_t dovmgr_item_pci_device_store(struct dovmgr_item *item,
+		const char *page, size_t count)
+{
+	int ret;
+	unsigned int val;
+
+	ret = kstrtouint(page, 0, &val);
+	if (ret != 0)
+		return ret;
+	item->pci.device = val;
+	return count;
+}
+
+static ssize_t dovmgr_item_pci_vendor_show(struct dovmgr_item *item, char *page)
+{
+	return snprintf(page, PAGE_SIZE, "0x%04x\n", item->pci.vendor);
+}
+
+static ssize_t dovmgr_item_pci_vendor_store(struct dovmgr_item *item,
+		const char *page, size_t count)
+{
+	int ret;
+	unsigned int val;
+
+	/* cannot modify when item is enabled */
+	if (item->enable)
+		return -EBUSY;
+
+	ret = kstrtouint(page, 0, &val);
+	if (ret != 0)
+		return ret;
+	item->pci.vendor = val;
+	return count;
+}
+
+static ssize_t dovmgr_item_pci_subdevice_show(struct dovmgr_item *item,
+		char *page)
+{
+	return snprintf(page, PAGE_SIZE, "0x%08x\n", item->pci.subdevice);
+}
+
+static ssize_t dovmgr_item_pci_subdevice_store(struct dovmgr_item *item,
+		const char *page, size_t count)
+{
+	int ret;
+	unsigned int val;
+
+	/* cannot modify when item is enabled */
+	if (item->enable)
+		return -EBUSY;
+
+	ret = kstrtouint(page, 0, &val);
+	if (ret != 0)
+		return ret;
+	item->pci.subdevice = val;
+	return count;
+}
+
+static ssize_t dovmgr_item_pci_subvendor_show(struct dovmgr_item *item,
+		char *page)
+{
+	return snprintf(page, PAGE_SIZE, "0x%08x\n", item->pci.subvendor);
+}
+
+static ssize_t dovmgr_item_pci_subvendor_store(struct dovmgr_item *item,
+		const char *page, size_t count)
+{
+	int ret;
+	unsigned int val;
+
+	/* cannot modify when item is enabled */
+	if (item->enable)
+		return -EBUSY;
+
+	ret = kstrtouint(page, 0, &val);
+	if (ret != 0)
+		return ret;
+	item->pci.subvendor = val;
+	return count;
+}
+
+static ssize_t dovmgr_item_pci_class_show(struct dovmgr_item *item, char *page)
+{
+	return snprintf(page, PAGE_SIZE, "0x%04x\n", item->pci.class);
+}
+
+static ssize_t dovmgr_item_pci_class_store(struct dovmgr_item *item,
+		const char *page, size_t count)
+{
+	int ret;
+	unsigned int val;
+
+	/* cannot modify when item is enabled */
+	if (item->enable)
+		return -EBUSY;
+
+	ret = kstrtouint(page, 0, &val);
+	if (ret != 0)
+		return ret;
+	item->pci.class = val;
+	return count;
+}
+
+static ssize_t dovmgr_item_pci_class_mask_show(struct dovmgr_item *item,
+		char *page)
+{
+	return snprintf(page, PAGE_SIZE, "0x%04x\n", item->pci.class_mask);
+}
+
+static ssize_t dovmgr_item_pci_class_mask_store(struct dovmgr_item *item,
+		const char *page, size_t count)
+{
+	int ret;
+	unsigned int val;
+
+	/* cannot modify when item is enabled */
+	if (item->enable)
+		return -EBUSY;
+
+	ret = kstrtouint(page, 0, &val);
+	if (ret != 0)
+		return ret;
+	item->pci.class_mask = val;
+	return count;
+}
+
+DOVMGR_ITEM_ATTR(device, S_IRUGO | S_IWUSR,
+	dovmgr_item_pci_device_show, dovmgr_item_pci_device_store);
+DOVMGR_ITEM_ATTR(vendor, S_IRUGO | S_IWUSR,
+	dovmgr_item_pci_vendor_show, dovmgr_item_pci_vendor_store);
+DOVMGR_ITEM_ATTR(subdevice, S_IRUGO | S_IWUSR,
+	dovmgr_item_pci_subdevice_show, dovmgr_item_pci_subdevice_store);
+DOVMGR_ITEM_ATTR(subvendor, S_IRUGO | S_IWUSR,
+	dovmgr_item_pci_subvendor_show, dovmgr_item_pci_subvendor_store);
+DOVMGR_ITEM_ATTR(class, S_IRUGO | S_IWUSR,
+	dovmgr_item_pci_class_show, dovmgr_item_pci_class_store);
+DOVMGR_ITEM_ATTR(class_mask, S_IRUGO | S_IWUSR,
+	dovmgr_item_pci_class_mask_show, dovmgr_item_pci_class_mask_store);
+#endif
+
+#if IS_ENABLED(CONFIG_USB)
+static ssize_t dovmgr_item_usb_idProduct_show(struct dovmgr_item *item,
+		char *page)
+{
+	return snprintf(page, PAGE_SIZE, "0x%04x\n",
+			item->usb.idProduct);
+}
+
+static ssize_t dovmgr_item_usb_idProduct_store(struct dovmgr_item *item,
+		const char *page, size_t count)
+{
+	int ret;
+	unsigned int val;
+
+	/* cannot modify when item is enabled */
+	if (item->enable)
+		return -EBUSY;
+
+	ret = kstrtouint(page, 0, &val);
+	if (ret != 0)
+		return ret;
+	item->usb.idProduct = val;
+	return count;
+}
+
+static ssize_t dovmgr_item_usb_idVendor_show(struct dovmgr_item *item,
+		char *page)
+{
+	return snprintf(page, PAGE_SIZE, "0x%04x\n",
+			item->usb.idVendor);
+}
+
+static ssize_t dovmgr_item_usb_idVendor_store(struct dovmgr_item *item,
+		const char *page, size_t count)
+{
+	int ret;
+	unsigned int val;
+
+	/* cannot modify when item is enabled */
+	if (item->enable)
+		return -EBUSY;
+
+	ret = kstrtouint(page, 0, &val);
+	if (ret != 0)
+		return ret;
+	item->usb.idVendor = val;
+	return count;
+}
+
+DOVMGR_ITEM_ATTR(idProduct, S_IRUGO | S_IWUSR,
+	dovmgr_item_usb_idProduct_show, dovmgr_item_usb_idProduct_store);
+DOVMGR_ITEM_ATTR(idVendor, S_IRUGO | S_IWUSR,
+	dovmgr_item_usb_idVendor_show, dovmgr_item_usb_idVendor_store);
+#endif
+
+#if IS_ENABLED(CONFIG_PCI)
+static struct configfs_attribute *dovmgr_pci_attrs[] = {
+	&dovmgr_item_attr_path.attr,
+	&dovmgr_item_attr_status.attr,
+	&dovmgr_item_attr_enable.attr,
+	&dovmgr_item_attr_overlay.attr,
+	&dovmgr_item_attr_device.attr,
+	&dovmgr_item_attr_vendor.attr,
+	&dovmgr_item_attr_subdevice.attr,
+	&dovmgr_item_attr_subvendor.attr,
+	&dovmgr_item_attr_class.attr,
+	&dovmgr_item_attr_class_mask.attr,
+	NULL,
+};
+#endif
+
+#if IS_ENABLED(CONFIG_USB)
+static struct configfs_attribute *dovmgr_usb_attrs[] = {
+	&dovmgr_item_attr_path.attr,
+	&dovmgr_item_attr_enable.attr,
+	&dovmgr_item_attr_status.attr,
+	&dovmgr_item_attr_overlay.attr,
+	&dovmgr_item_attr_idVendor.attr,
+	&dovmgr_item_attr_idProduct.attr,
+	NULL,
+};
+#endif
+
+static void dovmgr_release(struct config_item *cfsitem)
+{
+	struct dovmgr_item *item = to_dovmgr_item(cfsitem);
+
+	/* disable item (this removes the overlay and all) */
+	dovmgr_item_set_enable(item, false);
+
+	kfree(item->path);
+	kfree(item);
+}
+
+CONFIGFS_ATTR_OPS(dovmgr_item);
+static struct configfs_item_operations dovmgr_item_ops = {
+	.release		= dovmgr_release,
+	.show_attribute		= dovmgr_item_attr_show,
+	.store_attribute	= dovmgr_item_attr_store,
+};
+
+#if IS_ENABLED(CONFIG_PCI)
+static struct config_item_type dovmgr_pci_item_type = {
+	.ct_item_ops	= &dovmgr_item_ops,
+	.ct_attrs	= dovmgr_pci_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+#endif
+
+#if IS_ENABLED(CONFIG_USB)
+static struct config_item_type dovmgr_usb_item_type = {
+	.ct_item_ops	= &dovmgr_item_ops,
+	.ct_attrs	= dovmgr_usb_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+#endif
+
+static struct config_item *dovmgr_group_make_item(
+		struct config_group *group, const char *name,
+		enum dovmgr_type type)
+{
+	struct dovmgr_item *item;
+	struct config_item_type *item_type;
+
+	switch (type) {
+#if IS_ENABLED(CONFIG_PCI)
+	case ITEM_PCI:
+		item_type = &dovmgr_pci_item_type;
+		break;
+#endif
+#if IS_ENABLED(CONFIG_USB)
+	case ITEM_USB:
+		item_type = &dovmgr_usb_item_type;
+		break;
+#endif
+	default:
+		return ERR_PTR(-EINVAL);
+	};
+
+	item = kzalloc(sizeof(*item), GFP_KERNEL);
+	if (!item)
+		return ERR_PTR(-ENOMEM);
+
+	item->type = type;
+	item->enable = false;
+	mutex_init(&item->dev_item_mutex);
+	INIT_LIST_HEAD(&item->dev_item_list);
+
+	switch (type) {
+#if IS_ENABLED(CONFIG_PCI)
+	case ITEM_PCI:
+		/* default for matching device/vendor */
+		item->pci.vendor = PCI_ANY_ID;
+		item->pci.device = PCI_ANY_ID;
+		item->pci.subvendor = PCI_ANY_ID;
+		item->pci.subdevice = PCI_ANY_ID;
+		item->pci.class = 0;
+		item->pci.class_mask = 0;
+		break;
+#endif
+#if IS_ENABLED(CONFIG_USB)
+	case ITEM_USB:
+		/* default */
+		item->usb.match_flags = USB_DEVICE_ID_MATCH_DEVICE;
+		break;
+#endif
+	default:
+		return ERR_PTR(-EINVAL);
+	};
+
+	config_item_init_type_name(&item->item, name, item_type);
+	return &item->item;
+}
+
+#if IS_ENABLED(CONFIG_PCI)
+static struct config_item *dovmgr_group_pci_make_item(
+		struct config_group *group, const char *name)
+{
+	return dovmgr_group_make_item(group, name, ITEM_PCI);
+}
+#endif
+
+#if IS_ENABLED(CONFIG_USB)
+static struct config_item *dovmgr_group_usb_make_item(
+		struct config_group *group, const char *name)
+{
+	return dovmgr_group_make_item(group, name, ITEM_USB);
+}
+#endif
+
+static void dovmgr_group_drop_item(struct config_group *group,
+		struct config_item *cfsitem)
+{
+	struct dovmgr_item *item = to_dovmgr_item(cfsitem);
+
+	switch (item->type) {
+#if IS_ENABLED(CONFIG_PCI)
+	case ITEM_PCI:
+		break;
+#endif
+#if IS_ENABLED(CONFIG_USB)
+	case ITEM_USB:
+		break;
+#endif
+	default:
+		break;
+	}
+	config_item_put(&item->item);
+}
+
+#if IS_ENABLED(CONFIG_PCI)
+static struct configfs_group_operations dovmgr_pci_group_ops = {
+	.make_item	= dovmgr_group_pci_make_item,
+	.drop_item	= dovmgr_group_drop_item,
+};
+
+static struct config_item_type dovmgr_pci_type = {
+	.ct_group_ops   = &dovmgr_pci_group_ops,
+	.ct_owner       = THIS_MODULE,
+};
+#endif
+
+#if IS_ENABLED(CONFIG_USB)
+static struct configfs_group_operations dovmgr_usb_group_ops = {
+	.make_item	= dovmgr_group_usb_make_item,
+	.drop_item	= dovmgr_group_drop_item,
+};
+
+static struct config_item_type dovmgr_usb_type = {
+	.ct_group_ops   = &dovmgr_usb_group_ops,
+	.ct_owner       = THIS_MODULE,
+};
+#endif
+
+static struct configfs_group_operations dovmgr_ops = {
+	/* empty - we don't allow anything to be created */
+};
+
+static struct config_item_type dovmgr_type = {
+	.ct_group_ops   = &dovmgr_ops,
+	.ct_owner       = THIS_MODULE,
+};
+
+struct config_group *dovmgr_def_groups[] = {
+#if IS_ENABLED(CONFIG_PCI)
+	&dovmgr_pci_group,
+#endif
+#if IS_ENABLED(CONFIG_USB)
+	&dovmgr_usb_group,
+#endif
+	NULL
+};
+
+static struct configfs_subsystem dovmgr_subsys = {
+	.su_group = {
+		.cg_item = {
+			.ci_namebuf = "dovmgr",
+			.ci_type = &dovmgr_type,
+		},
+		.default_groups = dovmgr_def_groups,
+	},
+	.su_mutex = __MUTEX_INITIALIZER(dovmgr_subsys.su_mutex),
+};
+
+#if IS_ENABLED(CONFIG_PCI)
+static int pci_dev_instantiate(struct pci_dev *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device *bus_dev;
+	struct of_changeset cset;
+	struct device_node *np, *npb;
+	int ret;
+
+	npb = NULL;
+
+	/* already instantiated */
+	if (dev->of_node) {
+		pr_debug("%s: dev=%s of_node=%s\n", __func__,
+			kobject_name(&dev->kobj),
+			of_node_full_name(dev->of_node));
+		return 0;
+	}
+
+	bus_dev = &pdev->bus->dev;
+
+	pr_debug("%s: %s: %02x:%02x.%02x - node %s%s\n", __func__,
+			kobject_name(&dev->kobj),
+			pdev->bus->number, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
+			bus_dev->of_node ? of_node_full_name(bus_dev->of_node) : "<NULL>",
+			pci_is_bridge(pdev) ? " bridge" : "");
+
+	/* to create the node, the bus must be present */
+	if (!bus_dev->of_node) {
+		pr_err("%s: No node for %s because no bus device node\n",
+			__func__, kobject_name(&dev->kobj));
+		return 0;
+	}
+
+	of_changeset_init(&cset);
+
+	np = of_changeset_create_device_node(&cset, bus_dev->of_node,
+		"%s/pci-%04x-%02x-%02x.%d",
+		of_node_full_name(bus_dev->of_node),
+		pci_domain_nr(pdev->bus), pdev->bus->number,
+		PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+	if (IS_ERR(np)) {
+		ret = PTR_ERR(np);
+		goto out_cset_fail;
+	}
+
+	ret = of_changeset_add_property_stringf(&cset, np, "compatible",
+			"pciclass,%04x", (pdev->class >> 8) & 0xffffff);
+	if (ret != 0)
+		goto out_cset_fail;
+
+	ret = of_changeset_add_property_u32(&cset, np, "vendor",
+			pdev->vendor);
+	if (ret != 0)
+		goto out_cset_fail;
+
+	ret = of_changeset_add_property_u32(&cset, np, "device",
+			pdev->device);
+	if (ret != 0)
+		goto out_cset_fail;
+
+	ret = of_changeset_add_property_string(&cset, np, "status", "okay");
+	if (ret != 0)
+		goto out_cset_fail;
+
+	ret = of_changeset_add_property_bool(&cset, np, "auto-generated");
+	if (ret != 0)
+		goto out_cset_fail;
+
+	ret = of_changeset_attach_node(&cset, np);
+	if (ret != 0)
+		goto out_cset_fail;
+
+	/* are we creating a bridge; swell */
+	npb = NULL;
+	if (pci_is_bridge(pdev) && !pdev->subordinate->dev.of_node) {
+
+		pr_debug("%s: %s: bus->dev=%s subordinate=%s\n", __func__,
+			kobject_name(&dev->kobj),
+			kobject_name(&pdev->bus->dev.kobj),
+			kobject_name(&pdev->subordinate->dev.kobj));
+
+		npb = of_changeset_create_device_node(&cset, bus_dev->of_node,
+			"%s/pci-%04x-%02x",
+			of_node_full_name(bus_dev->of_node),
+			pci_domain_nr(pdev->subordinate),
+			pdev->subordinate->number);
+		if (IS_ERR(npb)) {
+			ret = PTR_ERR(npb);
+			goto out_cset_fail;
+		}
+
+		ret = of_changeset_add_property_string(&cset, npb, "compatible", "generic,pci-bus");
+		if (ret != 0)
+			goto out_cset_fail;
+
+		ret = of_changeset_add_property_string(&cset, npb, "device_type", "pci");
+		if (ret != 0)
+			goto out_cset_fail;
+
+		ret = of_changeset_add_property_string(&cset, npb, "status", "okay");
+		if (ret != 0)
+			goto out_cset_fail;
+
+		ret = of_changeset_add_property_bool(&cset, npb, "auto-generated");
+		if (ret != 0)
+			goto out_cset_fail;
+
+		ret = of_changeset_attach_node(&cset, npb);
+		if (ret != 0)
+			goto out_cset_fail;
+	}
+
+	ret = of_changeset_apply(&cset);
+	if (ret != 0)
+		goto out_cset_fail;
+
+	/* permanently commit */
+	of_changeset_destroy(&cset);
+
+	/* bind the node to the device */
+	dev->of_node = np;
+	ret = sysfs_create_link(&dev->kobj, &dev->of_node->kobj,
+			"of_node");
+	if (ret)
+		pr_warn("%s: %s Error %d creating of_node link\n",
+				__func__, kobject_name(&dev->kobj), ret);
+
+	if (npb) {
+		pdev->subordinate->dev.of_node = npb;
+		ret = sysfs_create_link(&pdev->subordinate->dev.kobj, &npb->kobj,
+				"of_node");
+		if (ret)
+			pr_warn("%s: %s Error %d creating of_node link\n",
+					__func__, kobject_name(&dev->kobj), ret);
+	}
+
+
+	return 0;
+
+out_cset_fail:
+	pr_err("%s: %s Failed to apply changeset (err=%d)\n", __func__,
+		kobject_name(&dev->kobj), ret);
+	of_changeset_destroy(&cset);
+	return ret;
+}
+
+static int pci_dev_uninstantiate(struct pci_dev *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np, *npb;
+	struct of_changeset cset;
+	int ret;
+
+	/* device node must exist */
+	np = dev->of_node;
+	if (!np)
+		return 0;
+
+	/* and the auto-generated property */
+	if (!of_property_read_bool(np, "auto-generated"))
+		return 0;
+
+	of_changeset_init(&cset);
+
+	ret = of_changeset_detach_node(&cset, np);
+	if (ret != 0)
+		goto out_cset_fail;
+
+	npb = NULL;
+	if (pci_is_bridge(pdev))
+		npb = pdev->subordinate->dev.of_node;
+
+	if (npb != NULL) {
+		ret = of_changeset_detach_node(&cset, npb);
+		if (ret != 0)
+			goto out_cset_fail;
+	}
+
+	ret = of_changeset_apply(&cset);
+	if (ret != 0)
+		goto out_cset_fail;
+
+	dev->of_node = NULL;
+	if (npb != NULL)
+		pdev->subordinate->dev.of_node = NULL;
+
+	pr_debug("%s: %s: %02x:%02x.%02x\n", __func__,
+			kobject_name(&dev->kobj),
+			pdev->bus->number, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+
+	/* TODO iterate over the properties and free */
+
+	return 0;
+
+out_cset_fail:
+	of_changeset_destroy(&cset);
+
+	return ret;
+}
+
+static int dovmgr_pci_notify(struct notifier_block *nb,
+				unsigned long action, void *arg)
+{
+	int ret;
+
+	if (action == BUS_NOTIFY_ADD_DEVICE)
+		pci_dev_instantiate(to_pci_dev(arg));
+
+	ret = dovmgr_notifier_action(&dovmgr_pci_group, action, arg,
+			dovmgr_pci_item_match, dovmgr_item_notify);
+
+	if (action == BUS_NOTIFY_REMOVED_DEVICE)
+		pci_dev_uninstantiate(to_pci_dev(arg));
+
+	return ret;
+}
+
+static struct notifier_block dovmgr_pci_notifier = {
+	.notifier_call = dovmgr_pci_notify,
+};
+
+static int pci_instantiate_iterator(struct device *dev, void *data)
+{
+	return pci_dev_instantiate(to_pci_dev(dev));
+}
+
+static int dovmgr_pci_init(void)
+{
+	int ret;
+
+	config_group_init_type_name(&dovmgr_pci_group, "pci", &dovmgr_pci_type);
+	ret = bus_register_notifier(&pci_bus_type, &dovmgr_pci_notifier);
+	if (ret != 0) {
+		pr_err("%s: bus_register_notifier() failed\n", __func__);
+		return ret;
+	}
+
+	ret = bus_for_each_dev(&pci_bus_type, NULL, NULL,
+			pci_instantiate_iterator);
+	if (ret != 0) {
+		pr_err("%s: bus_for_each_dev() failed\n", __func__);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void dovmgr_pci_cleanup(void)
+{
+	bus_unregister_notifier(&pci_bus_type, &dovmgr_pci_notifier);
+}
+#endif
+
+#if IS_ENABLED(CONFIG_USB)
+static int dovmgr_usb_notify(struct notifier_block *nb,
+				unsigned long action, void *arg)
+{
+	return dovmgr_notifier_action(&dovmgr_usb_group, action, arg,
+			dovmgr_usb_item_match, dovmgr_item_notify);
+}
+
+static struct notifier_block dovmgr_usb_notifier = {
+	.notifier_call = dovmgr_usb_notify,
+};
+
+static int dovmgr_usb_init(void)
+{
+	int ret;
+
+	config_group_init_type_name(&dovmgr_usb_group, "usb", &dovmgr_usb_type);
+	ret = bus_register_notifier(&usb_bus_type, &dovmgr_usb_notifier);
+	if (ret != 0) {
+		pr_err("%s: bus_register_notifier() failed\n", __func__);
+		return ret;
+	}
+	return 0;
+}
+
+static void dovmgr_usb_cleanup(void)
+{
+	bus_unregister_notifier(&usb_bus_type, &dovmgr_usb_notifier);
+}
+#endif
+
+static int __init dovmgr_init(void)
+{
+	int ret;
+
+	config_group_init(&dovmgr_subsys.su_group);
+
+#if IS_ENABLED(CONFIG_PCI)
+	ret = dovmgr_pci_init();
+	if (ret != 0)
+		goto err_no_pci_init;
+#endif
+#if IS_ENABLED(CONFIG_USB)
+	ret = dovmgr_usb_init();
+	if (ret != 0)
+		goto err_no_usb_init;
+#endif
+
+	ret = configfs_register_subsystem(&dovmgr_subsys);
+	if (ret != 0) {
+		pr_err("%s: failed to register subsys\n", __func__);
+		goto err_no_configfs;
+	}
+	pr_info("%s: OK\n", __func__);
+	return 0;
+
+err_no_configfs:
+#if IS_ENABLED(CONFIG_USB)
+	dovmgr_usb_cleanup();
+err_no_usb_init:
+#endif
+#if IS_ENABLED(CONFIG_PCI)
+	dovmgr_pci_cleanup();
+err_no_pci_init:
+#endif
+	return ret;
+}
+late_initcall(dovmgr_init);
-- 
1.7.12


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] RFC: Device overlay manager (PCI/USB + DT)
@ 2015-06-15 14:43   ` Paul Bolle
  0 siblings, 0 replies; 4+ messages in thread
From: Paul Bolle @ 2015-06-15 14:43 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: Rob Herring, Grant Likely, Matt Porter, Koen Kooi, Guenter Roeck,
	devicetree, linux-kernel, Pantelis Antoniou

Some remarks (that might not touch the subjects you want to get feedback
on for an RFC).

On Fri, 2015-06-12 at 23:04 +0300, Pantelis Antoniou wrote:
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
 
> +config DEV_OVERLAYMGR
> +	tristate "Device overlay manager"
> +	depends on OF
> +	select OF_OVERLAY
> +	default n

Why bother with "default n"? 

> +	help
> +	  Say Y here to include support for the automagical dev
> +	  overlay manager.

> --- /dev/null
> +++ b/drivers/misc/devovmgr.c

> +#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/sizes.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 <linux/pci.h>
> +#include <linux/usb.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/workqueue.h>
> +#include <linux/firmware.h>

You're including <linux/firmware.h> twice.

> +/* copy of drivers/pci/pci.h */

Because?

> +static inline const struct pci_device_id *
> +pci_match_one_device(const struct pci_device_id *id, const struct pci_dev *dev)
> +{
> +	if ((id->vendor == PCI_ANY_ID || id->vendor == dev->vendor) &&
> +	    (id->device == PCI_ANY_ID || id->device == dev->device) &&
> +	    (id->subvendor == PCI_ANY_ID ||
> +		id->subvendor == dev->subsystem_vendor) &&
> +	    (id->subdevice == PCI_ANY_ID ||
> +		id->subdevice == dev->subsystem_device) &&
> +	    !((id->class ^ dev->class) & id->class_mask))
> +		return id;
> +	return NULL;
> +}

> +#if IS_ENABLED(CONFIG_USB)
> +/* in drivers/usb/core/driver.c */
> +extern int usb_match_device(struct usb_device *dev,
> +		const struct usb_device_id *id);

And that's an internal function of the usb core, isn't it?

> +static int __init dovmgr_init(void)
> +{
> +	int ret;
> +
> +	config_group_init(&dovmgr_subsys.su_group);
> +
> +#if IS_ENABLED(CONFIG_PCI)
> +	ret = dovmgr_pci_init();
> +	if (ret != 0)
> +		goto err_no_pci_init;
> +#endif
> +#if IS_ENABLED(CONFIG_USB)
> +	ret = dovmgr_usb_init();
> +	if (ret != 0)
> +		goto err_no_usb_init;
> +#endif
> +
> +	ret = configfs_register_subsystem(&dovmgr_subsys);
> +	if (ret != 0) {
> +		pr_err("%s: failed to register subsys\n", __func__);
> +		goto err_no_configfs;
> +	}
> +	pr_info("%s: OK\n", __func__);
> +	return 0;
> +
> +err_no_configfs:
> +#if IS_ENABLED(CONFIG_USB)
> +	dovmgr_usb_cleanup();
> +err_no_usb_init:
> +#endif
> +#if IS_ENABLED(CONFIG_PCI)
> +	dovmgr_pci_cleanup();
> +err_no_pci_init:
> +#endif
> +	return ret;
> +}
> +late_initcall(dovmgr_init);

Lot's of "#if IS_ENABLED(CONFIG_USB)" and "#if IS_ENABLED(CONFIG_PCI)"
in the code. The above function is a rather ugly example.

Is there a point to all this if neither PCI nor USB is enabled?

USB can be 'm'. Does this build and work in that case?

There's no MODULE_LICENSE() macro. If this is a module then loading it
will taint the kernel.

There's also no function that is, well, called by module_exit() to allow
(easy) unloading (and do the needed cleaning up on unload). Did you
intend DEV_OVERLAYMGR to be bool instead?

Thanks,


Paul Bolle


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] RFC: Device overlay manager (PCI/USB + DT)
@ 2015-06-15 14:43   ` Paul Bolle
  0 siblings, 0 replies; 4+ messages in thread
From: Paul Bolle @ 2015-06-15 14:43 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: Rob Herring, Grant Likely, Matt Porter, Koen Kooi, Guenter Roeck,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Pantelis Antoniou

Some remarks (that might not touch the subjects you want to get feedback
on for an RFC).

On Fri, 2015-06-12 at 23:04 +0300, Pantelis Antoniou wrote:
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
 
> +config DEV_OVERLAYMGR
> +	tristate "Device overlay manager"
> +	depends on OF
> +	select OF_OVERLAY
> +	default n

Why bother with "default n"? 

> +	help
> +	  Say Y here to include support for the automagical dev
> +	  overlay manager.

> --- /dev/null
> +++ b/drivers/misc/devovmgr.c

> +#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/sizes.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 <linux/pci.h>
> +#include <linux/usb.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/workqueue.h>
> +#include <linux/firmware.h>

You're including <linux/firmware.h> twice.

> +/* copy of drivers/pci/pci.h */

Because?

> +static inline const struct pci_device_id *
> +pci_match_one_device(const struct pci_device_id *id, const struct pci_dev *dev)
> +{
> +	if ((id->vendor == PCI_ANY_ID || id->vendor == dev->vendor) &&
> +	    (id->device == PCI_ANY_ID || id->device == dev->device) &&
> +	    (id->subvendor == PCI_ANY_ID ||
> +		id->subvendor == dev->subsystem_vendor) &&
> +	    (id->subdevice == PCI_ANY_ID ||
> +		id->subdevice == dev->subsystem_device) &&
> +	    !((id->class ^ dev->class) & id->class_mask))
> +		return id;
> +	return NULL;
> +}

> +#if IS_ENABLED(CONFIG_USB)
> +/* in drivers/usb/core/driver.c */
> +extern int usb_match_device(struct usb_device *dev,
> +		const struct usb_device_id *id);

And that's an internal function of the usb core, isn't it?

> +static int __init dovmgr_init(void)
> +{
> +	int ret;
> +
> +	config_group_init(&dovmgr_subsys.su_group);
> +
> +#if IS_ENABLED(CONFIG_PCI)
> +	ret = dovmgr_pci_init();
> +	if (ret != 0)
> +		goto err_no_pci_init;
> +#endif
> +#if IS_ENABLED(CONFIG_USB)
> +	ret = dovmgr_usb_init();
> +	if (ret != 0)
> +		goto err_no_usb_init;
> +#endif
> +
> +	ret = configfs_register_subsystem(&dovmgr_subsys);
> +	if (ret != 0) {
> +		pr_err("%s: failed to register subsys\n", __func__);
> +		goto err_no_configfs;
> +	}
> +	pr_info("%s: OK\n", __func__);
> +	return 0;
> +
> +err_no_configfs:
> +#if IS_ENABLED(CONFIG_USB)
> +	dovmgr_usb_cleanup();
> +err_no_usb_init:
> +#endif
> +#if IS_ENABLED(CONFIG_PCI)
> +	dovmgr_pci_cleanup();
> +err_no_pci_init:
> +#endif
> +	return ret;
> +}
> +late_initcall(dovmgr_init);

Lot's of "#if IS_ENABLED(CONFIG_USB)" and "#if IS_ENABLED(CONFIG_PCI)"
in the code. The above function is a rather ugly example.

Is there a point to all this if neither PCI nor USB is enabled?

USB can be 'm'. Does this build and work in that case?

There's no MODULE_LICENSE() macro. If this is a module then loading it
will taint the kernel.

There's also no function that is, well, called by module_exit() to allow
(easy) unloading (and do the needed cleaning up on unload). Did you
intend DEV_OVERLAYMGR to be bool instead?

Thanks,


Paul Bolle

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] RFC: Device overlay manager (PCI/USB + DT)
  2015-06-15 14:43   ` Paul Bolle
  (?)
@ 2015-06-15 14:56   ` Pantelis Antoniou
  -1 siblings, 0 replies; 4+ messages in thread
From: Pantelis Antoniou @ 2015-06-15 14:56 UTC (permalink / raw)
  To: Paul Bolle
  Cc: Rob Herring, Grant Likely, Matt Porter, Koen Kooi, Guenter Roeck,
	devicetree, linux-kernel

Hi Paul,

> On Jun 15, 2015, at 17:43 , Paul Bolle <pebolle@tiscali.nl> wrote:
> 
> Some remarks (that might not touch the subjects you want to get feedback
> on for an RFC).
> 
> On Fri, 2015-06-12 at 23:04 +0300, Pantelis Antoniou wrote:
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
> 
>> +config DEV_OVERLAYMGR
>> +	tristate "Device overlay manager"
>> +	depends on OF
>> +	select OF_OVERLAY
>> +	default n
> 
> Why bother with "default n"? 
> 
>> +	help
>> +	  Say Y here to include support for the automagical dev
>> +	  overlay manager.
> 
>> --- /dev/null
>> +++ b/drivers/misc/devovmgr.c
> 
>> +#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/sizes.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 <linux/pci.h>
>> +#include <linux/usb.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/workqueue.h>
>> +#include <linux/firmware.h>
> 
> You're including <linux/firmware.h> twice.
> 

Right,

>> +/* copy of drivers/pci/pci.h */
> 
> Because?
> 

Cause it’s not exported out of PCI core.

>> +static inline const struct pci_device_id *
>> +pci_match_one_device(const struct pci_device_id *id, const struct pci_dev *dev)
>> +{
>> +	if ((id->vendor == PCI_ANY_ID || id->vendor == dev->vendor) &&
>> +	    (id->device == PCI_ANY_ID || id->device == dev->device) &&
>> +	    (id->subvendor == PCI_ANY_ID ||
>> +		id->subvendor == dev->subsystem_vendor) &&
>> +	    (id->subdevice == PCI_ANY_ID ||
>> +		id->subdevice == dev->subsystem_device) &&
>> +	    !((id->class ^ dev->class) & id->class_mask))
>> +		return id;
>> +	return NULL;
>> +}
> 
>> +#if IS_ENABLED(CONFIG_USB)
>> +/* in drivers/usb/core/driver.c */
>> +extern int usb_match_device(struct usb_device *dev,
>> +		const struct usb_device_id *id);
> 
> And that's an internal function of the usb core, isn't it?
> 

Yes.

>> +static int __init dovmgr_init(void)
>> +{
>> +	int ret;
>> +
>> +	config_group_init(&dovmgr_subsys.su_group);
>> +
>> +#if IS_ENABLED(CONFIG_PCI)
>> +	ret = dovmgr_pci_init();
>> +	if (ret != 0)
>> +		goto err_no_pci_init;
>> +#endif
>> +#if IS_ENABLED(CONFIG_USB)
>> +	ret = dovmgr_usb_init();
>> +	if (ret != 0)
>> +		goto err_no_usb_init;
>> +#endif
>> +
>> +	ret = configfs_register_subsystem(&dovmgr_subsys);
>> +	if (ret != 0) {
>> +		pr_err("%s: failed to register subsys\n", __func__);
>> +		goto err_no_configfs;
>> +	}
>> +	pr_info("%s: OK\n", __func__);
>> +	return 0;
>> +
>> +err_no_configfs:
>> +#if IS_ENABLED(CONFIG_USB)
>> +	dovmgr_usb_cleanup();
>> +err_no_usb_init:
>> +#endif
>> +#if IS_ENABLED(CONFIG_PCI)
>> +	dovmgr_pci_cleanup();
>> +err_no_pci_init:
>> +#endif
>> +	return ret;
>> +}
>> +late_initcall(dovmgr_init);
> 
> Lot's of "#if IS_ENABLED(CONFIG_USB)" and "#if IS_ENABLED(CONFIG_PCI)"
> in the code. The above function is a rather ugly example.
> 
> Is there a point to all this if neither PCI nor USB is enabled?
> 
> USB can be 'm'. Does this build and work in that case?
> 
> There's no MODULE_LICENSE() macro. If this is a module then loading it
> will taint the kernel.
> 
> There's also no function that is, well, called by module_exit() to allow
> (easy) unloading (and do the needed cleaning up on unload). Did you
> intend DEV_OVERLAYMGR to be bool instead?
> 

Note this is an RFC after all, it’s not intended for inclusion as is. I just
present the technique and how it works. Yes, all those defines are probably
not good, but I’d like some remarks on the implementation first.


> Thanks,
> 
> 
> Paul Bolle
> 

Regards

— Pantelis


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2015-06-15 14:56 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-12 20:04 [PATCH] RFC: Device overlay manager (PCI/USB + DT) Pantelis Antoniou
2015-06-15 14:43 ` Paul Bolle
2015-06-15 14:43   ` Paul Bolle
2015-06-15 14:56   ` Pantelis Antoniou

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.