[PATCH 0/3] bus: fsl-mc: enhance Management Complex userspace support

[PATCH 0/3] bus: fsl-mc: enhance Management Complex userspace support

[Ioana Ciornei]

This patch set adds restool support in fsl-mc bus along
with a rescan attribute to the root DPRC container.

Ioana Ciornei (3):
  bus: fsl-mc: add restool userspace support
  bus: fsl-mc: add root dprc rescan attribute
  bus: fsl-mc: add bus rescan attribute

 Documentation/ioctl/ioctl-number.txt        |   1 +
 Documentation/networking/dpaa2/overview.rst |   4 +
 drivers/bus/fsl-mc/Kconfig                  |   7 +
 drivers/bus/fsl-mc/Makefile                 |   3 +
 drivers/bus/fsl-mc/dprc-driver.c            |   4 +-
 drivers/bus/fsl-mc/fsl-mc-allocator.c       |   5 +
 drivers/bus/fsl-mc/fsl-mc-bus.c             |  95 ++++++++++++
 drivers/bus/fsl-mc/fsl-mc-private.h         |  59 ++++++++
 drivers/bus/fsl-mc/fsl-mc-restool.c         | 219 ++++++++++++++++++++++++++++
 9 files changed, 395 insertions(+), 2 deletions(-)
 create mode 100644 drivers/bus/fsl-mc/fsl-mc-restool.c

-- 
1.9.1

[PATCH 1/3] bus: fsl-mc: add restool userspace support

[Ioana Ciornei]

Adding kernel support for restool, a userspace tool for resource
management, means exporting an ioctl capable device file representing
the root resource container.
This new functionality in the fsl-mc bus driver intends to provide
restool an interface to interact with the MC firmware.
Commands that are composed in userspace are sent to the MC firmware
through the RESTOOL_SEND_MC_COMMAND ioctl.
By default the implicit MC I/O portal is used for this operation,
but if the implicit one is busy, a dynamic portal is allocated and then
freed upon execution.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 Documentation/ioctl/ioctl-number.txt        |   1 +
 Documentation/networking/dpaa2/overview.rst |   4 +
 drivers/bus/fsl-mc/Kconfig                  |   7 +
 drivers/bus/fsl-mc/Makefile                 |   3 +
 drivers/bus/fsl-mc/fsl-mc-allocator.c       |   5 +
 drivers/bus/fsl-mc/fsl-mc-bus.c             |  19 +++
 drivers/bus/fsl-mc/fsl-mc-private.h         |  56 +++++++
 drivers/bus/fsl-mc/fsl-mc-restool.c         | 219 ++++++++++++++++++++++++++++
 8 files changed, 314 insertions(+)
 create mode 100644 drivers/bus/fsl-mc/fsl-mc-restool.c

diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index 6501389..d427397 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -170,6 +170,7 @@ Code  Seq#(hex)	Include File		Comments
 'R'	00-1F	linux/random.h		conflict!
 'R'	01	linux/rfkill.h		conflict!
 'R'	C0-DF	net/bluetooth/rfcomm.h
+'R'	E0	drivers/bus/fsl-mc/fsl-mc-private.h
 'S'	all	linux/cdrom.h		conflict!
 'S'	80-81	scsi/scsi_ioctl.h	conflict!
 'S'	82-FF	scsi/scsi.h		conflict!
diff --git a/Documentation/networking/dpaa2/overview.rst b/Documentation/networking/dpaa2/overview.rst
index 79fede4..1056445 100644
--- a/Documentation/networking/dpaa2/overview.rst
+++ b/Documentation/networking/dpaa2/overview.rst
@@ -127,6 +127,10 @@ level.
 
 DPRCs can be defined statically and populated with objects
 via a config file passed to the MC when firmware starts it.
+There is also a Linux user space tool called "restool" that can be
+used to create/destroy containers and objects dynamically. The latest
+version of restool can be found at:
+        https://github.com/qoriq-open-source/restool
 
 DPAA2 Objects for an Ethernet Network Interface
 -----------------------------------------------
diff --git a/drivers/bus/fsl-mc/Kconfig b/drivers/bus/fsl-mc/Kconfig
index c23c77c..66ec3b9 100644
--- a/drivers/bus/fsl-mc/Kconfig
+++ b/drivers/bus/fsl-mc/Kconfig
@@ -14,3 +14,10 @@ config FSL_MC_BUS
 	  architecture.  The fsl-mc bus driver handles discovery of
 	  DPAA2 objects (which are represented as Linux devices) and
 	  binding objects to drivers.
+
+config FSL_MC_RESTOOL
+	bool "Management Complex (MC) restool support"
+	depends on FSL_MC_BUS
+	help
+	  Provides kernel support for the Management Complex resource
+	  manager user-space tool - restool.
diff --git a/drivers/bus/fsl-mc/Makefile b/drivers/bus/fsl-mc/Makefile
index 6a97f2c..9a155e3 100644
--- a/drivers/bus/fsl-mc/Makefile
+++ b/drivers/bus/fsl-mc/Makefile
@@ -14,3 +14,6 @@ mc-bus-driver-objs := fsl-mc-bus.o \
 		      fsl-mc-allocator.o \
 		      fsl-mc-msi.o \
 		      dpmcp.o
+
+# MC restool kernel support
+obj-$(CONFIG_FSL_MC_RESTOOL) += fsl-mc-restool.o
diff --git a/drivers/bus/fsl-mc/fsl-mc-allocator.c b/drivers/bus/fsl-mc/fsl-mc-allocator.c
index 452c5d7..fb1442b 100644
--- a/drivers/bus/fsl-mc/fsl-mc-allocator.c
+++ b/drivers/bus/fsl-mc/fsl-mc-allocator.c
@@ -646,3 +646,8 @@ int __init fsl_mc_allocator_driver_init(void)
 {
 	return fsl_mc_driver_register(&fsl_mc_allocator_driver);
 }
+
+void fsl_mc_allocator_driver_exit(void)
+{
+	fsl_mc_driver_unregister(&fsl_mc_allocator_driver);
+}
diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index 1b333c4..240b99d 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -792,6 +792,7 @@ static int fsl_mc_bus_probe(struct platform_device *pdev)
 	struct fsl_mc *mc;
 	struct fsl_mc_device *mc_bus_dev = NULL;
 	struct fsl_mc_io *mc_io = NULL;
+	struct fsl_mc_bus *mc_bus = NULL;
 	int container_id;
 	phys_addr_t mc_portal_phys_addr;
 	u32 mc_portal_size;
@@ -863,9 +864,18 @@ static int fsl_mc_bus_probe(struct platform_device *pdev)
 	if (error < 0)
 		goto error_cleanup_mc_io;
 
+	mc_bus = to_fsl_mc_bus(mc_bus_dev);
+	error = fsl_mc_restool_create_device_file(mc_bus);
+	if (error < 0)
+		goto error_cleanup_device;
+
 	mc->root_mc_bus_dev = mc_bus_dev;
+
 	return 0;
 
+error_cleanup_device:
+	fsl_mc_device_remove(mc_bus_dev);
+
 error_cleanup_mc_io:
 	fsl_destroy_mc_io(mc_io);
 	return error;
@@ -878,10 +888,12 @@ static int fsl_mc_bus_probe(struct platform_device *pdev)
 static int fsl_mc_bus_remove(struct platform_device *pdev)
 {
 	struct fsl_mc *mc = platform_get_drvdata(pdev);
+	struct fsl_mc_bus *mc_bus = to_fsl_mc_bus(mc->root_mc_bus_dev);
 
 	if (!fsl_mc_is_root_dprc(&mc->root_mc_bus_dev->dev))
 		return -EINVAL;
 
+	fsl_mc_restool_remove_device_file(mc_bus);
 	fsl_mc_device_remove(mc->root_mc_bus_dev);
 
 	fsl_destroy_mc_io(mc->root_mc_bus_dev->mc_io);
@@ -931,8 +943,15 @@ static int __init fsl_mc_bus_driver_init(void)
 	if (error < 0)
 		goto error_cleanup_dprc_driver;
 
+	error = fsl_mc_restool_init();
+	if (error < 0)
+		goto error_cleanup_mc_allocator;
+
 	return 0;
 
+error_cleanup_mc_allocator:
+	fsl_mc_allocator_driver_exit();
+
 error_cleanup_dprc_driver:
 	dprc_driver_exit();
 
diff --git a/drivers/bus/fsl-mc/fsl-mc-private.h b/drivers/bus/fsl-mc/fsl-mc-private.h
index bed990c..d6f67a8 100644
--- a/drivers/bus/fsl-mc/fsl-mc-private.h
+++ b/drivers/bus/fsl-mc/fsl-mc-private.h
@@ -10,6 +10,8 @@
 
 #include <linux/fsl/mc.h>
 #include <linux/mutex.h>
+#include <linux/cdev.h>
+#include <linux/ioctl.h>
 
 /*
  * Data Path Management Complex (DPMNG) General API
@@ -405,6 +407,24 @@ struct fsl_mc_resource_pool {
 };
 
 /**
+ * struct fsl_mc_restool - information associated with a restool device file
+ * @cdev: struct char device linked to the root dprc
+ * @dev: dev_t for the char device to be added
+ * @device: newly created device in /dev
+ * @mutex: mutex lock to serialize the open/release operations
+ * @local_instance_in_use: local MC I/O instance in use or not
+ * @dynamic_instance_count: number of dynamically created MC I/O instances
+ */
+struct fsl_mc_restool {
+	struct cdev cdev;
+	dev_t dev;
+	struct device *device;
+	struct mutex mutex; /* serialize open/release operations */
+	bool local_instance_in_use;
+	u32 dynamic_instance_count;
+};
+
+/**
  * struct fsl_mc_bus - logical bus that corresponds to a physical DPRC
  * @mc_dev: fsl-mc device for the bus device itself.
  * @resource_pools: array of resource pools (one pool per resource type)
@@ -413,6 +433,7 @@ struct fsl_mc_resource_pool {
  * @irq_resources: Pointer to array of IRQ objects for the IRQ pool
  * @scan_mutex: Serializes bus scanning
  * @dprc_attr: DPRC attributes
+ * @restool_misc: struct that abstracts the interaction with userspace restool
  */
 struct fsl_mc_bus {
 	struct fsl_mc_device mc_dev;
@@ -420,11 +441,18 @@ struct fsl_mc_bus {
 	struct fsl_mc_device_irq *irq_resources;
 	struct mutex scan_mutex;    /* serializes bus scanning */
 	struct dprc_attributes dprc_attr;
+	struct fsl_mc_restool restool_misc;
 };
 
 #define to_fsl_mc_bus(_mc_dev) \
 	container_of(_mc_dev, struct fsl_mc_bus, mc_dev)
 
+#define RESTOOL_IOCTL_TYPE	'R'
+#define RESTOOL_IOCTL_SEQ	0xE0
+
+#define RESTOOL_SEND_MC_COMMAND \
+	_IOWR(RESTOOL_IOCTL_TYPE, RESTOOL_IOCTL_SEQ, struct mc_command)
+
 int __must_check fsl_mc_device_add(struct fsl_mc_obj_desc *obj_desc,
 				   struct fsl_mc_io *mc_io,
 				   struct device *parent_dev,
@@ -438,6 +466,8 @@ int __must_check fsl_mc_device_add(struct fsl_mc_obj_desc *obj_desc,
 
 int __init fsl_mc_allocator_driver_init(void);
 
+void fsl_mc_allocator_driver_exit(void);
+
 void fsl_mc_init_all_resource_pools(struct fsl_mc_device *mc_bus_dev);
 
 void fsl_mc_cleanup_all_resource_pools(struct fsl_mc_device *mc_bus_dev);
@@ -472,4 +502,30 @@ int __must_check fsl_create_mc_io(struct device *dev,
 
 bool fsl_mc_is_root_dprc(struct device *dev);
 
+#ifdef CONFIG_FSL_MC_RESTOOL
+
+int fsl_mc_restool_create_device_file(struct fsl_mc_bus *mc_bus);
+
+void fsl_mc_restool_remove_device_file(struct fsl_mc_bus *mc_bus);
+
+int fsl_mc_restool_init(void);
+
+#else
+
+static inline int fsl_mc_restool_create_device_file(struct fsl_mc_bus *mc_bus)
+{
+	return 0;
+}
+
+static inline void fsl_mc_restool_remove_device_file(struct fsl_mc_bus *mc_bus)
+{
+}
+
+static inline int fsl_mc_restool_init(void)
+{
+	return 0;
+}
+
+#endif
+
 #endif /* _FSL_MC_PRIVATE_H_ */
diff --git a/drivers/bus/fsl-mc/fsl-mc-restool.c b/drivers/bus/fsl-mc/fsl-mc-restool.c
new file mode 100644
index 0000000..21ecee5
--- /dev/null
+++ b/drivers/bus/fsl-mc/fsl-mc-restool.c
@@ -0,0 +1,219 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Management Complex (MC) restool support
+ *
+ * Copyright 2018 NXP
+ *
+ */
+
+#include <linux/slab.h>
+#include <linux/cdev.h>
+#include <linux/fs.h>
+#include <linux/uaccess.h>
+
+#include "fsl-mc-private.h"
+
+#define FSL_MC_BUS_MAX_MINORS	1
+
+static struct class *fsl_mc_bus_class;
+static int fsl_mc_bus_major;
+
+static int fsl_mc_restool_send_command(unsigned long arg,
+				       struct fsl_mc_io *mc_io)
+{
+	struct mc_command mc_cmd;
+	int error;
+
+	error = copy_from_user(&mc_cmd, (void __user *)arg, sizeof(mc_cmd));
+	if (error)
+		return -EFAULT;
+
+	error = mc_send_command(mc_io, &mc_cmd);
+	if (error)
+		return error;
+
+	error = copy_to_user((void __user *)arg, &mc_cmd, sizeof(mc_cmd));
+	if (error)
+		return -EFAULT;
+
+	return 0;
+}
+
+int fsl_mc_restool_init(void)
+{
+	dev_t dev;
+	int error;
+
+	fsl_mc_bus_class = class_create(THIS_MODULE, "fsl_mc_bus");
+	if (IS_ERR(fsl_mc_bus_class)) {
+		error = PTR_ERR(fsl_mc_bus_class);
+		return error;
+	}
+
+	error = alloc_chrdev_region(&dev, 0,
+				    FSL_MC_BUS_MAX_MINORS,
+				    "fsl_mc_bus");
+	if (error < 0)
+		return error;
+
+	fsl_mc_bus_major = MAJOR(dev);
+
+	return 0;
+}
+
+static int fsl_mc_restool_dev_open(struct inode *inode, struct file *filep)
+{
+	struct fsl_mc_device *root_mc_device;
+	struct fsl_mc_restool *mc_restool;
+	struct fsl_mc_bus *mc_bus;
+	struct fsl_mc_io *dynamic_mc_io;
+	int error;
+
+	mc_restool = container_of(inode->i_cdev, struct fsl_mc_restool, cdev);
+	mc_bus = container_of(mc_restool, struct fsl_mc_bus, restool_misc);
+	root_mc_device = &mc_bus->mc_dev;
+
+	mutex_lock(&mc_restool->mutex);
+
+	if (!mc_restool->local_instance_in_use) {
+		filep->private_data = root_mc_device->mc_io;
+		mc_restool->local_instance_in_use = true;
+	} else {
+		dynamic_mc_io = kzalloc(sizeof(*dynamic_mc_io), GFP_KERNEL);
+		if (!dynamic_mc_io) {
+			error = -ENOMEM;
+			goto error_alloc_mc_io;
+		}
+
+		error = fsl_mc_portal_allocate(root_mc_device, 0,
+					       &dynamic_mc_io);
+		if (error) {
+			pr_err("Could not allocate MC portal\n");
+			goto error_portal_allocate;
+		}
+
+		mc_restool->dynamic_instance_count++;
+		filep->private_data = dynamic_mc_io;
+	}
+
+	mutex_unlock(&mc_restool->mutex);
+
+	return 0;
+
+error_portal_allocate:
+	kfree(dynamic_mc_io);
+
+error_alloc_mc_io:
+	mutex_unlock(&mc_restool->mutex);
+
+	return error;
+}
+
+static int fsl_mc_restool_dev_release(struct inode *inode, struct file *filep)
+{
+	struct fsl_mc_device *root_mc_device;
+	struct fsl_mc_restool *mc_restool;
+	struct fsl_mc_bus *mc_bus;
+	struct fsl_mc_io *mc_io;
+
+	mc_restool = container_of(inode->i_cdev, struct fsl_mc_restool, cdev);
+	mc_bus = container_of(mc_restool, struct fsl_mc_bus, restool_misc);
+	root_mc_device = &mc_bus->mc_dev;
+	mc_io = filep->private_data;
+
+	mutex_lock(&mc_restool->mutex);
+
+	if (WARN_ON(!mc_restool->local_instance_in_use &&
+		    mc_restool->dynamic_instance_count == 0)) {
+		mutex_unlock(&mc_restool->mutex);
+		return -EINVAL;
+	}
+
+	if (filep->private_data == root_mc_device->mc_io) {
+		mc_restool->local_instance_in_use = false;
+	} else {
+		fsl_mc_portal_free(mc_io);
+		kfree(mc_io);
+		mc_restool->dynamic_instance_count--;
+	}
+
+	filep->private_data = NULL;
+	mutex_unlock(&mc_restool->mutex);
+
+	return 0;
+}
+
+static long fsl_mc_restool_dev_ioctl(struct file *file,
+				     unsigned int cmd,
+				     unsigned long arg)
+{
+	int error;
+
+	switch (cmd) {
+	case RESTOOL_SEND_MC_COMMAND:
+		error = fsl_mc_restool_send_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,
+};
+
+int fsl_mc_restool_create_device_file(struct fsl_mc_bus *mc_bus)
+{
+	struct fsl_mc_device *mc_dev = &mc_bus->mc_dev;
+	struct fsl_mc_restool *mc_restool = &mc_bus->restool_misc;
+	int error;
+
+	mc_restool = &mc_bus->restool_misc;
+	mc_restool->dev = MKDEV(fsl_mc_bus_major, 0);
+	cdev_init(&mc_restool->cdev, &fsl_mc_restool_dev_fops);
+
+	error = cdev_add(&mc_restool->cdev,
+			 mc_restool->dev,
+			 FSL_MC_BUS_MAX_MINORS);
+	if (error)
+		return error;
+
+	mc_restool->device = device_create(fsl_mc_bus_class,
+					   NULL,
+					   mc_restool->dev,
+					   NULL,
+					   "%s",
+					   dev_name(&mc_dev->dev));
+	if (IS_ERR(mc_restool->device)) {
+		error = PTR_ERR(mc_restool->device);
+		goto error_device_create;
+	}
+
+	mutex_init(&mc_restool->mutex);
+
+	return 0;
+
+error_device_create:
+	cdev_del(&mc_restool->cdev);
+
+	return error;
+}
+
+void fsl_mc_restool_remove_device_file(struct fsl_mc_bus *mc_bus)
+{
+	struct fsl_mc_restool *mc_restool = &mc_bus->restool_misc;
+
+	if (WARN_ON(mc_restool->local_instance_in_use))
+		return;
+
+	if (WARN_ON(mc_restool->dynamic_instance_count != 0))
+		return;
+
+	cdev_del(&mc_restool->cdev);
+}
-- 
1.9.1

Re: [PATCH 1/3] bus: fsl-mc: add restool userspace support

[Greg KH]

On Wed, Mar 07, 2018 at 10:51:35AM -0600, Ioana Ciornei wrote:
> Adding kernel support for restool, a userspace tool for resource
> management, means exporting an ioctl capable device file representing
> the root resource container.
> This new functionality in the fsl-mc bus driver intends to provide
> restool an interface to interact with the MC firmware.
> Commands that are composed in userspace are sent to the MC firmware
> through the RESTOOL_SEND_MC_COMMAND ioctl.
> By default the implicit MC I/O portal is used for this operation,
> but if the implicit one is busy, a dynamic portal is allocated and then
> freed upon execution.
> 
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> ---
>  Documentation/ioctl/ioctl-number.txt        |   1 +
>  Documentation/networking/dpaa2/overview.rst |   4 +
>  drivers/bus/fsl-mc/Kconfig                  |   7 +
>  drivers/bus/fsl-mc/Makefile                 |   3 +
>  drivers/bus/fsl-mc/fsl-mc-allocator.c       |   5 +
>  drivers/bus/fsl-mc/fsl-mc-bus.c             |  19 +++
>  drivers/bus/fsl-mc/fsl-mc-private.h         |  56 +++++++
>  drivers/bus/fsl-mc/fsl-mc-restool.c         | 219 ++++++++++++++++++++++++++++

This is a "tiny" patch, yet I think it needs to be broken up more, as
you are mixing a few different things in the same patch, and you forgot
one big thing...

>  8 files changed, 314 insertions(+)
>  create mode 100644 drivers/bus/fsl-mc/fsl-mc-restool.c
> 
> diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
> index 6501389..d427397 100644
> --- a/Documentation/ioctl/ioctl-number.txt
> +++ b/Documentation/ioctl/ioctl-number.txt
> @@ -170,6 +170,7 @@ Code  Seq#(hex)	Include File		Comments
>  'R'	00-1F	linux/random.h		conflict!
>  'R'	01	linux/rfkill.h		conflict!
>  'R'	C0-DF	net/bluetooth/rfcomm.h
> +'R'	E0	drivers/bus/fsl-mc/fsl-mc-private.h
>  'S'	all	linux/cdrom.h		conflict!
>  'S'	80-81	scsi/scsi_ioctl.h	conflict!
>  'S'	82-FF	scsi/scsi.h		conflict!
> diff --git a/Documentation/networking/dpaa2/overview.rst b/Documentation/networking/dpaa2/overview.rst
> index 79fede4..1056445 100644
> --- a/Documentation/networking/dpaa2/overview.rst
> +++ b/Documentation/networking/dpaa2/overview.rst
> @@ -127,6 +127,10 @@ level.
>  
>  DPRCs can be defined statically and populated with objects
>  via a config file passed to the MC when firmware starts it.
> +There is also a Linux user space tool called "restool" that can be
> +used to create/destroy containers and objects dynamically. The latest
> +version of restool can be found at:
> +        https://github.com/qoriq-open-source/restool
>  
>  DPAA2 Objects for an Ethernet Network Interface
>  -----------------------------------------------
> diff --git a/drivers/bus/fsl-mc/Kconfig b/drivers/bus/fsl-mc/Kconfig
> index c23c77c..66ec3b9 100644
> --- a/drivers/bus/fsl-mc/Kconfig
> +++ b/drivers/bus/fsl-mc/Kconfig
> @@ -14,3 +14,10 @@ config FSL_MC_BUS
>  	  architecture.  The fsl-mc bus driver handles discovery of
>  	  DPAA2 objects (which are represented as Linux devices) and
>  	  binding objects to drivers.
> +
> +config FSL_MC_RESTOOL
> +	bool "Management Complex (MC) restool support"
> +	depends on FSL_MC_BUS
> +	help
> +	  Provides kernel support for the Management Complex resource
> +	  manager user-space tool - restool.

Why would you want to make this a build option?  Why would you ever
_not_ want this?


> diff --git a/drivers/bus/fsl-mc/Makefile b/drivers/bus/fsl-mc/Makefile
> index 6a97f2c..9a155e3 100644
> --- a/drivers/bus/fsl-mc/Makefile
> +++ b/drivers/bus/fsl-mc/Makefile
> @@ -14,3 +14,6 @@ mc-bus-driver-objs := fsl-mc-bus.o \
>  		      fsl-mc-allocator.o \
>  		      fsl-mc-msi.o \
>  		      dpmcp.o
> +
> +# MC restool kernel support
> +obj-$(CONFIG_FSL_MC_RESTOOL) += fsl-mc-restool.o
> diff --git a/drivers/bus/fsl-mc/fsl-mc-allocator.c b/drivers/bus/fsl-mc/fsl-mc-allocator.c
> index 452c5d7..fb1442b 100644
> --- a/drivers/bus/fsl-mc/fsl-mc-allocator.c
> +++ b/drivers/bus/fsl-mc/fsl-mc-allocator.c
> @@ -646,3 +646,8 @@ int __init fsl_mc_allocator_driver_init(void)
>  {
>  	return fsl_mc_driver_register(&fsl_mc_allocator_driver);
>  }
> +
> +void fsl_mc_allocator_driver_exit(void)
> +{
> +	fsl_mc_driver_unregister(&fsl_mc_allocator_driver);
> +}

Why are you mixing the bus/driver changes in with the addition of the
ioctl?  That should be broken out into the "first" patch of this series,
to make the addition of the ioctl easier to see and review.

> +#define RESTOOL_IOCTL_TYPE	'R'
> +#define RESTOOL_IOCTL_SEQ	0xE0
> +
> +#define RESTOOL_SEND_MC_COMMAND \
> +	_IOWR(RESTOOL_IOCTL_TYPE, RESTOOL_IOCTL_SEQ, struct mc_command)

"struct mc_command" is not defined as a structure that can cross the
user/kernel boundry at all.  At the least it is not in a public uapi
header file.  It also does not use the correct variable types, and it is
a very generic name for a global kernel structure that the whole world
is now going to be able to see.

Please fix all of that up first, before adding the ioctl itself :)

> +static int fsl_mc_restool_send_command(unsigned long arg,
> +				       struct fsl_mc_io *mc_io)
> +{
> +	struct mc_command mc_cmd;
> +	int error;
> +
> +	error = copy_from_user(&mc_cmd, (void __user *)arg, sizeof(mc_cmd));
> +	if (error)
> +		return -EFAULT;
> +
> +	error = mc_send_command(mc_io, &mc_cmd);

are you doing correct error and validation checking of this
user-provided structure?  Remember, you can not trust this data at all.

All input is evil.

thanks,

greg k-h

RE: [PATCH 1/3] bus: fsl-mc: add restool userspace support

[Ioana Ciornei]

Hi, 

Comments inline.

> > Adding kernel support for restool, a userspace tool for resource
> > management, means exporting an ioctl capable device file representing
> > the root resource container.
> > This new functionality in the fsl-mc bus driver intends to provide
> > restool an interface to interact with the MC firmware.
> > Commands that are composed in userspace are sent to the MC firmware
> > through the RESTOOL_SEND_MC_COMMAND ioctl.
> > By default the implicit MC I/O portal is used for this operation, but
> > if the implicit one is busy, a dynamic portal is allocated and then
> > freed upon execution.
> >
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > ---
> >  Documentation/ioctl/ioctl-number.txt        |   1 +
> >  Documentation/networking/dpaa2/overview.rst |   4 +
> >  drivers/bus/fsl-mc/Kconfig                  |   7 +
> >  drivers/bus/fsl-mc/Makefile                 |   3 +
> >  drivers/bus/fsl-mc/fsl-mc-allocator.c       |   5 +
> >  drivers/bus/fsl-mc/fsl-mc-bus.c             |  19 +++
> >  drivers/bus/fsl-mc/fsl-mc-private.h         |  56 +++++++
> >  drivers/bus/fsl-mc/fsl-mc-restool.c         | 219
> ++++++++++++++++++++++++++++
> 
> This is a "tiny" patch, yet I think it needs to be broken up more, as you are
> mixing a few different things in the same patch, and you forgot one big thing...

I will break the patch into multiple ones in the next version.

> 
> >  8 files changed, 314 insertions(+)
> >  create mode 100644 drivers/bus/fsl-mc/fsl-mc-restool.c
> >
> > diff --git a/Documentation/ioctl/ioctl-number.txt
> > b/Documentation/ioctl/ioctl-number.txt
> > index 6501389..d427397 100644
> > --- a/Documentation/ioctl/ioctl-number.txt
> > +++ b/Documentation/ioctl/ioctl-number.txt
> > @@ -170,6 +170,7 @@ Code  Seq#(hex)	Include File
> 	Comments
> >  'R'	00-1F	linux/random.h		conflict!
> >  'R'	01	linux/rfkill.h		conflict!
> >  'R'	C0-DF	net/bluetooth/rfcomm.h
> > +'R'	E0	drivers/bus/fsl-mc/fsl-mc-private.h
> >  'S'	all	linux/cdrom.h		conflict!
> >  'S'	80-81	scsi/scsi_ioctl.h	conflict!
> >  'S'	82-FF	scsi/scsi.h		conflict!
> > diff --git a/Documentation/networking/dpaa2/overview.rst
> > b/Documentation/networking/dpaa2/overview.rst
> > index 79fede4..1056445 100644
> > --- a/Documentation/networking/dpaa2/overview.rst
> > +++ b/Documentation/networking/dpaa2/overview.rst
> > @@ -127,6 +127,10 @@ level.
> >
> >  DPRCs can be defined statically and populated with objects  via a
> > config file passed to the MC when firmware starts it.
> > +There is also a Linux user space tool called "restool" that can be
> > +used to create/destroy containers and objects dynamically. The latest
> > +version of restool can be found at:
> > +
> > + https://github.com/qoriq-open-source/restool
> >  DPAA2 Objects for an Ethernet Network Interface
> >  -----------------------------------------------
> > diff --git a/drivers/bus/fsl-mc/Kconfig b/drivers/bus/fsl-mc/Kconfig
> > index c23c77c..66ec3b9 100644
> > --- a/drivers/bus/fsl-mc/Kconfig
> > +++ b/drivers/bus/fsl-mc/Kconfig
> > @@ -14,3 +14,10 @@ config FSL_MC_BUS
> >  	  architecture.  The fsl-mc bus driver handles discovery of
> >  	  DPAA2 objects (which are represented as Linux devices) and
> >  	  binding objects to drivers.
> > +
> > +config FSL_MC_RESTOOL
> > +	bool "Management Complex (MC) restool support"
> > +	depends on FSL_MC_BUS
> > +	help
> > +	  Provides kernel support for the Management Complex resource
> > +	  manager user-space tool - restool.
> 
> Why would you want to make this a build option?  Why would you ever _not_
> want this?

Using the restool user-space tool for managing MC resources is not the only possibility for creating/destroying MC objects, changing their properties, etc.
While restool's  intended use is in a dynamic context, users also have the option to deploy a static configuration using a Data Path Layout file that describes the MC resource configuration.
In this case, the restool support is no longer needed.
 
> 
> 
> > diff --git a/drivers/bus/fsl-mc/Makefile b/drivers/bus/fsl-mc/Makefile
> > index 6a97f2c..9a155e3 100644
> > --- a/drivers/bus/fsl-mc/Makefile
> > +++ b/drivers/bus/fsl-mc/Makefile
> > @@ -14,3 +14,6 @@ mc-bus-driver-objs := fsl-mc-bus.o \
> >  		      fsl-mc-allocator.o \
> >  		      fsl-mc-msi.o \
> >  		      dpmcp.o
> > +
> > +# MC restool kernel support
> > +obj-$(CONFIG_FSL_MC_RESTOOL) += fsl-mc-restool.o
> > diff --git a/drivers/bus/fsl-mc/fsl-mc-allocator.c
> > b/drivers/bus/fsl-mc/fsl-mc-allocator.c
> > index 452c5d7..fb1442b 100644
> > --- a/drivers/bus/fsl-mc/fsl-mc-allocator.c
> > +++ b/drivers/bus/fsl-mc/fsl-mc-allocator.c
> > @@ -646,3 +646,8 @@ int __init fsl_mc_allocator_driver_init(void)
> >  {
> >  	return fsl_mc_driver_register(&fsl_mc_allocator_driver);
> >  }
> > +
> > +void fsl_mc_allocator_driver_exit(void)
> > +{
> > +	fsl_mc_driver_unregister(&fsl_mc_allocator_driver);
> > +}
> 
> Why are you mixing the bus/driver changes in with the addition of the ioctl?
> That should be broken out into the "first" patch of this series, to make the
> addition of the ioctl easier to see and review.

Will split the bus changes into a separate patch.

> 
> > +#define RESTOOL_IOCTL_TYPE	'R'
> > +#define RESTOOL_IOCTL_SEQ	0xE0
> > +
> > +#define RESTOOL_SEND_MC_COMMAND \
> > +	_IOWR(RESTOOL_IOCTL_TYPE, RESTOOL_IOCTL_SEQ, struct
> mc_command)
> 
> "struct mc_command" is not defined as a structure that can cross the
> user/kernel boundry at all.  At the least it is not in a public uapi header file.  It
> also does not use the correct variable types, and it is a very generic name for
> a global kernel structure that the whole world is now going to be able to see.
> 
> Please fix all of that up first, before adding the ioctl itself :)

I will move the mc_command structure into a uapi header file for the fsl-mc bus in the next version of the patchset.


> 
> > +static int fsl_mc_restool_send_command(unsigned long arg,
> > +				       struct fsl_mc_io *mc_io)
> > +{
> > +	struct mc_command mc_cmd;
> > +	int error;
> > +
> > +	error = copy_from_user(&mc_cmd, (void __user *)arg,
> sizeof(mc_cmd));
> > +	if (error)
> > +		return -EFAULT;
> > +
> > +	error = mc_send_command(mc_io, &mc_cmd);
> 
> are you doing correct error and validation checking of this user-provided
> structure?  Remember, you can not trust this data at all.

The Management Complex is the one validating the commands received.
The restool support in the bus driver is just a passthrough for the commands passed from user-space and their associated responses from the MC firmware.

> 
> All input is evil.
> 
> thanks,
> 
> greg k-h

[PATCH 2/3] bus: fsl-mc: add root dprc rescan attribute

[Ioana Ciornei]

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: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/bus/fsl-mc/dprc-driver.c    |  4 ++--
 drivers/bus/fsl-mc/fsl-mc-bus.c     | 28 ++++++++++++++++++++++++++++
 drivers/bus/fsl-mc/fsl-mc-private.h |  3 +++
 3 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/fsl-mc/dprc-driver.c b/drivers/bus/fsl-mc/dprc-driver.c
index 52c7e15..be80e3a 100644
--- a/drivers/bus/fsl-mc/dprc-driver.c
+++ b/drivers/bus/fsl-mc/dprc-driver.c
@@ -214,8 +214,8 @@ static void dprc_add_new_devices(struct fsl_mc_device *mc_bus_dev,
  * populated before they can get allocation requests from probe callbacks
  * of the device drivers for the non-allocatable devices.
  */
-static int dprc_scan_objects(struct fsl_mc_device *mc_bus_dev,
-			     unsigned int *total_irq_count)
+int dprc_scan_objects(struct fsl_mc_device *mc_bus_dev,
+		      unsigned int *total_irq_count)
 {
 	int num_child_objects;
 	int dprc_get_obj_failures;
diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index 240b99d..763cbeb 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -137,8 +137,36 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(modalias);
 
+static ssize_t rescan_store(struct device *dev,
+			    struct device_attribute *attr,
+			    const char *buf, size_t count)
+{
+	struct fsl_mc_device *root_mc_dev;
+	struct fsl_mc_bus *root_mc_bus;
+	unsigned long val;
+
+	if (!fsl_mc_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, NULL);
+		mutex_unlock(&root_mc_bus->scan_mutex);
+	}
+
+	return count;
+}
+static DEVICE_ATTR_WO(rescan);
+
 static struct attribute *fsl_mc_dev_attrs[] = {
 	&dev_attr_modalias.attr,
+	&dev_attr_rescan.attr,
 	NULL,
 };
 
diff --git a/drivers/bus/fsl-mc/fsl-mc-private.h b/drivers/bus/fsl-mc/fsl-mc-private.h
index d6f67a8..8a244aa 100644
--- a/drivers/bus/fsl-mc/fsl-mc-private.h
+++ b/drivers/bus/fsl-mc/fsl-mc-private.h
@@ -464,6 +464,9 @@ int __must_check fsl_mc_device_add(struct fsl_mc_obj_desc *obj_desc,
 
 void dprc_driver_exit(void);
 
+int dprc_scan_objects(struct fsl_mc_device *mc_bus_dev,
+		      unsigned int *total_irq_count);
+
 int __init fsl_mc_allocator_driver_init(void);
 
 void fsl_mc_allocator_driver_exit(void);
-- 
1.9.1

Re: [PATCH 2/3] bus: fsl-mc: add root dprc rescan attribute

[Greg KH]

On Wed, Mar 07, 2018 at 10:51:36AM -0600, Ioana Ciornei wrote:
> 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: Ioana Ciornei <ioana.ciornei@nxp.com>
> ---
>  drivers/bus/fsl-mc/dprc-driver.c    |  4 ++--
>  drivers/bus/fsl-mc/fsl-mc-bus.c     | 28 ++++++++++++++++++++++++++++
>  drivers/bus/fsl-mc/fsl-mc-private.h |  3 +++
>  3 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bus/fsl-mc/dprc-driver.c b/drivers/bus/fsl-mc/dprc-driver.c
> index 52c7e15..be80e3a 100644
> --- a/drivers/bus/fsl-mc/dprc-driver.c
> +++ b/drivers/bus/fsl-mc/dprc-driver.c
> @@ -214,8 +214,8 @@ static void dprc_add_new_devices(struct fsl_mc_device *mc_bus_dev,
>   * populated before they can get allocation requests from probe callbacks
>   * of the device drivers for the non-allocatable devices.
>   */
> -static int dprc_scan_objects(struct fsl_mc_device *mc_bus_dev,
> -			     unsigned int *total_irq_count)
> +int dprc_scan_objects(struct fsl_mc_device *mc_bus_dev,
> +		      unsigned int *total_irq_count)
>  {
>  	int num_child_objects;
>  	int dprc_get_obj_failures;
> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
> index 240b99d..763cbeb 100644
> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
> @@ -137,8 +137,36 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(modalias);
>  
> +static ssize_t rescan_store(struct device *dev,
> +			    struct device_attribute *attr,
> +			    const char *buf, size_t count)
> +{
> +	struct fsl_mc_device *root_mc_dev;
> +	struct fsl_mc_bus *root_mc_bus;
> +	unsigned long val;
> +
> +	if (!fsl_mc_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, NULL);
> +		mutex_unlock(&root_mc_bus->scan_mutex);
> +	}
> +
> +	return count;
> +}
> +static DEVICE_ATTR_WO(rescan);

You did not add the correct new documentation in Documentation/ABI/ for
the new sysfs attributes you are creating.  Please do so as part of this
patch series.

thanks,

greg k-h

RE: [PATCH 2/3] bus: fsl-mc: add root dprc rescan attribute

[Ioana Ciornei]

> > 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: Ioana Ciornei <ioana.ciornei@nxp.com>
> > ---
> >  drivers/bus/fsl-mc/dprc-driver.c    |  4 ++--
> >  drivers/bus/fsl-mc/fsl-mc-bus.c     | 28 ++++++++++++++++++++++++++++
> >  drivers/bus/fsl-mc/fsl-mc-private.h |  3 +++
> >  3 files changed, 33 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/bus/fsl-mc/dprc-driver.c
> > b/drivers/bus/fsl-mc/dprc-driver.c
> > index 52c7e15..be80e3a 100644
> > --- a/drivers/bus/fsl-mc/dprc-driver.c
> > +++ b/drivers/bus/fsl-mc/dprc-driver.c
> > @@ -214,8 +214,8 @@ static void dprc_add_new_devices(struct
> fsl_mc_device *mc_bus_dev,
> >   * populated before they can get allocation requests from probe callbacks
> >   * of the device drivers for the non-allocatable devices.
> >   */
> > -static int dprc_scan_objects(struct fsl_mc_device *mc_bus_dev,
> > -			     unsigned int *total_irq_count)
> > +int dprc_scan_objects(struct fsl_mc_device *mc_bus_dev,
> > +		      unsigned int *total_irq_count)
> >  {
> >  	int num_child_objects;
> >  	int dprc_get_obj_failures;
> > diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c
> > b/drivers/bus/fsl-mc/fsl-mc-bus.c index 240b99d..763cbeb 100644
> > --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
> > +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
> > @@ -137,8 +137,36 @@ static ssize_t modalias_show(struct device *dev,
> > struct device_attribute *attr,  }  static DEVICE_ATTR_RO(modalias);
> >
> > +static ssize_t rescan_store(struct device *dev,
> > +			    struct device_attribute *attr,
> > +			    const char *buf, size_t count) {
> > +	struct fsl_mc_device *root_mc_dev;
> > +	struct fsl_mc_bus *root_mc_bus;
> > +	unsigned long val;
> > +
> > +	if (!fsl_mc_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, NULL);
> > +		mutex_unlock(&root_mc_bus->scan_mutex);
> > +	}
> > +
> > +	return count;
> > +}
> > +static DEVICE_ATTR_WO(rescan);
> 
> You did not add the correct new documentation in Documentation/ABI/ for
> the new sysfs attributes you are creating.  Please do so as part of this patch
> series.

Sorry for that. Will add the proper Documentation in the next version.

Ioana

> 
> thanks,
> 
> greg k-h

[PATCH 3/3] bus: fsl-mc: add bus rescan attribute

[Ioana Ciornei]

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: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/bus/fsl-mc/fsl-mc-bus.c | 48 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index 763cbeb..da9fd29 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -172,11 +172,59 @@ static ssize_t rescan_store(struct device *dev,
 
 ATTRIBUTE_GROUPS(fsl_mc_dev);
 
+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 (!fsl_mc_is_root_dprc(dev))
+		goto exit;
+
+	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, NULL);
+	mutex_unlock(&root_mc_bus->scan_mutex);
+
+exit:
+	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, 0220, 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);
 
-- 
1.9.1