[PATCH 0/5] Management Complex restool driver

[PATCH 0/5] Management Complex restool driver

[Lijun Pan]

This series of patches are based on
http://git.kernel.org/cgit/linux/kernel/git/gregkh/staging.git/commit/?h=staging-next&id=63f2be5c3b358db031f86eafa9cd450f6558a55b
patch 1/5 solves the section mismatch bug in fsl-mc bus
patches 2/5 - 4/5 add sysfs rescan atrributes, which is used in the
user space for synchronization of MC firmware and MC bus purpose.
patch 5/5 is the introduction of restool driver, which sits on fsl-mc bus and
utilizes fsl-mc bus to communicate with MC firmware.
restool driver is the first driver to start using fsl-mc bus.
This restool driver is small and helps fsl-mc bus move out of staging.

Lijun Pan (5):
  staging: fsl-mc: section mismatch bug fix
  staging: fsl-mc: define a macro to differentiate root dprc
  staging: fsl-mc: root dprc rescan attribute to sync kernel with MC
  staging: fsl-mc: bus rescan attribute to sync kernel with MC
  staging: fsl-mc: Management Complex restool driver

 drivers/staging/fsl-mc/bus/Kconfig          |   7 +-
 drivers/staging/fsl-mc/bus/Makefile         |   3 +
 drivers/staging/fsl-mc/bus/dprc-driver.c    |   2 +-
 drivers/staging/fsl-mc/bus/mc-bus.c         |  90 +++++
 drivers/staging/fsl-mc/bus/mc-ioctl.h       |  24 ++
 drivers/staging/fsl-mc/bus/mc-restool.c     | 488 ++++++++++++++++++++++++++++
 drivers/staging/fsl-mc/include/mc-private.h |   2 +-
 drivers/staging/fsl-mc/include/mc.h         |  10 +
 8 files changed, 623 insertions(+), 3 deletions(-)
 create mode 100644 drivers/staging/fsl-mc/bus/mc-ioctl.h
 create mode 100644 drivers/staging/fsl-mc/bus/mc-restool.c

-- 
2.3.3

[PATCH 1/5] staging: fsl-mc: section mismatch bug fix

[Lijun Pan]

WARNING: drivers/staging/built-in.o(.init.text+0xdc): Section mismatch in reference from the function fsl_mc_bus_driver_init() to the function .exit.text:dprc_driver_exit()
The function __init fsl_mc_bus_driver_init() references
a function __exit dprc_driver_exit().
This is often seen when error handling in the init function
uses functionality in the exit path.
The fix is often to remove the __exit annotation of
dprc_driver_exit() so it may be used outside an exit section.

Signed-off-by: Lijun Pan <Lijun.Pan@freescale.com>
---
 drivers/staging/fsl-mc/bus/dprc-driver.c    | 2 +-
 drivers/staging/fsl-mc/include/mc-private.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/fsl-mc/bus/dprc-driver.c b/drivers/staging/fsl-mc/bus/dprc-driver.c
index a9ead0d..2c4cd70 100644
--- a/drivers/staging/fsl-mc/bus/dprc-driver.c
+++ b/drivers/staging/fsl-mc/bus/dprc-driver.c
@@ -482,7 +482,7 @@ int __init dprc_driver_init(void)
 	return fsl_mc_driver_register(&dprc_driver);
 }
 
-void __exit dprc_driver_exit(void)
+void dprc_driver_exit(void)
 {
 	fsl_mc_driver_unregister(&dprc_driver);
 }
diff --git a/drivers/staging/fsl-mc/include/mc-private.h b/drivers/staging/fsl-mc/include/mc-private.h
index 2c4cc79..c706f77 100644
--- a/drivers/staging/fsl-mc/include/mc-private.h
+++ b/drivers/staging/fsl-mc/include/mc-private.h
@@ -103,7 +103,7 @@ int dprc_scan_objects(struct fsl_mc_device *mc_bus_dev);
 
 int __init dprc_driver_init(void);
 
-void __exit dprc_driver_exit(void);
+void dprc_driver_exit(void);
 
 int __init fsl_mc_allocator_driver_init(void);
 
-- 
2.3.3

[PATCH 2/5] staging: fsl-mc: define a macro to differentiate root dprc

[Lijun Pan]

Define is_root_dprc(dev) to tell whether a device is
root dprc or not via platform_bus_type.

Signed-off-by: Lijun Pan <Lijun.Pan@freescale.com>
---
 drivers/staging/fsl-mc/include/mc.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/staging/fsl-mc/include/mc.h b/drivers/staging/fsl-mc/include/mc.h
index a933291..483763e 100644
--- a/drivers/staging/fsl-mc/include/mc.h
+++ b/drivers/staging/fsl-mc/include/mc.h
@@ -14,6 +14,7 @@
 #include <linux/device.h>
 #include <linux/mod_devicetable.h>
 #include <linux/list.h>
+#include <linux/platform_device.h>
 #include "../include/dprc.h"
 
 #define FSL_MC_VENDOR_FREESCALE	0x1957
@@ -109,6 +110,15 @@ struct fsl_mc_resource {
 #define FSL_MC_IS_DPRC	0x0001
 
 /**
+  * root dprc's parent is a platform device
+  * that platform device's bus type is platform_bus_type.
+  */
+#define is_root_dprc(dev) \
+	((to_fsl_mc_device(dev)->flags & FSL_MC_IS_DPRC) && \
+	((dev)->bus == &fsl_mc_bus_type) && \
+	((dev)->parent->bus == &platform_bus_type))
+
+/**
  * Default DMA mask for devices on a fsl-mc bus
  */
 #define FSL_MC_DEFAULT_DMA_MASK	(~0ULL)
-- 
2.3.3

[PATCH 3/5] staging: fsl-mc: root dprc rescan attribute to sync kernel with MC

[Lijun Pan]

Introduce the rescan attribute as a device attribute to
synchronize the fsl-mc bus objects and the MC firmware.

To rescan the root dprc only, e.g.
echo 1 > /sys/bus/fsl-mc/devices/dprc.1/rescan

Signed-off-by: Lijun Pan <Lijun.Pan@freescale.com>
---
 drivers/staging/fsl-mc/bus/mc-bus.c | 44 +++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/staging/fsl-mc/bus/mc-bus.c b/drivers/staging/fsl-mc/bus/mc-bus.c
index 84db55b..33a56ad 100644
--- a/drivers/staging/fsl-mc/bus/mc-bus.c
+++ b/drivers/staging/fsl-mc/bus/mc-bus.c
@@ -102,13 +102,57 @@ static int fsl_mc_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
 	return 0;
 }
 
+static ssize_t rescan_store(struct device *dev,
+			    struct device_attribute *attr,
+			    const char *buf, size_t count)
+{
+	unsigned long val;
+	struct fsl_mc_device *root_mc_dev;
+	struct fsl_mc_bus *root_mc_bus;
+
+	if (!is_root_dprc(dev))
+		return -EINVAL;
+
+	root_mc_dev = to_fsl_mc_device(dev);
+	root_mc_bus = to_fsl_mc_bus(root_mc_dev);
+
+	if (kstrtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	if (val) {
+		mutex_lock(&root_mc_bus->scan_mutex);
+		dprc_scan_objects(root_mc_dev);
+		mutex_unlock(&root_mc_bus->scan_mutex);
+	}
+
+	return count;
+}
+
+static DEVICE_ATTR_WO(rescan);
+
+static struct attribute *fsl_mc_dev_attrs[] = {
+	&dev_attr_rescan.attr,
+	NULL,
+};
+
+static const struct attribute_group fsl_mc_dev_group = {
+	.attrs = fsl_mc_dev_attrs,
+};
+
+static const struct attribute_group *fsl_mc_dev_groups[] = {
+	&fsl_mc_dev_group,
+	NULL,
+};
+
 struct bus_type fsl_mc_bus_type = {
 	.name = "fsl-mc",
 	.match = fsl_mc_bus_match,
 	.uevent = fsl_mc_bus_uevent,
+	.dev_groups = fsl_mc_dev_groups,
 };
 EXPORT_SYMBOL_GPL(fsl_mc_bus_type);
 
+
 static atomic_t root_dprc_count = ATOMIC_INIT(0);
 
 static int fsl_mc_driver_probe(struct device *dev)
-- 
2.3.3

[PATCH 4/5] staging: fsl-mc: bus rescan attribute to sync kernel with MC

[Lijun Pan]

Introduce the rescan attribute as a bus attribute to
synchronize the fsl-mc bus objects and the MC firmware.

To rescan the fsl-mc bus, e.g.,
echo 1 > /sys/bus/fsl-mc/rescan

Signed-off-by: Lijun Pan <Lijun.Pan@freescale.com>
---
 drivers/staging/fsl-mc/bus/mc-bus.c | 46 +++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/staging/fsl-mc/bus/mc-bus.c b/drivers/staging/fsl-mc/bus/mc-bus.c
index 33a56ad..f1baad7 100644
--- a/drivers/staging/fsl-mc/bus/mc-bus.c
+++ b/drivers/staging/fsl-mc/bus/mc-bus.c
@@ -144,11 +144,57 @@ static const struct attribute_group *fsl_mc_dev_groups[] = {
 	NULL,
 };
 
+static int scan_fsl_mc_bus(struct device *dev, void *data)
+{
+	struct fsl_mc_device *root_mc_dev;
+	struct fsl_mc_bus *root_mc_bus;
+
+	if (is_root_dprc(dev)) {
+		root_mc_dev = to_fsl_mc_device(dev);
+		root_mc_bus = to_fsl_mc_bus(root_mc_dev);
+		mutex_lock(&root_mc_bus->scan_mutex);
+		dprc_scan_objects(root_mc_dev);
+		mutex_unlock(&root_mc_bus->scan_mutex);
+	}
+
+	return 0;
+}
+
+static ssize_t bus_rescan_store(struct bus_type *bus,
+			    const char *buf, size_t count)
+{
+	unsigned long val;
+
+	if (kstrtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	if (val)
+		bus_for_each_dev(bus, NULL, NULL, scan_fsl_mc_bus);
+
+	return count;
+}
+static BUS_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, bus_rescan_store);
+
+static struct attribute *fsl_mc_bus_attrs[] = {
+	&bus_attr_rescan.attr,
+	NULL,
+};
+
+static const struct attribute_group fsl_mc_bus_group = {
+	.attrs = fsl_mc_bus_attrs,
+};
+
+static const struct attribute_group *fsl_mc_bus_groups[] = {
+	&fsl_mc_bus_group,
+	NULL,
+};
+
 struct bus_type fsl_mc_bus_type = {
 	.name = "fsl-mc",
 	.match = fsl_mc_bus_match,
 	.uevent = fsl_mc_bus_uevent,
 	.dev_groups = fsl_mc_dev_groups,
+	.bus_groups = fsl_mc_bus_groups,
 };
 EXPORT_SYMBOL_GPL(fsl_mc_bus_type);
 
-- 
2.3.3

[PATCH 5/5] staging: fsl-mc: Management Complex restool driver

[Lijun Pan]

The kernel support for the restool (a user space resource management
tool) is a driver for the /dev/dprc.N device file.
Its purpose is to provide an ioctl interface,
which the restool uses to interact with the MC bus driver
and with the MC firmware.
We allocate a dpmcp at driver initialization,
and keep that dpmcp until driver exit.
We use that dpmcp by default.
If that dpmcp is in use, we create another portal at run time
and destroy the newly created portal after use.
The ioctl RESTOOL_SEND_MC_COMMAND sends user space command to fsl-mc
bus and utilizes the fsl-mc bus to communicate with MC firmware.
The ioctl RESTOOL_DPRC_SYNC request the mc-bus launch
objects scan under root dprc.
In order to support multiple root dprc, we utilize the bus notify
mechanism to scan fsl_mc_bus_type for the newly added root dprc.
After discovering the root dprc, it creates a miscdevice
/dev/dprc.N to associate with this root dprc.

Signed-off-by: Lijun Pan <Lijun.Pan@freescale.com>
---
 drivers/staging/fsl-mc/bus/Kconfig      |   7 +-
 drivers/staging/fsl-mc/bus/Makefile     |   3 +
 drivers/staging/fsl-mc/bus/mc-ioctl.h   |  24 ++
 drivers/staging/fsl-mc/bus/mc-restool.c | 488 ++++++++++++++++++++++++++++++++
 4 files changed, 521 insertions(+), 1 deletion(-)
 create mode 100644 drivers/staging/fsl-mc/bus/mc-ioctl.h
 create mode 100644 drivers/staging/fsl-mc/bus/mc-restool.c

diff --git a/drivers/staging/fsl-mc/bus/Kconfig b/drivers/staging/fsl-mc/bus/Kconfig
index 0d779d9..39c6ef9 100644
--- a/drivers/staging/fsl-mc/bus/Kconfig
+++ b/drivers/staging/fsl-mc/bus/Kconfig
@@ -21,4 +21,9 @@ config FSL_MC_BUS
 	  Only enable this option when building the kernel for
 	  Freescale QorQIQ LS2xxxx SoCs.
 
-
+config FSL_MC_RESTOOL
+        tristate "Freescale Management Complex (MC) restool driver"
+        depends on FSL_MC_BUS
+        help
+          Driver that provides kernel support for the Freescale Management
+	  Complex resource manager user-space tool.
diff --git a/drivers/staging/fsl-mc/bus/Makefile b/drivers/staging/fsl-mc/bus/Makefile
index 25433a9..28b5fc0 100644
--- a/drivers/staging/fsl-mc/bus/Makefile
+++ b/drivers/staging/fsl-mc/bus/Makefile
@@ -15,3 +15,6 @@ mc-bus-driver-objs := mc-bus.o \
 		      mc-allocator.o \
 		      dpmcp.o \
 		      dpbp.o
+
+# MC restool kernel support
+obj-$(CONFIG_FSL_MC_RESTOOL) += mc-restool.o
diff --git a/drivers/staging/fsl-mc/bus/mc-ioctl.h b/drivers/staging/fsl-mc/bus/mc-ioctl.h
new file mode 100644
index 0000000..e52f907
--- /dev/null
+++ b/drivers/staging/fsl-mc/bus/mc-ioctl.h
@@ -0,0 +1,24 @@
+/*
+ * Freescale Management Complex (MC) ioclt interface
+ *
+ * Copyright (C) 2014 Freescale Semiconductor, Inc.
+ * Author: Lijun Pan <Lijun.Pan@freescale.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+#ifndef _FSL_MC_IOCTL_H_
+#define _FSL_MC_IOCTL_H_
+
+#include <linux/ioctl.h>
+
+#define RESTOOL_IOCTL_TYPE   'R'
+
+#define RESTOOL_DPRC_SYNC \
+	_IO(RESTOOL_IOCTL_TYPE, 0x2)
+
+#define RESTOOL_SEND_MC_COMMAND \
+	_IOWR(RESTOOL_IOCTL_TYPE, 0x4, struct mc_command)
+
+#endif /* _FSL_MC_IOCTL_H_ */
diff --git a/drivers/staging/fsl-mc/bus/mc-restool.c b/drivers/staging/fsl-mc/bus/mc-restool.c
new file mode 100644
index 0000000..a219172
--- /dev/null
+++ b/drivers/staging/fsl-mc/bus/mc-restool.c
@@ -0,0 +1,488 @@
+/*
+ * Freescale Management Complex (MC) restool driver
+ *
+ * Copyright (C) 2014 Freescale Semiconductor, Inc.
+ * Author: Lijun Pan <Lijun.Pan@freescale.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include "../include/mc-private.h"
+#include <linux/module.h>
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/mm.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include "mc-ioctl.h"
+#include "../include/mc-sys.h"
+#include "../include/mc-cmd.h"
+#include "../include/dpmng.h"
+
+/**
+ * Maximum number of DPRCs that can be opened at the same time
+ */
+#define MAX_DPRC_HANDLES	    64
+
+/**
+ * restool_misc - information associated with the newly added miscdevice
+ * @misc: newly created miscdevice associated with root dprc
+ * @miscdevt: device id of this miscdevice
+ * @list: a linked list node representing this miscdevcie
+ * @static_mc_io: pointer to the static MC I/O object used by the restool
+ * @dynamic_instance_count: number of dynamically created instances
+ * @static_instance_in_use: static instance is in use or not
+ * @mutex: mutex lock to serialze the operations
+ * @dev: root dprc associated with this miscdevice
+ */
+struct restool_misc {
+	struct miscdevice misc;
+	dev_t miscdevt;
+	struct list_head list;
+	struct fsl_mc_io *static_mc_io;
+	uint32_t dynamic_instance_count;
+	bool static_instance_in_use;
+	struct mutex mutex;
+	struct device *dev;
+};
+
+/*
+ * initialize a global list to link all
+ * the miscdevice nodes (struct restool_misc)
+ */
+LIST_HEAD(misc_list);
+
+static int fsl_mc_restool_dev_open(struct inode *inode, struct file *filep)
+{
+	struct fsl_mc_device *root_mc_dev;
+	int error = 0;
+	struct fsl_mc_io *dynamic_mc_io = NULL;
+	struct restool_misc *restool_misc;
+	struct restool_misc *restool_misc_cursor;
+
+	pr_debug("%s: inode's dev_t == %u\n", __func__, inode->i_rdev);
+
+	list_for_each_entry(restool_misc_cursor, &misc_list, list) {
+		if (restool_misc_cursor->miscdevt == inode->i_rdev) {
+			pr_debug("%s: Found the restool_misc\n", __func__);
+			restool_misc = restool_misc_cursor;
+			break;
+		}
+	}
+
+	if (!restool_misc)
+		return -EINVAL;
+
+	if (WARN_ON(restool_misc->dev == NULL))
+		return -EINVAL;
+
+	mutex_lock(&restool_misc->mutex);
+
+	if (!restool_misc->static_instance_in_use) {
+		restool_misc->static_instance_in_use = true;
+		filep->private_data = restool_misc->static_mc_io;
+	} else {
+		dynamic_mc_io = kzalloc(sizeof(struct fsl_mc_io), GFP_KERNEL);
+		if (dynamic_mc_io == NULL) {
+			error = -ENOMEM;
+			goto error;
+		}
+
+		root_mc_dev = to_fsl_mc_device(restool_misc->dev);
+		error = fsl_mc_portal_allocate(root_mc_dev, 0, &dynamic_mc_io);
+		if (error < 0) {
+			pr_err("Not able to allocate MC portal\n");
+			goto error;
+		}
+		++restool_misc->dynamic_instance_count;
+		filep->private_data = dynamic_mc_io;
+	}
+
+	mutex_unlock(&restool_misc->mutex);
+
+	return 0;
+error:
+	if (dynamic_mc_io != NULL) {
+		fsl_mc_portal_free(dynamic_mc_io);
+		kfree(dynamic_mc_io);
+	}
+
+	mutex_unlock(&restool_misc->mutex);
+
+	return error;
+}
+
+static int fsl_mc_restool_dev_release(struct inode *inode, struct file *filep)
+{
+	struct fsl_mc_io *local_mc_io = filep->private_data;
+	struct restool_misc *restool_misc;
+	struct restool_misc *restool_misc_cursor;
+
+	if (WARN_ON(filep->private_data == NULL))
+		return -EINVAL;
+
+	pr_debug("%s: inode's dev_t == %u\n", __func__, inode->i_rdev);
+
+	list_for_each_entry(restool_misc_cursor, &misc_list, list) {
+		if (restool_misc_cursor->miscdevt == inode->i_rdev) {
+			pr_debug("%s: Found the restool_misc\n", __func__);
+			restool_misc = restool_misc_cursor;
+			break;
+		}
+	}
+
+	if (!restool_misc)
+		return -EINVAL;
+
+	mutex_lock(&restool_misc->mutex);
+
+	if (WARN_ON(restool_misc->dynamic_instance_count == 0 &&
+	    !restool_misc->static_instance_in_use)) {
+		mutex_unlock(&restool_misc->mutex);
+		return -EINVAL;
+	}
+
+	/* Globally clean up opened/untracked handles */
+	fsl_mc_portal_reset(local_mc_io);
+
+	pr_debug("dynamic instance count: %d\n",
+		restool_misc->dynamic_instance_count);
+	pr_debug("static instance count: %d\n",
+		restool_misc->static_instance_in_use);
+
+	/*
+	 * must check
+	 * whether local_mc_io is dynamic or static instance
+	 * Otherwise it will free up the reserved portal by accident
+	 * or even not free up the dynamic allocated portal
+	 * if 2 or more instances running concurrently
+	 */
+	if (local_mc_io == restool_misc->static_mc_io) {
+		pr_debug("this is reserved portal");
+		pr_debug("reserved portal not in use\n");
+		restool_misc->static_instance_in_use = false;
+	} else {
+		pr_debug("this is dynamically allocated  portal");
+		pr_debug("free one dynamically allocated portal\n");
+		fsl_mc_portal_free(local_mc_io);
+		kfree(filep->private_data);
+		--restool_misc->dynamic_instance_count;
+	}
+
+	filep->private_data = NULL;
+	mutex_unlock(&restool_misc->mutex);
+
+	return 0;
+}
+
+static int restool_dprc_sync(struct inode *inode)
+{
+	int error = 0;
+	struct fsl_mc_device *root_mc_dev;
+	struct fsl_mc_bus *root_mc_bus;
+	struct restool_misc *restool_misc;
+	struct restool_misc *restool_misc_cursor;
+
+	pr_debug("%s: inode's dev_t == %u\n", __func__, inode->i_rdev);
+
+	list_for_each_entry(restool_misc_cursor, &misc_list, list) {
+		if (restool_misc_cursor->miscdevt == inode->i_rdev) {
+			pr_debug("%s: Found the restool_misc\n", __func__);
+			restool_misc = restool_misc_cursor;
+			break;
+		}
+	}
+
+	if (!restool_misc)
+		return -EINVAL;
+
+	root_mc_dev = to_fsl_mc_device(restool_misc->dev);
+	root_mc_bus = to_fsl_mc_bus(root_mc_dev);
+
+	mutex_lock(&root_mc_bus->scan_mutex);
+	error = dprc_scan_objects(root_mc_dev);
+	mutex_unlock(&root_mc_bus->scan_mutex);
+	pr_debug("sync_error = %d\n", error);
+
+	return error;
+}
+
+static int restool_send_mc_command(unsigned long arg,
+				struct fsl_mc_io *local_mc_io)
+{
+	int error = -EINVAL;
+	struct mc_command mc_cmd;
+
+	error = copy_from_user(&mc_cmd, (void __user *)arg, sizeof(mc_cmd));
+	if (error < 0) {
+		pr_err("copy_to_user() failed with error %d\n", error);
+		goto error;
+	}
+
+	/*
+	 * Send MC command to the MC:
+	 */
+	error = mc_send_command(local_mc_io, &mc_cmd);
+	if (error < 0)
+		goto error;
+
+	error = copy_to_user((void __user *)arg, &mc_cmd, sizeof(mc_cmd));
+	if (error < 0) {
+		pr_err("copy_to_user() failed with error %d\n", error);
+		goto error;
+	}
+
+	return 0;
+error:
+	return error;
+}
+
+static long
+fsl_mc_restool_dev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	int error = 0;
+
+	switch (cmd) {
+	case RESTOOL_DPRC_SYNC:
+		pr_debug("syncing...\n");
+		error = restool_dprc_sync(file->f_inode);
+		pr_debug("syncing finished...\n");
+		break;
+	case RESTOOL_SEND_MC_COMMAND:
+		error = restool_send_mc_command(arg, file->private_data);
+		break;
+	default:
+		pr_err("%s: unexpected ioctl call number\n", __func__);
+		error = -EINVAL;
+	}
+
+	return error;
+}
+
+static const struct file_operations fsl_mc_restool_dev_fops = {
+	.owner = THIS_MODULE,
+	.open = fsl_mc_restool_dev_open,
+	.release = fsl_mc_restool_dev_release,
+	.unlocked_ioctl = fsl_mc_restool_dev_ioctl,
+};
+
+static int restool_add_device_file(struct device *dev)
+{
+	uint32_t name1 = 0;
+	char name2[20] = {0};
+	int error = 0;
+	struct fsl_mc_device *root_mc_dev;
+	struct restool_misc *restool_misc;
+
+	pr_debug("newly scanned/notified device: %s, whose parent:%s\n",
+		 dev_name(dev), dev_name(dev->parent));
+
+	if (dev->bus == &platform_bus_type && dev->driver_data) {
+		if (sscanf(dev_name(dev), "%x.%s", &name1, name2) != 2) {
+			pr_err("sscanf failure\n");
+			return -EINVAL;
+		}
+		if (strcmp(name2, "fsl-mc") == 0)
+			pr_debug("platform's root dprc name is: %s\n",
+			dev_name(&(((struct fsl_mc *)(dev->driver_data))
+			->root_mc_bus_dev->dev)));
+	}
+
+	if (dev->bus == &fsl_mc_bus_type)
+		pr_debug("%s's bus type: fsl_mc_bus_type\n", dev_name(dev));
+	else if (dev->bus == &platform_bus_type)
+		pr_debug("%s's bus type: platform_bus_type\n", dev_name(dev));
+	else
+		pr_debug("%s's bus type: NEITHER fsl_mc_bus_type NOR platform_bus_type\n",
+			 dev_name(dev));
+
+	if (is_root_dprc(dev)) {
+		pr_debug("I am root dprc, create /dev/%s\n", dev_name(dev));
+		restool_misc = kzalloc(sizeof(struct restool_misc), GFP_KERNEL);
+		if (restool_misc == NULL)
+			return -ENOMEM;
+
+		restool_misc->dev = dev;
+		root_mc_dev = to_fsl_mc_device(dev);
+		error = fsl_mc_portal_allocate(root_mc_dev, 0,
+				&restool_misc->static_mc_io);
+		if (error < 0) {
+			pr_err("Not able to allocate MC portal\n");
+			goto err_portal;
+		}
+
+		restool_misc->misc.minor = MISC_DYNAMIC_MINOR;
+		restool_misc->misc.name = dev_name(dev);
+		restool_misc->misc.fops = &fsl_mc_restool_dev_fops;
+
+		error = misc_register(&restool_misc->misc);
+		if (error < 0) {
+			pr_err("misc_register() failed: %d\n", error);
+			goto err_reg;
+		}
+
+		restool_misc->miscdevt = restool_misc->misc.this_device->devt;
+		mutex_init(&restool_misc->mutex);
+		list_add(&restool_misc->list, &misc_list);
+		pr_info("/dev/%s driver registered\n", dev_name(dev));
+	} else
+		pr_info("%s is not root dprc, miscdevice cannot be created/associated\n",
+			dev_name(dev));
+
+	return 0;
+err_reg:
+	misc_deregister(&restool_misc->misc);
+
+err_portal:
+	if (restool_misc->static_mc_io)
+		fsl_mc_portal_free(restool_misc->static_mc_io);
+	if (restool_misc)
+		kfree(restool_misc);
+
+	return error;
+}
+
+static int restool_bus_notifier(struct notifier_block *nb,
+			      unsigned long action, void *data)
+{
+	struct device *dev = data;
+
+	pr_debug("entering %s...\n", __func__);
+	pr_debug("being notified by device: %s\n", dev_name(dev));
+
+	if (dev->bus == &fsl_mc_bus_type)
+		pr_debug("%s's bus type: fsl_mc_bus_type\n", dev_name(dev));
+	else if (dev->bus == &platform_bus_type)
+		pr_debug("%s's bus type: platform_bus_type\n", dev_name(dev));
+	else
+		pr_debug("%s's bus type: NEITHER fsl_mc_bus_type NOR platform_bus_type\n",
+			 dev_name(dev));
+
+	switch (action) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		pr_info("bus notify device added: %s\n", dev_name(dev));
+		restool_add_device_file(dev);
+		break;
+	case BUS_NOTIFY_DEL_DEVICE:
+		pr_info("bus notify device to be removed: %s\n", dev_name(dev));
+		break;
+	case BUS_NOTIFY_REMOVED_DEVICE:
+		pr_info("bus notify device removed: %s\n", dev_name(dev));
+		break;
+	case BUS_NOTIFY_BIND_DRIVER:
+		pr_info("bus notify driver about to be bound to device: %s\n", dev_name(dev));
+		break;
+	case BUS_NOTIFY_BOUND_DRIVER:
+		pr_info("bus notify driver bound to device: %s\n", dev_name(dev));
+		break;
+	case BUS_NOTIFY_UNBIND_DRIVER:
+		pr_info("bus notify driver about to unbind from device: %s\n", dev_name(dev));
+		break;
+	case BUS_NOTIFY_UNBOUND_DRIVER:
+		pr_info("bus notify driver unbind from device: %s\n", dev_name(dev));
+		break;
+	default:
+		pr_err("%s: unrecognized device action from %s\n", __func__, dev_name(dev));
+		return -EINVAL;
+	}
+
+	pr_debug("leaving %s...\n", __func__);
+
+	return 0;
+}
+
+static int add_to_restool(struct device *dev, void *data)
+{
+	pr_debug("verify *data: %s\n", (char *)data);
+	restool_add_device_file(dev);
+	return 0;
+}
+
+static int __init fsl_mc_restool_driver_init(void)
+{
+	int error = 0;
+	struct notifier_block *nb;
+	char *data = "Add me to device file if I am a root dprc";
+
+	nb = kzalloc(sizeof(struct notifier_block), GFP_KERNEL);
+	if (!nb)
+		return -ENOMEM;
+
+	nb->notifier_call = restool_bus_notifier;
+	pr_debug("restool will register notifier...\n");
+	error = bus_register_notifier(&fsl_mc_bus_type, nb);
+	pr_debug("restool finish register notifier...\n");
+
+	if (error) {
+		kfree(nb);
+		return error;
+	}
+
+	pr_debug("restool scan bus for each device...\n");
+	/*
+	 * This driver runs after fsl-mc bus driver runs.
+	 * Hence, many of the root dprcs are already attached to fsl-mc bus
+	 * In order to make sure we find all the root dprcs,
+	 * we need to scan the fsl_mc_bus_type.
+	 */
+	error  = bus_for_each_dev(&fsl_mc_bus_type, NULL, data, add_to_restool);
+	if (error) {
+		bus_unregister_notifier(&fsl_mc_bus_type, nb);
+		kfree(nb);
+		pr_err("restool driver registration failure\n");
+		return error;
+	}
+	pr_debug("end restool scan bus for each device...\n");
+
+	return 0;
+}
+
+module_init(fsl_mc_restool_driver_init);
+
+static void __exit fsl_mc_restool_driver_exit(void)
+{
+	struct restool_misc *restool_misc;
+	struct restool_misc *restool_misc_tmp;
+	char name1[20] = {0};
+	uint32_t name2 = 0;
+
+	list_for_each_entry_safe(restool_misc, restool_misc_tmp,
+				 &misc_list, list) {
+		if (sscanf(restool_misc->misc.name, "%4s.%u", name1, &name2)
+		    != 2) {
+			pr_err("sscanf failure\n");
+			return;
+		}
+		pr_debug("name1=%s,name2=%u\n", name1, name2);
+		pr_debug("misc-device: %s\n", restool_misc->misc.name);
+		if (strcmp(name1, "dprc") == 0) {
+			if (WARN_ON(
+			    restool_misc->static_mc_io == NULL))
+				return;
+
+			if (WARN_ON(restool_misc->dynamic_instance_count != 0))
+				return;
+
+			if (WARN_ON(restool_misc->static_instance_in_use))
+				return;
+
+			misc_deregister(&restool_misc->misc);
+			pr_info("/dev/%s driver unregistered\n",
+				restool_misc->misc.name);
+			fsl_mc_portal_free(
+				restool_misc->static_mc_io);
+			list_del(&restool_misc->list);
+			kfree(restool_misc);
+		}
+	}
+}
+
+module_exit(fsl_mc_restool_driver_exit);
+
+MODULE_AUTHOR("Freescale Semiconductor Inc.");
+MODULE_DESCRIPTION("Freescale's MC restool driver");
+MODULE_LICENSE("GPL");
-- 
2.3.3

Re: [PATCH 2/5] staging: fsl-mc: define a macro to differentiate root dprc

[Greg KH]

On Sun, Oct 25, 2015 at 05:41:20PM -0500, Lijun Pan wrote:
> Define is_root_dprc(dev) to tell whether a device is
> root dprc or not via platform_bus_type.
> 
> Signed-off-by: Lijun Pan <Lijun.Pan@freescale.com>
> ---
>  drivers/staging/fsl-mc/include/mc.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/staging/fsl-mc/include/mc.h b/drivers/staging/fsl-mc/include/mc.h
> index a933291..483763e 100644
> --- a/drivers/staging/fsl-mc/include/mc.h
> +++ b/drivers/staging/fsl-mc/include/mc.h
> @@ -14,6 +14,7 @@
>  #include <linux/device.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/list.h>
> +#include <linux/platform_device.h>
>  #include "../include/dprc.h"
>  
>  #define FSL_MC_VENDOR_FREESCALE	0x1957
> @@ -109,6 +110,15 @@ struct fsl_mc_resource {
>  #define FSL_MC_IS_DPRC	0x0001
>  
>  /**
> +  * root dprc's parent is a platform device
> +  * that platform device's bus type is platform_bus_type.
> +  */
> +#define is_root_dprc(dev) \
> +	((to_fsl_mc_device(dev)->flags & FSL_MC_IS_DPRC) && \
> +	((dev)->bus == &fsl_mc_bus_type) && \
> +	((dev)->parent->bus == &platform_bus_type))
> +

It's best to make this type of thing a static inline function, to ensure
you get the correct typechecking.

thanks,

greg k-h

Re: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver

[Dan Carpenter]

A few style issues and error handling bugs.  See below.

On Sun, Oct 25, 2015 at 05:41:23PM -0500, Lijun Pan wrote:
> +static int fsl_mc_restool_dev_open(struct inode *inode, struct file *filep)
> +{
> +	struct fsl_mc_device *root_mc_dev;
> +	int error = 0;
> +	struct fsl_mc_io *dynamic_mc_io = NULL;
> +	struct restool_misc *restool_misc;
> +	struct restool_misc *restool_misc_cursor;
> +
> +	pr_debug("%s: inode's dev_t == %u\n", __func__, inode->i_rdev);
> +
> +	list_for_each_entry(restool_misc_cursor, &misc_list, list) {
> +		if (restool_misc_cursor->miscdevt == inode->i_rdev) {
> +			pr_debug("%s: Found the restool_misc\n", __func__);
> +			restool_misc = restool_misc_cursor;
> +			break;
> +		}
> +	}
> +
> +	if (!restool_misc)
> +		return -EINVAL;
> +
> +	if (WARN_ON(restool_misc->dev == NULL))
> +		return -EINVAL;
> +
> +	mutex_lock(&restool_misc->mutex);
> +
> +	if (!restool_misc->static_instance_in_use) {
> +		restool_misc->static_instance_in_use = true;
> +		filep->private_data = restool_misc->static_mc_io;
> +	} else {
> +		dynamic_mc_io = kzalloc(sizeof(struct fsl_mc_io), GFP_KERNEL);
> +		if (dynamic_mc_io == NULL) {
> +			error = -ENOMEM;
> +			goto error;
> +		}
> +
> +		root_mc_dev = to_fsl_mc_device(restool_misc->dev);
> +		error = fsl_mc_portal_allocate(root_mc_dev, 0, &dynamic_mc_io);
> +		if (error < 0) {
> +			pr_err("Not able to allocate MC portal\n");
> +			goto error;
> +		}
> +		++restool_misc->dynamic_instance_count;
> +		filep->private_data = dynamic_mc_io;
> +	}
> +
> +	mutex_unlock(&restool_misc->mutex);
> +
> +	return 0;
> +error:
> +	if (dynamic_mc_io != NULL) {
> +		fsl_mc_portal_free(dynamic_mc_io);
> +		kfree(dynamic_mc_io);
> +	}
> +
> +	mutex_unlock(&restool_misc->mutex);
> +
> +	return error;
> +}

Don't do One Err style error handling where there is only one error
label.  It is bug prone.  In this example if we call
fsl_mc_portal_free() when fsl_mc_portal_allocate() failed then it can
trigger a WARN_ON().  The handling.fsl_mc_portal_allocate() function has
some very complicated error handling so this probably causes a crash but
I am too last to untangle it...  Anyway do it like this:

	if (WARN_ON(restool_misc->dev == NULL))
		return -EINVAL;

	mutex_lock(&restool_misc->mutex);

	if (!restool_misc->static_instance_in_use) {
		restool_misc->static_instance_in_use = true;
		filep->private_data = restool_misc->static_mc_io;
	} else {
		dynamic_mc_io = kzalloc(sizeof(struct fsl_mc_io), GFP_KERNEL);
		if (dynamic_mc_io == NULL) {
			error = -ENOMEM;
			goto err_unlock;
		}

		root_mc_dev = to_fsl_mc_device(restool_misc->dev);
		error = fsl_mc_portal_allocate(root_mc_dev, 0, &dynamic_mc_io);
		if (error < 0) {
			pr_err("Not able to allocate MC portal\n");
			goto free_dynamic_mc_io;
		}
		++restool_misc->dynamic_instance_count;
		filep->private_data = dynamic_mc_io;
	}

	mutex_unlock(&restool_misc->mutex);

	return 0;

free_dynamic_mc_io:
	kfree(dynamic_mc_io);
err_unlock:
	mutex_unlock(&restool_misc->mutex);

	return error;
}

When you write it like this then 1) we don't free anything that we have
not allocated.  2)  There are no if statements or indenting so it is
simple.  The reason One Err style error handling is bug prone is that
when you combine all the error paths and try to handle them all at the
same time it's more complicated than doing one thing at a time.

> +static int restool_send_mc_command(unsigned long arg,
> +				struct fsl_mc_io *local_mc_io)
> +{
> +	int error = -EINVAL;

No need.

> +	struct mc_command mc_cmd;
> +
> +	error = copy_from_user(&mc_cmd, (void __user *)arg, sizeof(mc_cmd));
> +	if (error < 0) {
> +		pr_err("copy_to_user() failed with error %d\n", error);
> +		goto error;
> +	}

copy_from_user() returns the number of bytes NOT copied, it doesn't
return a negative number.  Don't print an error if the copy fails
because users can trigger it and fill /var/log/messages.  Do it like
this:

	if (copy_from_user(&mc_cmd, (void __user *)arg, sizeof(mc_cmd)))
		return -EFAULT;

> +
> +	/*
> +	 * Send MC command to the MC:
> +	 */
> +	error = mc_send_command(local_mc_io, &mc_cmd);
> +	if (error < 0)
> +		goto error;
> +
> +	error = copy_to_user((void __user *)arg, &mc_cmd, sizeof(mc_cmd));
> +	if (error < 0) {
> +		pr_err("copy_to_user() failed with error %d\n", error);
> +		goto error;
> +	}

Same.

> +
> +	return 0;
> +error:
> +	return error;

Ugh..  Don't do do-nothing gotos.  It's just a waste of time and does
not prevent future bugs.  I have examined this, by looking through git
history at locking bugs.

> +}
> +
> +static long
> +fsl_mc_restool_dev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	int error = 0;

No need.  Initializing variables to bogus values silences the GCC
warning about uninitialized variables and has caused some bugs.

> +
> +	switch (cmd) {
> +	case RESTOOL_DPRC_SYNC:
> +		pr_debug("syncing...\n");
> +		error = restool_dprc_sync(file->f_inode);
> +		pr_debug("syncing finished...\n");
> +		break;
> +	case RESTOOL_SEND_MC_COMMAND:
> +		error = restool_send_mc_command(arg, file->private_data);
> +		break;
> +	default:
> +		pr_err("%s: unexpected ioctl call number\n", __func__);
> +		error = -EINVAL;
> +	}
> +
> +	return error;
> +}
> +
> +static const struct file_operations fsl_mc_restool_dev_fops = {
> +	.owner = THIS_MODULE,
> +	.open = fsl_mc_restool_dev_open,
> +	.release = fsl_mc_restool_dev_release,
> +	.unlocked_ioctl = fsl_mc_restool_dev_ioctl,
> +};
> +
> +static int restool_add_device_file(struct device *dev)
> +{
> +	uint32_t name1 = 0;
> +	char name2[20] = {0};
> +	int error = 0;
> +	struct fsl_mc_device *root_mc_dev;
> +	struct restool_misc *restool_misc;
> +
> +	pr_debug("newly scanned/notified device: %s, whose parent:%s\n",
> +		 dev_name(dev), dev_name(dev->parent));
> +
> +	if (dev->bus == &platform_bus_type && dev->driver_data) {
> +		if (sscanf(dev_name(dev), "%x.%s", &name1, name2) != 2) {
> +			pr_err("sscanf failure\n");

Remove this printk.

> +			return -EINVAL;
> +		}
> +		if (strcmp(name2, "fsl-mc") == 0)
> +			pr_debug("platform's root dprc name is: %s\n",
> +			dev_name(&(((struct fsl_mc *)(dev->driver_data))
> +			->root_mc_bus_dev->dev)));

The indenting here is nasty.

> +	}
> +
> +	if (dev->bus == &fsl_mc_bus_type)
> +		pr_debug("%s's bus type: fsl_mc_bus_type\n", dev_name(dev));
> +	else if (dev->bus == &platform_bus_type)
> +		pr_debug("%s's bus type: platform_bus_type\n", dev_name(dev));
> +	else
> +		pr_debug("%s's bus type: NEITHER fsl_mc_bus_type NOR platform_bus_type\n",
> +			 dev_name(dev));
> +
> +	if (is_root_dprc(dev)) {

Flip this condition around and pull everything in one indent level.

> +		pr_debug("I am root dprc, create /dev/%s\n", dev_name(dev));
> +		restool_misc = kzalloc(sizeof(struct restool_misc), GFP_KERNEL);
> +		if (restool_misc == NULL)
> +			return -ENOMEM;
> +
> +		restool_misc->dev = dev;
> +		root_mc_dev = to_fsl_mc_device(dev);
> +		error = fsl_mc_portal_allocate(root_mc_dev, 0,
> +				&restool_misc->static_mc_io);
> +		if (error < 0) {
> +			pr_err("Not able to allocate MC portal\n");
> +			goto err_portal;

Label the names after what the label does not the goto location.  I'm
here at the goto location so I already know that the portal allocation
failed, I want to know what the goto will do.

If fsl_mc_portal_allocate() fails, it cleans up after itself.  This code
is buggy.  Just do:  goto free_restool_misc;

> +		}
> +
> +		restool_misc->misc.minor = MISC_DYNAMIC_MINOR;
> +		restool_misc->misc.name = dev_name(dev);
> +		restool_misc->misc.fops = &fsl_mc_restool_dev_fops;
> +
> +		error = misc_register(&restool_misc->misc);
> +		if (error < 0) {
> +			pr_err("misc_register() failed: %d\n", error);
> +			goto err_reg;

This should be goto free_portal.  If misc_register() fails, then we
do not need to call misc_deregister().  It is a bugy.

> +		}
> +
> +		restool_misc->miscdevt = restool_misc->misc.this_device->devt;
> +		mutex_init(&restool_misc->mutex);
> +		list_add(&restool_misc->list, &misc_list);
> +		pr_info("/dev/%s driver registered\n", dev_name(dev));
> +	} else
> +		pr_info("%s is not root dprc, miscdevice cannot be created/associated\n",
> +			dev_name(dev));
> +
> +	return 0;
> +err_reg:
> +	misc_deregister(&restool_misc->misc);
> +
> +err_portal:
> +	if (restool_misc->static_mc_io)
> +		fsl_mc_portal_free(restool_misc->static_mc_io);
> +	if (restool_misc)
> +		kfree(restool_misc);
> +
> +	return error;
> +}
> +
> +static int restool_bus_notifier(struct notifier_block *nb,
> +			      unsigned long action, void *data)
> +{
> +	struct device *dev = data;
> +
> +	pr_debug("entering %s...\n", __func__);

Too much debug code.  Also this can be done with ftrace.

> +	pr_debug("being notified by device: %s\n", dev_name(dev));
> +
> +	if (dev->bus == &fsl_mc_bus_type)
> +		pr_debug("%s's bus type: fsl_mc_bus_type\n", dev_name(dev));
> +	else if (dev->bus == &platform_bus_type)
> +		pr_debug("%s's bus type: platform_bus_type\n", dev_name(dev));
> +	else
> +		pr_debug("%s's bus type: NEITHER fsl_mc_bus_type NOR platform_bus_type\n",
> +			 dev_name(dev));
> +
> +	switch (action) {
> +	case BUS_NOTIFY_ADD_DEVICE:
> +		pr_info("bus notify device added: %s\n", dev_name(dev));
> +		restool_add_device_file(dev);

Add error handling.

> +		break;
> +	case BUS_NOTIFY_DEL_DEVICE:
> +		pr_info("bus notify device to be removed: %s\n", dev_name(dev));
> +		break;
> +	case BUS_NOTIFY_REMOVED_DEVICE:
> +		pr_info("bus notify device removed: %s\n", dev_name(dev));
> +		break;
> +	case BUS_NOTIFY_BIND_DRIVER:
> +		pr_info("bus notify driver about to be bound to device: %s\n", dev_name(dev));
> +		break;
> +	case BUS_NOTIFY_BOUND_DRIVER:
> +		pr_info("bus notify driver bound to device: %s\n", dev_name(dev));
> +		break;
> +	case BUS_NOTIFY_UNBIND_DRIVER:
> +		pr_info("bus notify driver about to unbind from device: %s\n", dev_name(dev));
> +		break;
> +	case BUS_NOTIFY_UNBOUND_DRIVER:
> +		pr_info("bus notify driver unbind from device: %s\n", dev_name(dev));
> +		break;
> +	default:
> +		pr_err("%s: unrecognized device action from %s\n", __func__, dev_name(dev));
> +		return -EINVAL;
> +	}
> +
> +	pr_debug("leaving %s...\n", __func__);

The whole rest of this function is just debugging stubs...  Does this
driver work yet?

> +
> +	return 0;
> +}
> +
> +static int add_to_restool(struct device *dev, void *data)
> +{
> +	pr_debug("verify *data: %s\n", (char *)data);
> +	restool_add_device_file(dev);
> +	return 0;
> +}
> +
> +static int __init fsl_mc_restool_driver_init(void)
> +{
> +	int error = 0;
> +	struct notifier_block *nb;
> +	char *data = "Add me to device file if I am a root dprc";
> +
> +	nb = kzalloc(sizeof(struct notifier_block), GFP_KERNEL);
> +	if (!nb)
> +		return -ENOMEM;
> +
> +	nb->notifier_call = restool_bus_notifier;
> +	pr_debug("restool will register notifier...\n");
> +	error = bus_register_notifier(&fsl_mc_bus_type, nb);
> +	pr_debug("restool finish register notifier...\n");
> +
> +	if (error) {
> +		kfree(nb);
> +		return error;
> +	}

The debug printks are so many that they obscure the code.  Put the error
hanling next to the function call.  Use a goto for error handling.

	nb->notifier_call = restool_bus_notifier;
	error = bus_register_notifier(&fsl_mc_bus_type, nb);
	if (error)
		goto free_nb;



> +
> +	pr_debug("restool scan bus for each device...\n");
> +	/*
> +	 * This driver runs after fsl-mc bus driver runs.
> +	 * Hence, many of the root dprcs are already attached to fsl-mc bus
> +	 * In order to make sure we find all the root dprcs,
> +	 * we need to scan the fsl_mc_bus_type.
> +	 */
> +	error  = bus_for_each_dev(&fsl_mc_bus_type, NULL, data, add_to_restool);

The add_to_restool() function ignores errors, but here we are saying
it can fail.  Probably, lets fix add_to_restool().

> +	if (error) {
> +		bus_unregister_notifier(&fsl_mc_bus_type, nb);
> +		kfree(nb);
> +		pr_err("restool driver registration failure\n");
> +		return error;
> +	}
> +	pr_debug("end restool scan bus for each device...\n");
> +
> +	return 0;
> +}
> +
> +module_init(fsl_mc_restool_driver_init);
> +
> +static void __exit fsl_mc_restool_driver_exit(void)
> +{
> +	struct restool_misc *restool_misc;
> +	struct restool_misc *restool_misc_tmp;
> +	char name1[20] = {0};
> +	uint32_t name2 = 0;
> +
> +	list_for_each_entry_safe(restool_misc, restool_misc_tmp,
> +				 &misc_list, list) {
> +		if (sscanf(restool_misc->misc.name, "%4s.%u", name1, &name2)
> +		    != 2) {
> +			pr_err("sscanf failure\n");

Delete the printk.

> +			return;

Should this be a continue?

> +		}
> +		pr_debug("name1=%s,name2=%u\n", name1, name2);
> +		pr_debug("misc-device: %s\n", restool_misc->misc.name);
> +		if (strcmp(name1, "dprc") == 0) {

Flip this around.

		if (strcmp(name1, "dprc") != 0)
			continue;

> +			if (WARN_ON(
> +			    restool_misc->static_mc_io == NULL))

This fits in 80 characters so no need for a line break.  And also these
days the prefered style is:

			if (WARN_ON(!restool_misc->static_mc_io))

> +				return;
> +
> +			if (WARN_ON(restool_misc->dynamic_instance_count != 0))
> +				return;
> +
> +			if (WARN_ON(restool_misc->static_instance_in_use))
> +				return;
> +
> +			misc_deregister(&restool_misc->misc);
> +			pr_info("/dev/%s driver unregistered\n",
> +				restool_misc->misc.name);
> +			fsl_mc_portal_free(
> +				restool_misc->static_mc_io);

This fits in 80 characters.

> +			list_del(&restool_misc->list);
> +			kfree(restool_misc);
> +		}
> +	}
> +}

regards,
dan carpenter

RE: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver

[Stuart Yoder]

> -----Original Message-----
> From: Lijun Pan [mailto:Lijun.Pan@freescale.com]
> Sent: Sunday, October 25, 2015 5:41 PM
> To: gregkh@linuxfoundation.org; arnd@arndb.de; devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org
> Cc: Yoder Stuart-B08248; katz Itai-RM05202; Rivera Jose-B46482; Li Yang-Leo-R58472; Wood Scott-B07421;
> agraf@suse.de; Hamciuc Bogdan-BHAMCIU1; Marginean Alexandru-R89243; Sharma Bhupesh-B45370; Erez Nir-RM30794;
> Schmitt Richard-B43082; dan.carpenter@oracle.com; Pan Lijun-B44306
> Subject: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver
> 
> The kernel support for the restool (a user space resource management
> tool) is a driver for the /dev/dprc.N device file.
> Its purpose is to provide an ioctl interface,
> which the restool uses to interact with the MC bus driver
> and with the MC firmware.
> We allocate a dpmcp at driver initialization,
> and keep that dpmcp until driver exit.
> We use that dpmcp by default.
> If that dpmcp is in use, we create another portal at run time
> and destroy the newly created portal after use.
> The ioctl RESTOOL_SEND_MC_COMMAND sends user space command to fsl-mc
> bus and utilizes the fsl-mc bus to communicate with MC firmware.
> The ioctl RESTOOL_DPRC_SYNC request the mc-bus launch
> objects scan under root dprc.
> In order to support multiple root dprc, we utilize the bus notify
> mechanism to scan fsl_mc_bus_type for the newly added root dprc.
> After discovering the root dprc, it creates a miscdevice
> /dev/dprc.N to associate with this root dprc.
> 
> Signed-off-by: Lijun Pan <Lijun.Pan@freescale.com>
> ---
>  drivers/staging/fsl-mc/bus/Kconfig      |   7 +-
>  drivers/staging/fsl-mc/bus/Makefile     |   3 +
>  drivers/staging/fsl-mc/bus/mc-ioctl.h   |  24 ++
>  drivers/staging/fsl-mc/bus/mc-restool.c | 488 ++++++++++++++++++++++++++++++++
>  4 files changed, 521 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/staging/fsl-mc/bus/mc-ioctl.h
>  create mode 100644 drivers/staging/fsl-mc/bus/mc-restool.c
> 
> diff --git a/drivers/staging/fsl-mc/bus/Kconfig b/drivers/staging/fsl-mc/bus/Kconfig
> index 0d779d9..39c6ef9 100644
> --- a/drivers/staging/fsl-mc/bus/Kconfig
> +++ b/drivers/staging/fsl-mc/bus/Kconfig
> @@ -21,4 +21,9 @@ config FSL_MC_BUS
>  	  Only enable this option when building the kernel for
>  	  Freescale QorQIQ LS2xxxx SoCs.
> 
> -
> +config FSL_MC_RESTOOL
> +        tristate "Freescale Management Complex (MC) restool driver"
> +        depends on FSL_MC_BUS
> +        help
> +          Driver that provides kernel support for the Freescale Management
> +	  Complex resource manager user-space tool.
> diff --git a/drivers/staging/fsl-mc/bus/Makefile b/drivers/staging/fsl-mc/bus/Makefile
> index 25433a9..28b5fc0 100644
> --- a/drivers/staging/fsl-mc/bus/Makefile
> +++ b/drivers/staging/fsl-mc/bus/Makefile
> @@ -15,3 +15,6 @@ mc-bus-driver-objs := mc-bus.o \
>  		      mc-allocator.o \
>  		      dpmcp.o \
>  		      dpbp.o
> +
> +# MC restool kernel support
> +obj-$(CONFIG_FSL_MC_RESTOOL) += mc-restool.o
> diff --git a/drivers/staging/fsl-mc/bus/mc-ioctl.h b/drivers/staging/fsl-mc/bus/mc-ioctl.h
> new file mode 100644
> index 0000000..e52f907
> --- /dev/null
> +++ b/drivers/staging/fsl-mc/bus/mc-ioctl.h
> @@ -0,0 +1,24 @@
> +/*
> + * Freescale Management Complex (MC) ioclt interface
> + *
> + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> + * Author: Lijun Pan <Lijun.Pan@freescale.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +#ifndef _FSL_MC_IOCTL_H_
> +#define _FSL_MC_IOCTL_H_
> +
> +#include <linux/ioctl.h>
> +
> +#define RESTOOL_IOCTL_TYPE   'R'
> +
> +#define RESTOOL_DPRC_SYNC \
> +	_IO(RESTOOL_IOCTL_TYPE, 0x2)

We no longer need a sync ioctl, as a the /sys/bus/fsl-mc/rescan mechanism
in the previous patch replaces this.

> +#define RESTOOL_SEND_MC_COMMAND \
> +	_IOWR(RESTOOL_IOCTL_TYPE, 0x4, struct mc_command)
> +
> +#endif /* _FSL_MC_IOCTL_H_ */
> diff --git a/drivers/staging/fsl-mc/bus/mc-restool.c b/drivers/staging/fsl-mc/bus/mc-restool.c
> new file mode 100644
> index 0000000..a219172
> --- /dev/null
> +++ b/drivers/staging/fsl-mc/bus/mc-restool.c
> @@ -0,0 +1,488 @@
> +/*
> + * Freescale Management Complex (MC) restool driver
> + *
> + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> + * Author: Lijun Pan <Lijun.Pan@freescale.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include "../include/mc-private.h"
> +#include <linux/module.h>
> +#include <linux/fs.h>
> +#include <linux/miscdevice.h>
> +#include <linux/mm.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include "mc-ioctl.h"
> +#include "../include/mc-sys.h"
> +#include "../include/mc-cmd.h"
> +#include "../include/dpmng.h"
> +
> +/**
> + * Maximum number of DPRCs that can be opened at the same time
> + */
> +#define MAX_DPRC_HANDLES	    64
> +
> +/**
> + * restool_misc - information associated with the newly added miscdevice
> + * @misc: newly created miscdevice associated with root dprc
> + * @miscdevt: device id of this miscdevice
> + * @list: a linked list node representing this miscdevcie
> + * @static_mc_io: pointer to the static MC I/O object used by the restool
> + * @dynamic_instance_count: number of dynamically created instances
> + * @static_instance_in_use: static instance is in use or not
> + * @mutex: mutex lock to serialze the operations
> + * @dev: root dprc associated with this miscdevice
> + */
> +struct restool_misc {
> +	struct miscdevice misc;
> +	dev_t miscdevt;
> +	struct list_head list;
> +	struct fsl_mc_io *static_mc_io;
> +	uint32_t dynamic_instance_count;
> +	bool static_instance_in_use;
> +	struct mutex mutex;
> +	struct device *dev;
> +};
> +
> +/*
> + * initialize a global list to link all
> + * the miscdevice nodes (struct restool_misc)
> + */
> +LIST_HEAD(misc_list);
> +
> +static int fsl_mc_restool_dev_open(struct inode *inode, struct file *filep)
> +{
> +	struct fsl_mc_device *root_mc_dev;
> +	int error = 0;
> +	struct fsl_mc_io *dynamic_mc_io = NULL;
> +	struct restool_misc *restool_misc;
> +	struct restool_misc *restool_misc_cursor;
> +
> +	pr_debug("%s: inode's dev_t == %u\n", __func__, inode->i_rdev);
> +
> +	list_for_each_entry(restool_misc_cursor, &misc_list, list) {
> +		if (restool_misc_cursor->miscdevt == inode->i_rdev) {
> +			pr_debug("%s: Found the restool_misc\n", __func__);
> +			restool_misc = restool_misc_cursor;
> +			break;
> +		}
> +	}
> +
> +	if (!restool_misc)
> +		return -EINVAL;
> +
> +	if (WARN_ON(restool_misc->dev == NULL))
> +		return -EINVAL;
> +
> +	mutex_lock(&restool_misc->mutex);
> +
> +	if (!restool_misc->static_instance_in_use) {
> +		restool_misc->static_instance_in_use = true;
> +		filep->private_data = restool_misc->static_mc_io;
> +	} else {
> +		dynamic_mc_io = kzalloc(sizeof(struct fsl_mc_io), GFP_KERNEL);
> +		if (dynamic_mc_io == NULL) {
> +			error = -ENOMEM;
> +			goto error;
> +		}
> +
> +		root_mc_dev = to_fsl_mc_device(restool_misc->dev);
> +		error = fsl_mc_portal_allocate(root_mc_dev, 0, &dynamic_mc_io);
> +		if (error < 0) {
> +			pr_err("Not able to allocate MC portal\n");
> +			goto error;
> +		}
> +		++restool_misc->dynamic_instance_count;
> +		filep->private_data = dynamic_mc_io;
> +	}
> +
> +	mutex_unlock(&restool_misc->mutex);
> +
> +	return 0;
> +error:
> +	if (dynamic_mc_io != NULL) {
> +		fsl_mc_portal_free(dynamic_mc_io);
> +		kfree(dynamic_mc_io);
> +	}
> +
> +	mutex_unlock(&restool_misc->mutex);
> +
> +	return error;
> +}
> +
> +static int fsl_mc_restool_dev_release(struct inode *inode, struct file *filep)
> +{
> +	struct fsl_mc_io *local_mc_io = filep->private_data;
> +	struct restool_misc *restool_misc;
> +	struct restool_misc *restool_misc_cursor;
> +
> +	if (WARN_ON(filep->private_data == NULL))
> +		return -EINVAL;
> +
> +	pr_debug("%s: inode's dev_t == %u\n", __func__, inode->i_rdev);
> +
> +	list_for_each_entry(restool_misc_cursor, &misc_list, list) {
> +		if (restool_misc_cursor->miscdevt == inode->i_rdev) {
> +			pr_debug("%s: Found the restool_misc\n", __func__);
> +			restool_misc = restool_misc_cursor;
> +			break;
> +		}
> +	}
> +
> +	if (!restool_misc)
> +		return -EINVAL;
> +
> +	mutex_lock(&restool_misc->mutex);
> +
> +	if (WARN_ON(restool_misc->dynamic_instance_count == 0 &&
> +	    !restool_misc->static_instance_in_use)) {
> +		mutex_unlock(&restool_misc->mutex);
> +		return -EINVAL;
> +	}
> +
> +	/* Globally clean up opened/untracked handles */
> +	fsl_mc_portal_reset(local_mc_io);
> +
> +	pr_debug("dynamic instance count: %d\n",
> +		restool_misc->dynamic_instance_count);
> +	pr_debug("static instance count: %d\n",
> +		restool_misc->static_instance_in_use);
> +
> +	/*
> +	 * must check
> +	 * whether local_mc_io is dynamic or static instance
> +	 * Otherwise it will free up the reserved portal by accident
> +	 * or even not free up the dynamic allocated portal
> +	 * if 2 or more instances running concurrently
> +	 */
> +	if (local_mc_io == restool_misc->static_mc_io) {
> +		pr_debug("this is reserved portal");
> +		pr_debug("reserved portal not in use\n");
> +		restool_misc->static_instance_in_use = false;
> +	} else {
> +		pr_debug("this is dynamically allocated  portal");
> +		pr_debug("free one dynamically allocated portal\n");
> +		fsl_mc_portal_free(local_mc_io);
> +		kfree(filep->private_data);
> +		--restool_misc->dynamic_instance_count;
> +	}
> +
> +	filep->private_data = NULL;
> +	mutex_unlock(&restool_misc->mutex);
> +
> +	return 0;
> +}
> +
> +static int restool_dprc_sync(struct inode *inode)
> +{
> +	int error = 0;
> +	struct fsl_mc_device *root_mc_dev;
> +	struct fsl_mc_bus *root_mc_bus;
> +	struct restool_misc *restool_misc;
> +	struct restool_misc *restool_misc_cursor;
> +
> +	pr_debug("%s: inode's dev_t == %u\n", __func__, inode->i_rdev);
> +
> +	list_for_each_entry(restool_misc_cursor, &misc_list, list) {
> +		if (restool_misc_cursor->miscdevt == inode->i_rdev) {
> +			pr_debug("%s: Found the restool_misc\n", __func__);
> +			restool_misc = restool_misc_cursor;
> +			break;
> +		}
> +	}
> +
> +	if (!restool_misc)
> +		return -EINVAL;
> +
> +	root_mc_dev = to_fsl_mc_device(restool_misc->dev);
> +	root_mc_bus = to_fsl_mc_bus(root_mc_dev);
> +
> +	mutex_lock(&root_mc_bus->scan_mutex);
> +	error = dprc_scan_objects(root_mc_dev);
> +	mutex_unlock(&root_mc_bus->scan_mutex);
> +	pr_debug("sync_error = %d\n", error);
> +
> +	return error;
> +}

We no longer need/use the ioctl interface for sync.

> +static int restool_send_mc_command(unsigned long arg,
> +				struct fsl_mc_io *local_mc_io)
> +{
> +	int error = -EINVAL;
> +	struct mc_command mc_cmd;
> +
> +	error = copy_from_user(&mc_cmd, (void __user *)arg, sizeof(mc_cmd));
> +	if (error < 0) {
> +		pr_err("copy_to_user() failed with error %d\n", error);
> +		goto error;
> +	}
> +
> +	/*
> +	 * Send MC command to the MC:
> +	 */
> +	error = mc_send_command(local_mc_io, &mc_cmd);
> +	if (error < 0)
> +		goto error;
> +
> +	error = copy_to_user((void __user *)arg, &mc_cmd, sizeof(mc_cmd));
> +	if (error < 0) {
> +		pr_err("copy_to_user() failed with error %d\n", error);
> +		goto error;
> +	}
> +
> +	return 0;
> +error:
> +	return error;
> +}
> +
> +static long
> +fsl_mc_restool_dev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	int error = 0;
> +
> +	switch (cmd) {
> +	case RESTOOL_DPRC_SYNC:
> +		pr_debug("syncing...\n");
> +		error = restool_dprc_sync(file->f_inode);
> +		pr_debug("syncing finished...\n");
> +		break;

Remove the sync ioctl.

> +	case RESTOOL_SEND_MC_COMMAND:
> +		error = restool_send_mc_command(arg, file->private_data);
> +		break;
> +	default:
> +		pr_err("%s: unexpected ioctl call number\n", __func__);
> +		error = -EINVAL;
> +	}
> +
> +	return error;
> +}
> +
> +static const struct file_operations fsl_mc_restool_dev_fops = {
> +	.owner = THIS_MODULE,
> +	.open = fsl_mc_restool_dev_open,
> +	.release = fsl_mc_restool_dev_release,
> +	.unlocked_ioctl = fsl_mc_restool_dev_ioctl,
> +};
> +
> +static int restool_add_device_file(struct device *dev)
> +{
> +	uint32_t name1 = 0;
> +	char name2[20] = {0};
> +	int error = 0;
> +	struct fsl_mc_device *root_mc_dev;
> +	struct restool_misc *restool_misc;
> +
> +	pr_debug("newly scanned/notified device: %s, whose parent:%s\n",
> +		 dev_name(dev), dev_name(dev->parent));
> +
> +	if (dev->bus == &platform_bus_type && dev->driver_data) {
> +		if (sscanf(dev_name(dev), "%x.%s", &name1, name2) != 2) {
> +			pr_err("sscanf failure\n");
> +			return -EINVAL;
> +		}
> +		if (strcmp(name2, "fsl-mc") == 0)
> +			pr_debug("platform's root dprc name is: %s\n",
> +			dev_name(&(((struct fsl_mc *)(dev->driver_data))
> +			->root_mc_bus_dev->dev)));
> +	}
> +
> +	if (dev->bus == &fsl_mc_bus_type)
> +		pr_debug("%s's bus type: fsl_mc_bus_type\n", dev_name(dev));
> +	else if (dev->bus == &platform_bus_type)
> +		pr_debug("%s's bus type: platform_bus_type\n", dev_name(dev));
> +	else
> +		pr_debug("%s's bus type: NEITHER fsl_mc_bus_type NOR platform_bus_type\n",
> +			 dev_name(dev));
> +
> +	if (is_root_dprc(dev)) {
> +		pr_debug("I am root dprc, create /dev/%s\n", dev_name(dev));
> +		restool_misc = kzalloc(sizeof(struct restool_misc), GFP_KERNEL);
> +		if (restool_misc == NULL)
> +			return -ENOMEM;
> +
> +		restool_misc->dev = dev;
> +		root_mc_dev = to_fsl_mc_device(dev);
> +		error = fsl_mc_portal_allocate(root_mc_dev, 0,
> +				&restool_misc->static_mc_io);
> +		if (error < 0) {
> +			pr_err("Not able to allocate MC portal\n");
> +			goto err_portal;
> +		}
> +
> +		restool_misc->misc.minor = MISC_DYNAMIC_MINOR;
> +		restool_misc->misc.name = dev_name(dev);
> +		restool_misc->misc.fops = &fsl_mc_restool_dev_fops;
> +
> +		error = misc_register(&restool_misc->misc);
> +		if (error < 0) {
> +			pr_err("misc_register() failed: %d\n", error);
> +			goto err_reg;
> +		}
> +
> +		restool_misc->miscdevt = restool_misc->misc.this_device->devt;
> +		mutex_init(&restool_misc->mutex);
> +		list_add(&restool_misc->list, &misc_list);
> +		pr_info("/dev/%s driver registered\n", dev_name(dev));
> +	} else
> +		pr_info("%s is not root dprc, miscdevice cannot be created/associated\n",
> +			dev_name(dev));
> +
> +	return 0;
> +err_reg:
> +	misc_deregister(&restool_misc->misc);
> +
> +err_portal:
> +	if (restool_misc->static_mc_io)
> +		fsl_mc_portal_free(restool_misc->static_mc_io);
> +	if (restool_misc)
> +		kfree(restool_misc);
> +
> +	return error;
> +}
> +
> +static int restool_bus_notifier(struct notifier_block *nb,
> +			      unsigned long action, void *data)
> +{
> +	struct device *dev = data;
> +
> +	pr_debug("entering %s...\n", __func__);
> +	pr_debug("being notified by device: %s\n", dev_name(dev));
> +
> +	if (dev->bus == &fsl_mc_bus_type)
> +		pr_debug("%s's bus type: fsl_mc_bus_type\n", dev_name(dev));
> +	else if (dev->bus == &platform_bus_type)
> +		pr_debug("%s's bus type: platform_bus_type\n", dev_name(dev));
> +	else
> +		pr_debug("%s's bus type: NEITHER fsl_mc_bus_type NOR platform_bus_type\n",
> +			 dev_name(dev));
> +
> +	switch (action) {
> +	case BUS_NOTIFY_ADD_DEVICE:
> +		pr_info("bus notify device added: %s\n", dev_name(dev));
> +		restool_add_device_file(dev);

The only time we want to do add a new device file is for root DPRCs.

This code would seem to add a device file any time any device is
added to the bus?  How are you filtering out all the other
object types?...it's not clear to me.

> +		break;
> +	case BUS_NOTIFY_DEL_DEVICE:
> +		pr_info("bus notify device to be removed: %s\n", dev_name(dev));
> +		break;
> +	case BUS_NOTIFY_REMOVED_DEVICE:
> +		pr_info("bus notify device removed: %s\n", dev_name(dev));
> +		break;
> +	case BUS_NOTIFY_BIND_DRIVER:
> +		pr_info("bus notify driver about to be bound to device: %s\n", dev_name(dev));
> +		break;
> +	case BUS_NOTIFY_BOUND_DRIVER:
> +		pr_info("bus notify driver bound to device: %s\n", dev_name(dev));
> +		break;
> +	case BUS_NOTIFY_UNBIND_DRIVER:
> +		pr_info("bus notify driver about to unbind from device: %s\n", dev_name(dev));
> +		break;
> +	case BUS_NOTIFY_UNBOUND_DRIVER:
> +		pr_info("bus notify driver unbind from device: %s\n", dev_name(dev));
> +		break;
> +	default:
> +		pr_err("%s: unrecognized device action from %s\n", __func__, dev_name(dev));
> +		return -EINVAL;
> +	}
> +
> +	pr_debug("leaving %s...\n", __func__);
> +
> +	return 0;
> +}

There is only a single line of functionality in the above function.  All the
rest pr_debug and pr_info.   We certainly don't need pr_info statements
every time a bus notify occurs.

In general I think this driver has too many debug prints in it.

Thanks,
Stuart

RE: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver

[Lijun Pan]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Monday, October 26, 2015 1:20 AM
> To: Pan Lijun-B44306 <Lijun.Pan@freescale.com>
> Cc: gregkh@linuxfoundation.org; arnd@arndb.de; devel@driverdev.osuosl.org;
> linux-kernel@vger.kernel.org; Yoder Stuart-B08248
> <stuart.yoder@freescale.com>; katz Itai-RM05202 <itai.katz@freescale.com>;
> Rivera Jose-B46482 <German.Rivera@freescale.com>; Li Yang-Leo-R58472
> <LeoLi@freescale.com>; Wood Scott-B07421 <scottwood@freescale.com>;
> agraf@suse.de; Hamciuc Bogdan-BHAMCIU1 <bhamciu1@freescale.com>;
> Marginean Alexandru-R89243 <R89243@freescale.com>; Sharma Bhupesh-
> B45370 <bhupesh.sharma@freescale.com>; Erez Nir-RM30794
> <nir.erez@freescale.com>; Schmitt Richard-B43082
> <richard.schmitt@freescale.com>
> Subject: Re: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver
> 
> A few style issues and error handling bugs.  See below.
> 
> On Sun, Oct 25, 2015 at 05:41:23PM -0500, Lijun Pan wrote:
> > +static int fsl_mc_restool_dev_open(struct inode *inode, struct file
> > +*filep) {
> > +	struct fsl_mc_device *root_mc_dev;
> > +	int error = 0;
> > +	struct fsl_mc_io *dynamic_mc_io = NULL;
> > +	struct restool_misc *restool_misc;
> > +	struct restool_misc *restool_misc_cursor;
> > +
> > +	pr_debug("%s: inode's dev_t == %u\n", __func__, inode->i_rdev);
> > +
> > +	list_for_each_entry(restool_misc_cursor, &misc_list, list) {
> > +		if (restool_misc_cursor->miscdevt == inode->i_rdev) {
> > +			pr_debug("%s: Found the restool_misc\n", __func__);
> > +			restool_misc = restool_misc_cursor;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!restool_misc)
> > +		return -EINVAL;
> > +
> > +	if (WARN_ON(restool_misc->dev == NULL))
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&restool_misc->mutex);
> > +
> > +	if (!restool_misc->static_instance_in_use) {
> > +		restool_misc->static_instance_in_use = true;
> > +		filep->private_data = restool_misc->static_mc_io;
> > +	} else {
> > +		dynamic_mc_io = kzalloc(sizeof(struct fsl_mc_io),
> GFP_KERNEL);
> > +		if (dynamic_mc_io == NULL) {
> > +			error = -ENOMEM;
> > +			goto error;
> > +		}
> > +
> > +		root_mc_dev = to_fsl_mc_device(restool_misc->dev);
> > +		error = fsl_mc_portal_allocate(root_mc_dev, 0,
> &dynamic_mc_io);
> > +		if (error < 0) {
> > +			pr_err("Not able to allocate MC portal\n");
> > +			goto error;
> > +		}
> > +		++restool_misc->dynamic_instance_count;
> > +		filep->private_data = dynamic_mc_io;
> > +	}
> > +
> > +	mutex_unlock(&restool_misc->mutex);
> > +
> > +	return 0;
> > +error:
> > +	if (dynamic_mc_io != NULL) {
> > +		fsl_mc_portal_free(dynamic_mc_io);
> > +		kfree(dynamic_mc_io);
> > +	}
> > +
> > +	mutex_unlock(&restool_misc->mutex);
> > +
> > +	return error;
> > +}
> 
> Don't do One Err style error handling where there is only one error label.  It is
> bug prone.  In this example if we call
> fsl_mc_portal_free() when fsl_mc_portal_allocate() failed then it can trigger a
> WARN_ON().  The handling.fsl_mc_portal_allocate() function has some very
> complicated error handling so this probably causes a crash but I am too last to
> untangle it...  Anyway do it like this:
> 
> 	if (WARN_ON(restool_misc->dev == NULL))
> 		return -EINVAL;
> 
> 	mutex_lock(&restool_misc->mutex);
> 
> 	if (!restool_misc->static_instance_in_use) {
> 		restool_misc->static_instance_in_use = true;
> 		filep->private_data = restool_misc->static_mc_io;
> 	} else {
> 		dynamic_mc_io = kzalloc(sizeof(struct fsl_mc_io),
> GFP_KERNEL);
> 		if (dynamic_mc_io == NULL) {
> 			error = -ENOMEM;
> 			goto err_unlock;
> 		}
> 
> 		root_mc_dev = to_fsl_mc_device(restool_misc->dev);
> 		error = fsl_mc_portal_allocate(root_mc_dev, 0,
> &dynamic_mc_io);
> 		if (error < 0) {
> 			pr_err("Not able to allocate MC portal\n");
> 			goto free_dynamic_mc_io;
> 		}
> 		++restool_misc->dynamic_instance_count;
> 		filep->private_data = dynamic_mc_io;
> 	}
> 
> 	mutex_unlock(&restool_misc->mutex);
> 
> 	return 0;
> 
> free_dynamic_mc_io:
> 	kfree(dynamic_mc_io);
> err_unlock:
> 	mutex_unlock(&restool_misc->mutex);
> 
> 	return error;
> }
> 
> When you write it like this then 1) we don't free anything that we have not
> allocated.  2)  There are no if statements or indenting so it is simple.  The reason
> One Err style error handling is bug prone is that when you combine all the error
> paths and try to handle them all at the same time it's more complicated than
> doing one thing at a time.
> 
> > +static int restool_send_mc_command(unsigned long arg,
> > +				struct fsl_mc_io *local_mc_io)
> > +{
> > +	int error = -EINVAL;
> 
> No need.
> 
> > +	struct mc_command mc_cmd;
> > +
> > +	error = copy_from_user(&mc_cmd, (void __user *)arg,
> sizeof(mc_cmd));
> > +	if (error < 0) {
> > +		pr_err("copy_to_user() failed with error %d\n", error);
> > +		goto error;
> > +	}
> 
> copy_from_user() returns the number of bytes NOT copied, it doesn't return a
> negative number.  Don't print an error if the copy fails because users can trigger
> it and fill /var/log/messages.  Do it like
> this:
> 
> 	if (copy_from_user(&mc_cmd, (void __user *)arg, sizeof(mc_cmd)))
> 		return -EFAULT;
> 
> > +
> > +	/*
> > +	 * Send MC command to the MC:
> > +	 */
> > +	error = mc_send_command(local_mc_io, &mc_cmd);
> > +	if (error < 0)
> > +		goto error;
> > +
> > +	error = copy_to_user((void __user *)arg, &mc_cmd, sizeof(mc_cmd));
> > +	if (error < 0) {
> > +		pr_err("copy_to_user() failed with error %d\n", error);
> > +		goto error;
> > +	}
> 
> Same.
> 
> > +
> > +	return 0;
> > +error:
> > +	return error;
> 
> Ugh..  Don't do do-nothing gotos.  It's just a waste of time and does not
> prevent future bugs.  I have examined this, by looking through git history at
> locking bugs.
> 
> > +}
> > +
> > +static long
> > +fsl_mc_restool_dev_ioctl(struct file *file, unsigned int cmd,
> > +unsigned long arg) {
> > +	int error = 0;
> 
> No need.  Initializing variables to bogus values silences the GCC warning about
> uninitialized variables and has caused some bugs.
> 
> > +
> > +	switch (cmd) {
> > +	case RESTOOL_DPRC_SYNC:
> > +		pr_debug("syncing...\n");
> > +		error = restool_dprc_sync(file->f_inode);
> > +		pr_debug("syncing finished...\n");
> > +		break;
> > +	case RESTOOL_SEND_MC_COMMAND:
> > +		error = restool_send_mc_command(arg, file->private_data);
> > +		break;
> > +	default:
> > +		pr_err("%s: unexpected ioctl call number\n", __func__);
> > +		error = -EINVAL;
> > +	}
> > +
> > +	return error;
> > +}
> > +
> > +static const struct file_operations fsl_mc_restool_dev_fops = {
> > +	.owner = THIS_MODULE,
> > +	.open = fsl_mc_restool_dev_open,
> > +	.release = fsl_mc_restool_dev_release,
> > +	.unlocked_ioctl = fsl_mc_restool_dev_ioctl, };
> > +
> > +static int restool_add_device_file(struct device *dev) {
> > +	uint32_t name1 = 0;
> > +	char name2[20] = {0};
> > +	int error = 0;
> > +	struct fsl_mc_device *root_mc_dev;
> > +	struct restool_misc *restool_misc;
> > +
> > +	pr_debug("newly scanned/notified device: %s, whose parent:%s\n",
> > +		 dev_name(dev), dev_name(dev->parent));
> > +
> > +	if (dev->bus == &platform_bus_type && dev->driver_data) {
> > +		if (sscanf(dev_name(dev), "%x.%s", &name1, name2) != 2) {
> > +			pr_err("sscanf failure\n");
> 
> Remove this printk.
> 
> > +			return -EINVAL;
> > +		}
> > +		if (strcmp(name2, "fsl-mc") == 0)
> > +			pr_debug("platform's root dprc name is: %s\n",
> > +			dev_name(&(((struct fsl_mc *)(dev->driver_data))
> > +			->root_mc_bus_dev->dev)));
> 
> The indenting here is nasty.
> 
> > +	}
> > +
> > +	if (dev->bus == &fsl_mc_bus_type)
> > +		pr_debug("%s's bus type: fsl_mc_bus_type\n",
> dev_name(dev));
> > +	else if (dev->bus == &platform_bus_type)
> > +		pr_debug("%s's bus type: platform_bus_type\n",
> dev_name(dev));
> > +	else
> > +		pr_debug("%s's bus type: NEITHER fsl_mc_bus_type NOR
> platform_bus_type\n",
> > +			 dev_name(dev));
> > +
> > +	if (is_root_dprc(dev)) {
> 
> Flip this condition around and pull everything in one indent level.
> 
> > +		pr_debug("I am root dprc, create /dev/%s\n",
> dev_name(dev));
> > +		restool_misc = kzalloc(sizeof(struct restool_misc),
> GFP_KERNEL);
> > +		if (restool_misc == NULL)
> > +			return -ENOMEM;
> > +
> > +		restool_misc->dev = dev;
> > +		root_mc_dev = to_fsl_mc_device(dev);
> > +		error = fsl_mc_portal_allocate(root_mc_dev, 0,
> > +				&restool_misc->static_mc_io);
> > +		if (error < 0) {
> > +			pr_err("Not able to allocate MC portal\n");
> > +			goto err_portal;
> 
> Label the names after what the label does not the goto location.  I'm here at
> the goto location so I already know that the portal allocation failed, I want to
> know what the goto will do.
> 
> If fsl_mc_portal_allocate() fails, it cleans up after itself.  This code is buggy.  Just
> do:  goto free_restool_misc;
> 
> > +		}
> > +
> > +		restool_misc->misc.minor = MISC_DYNAMIC_MINOR;
> > +		restool_misc->misc.name = dev_name(dev);
> > +		restool_misc->misc.fops = &fsl_mc_restool_dev_fops;
> > +
> > +		error = misc_register(&restool_misc->misc);
> > +		if (error < 0) {
> > +			pr_err("misc_register() failed: %d\n", error);
> > +			goto err_reg;
> 
> This should be goto free_portal.  If misc_register() fails, then we do not need
> to call misc_deregister().  It is a bugy.
> 
> > +		}
> > +
> > +		restool_misc->miscdevt = restool_misc->misc.this_device-
> >devt;
> > +		mutex_init(&restool_misc->mutex);
> > +		list_add(&restool_misc->list, &misc_list);
> > +		pr_info("/dev/%s driver registered\n", dev_name(dev));
> > +	} else
> > +		pr_info("%s is not root dprc, miscdevice cannot be
> created/associated\n",
> > +			dev_name(dev));
> > +
> > +	return 0;
> > +err_reg:
> > +	misc_deregister(&restool_misc->misc);
> > +
> > +err_portal:
> > +	if (restool_misc->static_mc_io)
> > +		fsl_mc_portal_free(restool_misc->static_mc_io);
> > +	if (restool_misc)
> > +		kfree(restool_misc);
> > +
> > +	return error;
> > +}
> > +
> > +static int restool_bus_notifier(struct notifier_block *nb,
> > +			      unsigned long action, void *data) {
> > +	struct device *dev = data;
> > +
> > +	pr_debug("entering %s...\n", __func__);
> 
> Too much debug code.  Also this can be done with ftrace.
> 
> > +	pr_debug("being notified by device: %s\n", dev_name(dev));
> > +
> > +	if (dev->bus == &fsl_mc_bus_type)
> > +		pr_debug("%s's bus type: fsl_mc_bus_type\n",
> dev_name(dev));
> > +	else if (dev->bus == &platform_bus_type)
> > +		pr_debug("%s's bus type: platform_bus_type\n",
> dev_name(dev));
> > +	else
> > +		pr_debug("%s's bus type: NEITHER fsl_mc_bus_type NOR
> platform_bus_type\n",
> > +			 dev_name(dev));
> > +
> > +	switch (action) {
> > +	case BUS_NOTIFY_ADD_DEVICE:
> > +		pr_info("bus notify device added: %s\n", dev_name(dev));
> > +		restool_add_device_file(dev);
> 
> Add error handling.
> 
> > +		break;
> > +	case BUS_NOTIFY_DEL_DEVICE:
> > +		pr_info("bus notify device to be removed: %s\n",
> dev_name(dev));
> > +		break;
> > +	case BUS_NOTIFY_REMOVED_DEVICE:
> > +		pr_info("bus notify device removed: %s\n", dev_name(dev));
> > +		break;
> > +	case BUS_NOTIFY_BIND_DRIVER:
> > +		pr_info("bus notify driver about to be bound to device: %s\n",
> dev_name(dev));
> > +		break;
> > +	case BUS_NOTIFY_BOUND_DRIVER:
> > +		pr_info("bus notify driver bound to device: %s\n",
> dev_name(dev));
> > +		break;
> > +	case BUS_NOTIFY_UNBIND_DRIVER:
> > +		pr_info("bus notify driver about to unbind from device: %s\n",
> dev_name(dev));
> > +		break;
> > +	case BUS_NOTIFY_UNBOUND_DRIVER:
> > +		pr_info("bus notify driver unbind from device: %s\n",
> dev_name(dev));
> > +		break;
> > +	default:
> > +		pr_err("%s: unrecognized device action from %s\n", __func__,
> dev_name(dev));
> > +		return -EINVAL;
> > +	}
> > +
> > +	pr_debug("leaving %s...\n", __func__);
> 
> The whole rest of this function is just debugging stubs...  Does this driver work
> yet?

Yes, this driver works for a long time internally.
This driver actually helps in the resource management and debug some way.
I will take your suggestions on all the areas and
I will remove pr_debug() stuff in the next Patch v2.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int add_to_restool(struct device *dev, void *data) {
> > +	pr_debug("verify *data: %s\n", (char *)data);
> > +	restool_add_device_file(dev);
> > +	return 0;
> > +}
> > +
> > +static int __init fsl_mc_restool_driver_init(void) {
> > +	int error = 0;
> > +	struct notifier_block *nb;
> > +	char *data = "Add me to device file if I am a root dprc";
> > +
> > +	nb = kzalloc(sizeof(struct notifier_block), GFP_KERNEL);
> > +	if (!nb)
> > +		return -ENOMEM;
> > +
> > +	nb->notifier_call = restool_bus_notifier;
> > +	pr_debug("restool will register notifier...\n");
> > +	error = bus_register_notifier(&fsl_mc_bus_type, nb);
> > +	pr_debug("restool finish register notifier...\n");
> > +
> > +	if (error) {
> > +		kfree(nb);
> > +		return error;
> > +	}
> 
> The debug printks are so many that they obscure the code.  Put the error
> hanling next to the function call.  Use a goto for error handling.
> 
> 	nb->notifier_call = restool_bus_notifier;
> 	error = bus_register_notifier(&fsl_mc_bus_type, nb);
> 	if (error)
> 		goto free_nb;
> 
> 
> 
> > +
> > +	pr_debug("restool scan bus for each device...\n");
> > +	/*
> > +	 * This driver runs after fsl-mc bus driver runs.
> > +	 * Hence, many of the root dprcs are already attached to fsl-mc bus
> > +	 * In order to make sure we find all the root dprcs,
> > +	 * we need to scan the fsl_mc_bus_type.
> > +	 */
> > +	error  = bus_for_each_dev(&fsl_mc_bus_type, NULL, data,
> > +add_to_restool);
> 
> The add_to_restool() function ignores errors, but here we are saying it can fail.
> Probably, lets fix add_to_restool().
> 
> > +	if (error) {
> > +		bus_unregister_notifier(&fsl_mc_bus_type, nb);
> > +		kfree(nb);
> > +		pr_err("restool driver registration failure\n");
> > +		return error;
> > +	}
> > +	pr_debug("end restool scan bus for each device...\n");
> > +
> > +	return 0;
> > +}
> > +
> > +module_init(fsl_mc_restool_driver_init);
> > +
> > +static void __exit fsl_mc_restool_driver_exit(void) {
> > +	struct restool_misc *restool_misc;
> > +	struct restool_misc *restool_misc_tmp;
> > +	char name1[20] = {0};
> > +	uint32_t name2 = 0;
> > +
> > +	list_for_each_entry_safe(restool_misc, restool_misc_tmp,
> > +				 &misc_list, list) {
> > +		if (sscanf(restool_misc->misc.name, "%4s.%u", name1,
> &name2)
> > +		    != 2) {
> > +			pr_err("sscanf failure\n");
> 
> Delete the printk.
> 
> > +			return;
> 
> Should this be a continue?
> 
> > +		}
> > +		pr_debug("name1=%s,name2=%u\n", name1, name2);
> > +		pr_debug("misc-device: %s\n", restool_misc->misc.name);
> > +		if (strcmp(name1, "dprc") == 0) {
> 
> Flip this around.
> 
> 		if (strcmp(name1, "dprc") != 0)
> 			continue;
> 
> > +			if (WARN_ON(
> > +			    restool_misc->static_mc_io == NULL))
> 
> This fits in 80 characters so no need for a line break.  And also these days the
> prefered style is:
> 
> 			if (WARN_ON(!restool_misc->static_mc_io))
> 
> > +				return;
> > +
> > +			if (WARN_ON(restool_misc-
> >dynamic_instance_count != 0))
> > +				return;
> > +
> > +			if (WARN_ON(restool_misc->static_instance_in_use))
> > +				return;
> > +
> > +			misc_deregister(&restool_misc->misc);
> > +			pr_info("/dev/%s driver unregistered\n",
> > +				restool_misc->misc.name);
> > +			fsl_mc_portal_free(
> > +				restool_misc->static_mc_io);
> 
> This fits in 80 characters.
> 
> > +			list_del(&restool_misc->list);
> > +			kfree(restool_misc);
> > +		}
> > +	}
> > +}
> 
> regards,
> dan carpenter

RE: [PATCH 2/5] staging: fsl-mc: define a macro to differentiate root dprc

[Lijun Pan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Sunday, October 25, 2015 7:13 PM
> To: Pan Lijun-B44306 <Lijun.Pan@freescale.com>
> Cc: arnd@arndb.de; devel@driverdev.osuosl.org; linux-
> kernel@vger.kernel.org; Yoder Stuart-B08248 <stuart.yoder@freescale.com>;
> katz Itai-RM05202 <itai.katz@freescale.com>; Rivera Jose-B46482
> <German.Rivera@freescale.com>; Li Yang-Leo-R58472 <LeoLi@freescale.com>;
> Wood Scott-B07421 <scottwood@freescale.com>; agraf@suse.de; Hamciuc
> Bogdan-BHAMCIU1 <bhamciu1@freescale.com>; Marginean Alexandru-R89243
> <R89243@freescale.com>; Sharma Bhupesh-B45370
> <bhupesh.sharma@freescale.com>; Erez Nir-RM30794
> <nir.erez@freescale.com>; Schmitt Richard-B43082
> <richard.schmitt@freescale.com>; dan.carpenter@oracle.com
> Subject: Re: [PATCH 2/5] staging: fsl-mc: define a macro to differentiate root
> dprc
> 
> On Sun, Oct 25, 2015 at 05:41:20PM -0500, Lijun Pan wrote:
> > Define is_root_dprc(dev) to tell whether a device is root dprc or not
> > via platform_bus_type.
> >
> > Signed-off-by: Lijun Pan <Lijun.Pan@freescale.com>
> > ---
> >  drivers/staging/fsl-mc/include/mc.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/staging/fsl-mc/include/mc.h
> > b/drivers/staging/fsl-mc/include/mc.h
> > index a933291..483763e 100644
> > --- a/drivers/staging/fsl-mc/include/mc.h
> > +++ b/drivers/staging/fsl-mc/include/mc.h
> > @@ -14,6 +14,7 @@
> >  #include <linux/device.h>
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/list.h>
> > +#include <linux/platform_device.h>
> >  #include "../include/dprc.h"
> >
> >  #define FSL_MC_VENDOR_FREESCALE	0x1957
> > @@ -109,6 +110,15 @@ struct fsl_mc_resource {
> >  #define FSL_MC_IS_DPRC	0x0001
> >
> >  /**
> > +  * root dprc's parent is a platform device
> > +  * that platform device's bus type is platform_bus_type.
> > +  */
> > +#define is_root_dprc(dev) \
> > +	((to_fsl_mc_device(dev)->flags & FSL_MC_IS_DPRC) && \
> > +	((dev)->bus == &fsl_mc_bus_type) && \
> > +	((dev)->parent->bus == &platform_bus_type))
> > +
> 
> It's best to make this type of thing a static inline function, to ensure you get the
> correct typechecking.
> 

I will take your suggestion and make changes in patch v2.
Lijun


> thanks,
> 
> greg k-h

RE: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver

[Stuart Yoder]

> -----Original Message-----
> From: Lijun Pan [mailto:Lijun.Pan@freescale.com]
> Sent: Sunday, October 25, 2015 5:41 PM
> To: gregkh@linuxfoundation.org; arnd@arndb.de; devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org
> Cc: Yoder Stuart-B08248; katz Itai-RM05202; Rivera Jose-B46482; Li Yang-Leo-R58472; Wood Scott-B07421;
> agraf@suse.de; Hamciuc Bogdan-BHAMCIU1; Marginean Alexandru-R89243; Sharma Bhupesh-B45370; Erez Nir-RM30794;
> Schmitt Richard-B43082; dan.carpenter@oracle.com; Pan Lijun-B44306
> Subject: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver
> 
> The kernel support for the restool (a user space resource management
> tool) is a driver for the /dev/dprc.N device file.
> Its purpose is to provide an ioctl interface,
> which the restool uses to interact with the MC bus driver

Name of the user space tool should be updated to be current:  s/restool/ls-restool/

> and with the MC firmware.
> We allocate a dpmcp at driver initialization,
> and keep that dpmcp until driver exit.
> We use that dpmcp by default.
> If that dpmcp is in use, we create another portal at run time
> and destroy the newly created portal after use.
> The ioctl RESTOOL_SEND_MC_COMMAND sends user space command to fsl-mc
> bus and utilizes the fsl-mc bus to communicate with MC firmware.
> The ioctl RESTOOL_DPRC_SYNC request the mc-bus launch
> objects scan under root dprc.
> In order to support multiple root dprc, we utilize the bus notify
> mechanism to scan fsl_mc_bus_type for the newly added root dprc.
> After discovering the root dprc, it creates a miscdevice
> /dev/dprc.N to associate with this root dprc.
> 
> Signed-off-by: Lijun Pan <Lijun.Pan@freescale.com>
> ---
>  drivers/staging/fsl-mc/bus/Kconfig      |   7 +-
>  drivers/staging/fsl-mc/bus/Makefile     |   3 +
>  drivers/staging/fsl-mc/bus/mc-ioctl.h   |  24 ++
>  drivers/staging/fsl-mc/bus/mc-restool.c | 488 ++++++++++++++++++++++++++++++++
>  4 files changed, 521 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/staging/fsl-mc/bus/mc-ioctl.h
>  create mode 100644 drivers/staging/fsl-mc/bus/mc-restool.c
> 
> diff --git a/drivers/staging/fsl-mc/bus/Kconfig b/drivers/staging/fsl-mc/bus/Kconfig
> index 0d779d9..39c6ef9 100644
> --- a/drivers/staging/fsl-mc/bus/Kconfig
> +++ b/drivers/staging/fsl-mc/bus/Kconfig
> @@ -21,4 +21,9 @@ config FSL_MC_BUS
>  	  Only enable this option when building the kernel for
>  	  Freescale QorQIQ LS2xxxx SoCs.
> 
> -
> +config FSL_MC_RESTOOL
> +        tristate "Freescale Management Complex (MC) restool driver"

s/restool/ls-restool/

Thanks,
Stuart

Re: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver

[Scott Wood]

On Sun, 2015-10-25 at 17:41 -0500, Lijun Pan wrote:
> The kernel support for the restool (a user space resource management
> tool) is a driver for the /dev/dprc.N device file.
> Its purpose is to provide an ioctl interface,
> which the restool uses to interact with the MC bus driver
> and with the MC firmware.
> We allocate a dpmcp at driver initialization,
> and keep that dpmcp until driver exit.
> We use that dpmcp by default.
> If that dpmcp is in use, we create another portal at run time
> and destroy the newly created portal after use.
> The ioctl RESTOOL_SEND_MC_COMMAND sends user space command to fsl-mc
> bus and utilizes the fsl-mc bus to communicate with MC firmware.
> The ioctl RESTOOL_DPRC_SYNC request the mc-bus launch
> objects scan under root dprc.
> In order to support multiple root dprc, we utilize the bus notify
> mechanism to scan fsl_mc_bus_type for the newly added root dprc.
> After discovering the root dprc, it creates a miscdevice
> /dev/dprc.N to associate with this root dprc.
> 
> Signed-off-by: Lijun Pan <Lijun.Pan@freescale.com>
> ---
>  drivers/staging/fsl-mc/bus/Kconfig      |   7 +-
>  drivers/staging/fsl-mc/bus/Makefile     |   3 +
>  drivers/staging/fsl-mc/bus/mc-ioctl.h   |  24 ++
>  drivers/staging/fsl-mc/bus/mc-restool.c | 488 
> ++++++++++++++++++++++++++++++++
>  4 files changed, 521 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/staging/fsl-mc/bus/mc-ioctl.h
>  create mode 100644 drivers/staging/fsl-mc/bus/mc-restool.c
> 
> diff --git a/drivers/staging/fsl-mc/bus/Kconfig b/drivers/staging/fsl-
> mc/bus/Kconfig
> index 0d779d9..39c6ef9 100644
> --- a/drivers/staging/fsl-mc/bus/Kconfig
> +++ b/drivers/staging/fsl-mc/bus/Kconfig
> @@ -21,4 +21,9 @@ config FSL_MC_BUS
>         Only enable this option when building the kernel for
>         Freescale QorQIQ LS2xxxx SoCs.
>  
> -
> +config FSL_MC_RESTOOL
> +        tristate "Freescale Management Complex (MC) restool driver"
> +        depends on FSL_MC_BUS
> +        help
> +          Driver that provides kernel support for the Freescale Management
> +       Complex resource manager user-space tool.
> diff --git a/drivers/staging/fsl-mc/bus/Makefile b/drivers/staging/fsl-
> mc/bus/Makefile
> index 25433a9..28b5fc0 100644
> --- a/drivers/staging/fsl-mc/bus/Makefile
> +++ b/drivers/staging/fsl-mc/bus/Makefile
> @@ -15,3 +15,6 @@ mc-bus-driver-objs := mc-bus.o \
>                     mc-allocator.o \
>                     dpmcp.o \
>                     dpbp.o
> +
> +# MC restool kernel support
> +obj-$(CONFIG_FSL_MC_RESTOOL) += mc-restool.o
> diff --git a/drivers/staging/fsl-mc/bus/mc-ioctl.h b/drivers/staging/fsl-
> mc/bus/mc-ioctl.h
> new file mode 100644
> index 0000000..e52f907
> --- /dev/null
> +++ b/drivers/staging/fsl-mc/bus/mc-ioctl.h
> @@ -0,0 +1,24 @@
> +/*
> + * Freescale Management Complex (MC) ioclt interface

ioctl

> + *
> + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> + * Author: Lijun Pan <Lijun.Pan@freescale.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +#ifndef _FSL_MC_IOCTL_H_
> +#define _FSL_MC_IOCTL_H_
> +
> +#include <linux/ioctl.h>
> +
> +#define RESTOOL_IOCTL_TYPE   'R'
> +
> +#define RESTOOL_DPRC_SYNC \
> +     _IO(RESTOOL_IOCTL_TYPE, 0x2)
> +
> +#define RESTOOL_SEND_MC_COMMAND \
> +     _IOWR(RESTOOL_IOCTL_TYPE, 0x4, struct mc_command)

Look at Documentation/ioctl/ioctl-number.txt and reserve a range within 'R' 
that doesn't conflict.

Add thorough documentation of this API.  

I'm not sure how it's usually handled with staging drivers, but eventually 
this will need to move to an appropriate uapi header.  Is this functionality 
even needed before the driver comes out of staging?  I don't see "userspace 
restool support" in drivers/staging/fsl-mc/TODO.

Don't reference struct mc_command without including the header that defines 
it.

> +#endif /* _FSL_MC_IOCTL_H_ */
> diff --git a/drivers/staging/fsl-mc/bus/mc-restool.c b/drivers/staging/fsl-
> mc/bus/mc-restool.c
> new file mode 100644
> index 0000000..a219172
> --- /dev/null
> +++ b/drivers/staging/fsl-mc/bus/mc-restool.c
> @@ -0,0 +1,488 @@
> +/*
> + * Freescale Management Complex (MC) restool driver
> + *
> + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> + * Author: Lijun Pan <Lijun.Pan@freescale.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include "../include/mc-private.h"
> +#include <linux/module.h>
> +#include <linux/fs.h>
> +#include <linux/miscdevice.h>
> +#include <linux/mm.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include "mc-ioctl.h"
> +#include "../include/mc-sys.h"
> +#include "../include/mc-cmd.h"
> +#include "../include/dpmng.h"
> +
> +/**
> + * Maximum number of DPRCs that can be opened at the same time
> + */
> +#define MAX_DPRC_HANDLES         64
> +
> +/**
> + * restool_misc - information associated with the newly added miscdevice
> + * @misc: newly created miscdevice associated with root dprc
> + * @miscdevt: device id of this miscdevice
> + * @list: a linked list node representing this miscdevcie
> + * @static_mc_io: pointer to the static MC I/O object used by the restool
> + * @dynamic_instance_count: number of dynamically created instances
> + * @static_instance_in_use: static instance is in use or not
> + * @mutex: mutex lock to serialze the operations
> + * @dev: root dprc associated with this miscdevice
> + */
> +struct restool_misc {
> +     struct miscdevice misc;
> +     dev_t miscdevt;
> +     struct list_head list;
> +     struct fsl_mc_io *static_mc_io;
> +     uint32_t dynamic_instance_count;
> +     bool static_instance_in_use;
> +     struct mutex mutex;
> +     struct device *dev;
> +};
> +
> +/*
> + * initialize a global list to link all
> + * the miscdevice nodes (struct restool_misc)
> + */
> +LIST_HEAD(misc_list);

static

> +static int fsl_mc_restool_dev_open(struct inode *inode, struct file *filep)
> +{
> +     struct fsl_mc_device *root_mc_dev;
> +     int error = 0;
> +     struct fsl_mc_io *dynamic_mc_io = NULL;
> +     struct restool_misc *restool_misc;
> +     struct restool_misc *restool_misc_cursor;
> +
> +     pr_debug("%s: inode's dev_t == %u\n", __func__, inode->i_rdev);
> +
> +     list_for_each_entry(restool_misc_cursor, &misc_list, list) {
> +             if (restool_misc_cursor->miscdevt == inode->i_rdev) {
> +                     pr_debug("%s: Found the restool_misc\n", __func__);
> +                     restool_misc = restool_misc_cursor;
> +                     break;
> +             }
> +     }

No synchronization on the list?

> +
> +     if (!restool_misc)
> +             return -EINVAL;
> +
> +     if (WARN_ON(restool_misc->dev == NULL))
> +             return -EINVAL;
> +
> +     mutex_lock(&restool_misc->mutex);
> +
> +     if (!restool_misc->static_instance_in_use) {
> +             restool_misc->static_instance_in_use = true;
> +             filep->private_data = restool_misc->static_mc_io;
> +     } else {
> +             dynamic_mc_io = kzalloc(sizeof(struct fsl_mc_io), GFP_KERNEL);
> +             if (dynamic_mc_io == NULL) {
> +                     error = -ENOMEM;
> +                     goto error;
> +             }
> +
> +             root_mc_dev = to_fsl_mc_device(restool_misc->dev);
> +             error = fsl_mc_portal_allocate(root_mc_dev, 0, &dynamic_mc_io);
> +             if (error < 0) {
> +                     pr_err("Not able to allocate MC portal\n");
> +                     goto error;
> +             }
> +             ++restool_misc->dynamic_instance_count;
> +             filep->private_data = dynamic_mc_io;
> +     }

Why is the first user handled differently from subsequent users?  Does each 
user really need its own portal, or can you just synchronize access?

-Scott

Re: [PATCH 4/5] staging: fsl-mc: bus rescan attribute to sync kernel with MC

[Greg KH]

On Sun, Oct 25, 2015 at 05:41:22PM -0500, Lijun Pan wrote:
> Introduce the rescan attribute as a bus attribute to
> synchronize the fsl-mc bus objects and the MC firmware.
> 
> To rescan the fsl-mc bus, e.g.,
> echo 1 > /sys/bus/fsl-mc/rescan
> 
> Signed-off-by: Lijun Pan <Lijun.Pan@freescale.com>
> ---
>  drivers/staging/fsl-mc/bus/mc-bus.c | 46 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/staging/fsl-mc/bus/mc-bus.c b/drivers/staging/fsl-mc/bus/mc-bus.c
> index 33a56ad..f1baad7 100644
> --- a/drivers/staging/fsl-mc/bus/mc-bus.c
> +++ b/drivers/staging/fsl-mc/bus/mc-bus.c
> @@ -144,11 +144,57 @@ static const struct attribute_group *fsl_mc_dev_groups[] = {
>  	NULL,
>  };
>  
> +static int scan_fsl_mc_bus(struct device *dev, void *data)
> +{
> +	struct fsl_mc_device *root_mc_dev;
> +	struct fsl_mc_bus *root_mc_bus;
> +
> +	if (is_root_dprc(dev)) {
> +		root_mc_dev = to_fsl_mc_device(dev);
> +		root_mc_bus = to_fsl_mc_bus(root_mc_dev);
> +		mutex_lock(&root_mc_bus->scan_mutex);
> +		dprc_scan_objects(root_mc_dev);
> +		mutex_unlock(&root_mc_bus->scan_mutex);
> +	}
> +
> +	return 0;
> +}
> +
> +static ssize_t bus_rescan_store(struct bus_type *bus,
> +			    const char *buf, size_t count)
> +{
> +	unsigned long val;
> +
> +	if (kstrtoul(buf, 0, &val) < 0)
> +		return -EINVAL;
> +
> +	if (val)
> +		bus_for_each_dev(bus, NULL, NULL, scan_fsl_mc_bus);
> +
> +	return count;
> +}
> +static BUS_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, bus_rescan_store);
> +
> +static struct attribute *fsl_mc_bus_attrs[] = {
> +	&bus_attr_rescan.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group fsl_mc_bus_group = {
> +	.attrs = fsl_mc_bus_attrs,
> +};
> +
> +static const struct attribute_group *fsl_mc_bus_groups[] = {
> +	&fsl_mc_bus_group,
> +	NULL,
> +};
> +
>  struct bus_type fsl_mc_bus_type = {
>  	.name = "fsl-mc",
>  	.match = fsl_mc_bus_match,
>  	.uevent = fsl_mc_bus_uevent,
>  	.dev_groups = fsl_mc_dev_groups,
> +	.bus_groups = fsl_mc_bus_groups,
>  };
>  EXPORT_SYMBOL_GPL(fsl_mc_bus_type);

No documentation update as well?

RE: [PATCH 4/5] staging: fsl-mc: bus rescan attribute to sync kernel with MC

[Lijun Pan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Tuesday, October 27, 2015 12:40 AM
> To: Pan Lijun-B44306 <Lijun.Pan@freescale.com>
> Cc: arnd@arndb.de; devel@driverdev.osuosl.org; linux-
> kernel@vger.kernel.org; Hamciuc Bogdan-BHAMCIU1
> <bhamciu1@freescale.com>; Sharma Bhupesh-B45370
> <bhupesh.sharma@freescale.com>; Rivera Jose-B46482
> <German.Rivera@freescale.com>; agraf@suse.de; Yoder Stuart-B08248
> <stuart.yoder@freescale.com>; Erez Nir-RM30794 <nir.erez@freescale.com>;
> katz Itai-RM05202 <itai.katz@freescale.com>; Wood Scott-B07421
> <scottwood@freescale.com>; Li Yang-Leo-R58472 <LeoLi@freescale.com>;
> Marginean Alexandru-R89243 <R89243@freescale.com>;
> dan.carpenter@oracle.com; Schmitt Richard-B43082
> <richard.schmitt@freescale.com>
> Subject: Re: [PATCH 4/5] staging: fsl-mc: bus rescan attribute to sync kernel
> with MC
> 
> On Sun, Oct 25, 2015 at 05:41:22PM -0500, Lijun Pan wrote:
> > Introduce the rescan attribute as a bus attribute to synchronize the
> > fsl-mc bus objects and the MC firmware.
> >
> > To rescan the fsl-mc bus, e.g.,
> > echo 1 > /sys/bus/fsl-mc/rescan
> >
> > Signed-off-by: Lijun Pan <Lijun.Pan@freescale.com>
> > ---
> >  drivers/staging/fsl-mc/bus/mc-bus.c | 46
> > +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 46 insertions(+)
> >
> > diff --git a/drivers/staging/fsl-mc/bus/mc-bus.c
> > b/drivers/staging/fsl-mc/bus/mc-bus.c
> > index 33a56ad..f1baad7 100644
> > --- a/drivers/staging/fsl-mc/bus/mc-bus.c
> > +++ b/drivers/staging/fsl-mc/bus/mc-bus.c
> > @@ -144,11 +144,57 @@ static const struct attribute_group
> *fsl_mc_dev_groups[] = {
> >  	NULL,
> >  };
> >
> > +static int scan_fsl_mc_bus(struct device *dev, void *data) {
> > +	struct fsl_mc_device *root_mc_dev;
> > +	struct fsl_mc_bus *root_mc_bus;
> > +
> > +	if (is_root_dprc(dev)) {
> > +		root_mc_dev = to_fsl_mc_device(dev);
> > +		root_mc_bus = to_fsl_mc_bus(root_mc_dev);
> > +		mutex_lock(&root_mc_bus->scan_mutex);
> > +		dprc_scan_objects(root_mc_dev);
> > +		mutex_unlock(&root_mc_bus->scan_mutex);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static ssize_t bus_rescan_store(struct bus_type *bus,
> > +			    const char *buf, size_t count) {
> > +	unsigned long val;
> > +
> > +	if (kstrtoul(buf, 0, &val) < 0)
> > +		return -EINVAL;
> > +
> > +	if (val)
> > +		bus_for_each_dev(bus, NULL, NULL, scan_fsl_mc_bus);
> > +
> > +	return count;
> > +}
> > +static BUS_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, bus_rescan_store);
> > +
> > +static struct attribute *fsl_mc_bus_attrs[] = {
> > +	&bus_attr_rescan.attr,
> > +	NULL,
> > +};
> > +
> > +static const struct attribute_group fsl_mc_bus_group = {
> > +	.attrs = fsl_mc_bus_attrs,
> > +};
> > +
> > +static const struct attribute_group *fsl_mc_bus_groups[] = {
> > +	&fsl_mc_bus_group,
> > +	NULL,
> > +};
> > +
> >  struct bus_type fsl_mc_bus_type = {
> >  	.name = "fsl-mc",
> >  	.match = fsl_mc_bus_match,
> >  	.uevent = fsl_mc_bus_uevent,
> >  	.dev_groups = fsl_mc_dev_groups,
> > +	.bus_groups = fsl_mc_bus_groups,
> >  };
> >  EXPORT_SYMBOL_GPL(fsl_mc_bus_type);
> 
> No documentation update as well?

Hi Greg,

Are you saying something like adding
Documentation/ABI/testing/sysfs-bus-fsl-mc? For the fsl-mc bus
Documentation/ABI/testing/sysfs-devices-dprc? For the dprc device?
Or 
drivers/staging/fsl-mc/[README.txt | TODO]?

Thanks
Lijun

RE: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver

[Lijun Pan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 9864 bytes --]



> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, October 27, 2015 12:17 AM
> To: Pan Lijun-B44306 <Lijun.Pan@freescale.com>
> Cc: gregkh@linuxfoundation.org; arnd@arndb.de; devel@driverdev.osuosl.org;
> linux-kernel@vger.kernel.org; Yoder Stuart-B08248
> <stuart.yoder@freescale.com>; katz Itai-RM05202 <itai.katz@freescale.com>;
> Rivera Jose-B46482 <German.Rivera@freescale.com>; Li Yang-Leo-R58472
> <LeoLi@freescale.com>; agraf@suse.de; Hamciuc Bogdan-BHAMCIU1
> <bhamciu1@freescale.com>; Marginean Alexandru-R89243
> <R89243@freescale.com>; Sharma Bhupesh-B45370
> <bhupesh.sharma@freescale.com>; Erez Nir-RM30794
> <nir.erez@freescale.com>; Schmitt Richard-B43082
> <richard.schmitt@freescale.com>; dan.carpenter@oracle.com
> Subject: Re: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver
> 
> On Sun, 2015-10-25 at 17:41 -0500, Lijun Pan wrote:
> > The kernel support for the restool (a user space resource management
> > tool) is a driver for the /dev/dprc.N device file.
> > Its purpose is to provide an ioctl interface, which the restool uses
> > to interact with the MC bus driver and with the MC firmware.
> > We allocate a dpmcp at driver initialization, and keep that dpmcp
> > until driver exit.
> > We use that dpmcp by default.
> > If that dpmcp is in use, we create another portal at run time and
> > destroy the newly created portal after use.
> > The ioctl RESTOOL_SEND_MC_COMMAND sends user space command to fsl-
> mc
> > bus and utilizes the fsl-mc bus to communicate with MC firmware.
> > The ioctl RESTOOL_DPRC_SYNC request the mc-bus launch objects scan
> > under root dprc.
> > In order to support multiple root dprc, we utilize the bus notify
> > mechanism to scan fsl_mc_bus_type for the newly added root dprc.
> > After discovering the root dprc, it creates a miscdevice /dev/dprc.N
> > to associate with this root dprc.
> >
> > Signed-off-by: Lijun Pan <Lijun.Pan@freescale.com>
> > ---
> >  drivers/staging/fsl-mc/bus/Kconfig      |   7 +-
> >  drivers/staging/fsl-mc/bus/Makefile     |   3 +
> >  drivers/staging/fsl-mc/bus/mc-ioctl.h   |  24 ++
> >  drivers/staging/fsl-mc/bus/mc-restool.c | 488
> > ++++++++++++++++++++++++++++++++
> >  4 files changed, 521 insertions(+), 1 deletion(-)  create mode 100644
> > drivers/staging/fsl-mc/bus/mc-ioctl.h
> >  create mode 100644 drivers/staging/fsl-mc/bus/mc-restool.c
> >
> > diff --git a/drivers/staging/fsl-mc/bus/Kconfig b/drivers/staging/fsl-
> > mc/bus/Kconfig index 0d779d9..39c6ef9 100644
> > --- a/drivers/staging/fsl-mc/bus/Kconfig
> > +++ b/drivers/staging/fsl-mc/bus/Kconfig
> > @@ -21,4 +21,9 @@ config FSL_MC_BUS
> >         Only enable this option when building the kernel for
> >         Freescale QorQIQ LS2xxxx SoCs.
> >
> > -
> > +config FSL_MC_RESTOOL
> > +        tristate "Freescale Management Complex (MC) restool driver"
> > +        depends on FSL_MC_BUS
> > +        help
> > +          Driver that provides kernel support for the Freescale Management
> > +       Complex resource manager user-space tool.
> > diff --git a/drivers/staging/fsl-mc/bus/Makefile
> > b/drivers/staging/fsl- mc/bus/Makefile index 25433a9..28b5fc0 100644
> > --- a/drivers/staging/fsl-mc/bus/Makefile
> > +++ b/drivers/staging/fsl-mc/bus/Makefile
> > @@ -15,3 +15,6 @@ mc-bus-driver-objs := mc-bus.o \
> >                     mc-allocator.o \
> >                     dpmcp.o \
> >                     dpbp.o
> > +
> > +# MC restool kernel support
> > +obj-$(CONFIG_FSL_MC_RESTOOL) += mc-restool.o
> > diff --git a/drivers/staging/fsl-mc/bus/mc-ioctl.h
> > b/drivers/staging/fsl- mc/bus/mc-ioctl.h new file mode 100644 index
> > 0000000..e52f907
> > --- /dev/null
> > +++ b/drivers/staging/fsl-mc/bus/mc-ioctl.h
> > @@ -0,0 +1,24 @@
> > +/*
> > + * Freescale Management Complex (MC) ioclt interface
> 
> ioctl
> 
> > + *
> > + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> > + * Author: Lijun Pan <Lijun.Pan@freescale.com>
> > + *
> > + * This file is licensed under the terms of the GNU General Public
> > + * License version 2. This program is licensed "as is" without any
> > + * warranty of any kind, whether express or implied.
> > + */
> > +#ifndef _FSL_MC_IOCTL_H_
> > +#define _FSL_MC_IOCTL_H_
> > +
> > +#include <linux/ioctl.h>
> > +
> > +#define RESTOOL_IOCTL_TYPE   'R'
> > +
> > +#define RESTOOL_DPRC_SYNC \
> > +     _IO(RESTOOL_IOCTL_TYPE, 0x2)
> > +
> > +#define RESTOOL_SEND_MC_COMMAND \
> > +     _IOWR(RESTOOL_IOCTL_TYPE, 0x4, struct mc_command)
> 
> Look at Documentation/ioctl/ioctl-number.txt and reserve a range within 'R'
> that doesn't conflict.
> 
> Add thorough documentation of this API.

What path do you recommend me to put documentation of this API?

> 
> I'm not sure how it's usually handled with staging drivers, but eventually this
> will need to move to an appropriate uapi header.  Is this functionality even
> needed before the driver comes out of staging?  I don't see "userspace restool
> support" in drivers/staging/fsl-mc/TODO.
> 
> Don't reference struct mc_command without including the header that defines
> it.
> 
> > +#endif /* _FSL_MC_IOCTL_H_ */
> > diff --git a/drivers/staging/fsl-mc/bus/mc-restool.c
> > b/drivers/staging/fsl- mc/bus/mc-restool.c new file mode 100644 index
> > 0000000..a219172
> > --- /dev/null
> > +++ b/drivers/staging/fsl-mc/bus/mc-restool.c
> > @@ -0,0 +1,488 @@
> > +/*
> > + * Freescale Management Complex (MC) restool driver
> > + *
> > + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> > + * Author: Lijun Pan <Lijun.Pan@freescale.com>
> > + *
> > + * This file is licensed under the terms of the GNU General Public
> > + * License version 2. This program is licensed "as is" without any
> > + * warranty of any kind, whether express or implied.
> > + */
> > +
> > +#include "../include/mc-private.h"
> > +#include <linux/module.h>
> > +#include <linux/fs.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/mm.h>
> > +#include <linux/slab.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/mutex.h>
> > +#include <linux/platform_device.h>
> > +#include "mc-ioctl.h"
> > +#include "../include/mc-sys.h"
> > +#include "../include/mc-cmd.h"
> > +#include "../include/dpmng.h"
> > +
> > +/**
> > + * Maximum number of DPRCs that can be opened at the same time  */
> > +#define MAX_DPRC_HANDLES         64
> > +
> > +/**
> > + * restool_misc - information associated with the newly added
> > +miscdevice
> > + * @misc: newly created miscdevice associated with root dprc
> > + * @miscdevt: device id of this miscdevice
> > + * @list: a linked list node representing this miscdevcie
> > + * @static_mc_io: pointer to the static MC I/O object used by the
> > +restool
> > + * @dynamic_instance_count: number of dynamically created instances
> > + * @static_instance_in_use: static instance is in use or not
> > + * @mutex: mutex lock to serialze the operations
> > + * @dev: root dprc associated with this miscdevice  */ struct
> > +restool_misc {
> > +     struct miscdevice misc;
> > +     dev_t miscdevt;
> > +     struct list_head list;
> > +     struct fsl_mc_io *static_mc_io;
> > +     uint32_t dynamic_instance_count;
> > +     bool static_instance_in_use;
> > +     struct mutex mutex;
> > +     struct device *dev;
> > +};
> > +
> > +/*
> > + * initialize a global list to link all
> > + * the miscdevice nodes (struct restool_misc)  */
> > +LIST_HEAD(misc_list);
> 
> static
> 
> > +static int fsl_mc_restool_dev_open(struct inode *inode, struct file
> > +*filep) {
> > +     struct fsl_mc_device *root_mc_dev;
> > +     int error = 0;
> > +     struct fsl_mc_io *dynamic_mc_io = NULL;
> > +     struct restool_misc *restool_misc;
> > +     struct restool_misc *restool_misc_cursor;
> > +
> > +     pr_debug("%s: inode's dev_t == %u\n", __func__, inode->i_rdev);
> > +
> > +     list_for_each_entry(restool_misc_cursor, &misc_list, list) {
> > +             if (restool_misc_cursor->miscdevt == inode->i_rdev) {
> > +                     pr_debug("%s: Found the restool_misc\n", __func__);
> > +                     restool_misc = restool_misc_cursor;
> > +                     break;
> > +             }
> > +     }
> 
> No synchronization on the list?
> 
> > +
> > +     if (!restool_misc)
> > +             return -EINVAL;
> > +
> > +     if (WARN_ON(restool_misc->dev == NULL))
> > +             return -EINVAL;
> > +
> > +     mutex_lock(&restool_misc->mutex);
> > +
> > +     if (!restool_misc->static_instance_in_use) {
> > +             restool_misc->static_instance_in_use = true;
> > +             filep->private_data = restool_misc->static_mc_io;
> > +     } else {
> > +             dynamic_mc_io = kzalloc(sizeof(struct fsl_mc_io), GFP_KERNEL);
> > +             if (dynamic_mc_io == NULL) {
> > +                     error = -ENOMEM;
> > +                     goto error;
> > +             }
> > +
> > +             root_mc_dev = to_fsl_mc_device(restool_misc->dev);
> > +             error = fsl_mc_portal_allocate(root_mc_dev, 0, &dynamic_mc_io);
> > +             if (error < 0) {
> > +                     pr_err("Not able to allocate MC portal\n");
> > +                     goto error;
> > +             }
> > +             ++restool_misc->dynamic_instance_count;
> > +             filep->private_data = dynamic_mc_io;
> > +     }
> 
> Why is the first user handled differently from subsequent users?  Does each
> user really need its own portal, or can you just synchronize access?

First user get the statically allocated portal.
Second user need to dynamically allocate one.
Some operations may take a long time (say 1 second) in MC object creation.
In order to support asynchronous portal operation, 
We allocate portal for each user.

Lijun

> 
> -Scott

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver

[Lijun Pan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 11277 bytes --]



> -----Original Message-----
> From: Pan Lijun-B44306
> Sent: Thursday, October 29, 2015 6:55 PM
> To: Wood Scott-B07421 <scottwood@freescale.com>
> Cc: gregkh@linuxfoundation.org; arnd@arndb.de; devel@driverdev.osuosl.org;
> linux-kernel@vger.kernel.org; Yoder Stuart-B08248
> <stuart.yoder@freescale.com>; katz Itai-RM05202 <itai.katz@freescale.com>;
> Rivera Jose-B46482 <German.Rivera@freescale.com>; Li Yang-Leo-R58472
> <LeoLi@freescale.com>; agraf@suse.de; Hamciuc Bogdan-BHAMCIU1
> <bhamciu1@freescale.com>; Marginean Alexandru-R89243
> <R89243@freescale.com>; Sharma Bhupesh-B45370
> <bhupesh.sharma@freescale.com>; Erez Nir-RM30794
> <nir.erez@freescale.com>; Schmitt Richard-B43082
> <richard.schmitt@freescale.com>; dan.carpenter@oracle.com
> Subject: RE: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver
> 
> 
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, October 27, 2015 12:17 AM
> > To: Pan Lijun-B44306 <Lijun.Pan@freescale.com>
> > Cc: gregkh@linuxfoundation.org; arnd@arndb.de;
> > devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org; Yoder
> > Stuart-B08248 <stuart.yoder@freescale.com>; katz Itai-RM05202
> > <itai.katz@freescale.com>; Rivera Jose-B46482
> > <German.Rivera@freescale.com>; Li Yang-Leo-R58472
> > <LeoLi@freescale.com>; agraf@suse.de; Hamciuc Bogdan-BHAMCIU1
> > <bhamciu1@freescale.com>; Marginean Alexandru-R89243
> > <R89243@freescale.com>; Sharma Bhupesh-B45370
> > <bhupesh.sharma@freescale.com>; Erez Nir-RM30794
> > <nir.erez@freescale.com>; Schmitt Richard-B43082
> > <richard.schmitt@freescale.com>; dan.carpenter@oracle.com
> > Subject: Re: [PATCH 5/5] staging: fsl-mc: Management Complex restool
> > driver
> >
> > On Sun, 2015-10-25 at 17:41 -0500, Lijun Pan wrote:
> > > The kernel support for the restool (a user space resource management
> > > tool) is a driver for the /dev/dprc.N device file.
> > > Its purpose is to provide an ioctl interface, which the restool uses
> > > to interact with the MC bus driver and with the MC firmware.
> > > We allocate a dpmcp at driver initialization, and keep that dpmcp
> > > until driver exit.
> > > We use that dpmcp by default.
> > > If that dpmcp is in use, we create another portal at run time and
> > > destroy the newly created portal after use.
> > > The ioctl RESTOOL_SEND_MC_COMMAND sends user space command to
> fsl-
> > mc
> > > bus and utilizes the fsl-mc bus to communicate with MC firmware.
> > > The ioctl RESTOOL_DPRC_SYNC request the mc-bus launch objects scan
> > > under root dprc.
> > > In order to support multiple root dprc, we utilize the bus notify
> > > mechanism to scan fsl_mc_bus_type for the newly added root dprc.
> > > After discovering the root dprc, it creates a miscdevice /dev/dprc.N
> > > to associate with this root dprc.
> > >
> > > Signed-off-by: Lijun Pan <Lijun.Pan@freescale.com>
> > > ---
> > >  drivers/staging/fsl-mc/bus/Kconfig      |   7 +-
> > >  drivers/staging/fsl-mc/bus/Makefile     |   3 +
> > >  drivers/staging/fsl-mc/bus/mc-ioctl.h   |  24 ++
> > >  drivers/staging/fsl-mc/bus/mc-restool.c | 488
> > > ++++++++++++++++++++++++++++++++
> > >  4 files changed, 521 insertions(+), 1 deletion(-)  create mode
> > > 100644 drivers/staging/fsl-mc/bus/mc-ioctl.h
> > >  create mode 100644 drivers/staging/fsl-mc/bus/mc-restool.c
> > >
> > > diff --git a/drivers/staging/fsl-mc/bus/Kconfig
> > > b/drivers/staging/fsl- mc/bus/Kconfig index 0d779d9..39c6ef9 100644
> > > --- a/drivers/staging/fsl-mc/bus/Kconfig
> > > +++ b/drivers/staging/fsl-mc/bus/Kconfig
> > > @@ -21,4 +21,9 @@ config FSL_MC_BUS
> > >         Only enable this option when building the kernel for
> > >         Freescale QorQIQ LS2xxxx SoCs.
> > >
> > > -
> > > +config FSL_MC_RESTOOL
> > > +        tristate "Freescale Management Complex (MC) restool driver"
> > > +        depends on FSL_MC_BUS
> > > +        help
> > > +          Driver that provides kernel support for the Freescale Management
> > > +       Complex resource manager user-space tool.
> > > diff --git a/drivers/staging/fsl-mc/bus/Makefile
> > > b/drivers/staging/fsl- mc/bus/Makefile index 25433a9..28b5fc0 100644
> > > --- a/drivers/staging/fsl-mc/bus/Makefile
> > > +++ b/drivers/staging/fsl-mc/bus/Makefile
> > > @@ -15,3 +15,6 @@ mc-bus-driver-objs := mc-bus.o \
> > >                     mc-allocator.o \
> > >                     dpmcp.o \
> > >                     dpbp.o
> > > +
> > > +# MC restool kernel support
> > > +obj-$(CONFIG_FSL_MC_RESTOOL) += mc-restool.o
> > > diff --git a/drivers/staging/fsl-mc/bus/mc-ioctl.h
> > > b/drivers/staging/fsl- mc/bus/mc-ioctl.h new file mode 100644 index
> > > 0000000..e52f907
> > > --- /dev/null
> > > +++ b/drivers/staging/fsl-mc/bus/mc-ioctl.h
> > > @@ -0,0 +1,24 @@
> > > +/*
> > > + * Freescale Management Complex (MC) ioclt interface
> >
> > ioctl
> >
> > > + *
> > > + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> > > + * Author: Lijun Pan <Lijun.Pan@freescale.com>
> > > + *
> > > + * This file is licensed under the terms of the GNU General Public
> > > + * License version 2. This program is licensed "as is" without any
> > > + * warranty of any kind, whether express or implied.
> > > + */
> > > +#ifndef _FSL_MC_IOCTL_H_
> > > +#define _FSL_MC_IOCTL_H_
> > > +
> > > +#include <linux/ioctl.h>
> > > +
> > > +#define RESTOOL_IOCTL_TYPE   'R'
> > > +
> > > +#define RESTOOL_DPRC_SYNC \
> > > +     _IO(RESTOOL_IOCTL_TYPE, 0x2)
> > > +
> > > +#define RESTOOL_SEND_MC_COMMAND \
> > > +     _IOWR(RESTOOL_IOCTL_TYPE, 0x4, struct mc_command)
> >
> > Look at Documentation/ioctl/ioctl-number.txt and reserve a range within 'R'
> > that doesn't conflict.
> >
> > Add thorough documentation of this API.
> 
> What path do you recommend me to put documentation of this API?

Is Documentation/misc-devices/mc-restool.txt the correct location? 

> >
> > I'm not sure how it's usually handled with staging drivers, but
> > eventually this will need to move to an appropriate uapi header.  Is
> > this functionality even needed before the driver comes out of staging?
> > I don't see "userspace restool support" in drivers/staging/fsl-mc/TODO.
> >
> > Don't reference struct mc_command without including the header that
> > defines it.
> >
> > > +#endif /* _FSL_MC_IOCTL_H_ */
> > > diff --git a/drivers/staging/fsl-mc/bus/mc-restool.c
> > > b/drivers/staging/fsl- mc/bus/mc-restool.c new file mode 100644
> > > index
> > > 0000000..a219172
> > > --- /dev/null
> > > +++ b/drivers/staging/fsl-mc/bus/mc-restool.c
> > > @@ -0,0 +1,488 @@
> > > +/*
> > > + * Freescale Management Complex (MC) restool driver
> > > + *
> > > + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> > > + * Author: Lijun Pan <Lijun.Pan@freescale.com>
> > > + *
> > > + * This file is licensed under the terms of the GNU General Public
> > > + * License version 2. This program is licensed "as is" without any
> > > + * warranty of any kind, whether express or implied.
> > > + */
> > > +
> > > +#include "../include/mc-private.h"
> > > +#include <linux/module.h>
> > > +#include <linux/fs.h>
> > > +#include <linux/miscdevice.h>
> > > +#include <linux/mm.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/uaccess.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/platform_device.h>
> > > +#include "mc-ioctl.h"
> > > +#include "../include/mc-sys.h"
> > > +#include "../include/mc-cmd.h"
> > > +#include "../include/dpmng.h"
> > > +
> > > +/**
> > > + * Maximum number of DPRCs that can be opened at the same time  */
> > > +#define MAX_DPRC_HANDLES         64
> > > +
> > > +/**
> > > + * restool_misc - information associated with the newly added
> > > +miscdevice
> > > + * @misc: newly created miscdevice associated with root dprc
> > > + * @miscdevt: device id of this miscdevice
> > > + * @list: a linked list node representing this miscdevcie
> > > + * @static_mc_io: pointer to the static MC I/O object used by the
> > > +restool
> > > + * @dynamic_instance_count: number of dynamically created instances
> > > + * @static_instance_in_use: static instance is in use or not
> > > + * @mutex: mutex lock to serialze the operations
> > > + * @dev: root dprc associated with this miscdevice  */ struct
> > > +restool_misc {
> > > +     struct miscdevice misc;
> > > +     dev_t miscdevt;
> > > +     struct list_head list;
> > > +     struct fsl_mc_io *static_mc_io;
> > > +     uint32_t dynamic_instance_count;
> > > +     bool static_instance_in_use;
> > > +     struct mutex mutex;
> > > +     struct device *dev;
> > > +};
> > > +
> > > +/*
> > > + * initialize a global list to link all
> > > + * the miscdevice nodes (struct restool_misc)  */
> > > +LIST_HEAD(misc_list);
> >
> > static
> >
> > > +static int fsl_mc_restool_dev_open(struct inode *inode, struct file
> > > +*filep) {
> > > +     struct fsl_mc_device *root_mc_dev;
> > > +     int error = 0;
> > > +     struct fsl_mc_io *dynamic_mc_io = NULL;
> > > +     struct restool_misc *restool_misc;
> > > +     struct restool_misc *restool_misc_cursor;
> > > +
> > > +     pr_debug("%s: inode's dev_t == %u\n", __func__,
> > > + inode->i_rdev);
> > > +
> > > +     list_for_each_entry(restool_misc_cursor, &misc_list, list) {
> > > +             if (restool_misc_cursor->miscdevt == inode->i_rdev) {
> > > +                     pr_debug("%s: Found the restool_misc\n", __func__);
> > > +                     restool_misc = restool_misc_cursor;
> > > +                     break;
> > > +             }
> > > +     }
> >
> > No synchronization on the list?
> >
> > > +
> > > +     if (!restool_misc)
> > > +             return -EINVAL;
> > > +
> > > +     if (WARN_ON(restool_misc->dev == NULL))
> > > +             return -EINVAL;
> > > +
> > > +     mutex_lock(&restool_misc->mutex);
> > > +
> > > +     if (!restool_misc->static_instance_in_use) {
> > > +             restool_misc->static_instance_in_use = true;
> > > +             filep->private_data = restool_misc->static_mc_io;
> > > +     } else {
> > > +             dynamic_mc_io = kzalloc(sizeof(struct fsl_mc_io), GFP_KERNEL);
> > > +             if (dynamic_mc_io == NULL) {
> > > +                     error = -ENOMEM;
> > > +                     goto error;
> > > +             }
> > > +
> > > +             root_mc_dev = to_fsl_mc_device(restool_misc->dev);
> > > +             error = fsl_mc_portal_allocate(root_mc_dev, 0, &dynamic_mc_io);
> > > +             if (error < 0) {
> > > +                     pr_err("Not able to allocate MC portal\n");
> > > +                     goto error;
> > > +             }
> > > +             ++restool_misc->dynamic_instance_count;
> > > +             filep->private_data = dynamic_mc_io;
> > > +     }
> >
> > Why is the first user handled differently from subsequent users?  Does
> > each user really need its own portal, or can you just synchronize access?
> 
> First user get the statically allocated portal.
> Second user need to dynamically allocate one.
> Some operations may take a long time (say 1 second) in MC object creation.
> In order to support asynchronous portal operation, We allocate portal for each
> user.
> 
> Lijun
> 
> >
> > -Scott

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥