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

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

[Ioana Ciornei]

This patch set adds userspace support in the fsl-mc bus along with a rescan
attribute to the root DPRC container.  Also, the patches here depend on one
other patch submitted but not yet accepted:
 - https://lkml.org/lkml/2018/11/15/776

An earlier discussion on this functionality can be found here:
https://lkml.org/lkml/2018/3/23/941

The FSL_MC_SEND_MC_COMMAND ioctl acts as a direct passthrough to the Management
Complex (or MC) by passing fixed-length command/response pairs. Keeping in mind
that the MC manages DPAA2 hardware resources and provides and object-based
abstraction for sofware drivers, the proposed ioctl interface enables userspace
tools to do the following:
 - create and destroy DPAA2 objects
 - manage the DPRC container that objects live in (by moving objects between
   containers)
 - establish connections between different objects as needed

The ioctl interface is intended for dynamic usecases where DPAA2 objects need
to be created on demand or destroyed so that the underlying hardware resources
can be further repurposed. In static usecases, depending on the requirements, a
firmware configuration file can be used to describe the needed DPAA2 objects,
their attributes or any link between them.

Some examples of device drivers that probe on said DPAA2 objects are listed
below:
 - https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/soc/fsl/dpio
 - https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/tree/drivers/net/ethernet/freescale/dpaa2
 - https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/tree/drivers/crypto/caam/

Having a low-level ioctl interface where the command is passed through to the
MC without any intervention from the fsl-mc bus reduces the complexity added in
the kernel while also making the command structure, in turn the ioctl, very
stable.

Changes in v2:
   - use root dprc MC portal by default
   - create the uapi misc device from the root dprc probe function
   - remove the rescan dev_attr on the remove path

Ioana Ciornei (4):
  bus: fsl-mc: move fsl_mc_command struct in a uapi header
  bus: fsl-mc: add fsl-mc userspace support
  bus: fsl-mc: add root dprc rescan attribute
  bus: fsl-mc: add bus rescan attribute

 Documentation/ABI/stable/sysfs-bus-fsl-mc |  19 ++++
 Documentation/ioctl/ioctl-number.txt      |   1 +
 MAINTAINERS                               |   2 +
 drivers/bus/fsl-mc/Kconfig                |   7 ++
 drivers/bus/fsl-mc/Makefile               |   3 +
 drivers/bus/fsl-mc/dprc-driver.c          |  54 ++++++++-
 drivers/bus/fsl-mc/fsl-mc-bus.c           |  41 +++++++
 drivers/bus/fsl-mc/fsl-mc-private.h       |  43 +++++++
 drivers/bus/fsl-mc/fsl-mc-uapi.c          | 179 ++++++++++++++++++++++++++++++
 include/linux/fsl/mc.h                    |   8 +-
 include/uapi/linux/fsl_mc.h               |  34 ++++++
 11 files changed, 381 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/ABI/stable/sysfs-bus-fsl-mc
 create mode 100644 drivers/bus/fsl-mc/fsl-mc-uapi.c
 create mode 100644 include/uapi/linux/fsl_mc.h

-- 
1.9.1

[PATCH v2 1/4] bus: fsl-mc: move fsl_mc_command struct in a uapi header

[Ioana Ciornei]

Define "struct fsl_mc_command" as a structure that can cross the
user/kernel boundary.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v2:
  - none

 MAINTAINERS                 |  1 +
 include/linux/fsl/mc.h      |  8 +-------
 include/uapi/linux/fsl_mc.h | 25 +++++++++++++++++++++++++
 3 files changed, 27 insertions(+), 7 deletions(-)
 create mode 100644 include/uapi/linux/fsl_mc.h

diff --git a/MAINTAINERS b/MAINTAINERS
index f485597..e841cc4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12282,6 +12282,7 @@ S:	Maintained
 F:	drivers/bus/fsl-mc/
 F:	Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
 F:	Documentation/networking/dpaa2/overview.rst
+F:	include/uapi/linux/fsl_mc.h
 
 QT1010 MEDIA DRIVER
 M:	Antti Palosaari <crope@iki.fi>
diff --git a/include/linux/fsl/mc.h b/include/linux/fsl/mc.h
index 741f567..64fc9f8 100644
--- a/include/linux/fsl/mc.h
+++ b/include/linux/fsl/mc.h
@@ -12,6 +12,7 @@
 #include <linux/device.h>
 #include <linux/mod_devicetable.h>
 #include <linux/interrupt.h>
+#include <uapi/linux/fsl_mc.h>
 
 #define FSL_MC_VENDOR_FREESCALE	0x1957
 
@@ -198,8 +199,6 @@ struct fsl_mc_device {
 #define to_fsl_mc_device(_dev) \
 	container_of(_dev, struct fsl_mc_device, dev)
 
-#define MC_CMD_NUM_OF_PARAMS	7
-
 struct mc_cmd_header {
 	u8 src_id;
 	u8 flags_hw;
@@ -209,11 +208,6 @@ struct mc_cmd_header {
 	__le16 cmd_id;
 };
 
-struct fsl_mc_command {
-	__le64 header;
-	__le64 params[MC_CMD_NUM_OF_PARAMS];
-};
-
 enum mc_cmd_status {
 	MC_CMD_STATUS_OK = 0x0, /* Completed successfully */
 	MC_CMD_STATUS_READY = 0x1, /* Ready to be processed */
diff --git a/include/uapi/linux/fsl_mc.h b/include/uapi/linux/fsl_mc.h
new file mode 100644
index 0000000..6c19cfc
--- /dev/null
+++ b/include/uapi/linux/fsl_mc.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Management Complex (MC) userspace public interface
+ *
+ * Copyright 2018 NXP
+ *
+ */
+#ifndef _UAPI_FSL_MC_H_
+#define _UAPI_FSL_MC_H_
+
+#include <linux/types.h>
+
+#define MC_CMD_NUM_OF_PARAMS	7
+
+/**
+ * struct fsl_mc_command - Management Complex (MC) command structure
+ * @header: MC command header
+ * @params: MC command parameters
+ */
+struct fsl_mc_command {
+	__le64 header;
+	__le64 params[MC_CMD_NUM_OF_PARAMS];
+};
+
+#endif /* _UAPI_FSL_MC_H_ */
-- 
1.9.1

[PATCH v2 2/4] bus: fsl-mc: add fsl-mc userspace support

[Ioana Ciornei]

Adding userspace support for the MC (Management Complex) means exporting
an ioctl capable device file representing the root resource container.

This new functionality in the fsl-mc bus driver intends to provide
userspace applications an interface to interact with the MC firmware.

Commands that are composed in userspace are sent to the MC firmware
through the FSL_MC_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>
---
Changes in v2:
   - use root dprc MC portal by default
   - create the uapi misc device from the root dprc probe function

 Documentation/ioctl/ioctl-number.txt |   1 +
 drivers/bus/fsl-mc/Kconfig           |   7 ++
 drivers/bus/fsl-mc/Makefile          |   3 +
 drivers/bus/fsl-mc/dprc-driver.c     |  14 ++-
 drivers/bus/fsl-mc/fsl-mc-private.h  |  41 ++++++++
 drivers/bus/fsl-mc/fsl-mc-uapi.c     | 179 +++++++++++++++++++++++++++++++++++
 include/uapi/linux/fsl_mc.h          |   9 ++
 7 files changed, 253 insertions(+), 1 deletion(-)
 create mode 100644 drivers/bus/fsl-mc/fsl-mc-uapi.c

diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index af6f6ba..eaae7bf 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -171,6 +171,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	uapi/linux/fsl_mc.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/drivers/bus/fsl-mc/Kconfig b/drivers/bus/fsl-mc/Kconfig
index c23c77c..cde6f40 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_UAPI_SUPPORT
+	bool "Management Complex (MC) userspace support"
+	depends on FSL_MC_BUS
+	help
+	  Provides userspace support for creating/destroying/configuring
+	  DPAA2 objects in the Management Complex.
diff --git a/drivers/bus/fsl-mc/Makefile b/drivers/bus/fsl-mc/Makefile
index 3c518c7..4ae292a 100644
--- a/drivers/bus/fsl-mc/Makefile
+++ b/drivers/bus/fsl-mc/Makefile
@@ -16,3 +16,6 @@ mc-bus-driver-objs := fsl-mc-bus.o \
 		      fsl-mc-allocator.o \
 		      fsl-mc-msi.o \
 		      dpmcp.o
+
+# MC userspace support
+obj-$(CONFIG_FSL_MC_UAPI_SUPPORT) += fsl-mc-uapi.o
diff --git a/drivers/bus/fsl-mc/dprc-driver.c b/drivers/bus/fsl-mc/dprc-driver.c
index 52c7e15..3919d9b 100644
--- a/drivers/bus/fsl-mc/dprc-driver.c
+++ b/drivers/bus/fsl-mc/dprc-driver.c
@@ -593,6 +593,7 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
 	struct fsl_mc_bus *mc_bus = to_fsl_mc_bus(mc_dev);
 	bool mc_io_created = false;
 	bool msi_domain_set = false;
+	bool uapi_created = false;
 	u16 major_ver, minor_ver;
 
 	if (!is_fsl_mc_bus_dprc(mc_dev))
@@ -647,6 +648,11 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
 		} else {
 			dev_set_msi_domain(&mc_dev->dev, mc_msi_domain);
 			msi_domain_set = true;
+
+			error = fsl_mc_uapi_create_device_file(mc_bus);
+			if (error < 0)
+				goto error_cleanup_msi_domain;
+			uapi_created = true;
 		}
 	}
 
@@ -654,7 +660,7 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
 			  &mc_dev->mc_handle);
 	if (error < 0) {
 		dev_err(&mc_dev->dev, "dprc_open() failed: %d\n", error);
-		goto error_cleanup_msi_domain;
+		goto error_cleanup_uapi;
 	}
 
 	error = dprc_get_attributes(mc_dev->mc_io, 0, mc_dev->mc_handle,
@@ -706,6 +712,10 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
 error_cleanup_open:
 	(void)dprc_close(mc_dev->mc_io, 0, mc_dev->mc_handle);
 
+error_cleanup_uapi:
+	if (uapi_created)
+		fsl_mc_uapi_remove_device_file(mc_bus);
+
 error_cleanup_msi_domain:
 	if (msi_domain_set)
 		dev_set_msi_domain(&mc_dev->dev, NULL);
@@ -774,6 +784,8 @@ static int dprc_remove(struct fsl_mc_device *mc_dev)
 	if (!fsl_mc_is_root_dprc(&mc_dev->dev)) {
 		fsl_destroy_mc_io(mc_dev->mc_io);
 		mc_dev->mc_io = NULL;
+	} else {
+		fsl_mc_uapi_remove_device_file(mc_bus);
 	}
 
 	dev_info(&mc_dev->dev, "DPRC device unbound from driver");
diff --git a/drivers/bus/fsl-mc/fsl-mc-private.h b/drivers/bus/fsl-mc/fsl-mc-private.h
index ea11b4f..16902f9 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/ioctl.h>
+#include <linux/miscdevice.h>
 
 /*
  * Data Path Management Complex (DPMNG) General API
@@ -492,6 +494,24 @@ struct fsl_mc_resource_pool {
 };
 
 /**
+ * struct fsl_mc_uapi - information associated with a device file
+ * @misc: struct miscdevice linked to the root dprc
+ * @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
+ * @static_mc_io: pointer to the static MC I/O object
+ */
+struct fsl_mc_uapi {
+	struct miscdevice misc;
+	struct device *device;
+	struct mutex mutex; /* serialize open/release operations */
+	u32 local_instance_in_use;
+	u32 dynamic_instance_count;
+	struct fsl_mc_io *static_mc_io;
+};
+
+/**
  * 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)
@@ -500,6 +520,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
+ * @uapi_misc: struct that abstracts the interaction with userspace
  */
 struct fsl_mc_bus {
 	struct fsl_mc_device mc_dev;
@@ -507,6 +528,7 @@ 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_uapi uapi_misc;
 };
 
 #define to_fsl_mc_bus(_mc_dev) \
@@ -561,4 +583,23 @@ int __must_check fsl_create_mc_io(struct device *dev,
 
 bool fsl_mc_is_root_dprc(struct device *dev);
 
+#ifdef CONFIG_FSL_MC_UAPI_SUPPORT
+
+int fsl_mc_uapi_create_device_file(struct fsl_mc_bus *mc_bus);
+
+void fsl_mc_uapi_remove_device_file(struct fsl_mc_bus *mc_bus);
+
+#else
+
+static inline int fsl_mc_uapi_create_device_file(struct fsl_mc_bus *mc_bus)
+{
+	return 0;
+}
+
+static inline void fsl_mc_uapi_remove_device_file(struct fsl_mc_bus *mc_bus)
+{
+}
+
+#endif
+
 #endif /* _FSL_MC_PRIVATE_H_ */
diff --git a/drivers/bus/fsl-mc/fsl-mc-uapi.c b/drivers/bus/fsl-mc/fsl-mc-uapi.c
new file mode 100644
index 0000000..8c9debb
--- /dev/null
+++ b/drivers/bus/fsl-mc/fsl-mc-uapi.c
@@ -0,0 +1,179 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Management Complex (MC) userspace support
+ *
+ * Copyright 2018 NXP
+ *
+ */
+
+#include <linux/slab.h>
+#include <linux/fs.h>
+#include <linux/uaccess.h>
+#include <linux/miscdevice.h>
+
+#include "fsl-mc-private.h"
+
+struct uapi_priv_data {
+	struct fsl_mc_uapi *uapi;
+	struct fsl_mc_io *mc_io;
+};
+
+static int fsl_mc_uapi_send_command(unsigned long arg,
+				    struct fsl_mc_io *mc_io)
+{
+	struct fsl_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;
+}
+
+static int fsl_mc_uapi_dev_open(struct inode *inode, struct file *filep)
+{
+	struct fsl_mc_device *root_mc_device;
+	struct uapi_priv_data *priv_data;
+	struct fsl_mc_io *dynamic_mc_io;
+	struct fsl_mc_uapi *mc_uapi;
+	struct fsl_mc_bus *mc_bus;
+	int error;
+
+	priv_data = kzalloc(sizeof(*priv_data), GFP_KERNEL);
+	if (!priv_data)
+		return -ENOMEM;
+
+	mc_uapi = container_of(filep->private_data, struct fsl_mc_uapi, misc);
+	mc_bus = container_of(mc_uapi, struct fsl_mc_bus, uapi_misc);
+	root_mc_device = &mc_bus->mc_dev;
+
+	mutex_lock(&mc_uapi->mutex);
+
+	if (!mc_uapi->local_instance_in_use) {
+		priv_data->mc_io = mc_uapi->static_mc_io;
+		mc_uapi->local_instance_in_use = true;
+	} else {
+		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_uapi->dynamic_instance_count++;
+		priv_data->mc_io = dynamic_mc_io;
+	}
+	priv_data->uapi = mc_uapi;
+	filep->private_data = priv_data;
+
+	mutex_unlock(&mc_uapi->mutex);
+
+	return 0;
+
+error_portal_allocate:
+	mutex_unlock(&mc_uapi->mutex);
+
+	return error;
+}
+
+static int fsl_mc_uapi_dev_release(struct inode *inode, struct file *filep)
+{
+	struct uapi_priv_data *priv_data;
+	struct fsl_mc_uapi *mc_uapi;
+	struct fsl_mc_io *mc_io;
+
+	priv_data = filep->private_data;
+	mc_uapi = priv_data->uapi;
+	mc_io = priv_data->mc_io;
+
+	mutex_lock(&mc_uapi->mutex);
+
+	if (WARN_ON(!mc_uapi->local_instance_in_use &&
+		    mc_uapi->dynamic_instance_count == 0)) {
+		mutex_unlock(&mc_uapi->mutex);
+		return -EINVAL;
+	}
+
+	if (mc_io == mc_uapi->static_mc_io) {
+		mc_uapi->local_instance_in_use = false;
+	} else {
+		fsl_mc_portal_free(mc_io);
+		mc_uapi->dynamic_instance_count--;
+	}
+
+	kfree(filep->private_data);
+	filep->private_data =  NULL;
+
+	mutex_unlock(&mc_uapi->mutex);
+
+	return 0;
+}
+
+static long fsl_mc_uapi_dev_ioctl(struct file *file,
+				  unsigned int cmd,
+				  unsigned long arg)
+{
+	struct uapi_priv_data *priv_data = file->private_data;
+	int error;
+
+	switch (cmd) {
+	case FSL_MC_SEND_MC_COMMAND:
+		error = fsl_mc_uapi_send_command(arg, priv_data->mc_io);
+		break;
+	default:
+		pr_err("%s: unexpected ioctl call number\n", __func__);
+		error = -EINVAL;
+	}
+
+	return error;
+}
+
+static const struct file_operations fsl_mc_uapi_dev_fops = {
+	.owner = THIS_MODULE,
+	.open = fsl_mc_uapi_dev_open,
+	.release = fsl_mc_uapi_dev_release,
+	.unlocked_ioctl = fsl_mc_uapi_dev_ioctl,
+};
+
+int fsl_mc_uapi_create_device_file(struct fsl_mc_bus *mc_bus)
+{
+	struct fsl_mc_device *mc_dev = &mc_bus->mc_dev;
+	struct fsl_mc_uapi *mc_uapi = &mc_bus->uapi_misc;
+	int error;
+
+	mc_uapi->misc.minor = MISC_DYNAMIC_MINOR;
+	mc_uapi->misc.name = dev_name(&mc_dev->dev);
+	mc_uapi->misc.fops = &fsl_mc_uapi_dev_fops;
+
+	error = misc_register(&mc_uapi->misc);
+	if (error)
+		return -EPROBE_DEFER;
+
+	mc_uapi->static_mc_io = mc_bus->mc_dev.mc_io;
+
+	mutex_init(&mc_uapi->mutex);
+
+	return 0;
+}
+
+void fsl_mc_uapi_remove_device_file(struct fsl_mc_bus *mc_bus)
+{
+	struct fsl_mc_uapi *mc_uapi = &mc_bus->uapi_misc;
+
+	if (WARN_ON(mc_uapi->local_instance_in_use))
+		return;
+
+	if (WARN_ON(mc_uapi->dynamic_instance_count != 0))
+		return;
+
+	misc_deregister(&mc_uapi->misc);
+}
diff --git a/include/uapi/linux/fsl_mc.h b/include/uapi/linux/fsl_mc.h
index 6c19cfc..4989c1b 100644
--- a/include/uapi/linux/fsl_mc.h
+++ b/include/uapi/linux/fsl_mc.h
@@ -16,10 +16,19 @@
  * struct fsl_mc_command - Management Complex (MC) command structure
  * @header: MC command header
  * @params: MC command parameters
+ *
+ * Used by FSL_MC_SEND_MC_COMMAND
  */
 struct fsl_mc_command {
 	__le64 header;
 	__le64 params[MC_CMD_NUM_OF_PARAMS];
 };
 
+#define FSL_MC_SEND_CMD_IOCTL_TYPE	'R'
+#define FSL_MC_SEND_CMD_IOCTL_SEQ	0xE0
+
+#define FSL_MC_SEND_MC_COMMAND \
+	_IOWR(FSL_MC_SEND_CMD_IOCTL_TYPE, FSL_MC_SEND_CMD_IOCTL_SEQ, \
+	struct fsl_mc_command)
+
 #endif /* _UAPI_FSL_MC_H_ */
-- 
1.9.1

[PATCH v2 3/4] 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>
---
Changes in v2:
   - remove the rescan dev_attr on the remove path

 Documentation/ABI/stable/sysfs-bus-fsl-mc |  9 ++++++++
 MAINTAINERS                               |  1 +
 drivers/bus/fsl-mc/dprc-driver.c          | 36 +++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+)
 create mode 100644 Documentation/ABI/stable/sysfs-bus-fsl-mc

diff --git a/Documentation/ABI/stable/sysfs-bus-fsl-mc b/Documentation/ABI/stable/sysfs-bus-fsl-mc
new file mode 100644
index 0000000..9f3a95e
--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-bus-fsl-mc
@@ -0,0 +1,9 @@
+What:		/sys/bus/fsl-mc/devices/dprc.*/rescan
+Date:		November 2018
+KernelVersion:	5.0
+Contact:	Ioana Ciornei <ioana.ciornei@nxp.com>
+Description:	Writing a non-zero value to this attribute will
+		force a rescan of dprc.X container in the system and
+		synchronize the objects under dprc.X and the
+		Management Complex firmware.
+Users:		Userspace drivers and management tools
diff --git a/MAINTAINERS b/MAINTAINERS
index e841cc4..7b76a80 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12283,6 +12283,7 @@ F:	drivers/bus/fsl-mc/
 F:	Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
 F:	Documentation/networking/dpaa2/overview.rst
 F:	include/uapi/linux/fsl_mc.h
+F:	Documentation/ABI/stable/sysfs-bus-fsl-mc
 
 QT1010 MEDIA DRIVER
 M:	Antti Palosaari <crope@iki.fi>
diff --git a/drivers/bus/fsl-mc/dprc-driver.c b/drivers/bus/fsl-mc/dprc-driver.c
index 3919d9b..b3cde63 100644
--- a/drivers/bus/fsl-mc/dprc-driver.c
+++ b/drivers/bus/fsl-mc/dprc-driver.c
@@ -354,6 +354,33 @@ static int dprc_scan_container(struct fsl_mc_device *mc_bus_dev)
 	return 0;
 }
 
+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);
+
 /**
  * dprc_irq0_handler - Regular ISR for DPRC interrupt 0
  *
@@ -692,6 +719,13 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
 
 	mutex_init(&mc_bus->scan_mutex);
 
+	error = device_create_file(&mc_dev->dev, &dev_attr_rescan);
+	if (error < 0) {
+		dev_err(&mc_dev->dev, "device_create_file() failed: %d\n",
+			error);
+		goto error_cleanup_open;
+	}
+
 	/*
 	 * Discover MC objects in DPRC object:
 	 */
@@ -788,6 +822,8 @@ static int dprc_remove(struct fsl_mc_device *mc_dev)
 		fsl_mc_uapi_remove_device_file(mc_bus);
 	}
 
+	device_remove_file(&mc_dev->dev, &dev_attr_rescan);
+
 	dev_info(&mc_dev->dev, "DPRC device unbound from driver");
 	return 0;
 }
-- 
1.9.1

[PATCH v2 4/4] 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>
---
Changes in v2:
  - none

 Documentation/ABI/stable/sysfs-bus-fsl-mc | 10 ++++++++
 drivers/bus/fsl-mc/dprc-driver.c          |  4 +--
 drivers/bus/fsl-mc/fsl-mc-bus.c           | 41 +++++++++++++++++++++++++++++++
 drivers/bus/fsl-mc/fsl-mc-private.h       |  2 ++
 4 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/stable/sysfs-bus-fsl-mc b/Documentation/ABI/stable/sysfs-bus-fsl-mc
index 9f3a95e..e5c7f7f 100644
--- a/Documentation/ABI/stable/sysfs-bus-fsl-mc
+++ b/Documentation/ABI/stable/sysfs-bus-fsl-mc
@@ -7,3 +7,13 @@ Description:	Writing a non-zero value to this attribute will
 		synchronize the objects under dprc.X and the
 		Management Complex firmware.
 Users:		Userspace drivers and management tools
+
+What:		/sys/bus/fsl-mc/rescan
+Date:		November 2018
+KernelVersion:	5.0
+Contact:	Ioana Ciornei <ioana.ciornei@nxp.com>
+Description:	Writing a non-zero value to this attribute will
+		force a rescan of fsl-mc bus in the system and
+		synchronize the objects under fsl-mc bus and the
+		Management Complex firmware.
+Users:		Userspace drivers and management tools
diff --git a/drivers/bus/fsl-mc/dprc-driver.c b/drivers/bus/fsl-mc/dprc-driver.c
index b3cde63..0d5f4c0 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 f0404c6..ea8991d 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -154,12 +154,53 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
 
 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 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_WO(rescan);
+
+static struct attribute *fsl_mc_bus_attrs[] = {
+	&bus_attr_rescan.attr,
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(fsl_mc_bus);
+
 struct bus_type fsl_mc_bus_type = {
 	.name = "fsl-mc",
 	.match = fsl_mc_bus_match,
 	.uevent = fsl_mc_bus_uevent,
 	.dma_configure  = fsl_mc_dma_configure,
 	.dev_groups = fsl_mc_dev_groups,
+	.bus_groups = fsl_mc_bus_groups,
 };
 EXPORT_SYMBOL_GPL(fsl_mc_bus_type);
 
diff --git a/drivers/bus/fsl-mc/fsl-mc-private.h b/drivers/bus/fsl-mc/fsl-mc-private.h
index 16902f9..ab22347 100644
--- a/drivers/bus/fsl-mc/fsl-mc-private.h
+++ b/drivers/bus/fsl-mc/fsl-mc-private.h
@@ -545,6 +545,8 @@ 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 v2 2/4] bus: fsl-mc: add fsl-mc userspace support

[gregkh]

On Fri, Nov 16, 2018 at 05:55:41PM +0000, Ioana Ciornei wrote:
> Adding userspace support for the MC (Management Complex) means exporting
> an ioctl capable device file representing the root resource container.
> 
> This new functionality in the fsl-mc bus driver intends to provide
> userspace applications an interface to interact with the MC firmware.
> 
> Commands that are composed in userspace are sent to the MC firmware
> through the FSL_MC_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>

Overall, this looks almost sane :)

But I do have some minor comments below on the code after only a quick
review.


> ---
> Changes in v2:
>    - use root dprc MC portal by default
>    - create the uapi misc device from the root dprc probe function
> 
>  Documentation/ioctl/ioctl-number.txt |   1 +
>  drivers/bus/fsl-mc/Kconfig           |   7 ++
>  drivers/bus/fsl-mc/Makefile          |   3 +
>  drivers/bus/fsl-mc/dprc-driver.c     |  14 ++-
>  drivers/bus/fsl-mc/fsl-mc-private.h  |  41 ++++++++
>  drivers/bus/fsl-mc/fsl-mc-uapi.c     | 179 +++++++++++++++++++++++++++++++++++
>  include/uapi/linux/fsl_mc.h          |   9 ++
>  7 files changed, 253 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/bus/fsl-mc/fsl-mc-uapi.c
> 
> diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
> index af6f6ba..eaae7bf 100644
> --- a/Documentation/ioctl/ioctl-number.txt
> +++ b/Documentation/ioctl/ioctl-number.txt
> @@ -171,6 +171,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	uapi/linux/fsl_mc.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/drivers/bus/fsl-mc/Kconfig b/drivers/bus/fsl-mc/Kconfig
> index c23c77c..cde6f40 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_UAPI_SUPPORT
> +	bool "Management Complex (MC) userspace support"
> +	depends on FSL_MC_BUS
> +	help
> +	  Provides userspace support for creating/destroying/configuring
> +	  DPAA2 objects in the Management Complex.
> diff --git a/drivers/bus/fsl-mc/Makefile b/drivers/bus/fsl-mc/Makefile
> index 3c518c7..4ae292a 100644
> --- a/drivers/bus/fsl-mc/Makefile
> +++ b/drivers/bus/fsl-mc/Makefile
> @@ -16,3 +16,6 @@ mc-bus-driver-objs := fsl-mc-bus.o \
>  		      fsl-mc-allocator.o \
>  		      fsl-mc-msi.o \
>  		      dpmcp.o
> +
> +# MC userspace support
> +obj-$(CONFIG_FSL_MC_UAPI_SUPPORT) += fsl-mc-uapi.o
> diff --git a/drivers/bus/fsl-mc/dprc-driver.c b/drivers/bus/fsl-mc/dprc-driver.c
> index 52c7e15..3919d9b 100644
> --- a/drivers/bus/fsl-mc/dprc-driver.c
> +++ b/drivers/bus/fsl-mc/dprc-driver.c
> @@ -593,6 +593,7 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
>  	struct fsl_mc_bus *mc_bus = to_fsl_mc_bus(mc_dev);
>  	bool mc_io_created = false;
>  	bool msi_domain_set = false;
> +	bool uapi_created = false;

I dont understand this "created" need/use.  Why is that needed?

>  	u16 major_ver, minor_ver;
>  
>  	if (!is_fsl_mc_bus_dprc(mc_dev))
> @@ -647,6 +648,11 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
>  		} else {
>  			dev_set_msi_domain(&mc_dev->dev, mc_msi_domain);
>  			msi_domain_set = true;
> +
> +			error = fsl_mc_uapi_create_device_file(mc_bus);
> +			if (error < 0)
> +				goto error_cleanup_msi_domain;
> +			uapi_created = true;

Why are you setting this, can't you always just "know" if the misc file
was created so when you clean up from errors you can remove it?  Or is
the logic here just too complex?


>  		}
>  	}
>  
> @@ -654,7 +660,7 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
>  			  &mc_dev->mc_handle);
>  	if (error < 0) {
>  		dev_err(&mc_dev->dev, "dprc_open() failed: %d\n", error);
> -		goto error_cleanup_msi_domain;
> +		goto error_cleanup_uapi;
>  	}
>  
>  	error = dprc_get_attributes(mc_dev->mc_io, 0, mc_dev->mc_handle,
> @@ -706,6 +712,10 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
>  error_cleanup_open:
>  	(void)dprc_close(mc_dev->mc_io, 0, mc_dev->mc_handle);
>  
> +error_cleanup_uapi:
> +	if (uapi_created)
> +		fsl_mc_uapi_remove_device_file(mc_bus);
> +
>  error_cleanup_msi_domain:
>  	if (msi_domain_set)
>  		dev_set_msi_domain(&mc_dev->dev, NULL);
> @@ -774,6 +784,8 @@ static int dprc_remove(struct fsl_mc_device *mc_dev)
>  	if (!fsl_mc_is_root_dprc(&mc_dev->dev)) {
>  		fsl_destroy_mc_io(mc_dev->mc_io);
>  		mc_dev->mc_io = NULL;
> +	} else {
> +		fsl_mc_uapi_remove_device_file(mc_bus);

You aren't checkig that bool flag here, which kind of implies that you
didn't need it above as well.

>  	}
>  
>  	dev_info(&mc_dev->dev, "DPRC device unbound from driver");
> diff --git a/drivers/bus/fsl-mc/fsl-mc-private.h b/drivers/bus/fsl-mc/fsl-mc-private.h
> index ea11b4f..16902f9 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/ioctl.h>
> +#include <linux/miscdevice.h>
>  
>  /*
>   * Data Path Management Complex (DPMNG) General API
> @@ -492,6 +494,24 @@ struct fsl_mc_resource_pool {
>  };
>  
>  /**
> + * struct fsl_mc_uapi - information associated with a device file
> + * @misc: struct miscdevice linked to the root dprc
> + * @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
> + * @static_mc_io: pointer to the static MC I/O object
> + */
> +struct fsl_mc_uapi {
> +	struct miscdevice misc;
> +	struct device *device;
> +	struct mutex mutex; /* serialize open/release operations */
> +	u32 local_instance_in_use;

Why do you care/need to know if it is in use or not?
Why does it matter, that's a userspace decision to make, not the
kernel's.


> +	u32 dynamic_instance_count;
> +	struct fsl_mc_io *static_mc_io;
> +};
> +
> +/**
>   * 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)
> @@ -500,6 +520,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
> + * @uapi_misc: struct that abstracts the interaction with userspace
>   */
>  struct fsl_mc_bus {
>  	struct fsl_mc_device mc_dev;
> @@ -507,6 +528,7 @@ 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_uapi uapi_misc;
>  };
>  
>  #define to_fsl_mc_bus(_mc_dev) \
> @@ -561,4 +583,23 @@ int __must_check fsl_create_mc_io(struct device *dev,
>  
>  bool fsl_mc_is_root_dprc(struct device *dev);
>  
> +#ifdef CONFIG_FSL_MC_UAPI_SUPPORT
> +
> +int fsl_mc_uapi_create_device_file(struct fsl_mc_bus *mc_bus);
> +
> +void fsl_mc_uapi_remove_device_file(struct fsl_mc_bus *mc_bus);
> +
> +#else
> +
> +static inline int fsl_mc_uapi_create_device_file(struct fsl_mc_bus *mc_bus)
> +{
> +	return 0;
> +}
> +
> +static inline void fsl_mc_uapi_remove_device_file(struct fsl_mc_bus *mc_bus)
> +{
> +}
> +
> +#endif
> +
>  #endif /* _FSL_MC_PRIVATE_H_ */
> diff --git a/drivers/bus/fsl-mc/fsl-mc-uapi.c b/drivers/bus/fsl-mc/fsl-mc-uapi.c
> new file mode 100644
> index 0000000..8c9debb
> --- /dev/null
> +++ b/drivers/bus/fsl-mc/fsl-mc-uapi.c
> @@ -0,0 +1,179 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Management Complex (MC) userspace support
> + *
> + * Copyright 2018 NXP
> + *
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/fs.h>
> +#include <linux/uaccess.h>
> +#include <linux/miscdevice.h>
> +
> +#include "fsl-mc-private.h"
> +
> +struct uapi_priv_data {
> +	struct fsl_mc_uapi *uapi;
> +	struct fsl_mc_io *mc_io;
> +};
> +
> +static int fsl_mc_uapi_send_command(unsigned long arg,
> +				    struct fsl_mc_io *mc_io)
> +{
> +	struct fsl_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);

Is there no need for any type of validation of the command at all?
Previously you were always sending "known good" commands from within the
kernel.  THat is suddenly changed here, how good and robust is your
error handling in this function?


> +	if (error)
> +		return error;
> +
> +	error = copy_to_user((void __user *)arg, &mc_cmd, sizeof(mc_cmd));
> +	if (error)
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static int fsl_mc_uapi_dev_open(struct inode *inode, struct file *filep)
> +{
> +	struct fsl_mc_device *root_mc_device;
> +	struct uapi_priv_data *priv_data;
> +	struct fsl_mc_io *dynamic_mc_io;
> +	struct fsl_mc_uapi *mc_uapi;
> +	struct fsl_mc_bus *mc_bus;
> +	int error;
> +
> +	priv_data = kzalloc(sizeof(*priv_data), GFP_KERNEL);
> +	if (!priv_data)
> +		return -ENOMEM;
> +
> +	mc_uapi = container_of(filep->private_data, struct fsl_mc_uapi, misc);
> +	mc_bus = container_of(mc_uapi, struct fsl_mc_bus, uapi_misc);
> +	root_mc_device = &mc_bus->mc_dev;
> +
> +	mutex_lock(&mc_uapi->mutex);
> +
> +	if (!mc_uapi->local_instance_in_use) {
> +		priv_data->mc_io = mc_uapi->static_mc_io;
> +		mc_uapi->local_instance_in_use = true;

You are setting a u32 to "true"?  The compiler did not complain?

And again, why is this needed, I don't understand the distinction.

> +	} else {
> +		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_uapi->dynamic_instance_count++;

Why care about the "count"?


> +		priv_data->mc_io = dynamic_mc_io;
> +	}
> +	priv_data->uapi = mc_uapi;
> +	filep->private_data = priv_data;
> +
> +	mutex_unlock(&mc_uapi->mutex);
> +
> +	return 0;
> +
> +error_portal_allocate:
> +	mutex_unlock(&mc_uapi->mutex);
> +
> +	return error;
> +}
> +
> +static int fsl_mc_uapi_dev_release(struct inode *inode, struct file *filep)
> +{
> +	struct uapi_priv_data *priv_data;
> +	struct fsl_mc_uapi *mc_uapi;
> +	struct fsl_mc_io *mc_io;
> +
> +	priv_data = filep->private_data;
> +	mc_uapi = priv_data->uapi;
> +	mc_io = priv_data->mc_io;
> +
> +	mutex_lock(&mc_uapi->mutex);
> +
> +	if (WARN_ON(!mc_uapi->local_instance_in_use &&
> +		    mc_uapi->dynamic_instance_count == 0)) {
> +		mutex_unlock(&mc_uapi->mutex);
> +		return -EINVAL;

Never have a WARN_ON() on a path that userspace can generate, otherwise
when you run with panic_on_warn enabled, you just crashed the machine.

If this really is such a horrible error, do a "dev_warn()" and move on.
But again, do not print something if userspace can trigger it all the
time, that's a local DoS.


> +	}
> +
> +	if (mc_io == mc_uapi->static_mc_io) {
> +		mc_uapi->local_instance_in_use = false;
> +	} else {
> +		fsl_mc_portal_free(mc_io);
> +		mc_uapi->dynamic_instance_count--;
> +	}
> +
> +	kfree(filep->private_data);
> +	filep->private_data =  NULL;
> +
> +	mutex_unlock(&mc_uapi->mutex);
> +
> +	return 0;
> +}
> +
> +static long fsl_mc_uapi_dev_ioctl(struct file *file,
> +				  unsigned int cmd,
> +				  unsigned long arg)
> +{
> +	struct uapi_priv_data *priv_data = file->private_data;
> +	int error;
> +
> +	switch (cmd) {
> +	case FSL_MC_SEND_MC_COMMAND:
> +		error = fsl_mc_uapi_send_command(arg, priv_data->mc_io);
> +		break;
> +	default:
> +		pr_err("%s: unexpected ioctl call number\n", __func__);
> +		error = -EINVAL;

dev_err()?  Same for all of your pr_* calls, use the dev_* calls
instead.

And again, this is something that userspace can easily trigger, do not
spam the logs with this type of message.  Make it dev_dbg() instead for
your own debugging use.


> +	}
> +
> +	return error;
> +}
> +
> +static const struct file_operations fsl_mc_uapi_dev_fops = {
> +	.owner = THIS_MODULE,
> +	.open = fsl_mc_uapi_dev_open,
> +	.release = fsl_mc_uapi_dev_release,
> +	.unlocked_ioctl = fsl_mc_uapi_dev_ioctl,
> +};
> +
> +int fsl_mc_uapi_create_device_file(struct fsl_mc_bus *mc_bus)
> +{
> +	struct fsl_mc_device *mc_dev = &mc_bus->mc_dev;
> +	struct fsl_mc_uapi *mc_uapi = &mc_bus->uapi_misc;
> +	int error;
> +
> +	mc_uapi->misc.minor = MISC_DYNAMIC_MINOR;
> +	mc_uapi->misc.name = dev_name(&mc_dev->dev);
> +	mc_uapi->misc.fops = &fsl_mc_uapi_dev_fops;
> +
> +	error = misc_register(&mc_uapi->misc);
> +	if (error)
> +		return -EPROBE_DEFER;

No, return the error given to you, don't eat it and make up a new one.

> +
> +	mc_uapi->static_mc_io = mc_bus->mc_dev.mc_io;
> +
> +	mutex_init(&mc_uapi->mutex);
> +
> +	return 0;
> +}
> +
> +void fsl_mc_uapi_remove_device_file(struct fsl_mc_bus *mc_bus)
> +{
> +	struct fsl_mc_uapi *mc_uapi = &mc_bus->uapi_misc;
> +
> +	if (WARN_ON(mc_uapi->local_instance_in_use))
> +		return;
> +
> +	if (WARN_ON(mc_uapi->dynamic_instance_count != 0))
> +		return;

Same comments as above on the WARN_ON(), don't do it.  make it
dev_dbg() if you really just want to make sure your code is correct.  If
it can happen "in the wild", then you need to refactor the code to
prevent this from ever happening (hint, can it ever happen?  I don't
think so but I might be wrong...)

thanks,

greg k-h

RE: [PATCH v2 2/4] bus: fsl-mc: add fsl-mc userspace support

[Ioana Ciornei]

> > Adding userspace support for the MC (Management Complex) means
> > exporting an ioctl capable device file representing the root resource container.
> >
> > This new functionality in the fsl-mc bus driver intends to provide
> > userspace applications an interface to interact with the MC firmware.
> >
> > Commands that are composed in userspace are sent to the MC firmware
> > through the FSL_MC_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>
> 
> Overall, this looks almost sane :)
> 
> But I do have some minor comments below on the code after only a quick
> review.
> 
> 
> > ---
> > Changes in v2:
> >    - use root dprc MC portal by default
> >    - create the uapi misc device from the root dprc probe function
> >
> >  Documentation/ioctl/ioctl-number.txt |   1 +
> >  drivers/bus/fsl-mc/Kconfig           |   7 ++
> >  drivers/bus/fsl-mc/Makefile          |   3 +
> >  drivers/bus/fsl-mc/dprc-driver.c     |  14 ++-
> >  drivers/bus/fsl-mc/fsl-mc-private.h  |  41 ++++++++
> >  drivers/bus/fsl-mc/fsl-mc-uapi.c     | 179
> +++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/fsl_mc.h          |   9 ++
> >  7 files changed, 253 insertions(+), 1 deletion(-)  create mode 100644
> > drivers/bus/fsl-mc/fsl-mc-uapi.c
> >
> > diff --git a/Documentation/ioctl/ioctl-number.txt
> > b/Documentation/ioctl/ioctl-number.txt
> > index af6f6ba..eaae7bf 100644
> > --- a/Documentation/ioctl/ioctl-number.txt
> > +++ b/Documentation/ioctl/ioctl-number.txt
> > @@ -171,6 +171,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	uapi/linux/fsl_mc.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/drivers/bus/fsl-mc/Kconfig b/drivers/bus/fsl-mc/Kconfig
> > index c23c77c..cde6f40 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_UAPI_SUPPORT
> > +	bool "Management Complex (MC) userspace support"
> > +	depends on FSL_MC_BUS
> > +	help
> > +	  Provides userspace support for creating/destroying/configuring
> > +	  DPAA2 objects in the Management Complex.
> > diff --git a/drivers/bus/fsl-mc/Makefile b/drivers/bus/fsl-mc/Makefile
> > index 3c518c7..4ae292a 100644
> > --- a/drivers/bus/fsl-mc/Makefile
> > +++ b/drivers/bus/fsl-mc/Makefile
> > @@ -16,3 +16,6 @@ mc-bus-driver-objs := fsl-mc-bus.o \
> >  		      fsl-mc-allocator.o \
> >  		      fsl-mc-msi.o \
> >  		      dpmcp.o
> > +
> > +# MC userspace support
> > +obj-$(CONFIG_FSL_MC_UAPI_SUPPORT) += fsl-mc-uapi.o
> > diff --git a/drivers/bus/fsl-mc/dprc-driver.c
> > b/drivers/bus/fsl-mc/dprc-driver.c
> > index 52c7e15..3919d9b 100644
> > --- a/drivers/bus/fsl-mc/dprc-driver.c
> > +++ b/drivers/bus/fsl-mc/dprc-driver.c
> > @@ -593,6 +593,7 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
> >  	struct fsl_mc_bus *mc_bus = to_fsl_mc_bus(mc_dev);
> >  	bool mc_io_created = false;
> >  	bool msi_domain_set = false;
> > +	bool uapi_created = false;
> 
> I dont understand this "created" need/use.  Why is that needed?

The misc device should be created only for the root dprc device probed by this driver. The uapi_created flag is used to keep track if we are trying to probe the root dprc and if we succeeded in creating its misc device. Also, this is used on the cleanup path. If there are any other errors afterwards, we should know if the misc device is to be deregistered or not.

> 
> >  	u16 major_ver, minor_ver;
> >
> >  	if (!is_fsl_mc_bus_dprc(mc_dev))
> > @@ -647,6 +648,11 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
> >  		} else {
> >  			dev_set_msi_domain(&mc_dev->dev,
> mc_msi_domain);
> >  			msi_domain_set = true;
> > +
> > +			error = fsl_mc_uapi_create_device_file(mc_bus);
> > +			if (error < 0)
> > +				goto error_cleanup_msi_domain;
> > +			uapi_created = true;
> 
> Why are you setting this, can't you always just "know" if the misc file was
> created so when you clean up from errors you can remove it?  Or is the logic
> here just too complex?
> 

We end up here if the dprc-driver is probing a root device. If this is the case, the misc_register called from fsl_mc_uapi_create_device_file could still fail if the misc driver is not yet probed. If this is the case, we just want to delay the probe.

This being said, I can just check for the root dprc and cleanup its misc device on the error path (in the error_cleanup_uapi label) without the need of the created flag.

> 
> >  		}
> >  	}
> >
> > @@ -654,7 +660,7 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
> >  			  &mc_dev->mc_handle);
> >  	if (error < 0) {
> >  		dev_err(&mc_dev->dev, "dprc_open() failed: %d\n", error);
> > -		goto error_cleanup_msi_domain;
> > +		goto error_cleanup_uapi;
> >  	}
> >
> >  	error = dprc_get_attributes(mc_dev->mc_io, 0, mc_dev->mc_handle,
> @@
> > -706,6 +712,10 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
> >  error_cleanup_open:
> >  	(void)dprc_close(mc_dev->mc_io, 0, mc_dev->mc_handle);
> >
> > +error_cleanup_uapi:
> > +	if (uapi_created)
> > +		fsl_mc_uapi_remove_device_file(mc_bus);
> > +
> >  error_cleanup_msi_domain:
> >  	if (msi_domain_set)
> >  		dev_set_msi_domain(&mc_dev->dev, NULL); @@ -774,6
> +784,8 @@ static
> > int dprc_remove(struct fsl_mc_device *mc_dev)
> >  	if (!fsl_mc_is_root_dprc(&mc_dev->dev)) {
> >  		fsl_destroy_mc_io(mc_dev->mc_io);
> >  		mc_dev->mc_io = NULL;
> > +	} else {
> > +		fsl_mc_uapi_remove_device_file(mc_bus);
> 
> You aren't checkig that bool flag here, which kind of implies that you didn't need
> it above as well.

In the _probe function, the uapi_created flag has somewhat the same meaning as the fsl_mc_is_root_dprc() here. I will change the probe function to use it also.

> 
> >  	}
> >
> >  	dev_info(&mc_dev->dev, "DPRC device unbound from driver"); diff
> > --git a/drivers/bus/fsl-mc/fsl-mc-private.h
> > b/drivers/bus/fsl-mc/fsl-mc-private.h
> > index ea11b4f..16902f9 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/ioctl.h>
> > +#include <linux/miscdevice.h>
> >
> >  /*
> >   * Data Path Management Complex (DPMNG) General API @@ -492,6 +494,24
> > @@ struct fsl_mc_resource_pool {  };
> >
> >  /**
> > + * struct fsl_mc_uapi - information associated with a device file
> > + * @misc: struct miscdevice linked to the root dprc
> > + * @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
> > + * @static_mc_io: pointer to the static MC I/O object  */ struct
> > +fsl_mc_uapi {
> > +	struct miscdevice misc;
> > +	struct device *device;
> > +	struct mutex mutex; /* serialize open/release operations */
> > +	u32 local_instance_in_use;
> 
> Why do you care/need to know if it is in use or not?
> Why does it matter, that's a userspace decision to make, not the kernel's.

If the userspace is trying to make concurrent calls to the MC, those should be made using different MC portals.
A default/local portal is used by default so we keep track if this is in use or not so that we can make the next decision, whether we need to dynamically allocate another one when we encounter concurrent calls. 
I do not see how this is a userspace decision to make.

> 
> 
> > +	u32 dynamic_instance_count;
> > +	struct fsl_mc_io *static_mc_io;
> > +};
> > +
> > +/**
> >   * 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) @@ -500,6 +520,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
> > + * @uapi_misc: struct that abstracts the interaction with userspace
> >   */
> >  struct fsl_mc_bus {
> >  	struct fsl_mc_device mc_dev;
> > @@ -507,6 +528,7 @@ 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_uapi uapi_misc;
> >  };
> >
> >  #define to_fsl_mc_bus(_mc_dev) \
> > @@ -561,4 +583,23 @@ int __must_check fsl_create_mc_io(struct device
> > *dev,
> >
> >  bool fsl_mc_is_root_dprc(struct device *dev);
> >
> > +#ifdef CONFIG_FSL_MC_UAPI_SUPPORT
> > +
> > +int fsl_mc_uapi_create_device_file(struct fsl_mc_bus *mc_bus);
> > +
> > +void fsl_mc_uapi_remove_device_file(struct fsl_mc_bus *mc_bus);
> > +
> > +#else
> > +
> > +static inline int fsl_mc_uapi_create_device_file(struct fsl_mc_bus
> > +*mc_bus) {
> > +	return 0;
> > +}
> > +
> > +static inline void fsl_mc_uapi_remove_device_file(struct fsl_mc_bus
> > +*mc_bus) { }
> > +
> > +#endif
> > +
> >  #endif /* _FSL_MC_PRIVATE_H_ */
> > diff --git a/drivers/bus/fsl-mc/fsl-mc-uapi.c
> > b/drivers/bus/fsl-mc/fsl-mc-uapi.c
> > new file mode 100644
> > index 0000000..8c9debb
> > --- /dev/null
> > +++ b/drivers/bus/fsl-mc/fsl-mc-uapi.c
> > @@ -0,0 +1,179 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Management Complex (MC) userspace support
> > + *
> > + * Copyright 2018 NXP
> > + *
> > + */
> > +
> > +#include <linux/slab.h>
> > +#include <linux/fs.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/miscdevice.h>
> > +
> > +#include "fsl-mc-private.h"
> > +
> > +struct uapi_priv_data {
> > +	struct fsl_mc_uapi *uapi;
> > +	struct fsl_mc_io *mc_io;
> > +};
> > +
> > +static int fsl_mc_uapi_send_command(unsigned long arg,
> > +				    struct fsl_mc_io *mc_io)
> > +{
> > +	struct fsl_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);
> 
> Is there no need for any type of validation of the command at all?
> Previously you were always sending "known good" commands from within the
> kernel.  THat is suddenly changed here, how good and robust is your error
> handling in this function?

The only measure taken here is the fact that we only accept data of fsl_mc_command size.
This is because we are relying on the MC firmware, which already has validation logic, to parse the commands and return an error in case the command is malformed or with invalid parameters
Also, doubling the command validation code inside the kernel would create unnecessary complexity.

> 
> 
> > +	if (error)
> > +		return error;
> > +
> > +	error = copy_to_user((void __user *)arg, &mc_cmd, sizeof(mc_cmd));
> > +	if (error)
> > +		return -EFAULT;
> > +
> > +	return 0;
> > +}
> > +
> > +static int fsl_mc_uapi_dev_open(struct inode *inode, struct file
> > +*filep) {
> > +	struct fsl_mc_device *root_mc_device;
> > +	struct uapi_priv_data *priv_data;
> > +	struct fsl_mc_io *dynamic_mc_io;
> > +	struct fsl_mc_uapi *mc_uapi;
> > +	struct fsl_mc_bus *mc_bus;
> > +	int error;
> > +
> > +	priv_data = kzalloc(sizeof(*priv_data), GFP_KERNEL);
> > +	if (!priv_data)
> > +		return -ENOMEM;
> > +
> > +	mc_uapi = container_of(filep->private_data, struct fsl_mc_uapi, misc);
> > +	mc_bus = container_of(mc_uapi, struct fsl_mc_bus, uapi_misc);
> > +	root_mc_device = &mc_bus->mc_dev;
> > +
> > +	mutex_lock(&mc_uapi->mutex);
> > +
> > +	if (!mc_uapi->local_instance_in_use) {
> > +		priv_data->mc_io = mc_uapi->static_mc_io;
> > +		mc_uapi->local_instance_in_use = true;
> 
> You are setting a u32 to "true"?  The compiler did not complain?
> 

Sorry for this, will fix. The compiler did not complain.. maybe I should upgrade.

> And again, why is this needed, I don't understand the distinction.

I tried to answer this question above. We should know if the default MC portal is in use so that we can allocate a new one if this the case.

> 
> > +	} else {
> > +		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_uapi->dynamic_instance_count++;
> 
> Why care about the "count"?

We can certainly live without it. It was mainly used as a debug counter to check of everything is freed properly.

> 
> 
> > +		priv_data->mc_io = dynamic_mc_io;
> > +	}
> > +	priv_data->uapi = mc_uapi;
> > +	filep->private_data = priv_data;
> > +
> > +	mutex_unlock(&mc_uapi->mutex);
> > +
> > +	return 0;
> > +
> > +error_portal_allocate:
> > +	mutex_unlock(&mc_uapi->mutex);
> > +
> > +	return error;
> > +}
> > +
> > +static int fsl_mc_uapi_dev_release(struct inode *inode, struct file
> > +*filep) {
> > +	struct uapi_priv_data *priv_data;
> > +	struct fsl_mc_uapi *mc_uapi;
> > +	struct fsl_mc_io *mc_io;
> > +
> > +	priv_data = filep->private_data;
> > +	mc_uapi = priv_data->uapi;
> > +	mc_io = priv_data->mc_io;
> > +
> > +	mutex_lock(&mc_uapi->mutex);
> > +
> > +	if (WARN_ON(!mc_uapi->local_instance_in_use &&
> > +		    mc_uapi->dynamic_instance_count == 0)) {
> > +		mutex_unlock(&mc_uapi->mutex);
> > +		return -EINVAL;
> 
> Never have a WARN_ON() on a path that userspace can generate, otherwise
> when you run with panic_on_warn enabled, you just crashed the machine.
> 
> If this really is such a horrible error, do a "dev_warn()" and move on.
> But again, do not print something if userspace can trigger it all the time, that's a
> local DoS.

The entire WARN_ON can be removed. This could never happen.

> 
> 
> > +	}
> > +
> > +	if (mc_io == mc_uapi->static_mc_io) {
> > +		mc_uapi->local_instance_in_use = false;
> > +	} else {
> > +		fsl_mc_portal_free(mc_io);
> > +		mc_uapi->dynamic_instance_count--;
> > +	}
> > +
> > +	kfree(filep->private_data);
> > +	filep->private_data =  NULL;
> > +
> > +	mutex_unlock(&mc_uapi->mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +static long fsl_mc_uapi_dev_ioctl(struct file *file,
> > +				  unsigned int cmd,
> > +				  unsigned long arg)
> > +{
> > +	struct uapi_priv_data *priv_data = file->private_data;
> > +	int error;
> > +
> > +	switch (cmd) {
> > +	case FSL_MC_SEND_MC_COMMAND:
> > +		error = fsl_mc_uapi_send_command(arg, priv_data->mc_io);
> > +		break;
> > +	default:
> > +		pr_err("%s: unexpected ioctl call number\n", __func__);
> > +		error = -EINVAL;
> 
> dev_err()?  Same for all of your pr_* calls, use the dev_* calls instead.
> 
> And again, this is something that userspace can easily trigger, do not spam the
> logs with this type of message.  Make it dev_dbg() instead for your own
> debugging use.
> 

Will do.

> 
> > +	}
> > +
> > +	return error;
> > +}
> > +
> > +static const struct file_operations fsl_mc_uapi_dev_fops = {
> > +	.owner = THIS_MODULE,
> > +	.open = fsl_mc_uapi_dev_open,
> > +	.release = fsl_mc_uapi_dev_release,
> > +	.unlocked_ioctl = fsl_mc_uapi_dev_ioctl, };
> > +
> > +int fsl_mc_uapi_create_device_file(struct fsl_mc_bus *mc_bus) {
> > +	struct fsl_mc_device *mc_dev = &mc_bus->mc_dev;
> > +	struct fsl_mc_uapi *mc_uapi = &mc_bus->uapi_misc;
> > +	int error;
> > +
> > +	mc_uapi->misc.minor = MISC_DYNAMIC_MINOR;
> > +	mc_uapi->misc.name = dev_name(&mc_dev->dev);
> > +	mc_uapi->misc.fops = &fsl_mc_uapi_dev_fops;
> > +
> > +	error = misc_register(&mc_uapi->misc);
> > +	if (error)
> > +		return -EPROBE_DEFER;
> 
> No, return the error given to you, don't eat it and make up a new one.

Ok. So the caller should make the decision to defer or not?

> 
> > +
> > +	mc_uapi->static_mc_io = mc_bus->mc_dev.mc_io;
> > +
> > +	mutex_init(&mc_uapi->mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +void fsl_mc_uapi_remove_device_file(struct fsl_mc_bus *mc_bus) {
> > +	struct fsl_mc_uapi *mc_uapi = &mc_bus->uapi_misc;
> > +
> > +	if (WARN_ON(mc_uapi->local_instance_in_use))
> > +		return;
> > +
> > +	if (WARN_ON(mc_uapi->dynamic_instance_count != 0))
> > +		return;
> 
> Same comments as above on the WARN_ON(), don't do it.  make it
> dev_dbg() if you really just want to make sure your code is correct.  If it can
> happen "in the wild", then you need to refactor the code to prevent this from
> ever happening (hint, can it ever happen?  I don't think so but I might be
> wrong...)

Yep, these are more paranoid checks and cannot ever happen. I will just remove them. 

Will come back with a v3 to include the needed changes.

Thanks,
Ioana C

> 
> thanks,
> 
> greg k-h