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

[PATCH v3 0/4] 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.

Changes in v2:
 - changed mc_command in fsl_mc_command structure
 - updated fsl_mc_command to use the proper types and moved it in uapi
 - added proper sysfs documentation

Changes in v3:
 - added the correct license for UAPI files
 - removed patches 1 and 3 since they were applied

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

 Documentation/ABI/stable/sysfs-bus-fsl-mc   |  13 ++
 Documentation/ioctl/ioctl-number.txt        |   1 +
 Documentation/networking/dpaa2/overview.rst |   4 +
 MAINTAINERS                                 |   2 +
 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-bus.c             |  95 ++++++++++++
 drivers/bus/fsl-mc/fsl-mc-private.h         |  51 +++++++
 drivers/bus/fsl-mc/fsl-mc-restool.c         | 219 ++++++++++++++++++++++++++++
 include/linux/fsl/mc.h                      |   8 +-
 include/uapi/linux/fsl_mc.h                 |  31 ++++
 12 files changed, 429 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/ABI/stable/sysfs-bus-fsl-mc
 create mode 100644 drivers/bus/fsl-mc/fsl-mc-restool.c
 create mode 100644 include/uapi/linux/fsl_mc.h

-- 
1.9.1

[PATCH v3 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. Also change the variable types used with
the proper ones.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v2:
  - added the patch itself
Changes in v3:
  - added the correct license for UAPI files

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 61fd418..861cf1d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11485,6 +11485,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 f27cb14..19a352b 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 {
-	u64 header;
-	u64 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..54590a2
--- /dev/null
+++ b/include/uapi/linux/fsl_mc.h
@@ -0,0 +1,23 @@
+/* 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_
+
+#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 {
+	__u64 header;
+	__u64 params[MC_CMD_NUM_OF_PARAMS];
+};
+
+#endif /* _UAPI_FSL_MC_H_ */
-- 
1.9.1

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 888 bytes --]

Hi Ioana,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]
[cannot apply to linus/master v4.16-rc6 next-20180323]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ioana-Ciornei/bus-fsl-mc-enhance-Management-Complex-userspace-support/20180325-034313
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

>> ./usr/include/linux/fsl_mc.h:19: found __[us]{8,16,32,64} type without #include <linux/types.h>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6733 bytes --]

[PATCH v3 2/4] 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>
---
Changes in v2:
  - split the bus/driver changes in a separate patch
  - moved the ioctl in the uapi header file
Changes in v3:
  - no change

 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-bus.c             |  19 +++
 drivers/bus/fsl-mc/fsl-mc-private.h         |  48 ++++++
 drivers/bus/fsl-mc/fsl-mc-restool.c         | 219 ++++++++++++++++++++++++++++
 include/uapi/linux/fsl_mc.h                 |   8 +
 8 files changed, 309 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..1a2d132 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	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/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 3c518c7..2017bdb 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 restool kernel support
+obj-$(CONFIG_FSL_MC_RESTOOL) += fsl-mc-restool.o
diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index 5d8266c..0ade415 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 ea11b4f..00cca7d 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
@@ -492,6 +494,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)
@@ -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
+ * @restool_misc: struct that abstracts the interaction with userspace restool
  */
 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_restool restool_misc;
 };
 
 #define to_fsl_mc_bus(_mc_dev) \
@@ -561,4 +583,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..c39b8e8
--- /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 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;
+}
+
+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);
+}
diff --git a/include/uapi/linux/fsl_mc.h b/include/uapi/linux/fsl_mc.h
index 54590a2..e4c8dd0 100644
--- a/include/uapi/linux/fsl_mc.h
+++ b/include/uapi/linux/fsl_mc.h
@@ -14,10 +14,18 @@
  * struct fsl_mc_command - Management Complex (MC) command structure
  * @header: MC command header
  * @params: MC command parameters
+ *
+ * Used by RESTOOL_SEND_MC_COMMAND
  */
 struct fsl_mc_command {
 	__u64 header;
 	__u64 params[MC_CMD_NUM_OF_PARAMS];
 };
 
+#define RESTOOL_IOCTL_TYPE	'R'
+#define RESTOOL_IOCTL_SEQ	0xE0
+
+#define RESTOOL_SEND_MC_COMMAND \
+	_IOWR(RESTOOL_IOCTL_TYPE, RESTOOL_IOCTL_SEQ, struct fsl_mc_command)
+
 #endif /* _UAPI_FSL_MC_H_ */
-- 
1.9.1

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Greg KH]

On Fri, Mar 23, 2018 at 10:38:56AM -0500, Ioana Ciornei wrote:
> +#include "fsl-mc-private.h"
> +
> +#define FSL_MC_BUS_MAX_MINORS	1

As you only need/want one character device here, why not just use the
misc device api?  It's much simpler, and handles all of the housekeeping
for you correctly.  It also means I don't have to audit all of your
chardev code to verify it is correct :)

And it will save you lines-of-code, always a good thing.

thanks,

greg k-h

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

[Ioana Ciornei]

> 
> On Fri, Mar 23, 2018 at 10:38:56AM -0500, Ioana Ciornei wrote:
> > +#include "fsl-mc-private.h"
> > +
> > +#define FSL_MC_BUS_MAX_MINORS	1
> 
> As you only need/want one character device here, why not just use the misc
> device api?  It's much simpler, and handles all of the housekeeping for you
> correctly.  It also means I don't have to audit all of your chardev code to verify it
> is correct :)

I have considered the misc device api but the fsl-mc bus, since it is a platform driver, is probing before the misc char driver.

Ioana


> 
> And it will save you lines-of-code, always a good thing.
> 
> thanks,
> 
> greg k-h

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Greg KH]

On Fri, Mar 23, 2018 at 03:56:24PM +0000, Ioana Ciornei wrote:
> 
> > 
> > On Fri, Mar 23, 2018 at 10:38:56AM -0500, Ioana Ciornei wrote:
> > > +#include "fsl-mc-private.h"
> > > +
> > > +#define FSL_MC_BUS_MAX_MINORS	1
> > 
> > As you only need/want one character device here, why not just use the misc
> > device api?  It's much simpler, and handles all of the housekeeping for you
> > correctly.  It also means I don't have to audit all of your chardev code to verify it
> > is correct :)
> 
> I have considered the misc device api but the fsl-mc bus, since it is
> a platform driver, is probing before the misc char driver.

How?  The misc code is just "core code" there's nothing to "probe" here.
Is this an init-call ordering issue somehow?  A platform driver probe
should be after the misc core is initialized.  And if not, you can
always defer your probe.

thanks,

greg k-h

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

[Ioana Ciornei]

> >
> > >
> > > On Fri, Mar 23, 2018 at 10:38:56AM -0500, Ioana Ciornei wrote:
> > > > +#include "fsl-mc-private.h"
> > > > +
> > > > +#define FSL_MC_BUS_MAX_MINORS	1
> > >
> > > As you only need/want one character device here, why not just use
> > > the misc device api?  It's much simpler, and handles all of the
> > > housekeeping for you correctly.  It also means I don't have to audit
> > > all of your chardev code to verify it is correct :)
> >
> > I have considered the misc device api but the fsl-mc bus, since it is
> > a platform driver, is probing before the misc char driver.
> 
> How?  The misc code is just "core code" there's nothing to "probe" here.
> Is this an init-call ordering issue somehow?  A platform driver probe should be
> after the misc core is initialized.  And if not, you can always defer your probe.

Sorry for the previous improper wording. I have tested again using the misc device api and indeed the devices on the fsl-mc bus are probing before the misc core is initialized but I am able to complete the misc registration deferring the probe the first time.

I will update the patch set and send a new version.

Ioana C


> 
> thanks,
> 
> greg k-h

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Arnd Bergmann]

On Fri, Mar 23, 2018 at 11:38 PM, Ioana Ciornei <ioana.ciornei@nxp.com> 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.

Hi Ioana,

So this driver is a direct passthrough to your hardware for passing
fixed-length command/response pairs. Have you considered using
a higher-level interface instead?

Can you list some of the commands that are passed here as
clarification, and explain what the tradeoffs are that have led
to adopting a low-level interface instead of a high-level interface?

The main downside of the direct passthrough obviously is that you
tie your user space to a particular hardware implementation, while
a high-level abstraction could in principle work across a wider range
of hardware revisions or even across multiple vendors implementing
the same concept by different means.

> +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;

> @@ -14,10 +14,18 @@
>   * struct fsl_mc_command - Management Complex (MC) command structure
>   * @header: MC command header
>   * @params: MC command parameters
> + *
> + * Used by RESTOOL_SEND_MC_COMMAND
>   */
>  struct fsl_mc_command {
>         __u64 header;
>         __u64 params[MC_CMD_NUM_OF_PARAMS];
>  };
>
> +#define RESTOOL_IOCTL_TYPE     'R'
> +#define RESTOOL_IOCTL_SEQ      0xE0

I tried to follow the code path into the hardware and am a bit confused
about the semantics of the 'header' field and the data. Both are
accessed passed to the hardware using

       writeq(le64_to_cpu(cmd->header))

which would indicate a fixed byte layout on the user structure and that
it should really be a '__le64' instead of '__u64', or possibly should be
represented as '__u8 header[8]' to clarify that the byte ordering is
supposed to match the order of the byte addresses of the register.

However, the in-kernel usage of that field suggests that we treat it
as a 64-bit cpu-endian number, for which the hardware needs to know
the endianess of the currently running kernel and user space.

Can you have a look at where the mistake is and what the byteorder
for the fsl_mc_command structure is supposed to be? Obviously,
this is one thing that would be simplified by using a high-level
interface, but it's possible to do it like this as long as it's completely
clear how the structure layout is meant to be used in the uapi header.

        Arnd

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

[Ioana Ciornei]

Hi,

> 
> Hi Ioana,
> 
> So this driver is a direct passthrough to your hardware for passing fixed-
> length command/response pairs. Have you considered using a higher-level
> interface instead?
> 
> Can you list some of the commands that are passed here as clarification, and
> explain what the tradeoffs are that have led to adopting a low-level interface
> instead of a high-level interface?
> 
> The main downside of the direct passthrough obviously is that you tie your
> user space to a particular hardware implementation, while a high-level
> abstraction could in principle work across a wider range of hardware revisions
> or even across multiple vendors implementing the same concept by different
> means.

If by "higher-level" you mean an implementation where commands are created by the kernel at userspace's request, then I believe this approach is not really viable because of the sheer number of possible commands that would bloat the driver.

For example, a DPNI (Data Path Network Interface) can be created using a command that has the following structure:

struct dpni_cmd_create {
	uint32_t options;
	uint8_t num_queues;
	uint8_t num_tcs;
	uint8_t mac_filter_entries;
	uint8_t pad1;
	uint8_t vlan_filter_entries;
	uint8_t pad2;
	uint8_t qos_entries;
	uint8_t pad3;
	uint16_t fs_entries;
};

In the above structure, each field has a meaning that the end-user might want to be able to change according to their particular use-case (not much is left at its default value).
The same level of complexity is encountered for all the commands that interact with Data Path objects such as DPBP(buffer pools), DPRC(Resource Container) etc.
You can find more examples of commands in restool's repo: https://github.com/qoriq-open-source/restool/tree/integration/mc_v10

In my opinion, an in-kernel implementation that is equivalent in terms of flexibility will turn into a giant ioctl parser, all while also exposing an userspace API that is not as simple/easy to use.

> > @@ -14,10 +14,18 @@
> >   * struct fsl_mc_command - Management Complex (MC) command
> structure
> >   * @header: MC command header
> >   * @params: MC command parameters
> > + *
> > + * Used by RESTOOL_SEND_MC_COMMAND
> >   */
> >  struct fsl_mc_command {
> >         __u64 header;
> >         __u64 params[MC_CMD_NUM_OF_PARAMS];  };
> >
> > +#define RESTOOL_IOCTL_TYPE     'R'
> > +#define RESTOOL_IOCTL_SEQ      0xE0
> 
> I tried to follow the code path into the hardware and am a bit confused about
> the semantics of the 'header' field and the data. Both are accessed passed to
> the hardware using
> 
>        writeq(le64_to_cpu(cmd->header))
> 
> which would indicate a fixed byte layout on the user structure and that it
> should really be a '__le64' instead of '__u64', or possibly should be
> represented as '__u8 header[8]' to clarify that the byte ordering is supposed
> to match the order of the byte addresses of the register.
> 

Indeed, the struct fsl_mc_command should use '__le64' instead of '__u64', so that the UAPI header file clearly states what endianness should the userspace prepare.
The writeq appeared as a consequence of a previous mail thread https://lkml.org/lkml/2017/7/17/415, because the endianness of the command is handled by the user and the bus should leave the binary blob intact.

> However, the in-kernel usage of that field suggests that we treat it as a 64-bit
> cpu-endian number, for which the hardware needs to know the endianess of
> the currently running kernel and user space.
> 

I might have not understood what you are trying to say, but the in-kernel code treats the fsl_mc_command as little endian by using cpu_to_leXX and leXX_to_cpu in order to setup the command structure.

Ioana

> Can you have a look at where the mistake is and what the byteorder for the
> fsl_mc_command structure is supposed to be? Obviously, this is one thing
> that would be simplified by using a high-level interface, but it's possible to do
> it like this as long as it's completely clear how the structure layout is meant to
> be used in the uapi header.
> 
>         Arnd

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Arnd Bergmann]

On Wed, Mar 28, 2018 at 4:27 PM, Ioana Ciornei <ioana.ciornei@nxp.com> wrote:
> Hi,
>
>>
>> Hi Ioana,
>>
>> So this driver is a direct passthrough to your hardware for passing fixed-
>> length command/response pairs. Have you considered using a higher-level
>> interface instead?
>>
>> Can you list some of the commands that are passed here as clarification, and
>> explain what the tradeoffs are that have led to adopting a low-level interface
>> instead of a high-level interface?
>>
>> The main downside of the direct passthrough obviously is that you tie your
>> user space to a particular hardware implementation, while a high-level
>> abstraction could in principle work across a wider range of hardware revisions
>> or even across multiple vendors implementing the same concept by different
>> means.
>
> If by "higher-level" you mean an implementation where commands are created by the kernel at userspace's request, then I believe this approach is not really viable because of the sheer number of possible commands that would bloat the driver.
>
> For example, a DPNI (Data Path Network Interface) can be created using a command that has the following structure:
>
> struct dpni_cmd_create {
>         uint32_t options;
>         uint8_t num_queues;
>         uint8_t num_tcs;
>         uint8_t mac_filter_entries;
>         uint8_t pad1;
>         uint8_t vlan_filter_entries;
>         uint8_t pad2;
>         uint8_t qos_entries;
>         uint8_t pad3;
>         uint16_t fs_entries;
> };
>
> In the above structure, each field has a meaning that the end-user might want to be able to change according to their particular use-case (not much is left at its default value).
> The same level of complexity is encountered for all the commands that interact with Data Path objects such as DPBP(buffer pools), DPRC(Resource Container) etc.
> You can find more examples of commands in restool's repo: https://github.com/qoriq-open-source/restool/tree/integration/mc_v10
>
> In my opinion, an in-kernel implementation that is equivalent in terms of flexibility will turn
> into a giant ioctl parser, all while also exposing an userspace API that is not as simple/easy to use.

(adding the netdev list)

The command you list there seems to be networking related, so instead of
an ioctl based interface, a high-lever interface would likely use netlink
for consistency with other drivers. Are all commands for networking
or are there some that are talking to the device to do something unrelated?

Obviously creating a high-level interface would be a lot of work in the kernel,
and it only pays off if there are multiple independent users, we wouldn't do
that for just one driver.

I'm still not convinced either way (high-level or low-level
interface), but I think
this needs to be discussed with the networking maintainers. Given the examples
on the github page you linked to, the high-level user space commands
based on these ioctls

   ls-addni   # adds a network interface
   ls-addmux  # adds a dpdmux
   ls-addsw   # adds an l2switch
   ls-listmac # lists MACs and their connections
   ls-listni  # lists network interfaces and their connections

and I see that you also support the switchdev interface in
drivers/staging/fsl-dpaa2, which I think does some of the same
things, presumably by implementing the switchdev API using
fsl_mc_command low-level interfaces in the kernel.

Is that a correct interpretation? If yes, could we extend switchdev
or other networking interfaces to fill in whatever those don't handle
yet?

>> > @@ -14,10 +14,18 @@
>> >   * struct fsl_mc_command - Management Complex (MC) command
>> structure
>> >   * @header: MC command header
>> >   * @params: MC command parameters
>> > + *
>> > + * Used by RESTOOL_SEND_MC_COMMAND
>> >   */
>> >  struct fsl_mc_command {
>> >         __u64 header;
>> >         __u64 params[MC_CMD_NUM_OF_PARAMS];  };
>> >
>> > +#define RESTOOL_IOCTL_TYPE     'R'
>> > +#define RESTOOL_IOCTL_SEQ      0xE0
>>
>> I tried to follow the code path into the hardware and am a bit confused about
>> the semantics of the 'header' field and the data. Both are accessed passed to
>> the hardware using
>>
>>        writeq(le64_to_cpu(cmd->header))
>>
>> which would indicate a fixed byte layout on the user structure and that it
>> should really be a '__le64' instead of '__u64', or possibly should be
>> represented as '__u8 header[8]' to clarify that the byte ordering is supposed
>> to match the order of the byte addresses of the register.
>>
>
> Indeed, the struct fsl_mc_command should use '__le64' instead of '__u64', so that the UAPI header file clearly states what endianness should the userspace prepare.
> The writeq appeared as a consequence of a previous mail thread https://lkml.org/lkml/2017/7/17/415, because the endianness of the command is handled by the user and the bus should leave the binary blob intact.

Ok. However, with the dpni_cmd_create structure you list above, it
seems that it doesn't actually contain a set of __le64 but rather
an array of bytes, so it might be best to

>> However, the in-kernel usage of that field suggests that we treat it as a 64-bit
>> cpu-endian number, for which the hardware needs to know the endianess of
>> the currently running kernel and user space.
>>
>
> I might have not understood what you are trying to say, but the in-kernel code treats the fsl_mc_command as little endian by using cpu_to_leXX and leXX_to_cpu in order to setup the command structure.

One function that seems to have a problem is this one:

static enum mc_cmd_status mc_cmd_hdr_read_status(struct fsl_mc_command *cmd)
{
        struct mc_cmd_header *hdr = (struct mc_cmd_header *)&cmd->header;

        return (enum mc_cmd_status)hdr->status;
}

If cmd->header is little-endian here, then the result is not
an 'enum mc_cmd_status'. I think it would be good to change the
types for fsl_mc_command to __le64 first and run everything through
'sparse' with 'make C=1' to find any remaining endianess problems.

        Arnd

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Andrew Lunn]

> I'm still not convinced either way (high-level or low-level
> interface), but I think
> this needs to be discussed with the networking maintainers. Given the examples
> on the github page you linked to, the high-level user space commands
> based on these ioctls
> 
>    ls-addni   # adds a network interface
>    ls-addmux  # adds a dpdmux
>    ls-addsw   # adds an l2switch
>    ls-listmac # lists MACs and their connections
>    ls-listni  # lists network interfaces and their connections
> 
> and I see that you also support the switchdev interface in
> drivers/staging/fsl-dpaa2, which I think does some of the same
> things, presumably by implementing the switchdev API using
> fsl_mc_command low-level interfaces in the kernel.

Hi Arnd

I agree that switchdev and devlink should be the correct way to handle
this. The low level plumbing of the hardware should all be
hidden. There should not be any user space commands needed other than
the usual network configuration tools and devlink.

      Andrew

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

[Ioana Ciornei]

> 
> > I'm still not convinced either way (high-level or low-level
> > interface), but I think this needs to be discussed with the networking
> > maintainers. Given the examples on the github page you linked to, the
> > high-level user space commands based on these ioctls
> >
> >    ls-addni   # adds a network interface
> >    ls-addmux  # adds a dpdmux
> >    ls-addsw   # adds an l2switch
> >    ls-listmac # lists MACs and their connections
> >    ls-listni  # lists network interfaces and their connections
> >
> > and I see that you also support the switchdev interface in
> > drivers/staging/fsl-dpaa2, which I think does some of the same things,
> > presumably by implementing the switchdev API using fsl_mc_command
> > low-level interfaces in the kernel.
> 
> Hi Arnd
> 
> I agree that switchdev and devlink should be the correct way to handle this. The
> low level plumbing of the hardware should all be hidden. There should not be
> any user space commands needed other than the usual network configuration
> tools and devlink.
> 

Hi,

The commands listed above are for creating/destroying DPAA2 objects in Management Complex and not for runtime configuration where standard userspace tools are used.
Restool is responsible for creating objects in Management complex and this process can be seen as the equivalent of hotplugging a peripheral rather than configuring it, thus there is no standard userspace tool to handle that.

* The Management Complex is configured to create a specific set of DPAA2 objects dynamically through Restool (by sending create commands) or statically, at boot time, through a configuration file (Data Path Layout file)
* The objects are then probed and configured by the corresponding drivers
* The objects are controlled at runtime by the user via standard tools (e.g. ethtool for network interfaces).
  
I hope this gives a better understanding on the DPAA2 hardware and software architecture. The fsl-mc bus documentation gives more details on this: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/Documentation/networking/dpaa2/overview.rst?h=staging-next

Ioana

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Andrew Lunn]

Hi Ioana

> The commands listed above are for creating/destroying DPAA2 objects
> in Management Complex and not for runtime configuration where
> standard userspace tools are used.

Please can you explain why this is not just plumbing inside a
switchdev driver?

The hardware has a number of physical ports. So on probe, i would
expect it to create a DPMAC, DPNI, and DPIO for each port, and a linux
netdev. From then on, standard tools are all that are needed. The
switchdev driver can create a l2 switch object when the user uses the
ip link add name br0 type bridge. It can then connect the switch
object to the DPNI when the user adds an interface to the switch, etc.

       Andrew

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

[Razvan Stefanescu]

Hello Andrew,

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Monday, April 2, 2018 4:45 PM
> To: Ioana Ciornei <ioana.ciornei@nxp.com>
> Cc: Arnd Bergmann <arnd@arndb.de>; gregkh
> <gregkh@linuxfoundation.org>; Laurentiu Tudor
> <laurentiu.tudor@nxp.com>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; Stuart Yoder <stuyoder@gmail.com>; Ruxandra
> Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com>; Razvan Stefanescu
> <razvan.stefanescu@nxp.com>; Roy Pledge <roy.pledge@nxp.com>;
> Networking <netdev@vger.kernel.org>
> Subject: Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
> 
> Hi Ioana
> 
> > The commands listed above are for creating/destroying DPAA2 objects
> > in Management Complex and not for runtime configuration where
> > standard userspace tools are used.
> 
> Please can you explain why this is not just plumbing inside a
> switchdev driver?
> 
> The hardware has a number of physical ports. So on probe, i would
> expect it to create a DPMAC, DPNI, and DPIO for each port, and a linux
> netdev. From then on, standard tools are all that are needed. The
> switchdev driver can create a l2 switch object when the user uses the
> ip link add name br0 type bridge. It can then connect the switch
> object to the DPNI when the user adds an interface to the switch, etc.
> 

I'll chime in as you mentioned switchdev driver. 

DPAA2 offers several object-based abstractions for modeling network
related devices (interfaces, L2 Ethernet switch) or accelerators
(DPSECI - crypto and DPDCEI - compression), the latter not up-streamed yet.
They are modeled using various low-level resources (e.g. queues,
classification tables, physical ports) and have multiple configuration and
interconnectivity options, managed by the Management Complex. 
Resources are limited and they are only used when needed by the objects,
to accommodate more configurations and usage scenarios.
 
Some of the objects have a 1-to-1 correspondence to physical resources
(e.g. DPMACs to physical ports), while others (like DPNIs and DPSW)
can be seen as a collection of the mentioned resources. The types and 
number of such objects are not predetermined.

When the board boots up, none of them exist yet. Restool allows a user to
define the system topology, by providing a way to dynamically create, destroy
and interconnect these objects.

After an object is created, it will be presented on the fsl-mc bus. A driver
is loaded to implement the required kernel interfaces specific to that object
type. Kernel can boot and afterwards the DPAA2 objects are added, as the user
requires.

As you mentioned DPMACs: objects of this type can be connected only to a DPNI
(a network interface like object) or to a DPSW (L2 ethernet switch) port.
Likewise, a DPNI can have only one connection (to a DPMAC, a DPSW port or
another DPNI object).

Here's several examples of valid connection types:
  * DPMAC <----> DPNI (standard network i/f corresponding to a physical port)
  * DPMAC <----> DPSW (physical port in a switch)
  * DPNI <----> DPSW (virtual network interface connected to a switch port)
  * DPNI <----> DPNI

In the latter case, the two DPNIs will not be connected to any physical
port, but can be used as a point-to-point connection between two virtual
machines for instance.

So, it is not possible to connect a DPNI to a DPSW after it was connected
to a DPMAC. The DPNI-DPMAC pair would have to be disconnected and
DPMAC will be reconnected to the switch. DPNI interface that is no longer
connected to a DPMAC will be destroyed and any new addition/deletion of
a DPNI/DPMAC interface to the switch port will trigger the entire switch
re-configuration.

Best regards,
Razvan Stefanescu

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Andrew Lunn]

On Tue, Apr 03, 2018 at 11:12:52AM +0000, Razvan Stefanescu wrote:
> DPAA2 offers several object-based abstractions for modeling network
> related devices (interfaces, L2 Ethernet switch) or accelerators
> (DPSECI - crypto and DPDCEI - compression), the latter not up-streamed yet.
> They are modeled using various low-level resources (e.g. queues,
> classification tables, physical ports) and have multiple configuration and
> interconnectivity options, managed by the Management Complex. 
> Resources are limited and they are only used when needed by the objects,
> to accommodate more configurations and usage scenarios.
>  
> Some of the objects have a 1-to-1 correspondence to physical resources
> (e.g. DPMACs to physical ports), while others (like DPNIs and DPSW)
> can be seen as a collection of the mentioned resources. The types and 
> number of such objects are not predetermined.
> 
> When the board boots up, none of them exist yet. Restool allows a user to
> define the system topology, by providing a way to dynamically create, destroy
> and interconnect these objects.

Hi Razvan

The core concept with Linux networking and offload is that the
hardware is there to accelerate what Linux can already do. Since Linux
can already do it, i don't need any additional tools.

You have new hardware. It might offer features which we currently
don't have offload support for. But all the means is you need to
extend the core networking code which implements the software version
of that feature to offload to the hardware.

The board knows how many physical ports it has. switchdev can then
setup the plumbing to create the objects needed to represent the
ports. Restool is not needed for that.

> In the latter case, the two DPNIs will not be connected to any physical
> port, but can be used as a point-to-point connection between two virtual
> machines for instance.
 
Can Linux already do this? Isn't that what PCI Virtual Functions are
all about? You need to find the current Linux concept for this, and
extend it to offload the functionality to hardware. If Linux can do
it, it already has the tools to configure it. Restool is not needed
for that.

> So, it is not possible to connect a DPNI to a DPSW after it was
> connected to a DPMAC. The DPNI-DPMAC pair would have to be
> disconnected and DPMAC will be reconnected to the switch. DPNI
> interface that is no longer connected to a DPMAC will be destroyed
> and any new addition/deletion of a DPNI/DPMAC interface to the
> switch port will trigger the entire switch re-configuration.

Switches and ports connected to switches are dynamic. They come and
go. You don't expect it to happen very often, but Linux has no
restrictions on this. You need to figure out how best to offload this
to your hardware. Maybe when you create the switch object you make a
guess as to how many ports you need. Leave some of the ports not
connected to anything. You can then add ports to the switch using the
free ports. If you run out of ports, you have no choice but to destroy
the switch object and create a new one. Hopefully that does not take
too long. Restool is not needed for this, it all happens within the
switchdev driver.

	  Andrew

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Stuart Yoder]

On Wed, Mar 28, 2018 at 10:43 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, Mar 28, 2018 at 4:27 PM, Ioana Ciornei <ioana.ciornei@nxp.com> wrote:
>> Hi,
>>
>>>
>>> Hi Ioana,
>>>
>>> So this driver is a direct passthrough to your hardware for passing fixed-
>>> length command/response pairs. Have you considered using a higher-level
>>> interface instead?
>>>
>>> Can you list some of the commands that are passed here as clarification, and
>>> explain what the tradeoffs are that have led to adopting a low-level interface
>>> instead of a high-level interface?
>>>
>>> The main downside of the direct passthrough obviously is that you tie your
>>> user space to a particular hardware implementation, while a high-level
>>> abstraction could in principle work across a wider range of hardware revisions
>>> or even across multiple vendors implementing the same concept by different
>>> means.
>>
>> If by "higher-level" you mean an implementation where commands are created by the kernel at userspace's request, then I believe this approach is not really viable because of the sheer number of possible commands that would bloat the driver.
>>
>> For example, a DPNI (Data Path Network Interface) can be created using a command that has the following structure:
>>
>> struct dpni_cmd_create {
>>         uint32_t options;
>>         uint8_t num_queues;
>>         uint8_t num_tcs;
>>         uint8_t mac_filter_entries;
>>         uint8_t pad1;
>>         uint8_t vlan_filter_entries;
>>         uint8_t pad2;
>>         uint8_t qos_entries;
>>         uint8_t pad3;
>>         uint16_t fs_entries;
>> };
>>
>> In the above structure, each field has a meaning that the end-user might want to be able to change according to their particular use-case (not much is left at its default value).
>> The same level of complexity is encountered for all the commands that interact with Data Path objects such as DPBP(buffer pools), DPRC(Resource Container) etc.
>> You can find more examples of commands in restool's repo: https://github.com/qoriq-open-source/restool/tree/integration/mc_v10
>>
>> In my opinion, an in-kernel implementation that is equivalent in terms of flexibility will turn
>> into a giant ioctl parser, all while also exposing an userspace API that is not as simple/easy to use.
>
> (adding the netdev list)
>
> The command you list there seems to be networking related, so instead of
> an ioctl based interface, a high-lever interface would likely use netlink
> for consistency with other drivers. Are all commands for networking
> or are there some that are talking to the device to do something unrelated?
>
> Obviously creating a high-level interface would be a lot of work in the kernel,
> and it only pays off if there are multiple independent users, we wouldn't do
> that for just one driver.
>
> I'm still not convinced either way (high-level or low-level
> interface), but I think
> this needs to be discussed with the networking maintainers. Given the examples
> on the github page you linked to, the high-level user space commands
> based on these ioctls
>
>    ls-addni   # adds a network interface
>    ls-addmux  # adds a dpdmux
>    ls-addsw   # adds an l2switch
>    ls-listmac # lists MACs and their connections
>    ls-listni  # lists network interfaces and their connections
>
> and I see that you also support the switchdev interface in
> drivers/staging/fsl-dpaa2, which I think does some of the same
> things, presumably by implementing the switchdev API using
> fsl_mc_command low-level interfaces in the kernel.
>
> Is that a correct interpretation? If yes, could we extend switchdev
> or other networking interfaces to fill in whatever those don't handle
> yet?

The wrapper scripts you referenced are not sufficient to show the scope
of what the proposed user space interface is for.  The command list is
not just about networking related objects, as there are quite a few
other types of  objects as well:
    dprc - container object representing an fsl-mc bus instance...i.e. other
           objects are attached to this bus
    dpio - used for queuing operations towards any accelerator or network
           interface
    dpbp - buffer pool object
    dpmcp - command portal interface
    dpdmai - DMA engine
    dpseci - crypto accelerator
    dpdcei - compression/decompression accelerator
    dpni - network interface
    dprtc - real time counter
    dpaiop - heterogenous core complex for packet processing offload
    dpmac - represents an Ethernet MAC
    dpsw - network switch
    dpcon - network concentrator
    dpci - communication interface

The proposed ioctl interface is about:
   A)  creating and destroying all those object types
   B)  managing the dprc containers they live in, including moving
       objects between containers
   C)  where applicable establishing connections between different
       objects

No _operational_ aspects of these object/devices is being handled
through this interface.  The network interface should be managed
through ethtool, for example.  The proposed ioctl interface is about
bringing the devices into existence and getting them "wired" up.

Suppose you want to create and assign a network interface to a KVM
virtual machine, you would do something like the following using
a user space tool like restool:
   -create a new (empty) dprc object
   -create a new dpni and assign it to the dprc
   -create a new dpio and assign it to the dprc
   -create a new dpbp and assign it to the dprc
   -create a new dpmcp and assign it to the dprc
   -create a new dpmac and assign it to the dprc
   -connect the dpni to the dpmac

Now, at this point you have a functional set of objects that
can function as a network interface.

That dprc can now be assigned to a KVM VM using vfio and the
guest will see a dprc that it can probe and enumerate using the
fsl-mc bus infrastructure that is now upstream.

There is no existing kernel <--> user space mechanism that will
work to do all that, so something new was needed.

As far as low-level vs high-level...we did consider a higher level
interface that would expose operations on individual object such
as "create dpbp", but the user space API gets complex and
fragile for no obvious value.  Every object needs commands to
create/destroy and get attributes.  There is a sizeable dprc command
set.  Every time an object is enhanced (with a corresponding major
or minor version rev) you have to change the ioctl interface.

Having a simple command passthrough interface reduces complexity
in the kernel and provides an interface that should be very stable.

The fsl-mc bus and DPAA2 is very NXP-specific, so there doesn't
seem to be anything that can be made generic here to provide
more common benefit.

Thanks,
Stuart

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Andrew Lunn]

> Suppose you want to create and assign a network interface to a KVM
> virtual machine, you would do something like the following using
> a user space tool like restool:
>    -create a new (empty) dprc object
>    -create a new dpni and assign it to the dprc
>    -create a new dpio and assign it to the dprc
>    -create a new dpbp and assign it to the dprc
>    -create a new dpmcp and assign it to the dprc
>    -create a new dpmac and assign it to the dprc
>    -connect the dpni to the dpmac

Hi Stuart

It this connecting to a physical port at the bottom?

If so, i would expect that when you probe the device you just create
all these for each physical port. You then just need to map one of
them into the KVM, in the same way you map one PCI device into a KVM.

If these are virtual devices, VF devices you would normally do

echo 4 > /sys/class/net/<device name>/device/sriov_numvfs

on the physical device to create virtual devices.

> The fsl-mc bus and DPAA2 is very NXP-specific, so there doesn't
> seem to be anything that can be made generic here to provide
> more common benefit.

Which is why you should try to avoid all of this.  The user knows how
to use standard linux commands and concepts. They don't want to have
to learn the inside plumbing of your hardware.

       Andrew

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Stuart Yoder]

On Tue, Apr 3, 2018 at 8:05 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> Suppose you want to create and assign a network interface to a KVM
>> virtual machine, you would do something like the following using
>> a user space tool like restool:
>>    -create a new (empty) dprc object
>>    -create a new dpni and assign it to the dprc
>>    -create a new dpio and assign it to the dprc
>>    -create a new dpbp and assign it to the dprc
>>    -create a new dpmcp and assign it to the dprc
>>    -create a new dpmac and assign it to the dprc
>>    -connect the dpni to the dpmac
>
> Hi Stuart
>
> It this connecting to a physical port at the bottom?

Yes.

> If so, i would expect that when you probe the device you just create
> all these for each physical port.

The problem is that there is not just one set of objects to implement a network
interface.  For the highest throughput packet processing you need one dpio
per core.  So, it will depend on what the requirements are.  You might want
multiple dpbp (buffer pools) and set up pools of different size
buffers for different
packet classifications.

You might want to have other objects like a crypto accelerator (dpseci) in the
container as well.

The dprc is a container holding any combination of those objects.  So you have
complete flexibility.

> You then just need to map one of
> them into the KVM, in the same way you map one PCI device into a KVM.
>
> If these are virtual devices, VF devices you would normally do
>
> echo 4 > /sys/class/net/<device name>/device/sriov_numvfs
>
> on the physical device to create virtual devices.
>
>> The fsl-mc bus and DPAA2 is very NXP-specific, so there doesn't
>> seem to be anything that can be made generic here to provide
>> more common benefit.
>
> Which is why you should try to avoid all of this.  The user knows how
> to use standard linux commands and concepts. They don't want to have
> to learn the inside plumbing of your hardware.

I hear you.  It is more complicated this way...having all these individual
objects vs just a single "bundle" of them that represents a NIC.  But, that's
the way the DPAA2 hardware is, and we're implementing kernel support for
the hardware as it is.

Thanks,
Stuart

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Andrew Lunn]

> I hear you.  It is more complicated this way...having all these individual
> objects vs just a single "bundle" of them that represents a NIC.  But, that's
> the way the DPAA2 hardware is, and we're implementing kernel support for
> the hardware as it is.

Hi Stuart

I see we are not making any progress here.

So what i suggest is you post the kernel code and configuration tool
concept to netdev for a full review. You want reviews from David
Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc.

	Andrew

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Stuart Yoder]

On Wed, Apr 4, 2018 at 7:42 AM, Andrew Lunn <andrew@lunn.ch> wrote:
>> I hear you.  It is more complicated this way...having all these individual
>> objects vs just a single "bundle" of them that represents a NIC.  But, that's
>> the way the DPAA2 hardware is, and we're implementing kernel support for
>> the hardware as it is.
>
> Hi Stuart
>
> I see we are not making any progress here.
>
> So what i suggest is you post the kernel code and configuration tool
> concept to netdev for a full review. You want reviews from David
> Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc.

I know Ioana has other feedback she is addressing so a respin will be coming
soon, and she can include those additional reviewers.

Thanks,
Stuart

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Laurentiu Tudor]

Hello,

My 2c below.

On 04/04/2018 03:42 PM, Andrew Lunn wrote:
>> I hear you.  It is more complicated this way...having all these individual
>> objects vs just a single "bundle" of them that represents a NIC.  But, that's
>> the way the DPAA2 hardware is, and we're implementing kernel support for
>> the hardware as it is.
>
> Hi Stuart
>
> I see we are not making any progress here.
>
> So what i suggest is you post the kernel code and configuration tool
> concept to netdev for a full review. You want reviews from David
> Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc.
>

I think that the discussion steered too much towards networking related 
topics, while this ioctl doesn't have much to do with networking.
It's just an ioctl for our mc-bus bus driver that is used to manage the 
devices on this bus through userspace tools.
In addition, I'd drop any mention of our reference user space app 
(restool) to emphasize that this ioctl is not added just for a 
particular user space app. I think Stuart also mentioned this.

---
Thanks & Best Regards, Laurentiu

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Andrew Lunn]

On Thu, Apr 05, 2018 at 10:30:01AM +0000, Laurentiu Tudor wrote:
> Hello,
> 
> My 2c below.
> 
> On 04/04/2018 03:42 PM, Andrew Lunn wrote:
> >> I hear you.  It is more complicated this way...having all these individual
> >> objects vs just a single "bundle" of them that represents a NIC.  But, that's
> >> the way the DPAA2 hardware is, and we're implementing kernel support for
> >> the hardware as it is.
> >
> > Hi Stuart
> >
> > I see we are not making any progress here.
> >
> > So what i suggest is you post the kernel code and configuration tool
> > concept to netdev for a full review. You want reviews from David
> > Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc.
> >
> 
> I think that the discussion steered too much towards networking related 
> topics, while this ioctl doesn't have much to do with networking.

Hi Laurentiu

So i can use switchdev without it? I can modprobe the switchdev
driver, all the physical interfaces will appear, and i can use ip addr
add etc. I do not need to use a user space tool at all in order to use
the network functionality?

    Andrew

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Laurentiu Tudor]

On 04/05/2018 02:47 PM, Andrew Lunn wrote:
> On Thu, Apr 05, 2018 at 10:30:01AM +0000, Laurentiu Tudor wrote:
>> Hello,
>>
>> My 2c below.
>>
>> On 04/04/2018 03:42 PM, Andrew Lunn wrote:
>>>> I hear you.  It is more complicated this way...having all these individual
>>>> objects vs just a single "bundle" of them that represents a NIC.  But, that's
>>>> the way the DPAA2 hardware is, and we're implementing kernel support for
>>>> the hardware as it is.
>>>
>>> Hi Stuart
>>>
>>> I see we are not making any progress here.
>>>
>>> So what i suggest is you post the kernel code and configuration tool
>>> concept to netdev for a full review. You want reviews from David
>>> Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc.
>>>
>>
>> I think that the discussion steered too much towards networking related
>> topics, while this ioctl doesn't have much to do with networking.
>
> Hi Laurentiu
>
> So i can use switchdev without it? I can modprobe the switchdev
> driver, all the physical interfaces will appear, and i can use ip addr
> add etc. I do not need to use a user space tool at all in order to use
> the network functionality?

Absolutely!
In normal use cases the system designer, depending on the requirements, 
configures the various devices that it desires through a firmware 
configuration (think something like a device tree). The devices 
configured are presented on the mc-bus and probed normally by the 
kernel. The standard networking linux tools can be used as expected.

The ioctl is necessary only for more advanced use cases that are 
supported by this bus. Think "more dynamic" scenarios that involve 
linking & unlinking various devices at runtime, maybe some 
virtualization scenarios. Unfortunately I'm not the architect type of 
guy so I don't have more specific examples to better illustrate ...

---
Best Regards, Laurentiu

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Andrew Lunn]

> > Hi Laurentiu
> >
> > So i can use switchdev without it? I can modprobe the switchdev
> > driver, all the physical interfaces will appear, and i can use ip addr
> > add etc. I do not need to use a user space tool at all in order to use
> > the network functionality?
> 
> Absolutely!

Great.

Then the easiest way forwards is to simply drop the IOCTL code for the
moment. Get the basic support for the hardware into the kernel
first. Then come back later to look at dynamic behaviour which needs
some form of configuration.

> In normal use cases the system designer, depending on the requirements, 
> configures the various devices that it desires through a firmware 
> configuration (think something like a device tree). The devices 
> configured are presented on the mc-bus and probed normally by the 
> kernel. The standard networking linux tools can be used as expected.

So what you should probably do is start a discussion on what this
device tree binding looks like. But you need to be careful even
here. Device tree describes the hardware, not how you configure the
hardware. So maybe DT does not actually fit.

	  Andrew

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Laurentiu Tudor]

Hi Andrew,

On 04/05/2018 03:48 PM, Andrew Lunn wrote:
>>> Hi Laurentiu
>>>
>>> So i can use switchdev without it? I can modprobe the switchdev
>>> driver, all the physical interfaces will appear, and i can use ip addr
>>> add etc. I do not need to use a user space tool at all in order to use
>>> the network functionality?
>>
>> Absolutely!
>
> Great.
>
> Then the easiest way forwards is to simply drop the IOCTL code for the
> moment. Get the basic support for the hardware into the kernel
> first. Then come back later to look at dynamic behaviour which needs
> some form of configuration.

Hmm, not sure I understand. We already have a fully functional ethernet 
driver [1] and a switch driver [2] ...

>> In normal use cases the system designer, depending on the requirements,
>> configures the various devices that it desires through a firmware
>> configuration (think something like a device tree). The devices
>> configured are presented on the mc-bus and probed normally by the
>> kernel. The standard networking linux tools can be used as expected.
>
> So what you should probably do is start a discussion on what this
> device tree binding looks like. But you need to be careful even
> here. Device tree describes the hardware, not how you configure the
> hardware. So maybe DT does not actually fit.

It's not an actual device tree, but a configuration file that happens to 
reuse the DTS format. I guess my analogy with a device tree was not the 
best.
Detailed documentation on the syntax can be found here [3], chapter 22.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/fsl-dpaa2/ethernet
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/fsl-dpaa2/ethsw
[3] https://www.nxp.com/docs/en/user-guide/DPAA2_UM.pdf

---
Best Regards, Laurentiu

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Andrew Lunn]

On Thu, Apr 05, 2018 at 02:43:29PM +0000, Laurentiu Tudor wrote:
> Hi Andrew,
> 
> On 04/05/2018 03:48 PM, Andrew Lunn wrote:
> >>> Hi Laurentiu
> >>>
> >>> So i can use switchdev without it? I can modprobe the switchdev
> >>> driver, all the physical interfaces will appear, and i can use ip addr
> >>> add etc. I do not need to use a user space tool at all in order to use
> >>> the network functionality?
> >>
> >> Absolutely!
> >
> > Great.
> >
> > Then the easiest way forwards is to simply drop the IOCTL code for the
> > moment. Get the basic support for the hardware into the kernel
> > first. Then come back later to look at dynamic behaviour which needs
> > some form of configuration.
> 
> Hmm, not sure I understand. We already have a fully functional ethernet 
> driver [1] and a switch driver [2] ...

In staging, the tree of crap. You want to get it out of there and into
the main tree. But that effort is being side lined by the discussion
around this IOCTL call.  The best way forward is to to accept Greg is
not going to take this patchset at the moment, and move on. As you
said, it is not needed for the Ethernet and switchdev driver.

What needs to happen before the Ethernet driver can be reviewed for
moving out of staging?

       Andrew

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

[Ruxandra Ioana Ciocoi Radulescu]

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Thursday, April 5, 2018 6:24 PM
> To: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> Cc: Stuart Yoder <stuyoder@gmail.com>; Arnd Bergmann <arnd@arndb.de>;
> Ioana Ciornei <ioana.ciornei@nxp.com>; gregkh
> <gregkh@linuxfoundation.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; Ruxandra Ioana Ciocoi Radulescu
> <ruxandra.radulescu@nxp.com>; Razvan Stefanescu
> <razvan.stefanescu@nxp.com>; Roy Pledge <roy.pledge@nxp.com>;
> Networking <netdev@vger.kernel.org>
> Subject: Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
> 
> On Thu, Apr 05, 2018 at 02:43:29PM +0000, Laurentiu Tudor wrote:
> > Hi Andrew,
> >
> > On 04/05/2018 03:48 PM, Andrew Lunn wrote:
> > >>> Hi Laurentiu
> > >>>
> > >>> So i can use switchdev without it? I can modprobe the switchdev
> > >>> driver, all the physical interfaces will appear, and i can use ip addr
> > >>> add etc. I do not need to use a user space tool at all in order to use
> > >>> the network functionality?
> > >>
> > >> Absolutely!
> > >
> > > Great.
> > >
> > > Then the easiest way forwards is to simply drop the IOCTL code for the
> > > moment. Get the basic support for the hardware into the kernel
> > > first. Then come back later to look at dynamic behaviour which needs
> > > some form of configuration.
> >
> > Hmm, not sure I understand. We already have a fully functional ethernet
> > driver [1] and a switch driver [2] ...
> 
> In staging, the tree of crap. You want to get it out of there and into
> the main tree. But that effort is being side lined by the discussion
> around this IOCTL call.  The best way forward is to to accept Greg is
> not going to take this patchset at the moment, and move on. As you
> said, it is not needed for the Ethernet and switchdev driver.
> 
> What needs to happen before the Ethernet driver can be reviewed for
> moving out of staging?

Hi Andrew,

We're waiting for the DPIO driver (which we depend on) to be moved
out of staging first, it's currently under review:
https://lkml.org/lkml/2018/3/27/1086

Ioana

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[gregkh]

On Thu, Apr 05, 2018 at 03:35:30PM +0000, Ruxandra Ioana Ciocoi Radulescu wrote:
> > -----Original Message-----
> > From: Andrew Lunn [mailto:andrew@lunn.ch]
> > Sent: Thursday, April 5, 2018 6:24 PM
> > To: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> > Cc: Stuart Yoder <stuyoder@gmail.com>; Arnd Bergmann <arnd@arndb.de>;
> > Ioana Ciornei <ioana.ciornei@nxp.com>; gregkh
> > <gregkh@linuxfoundation.org>; Linux Kernel Mailing List <linux-
> > kernel@vger.kernel.org>; Ruxandra Ioana Ciocoi Radulescu
> > <ruxandra.radulescu@nxp.com>; Razvan Stefanescu
> > <razvan.stefanescu@nxp.com>; Roy Pledge <roy.pledge@nxp.com>;
> > Networking <netdev@vger.kernel.org>
> > Subject: Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
> > 
> > On Thu, Apr 05, 2018 at 02:43:29PM +0000, Laurentiu Tudor wrote:
> > > Hi Andrew,
> > >
> > > On 04/05/2018 03:48 PM, Andrew Lunn wrote:
> > > >>> Hi Laurentiu
> > > >>>
> > > >>> So i can use switchdev without it? I can modprobe the switchdev
> > > >>> driver, all the physical interfaces will appear, and i can use ip addr
> > > >>> add etc. I do not need to use a user space tool at all in order to use
> > > >>> the network functionality?
> > > >>
> > > >> Absolutely!
> > > >
> > > > Great.
> > > >
> > > > Then the easiest way forwards is to simply drop the IOCTL code for the
> > > > moment. Get the basic support for the hardware into the kernel
> > > > first. Then come back later to look at dynamic behaviour which needs
> > > > some form of configuration.
> > >
> > > Hmm, not sure I understand. We already have a fully functional ethernet
> > > driver [1] and a switch driver [2] ...
> > 
> > In staging, the tree of crap. You want to get it out of there and into
> > the main tree. But that effort is being side lined by the discussion
> > around this IOCTL call.  The best way forward is to to accept Greg is
> > not going to take this patchset at the moment, and move on. As you
> > said, it is not needed for the Ethernet and switchdev driver.
> > 
> > What needs to happen before the Ethernet driver can be reviewed for
> > moving out of staging?
> 
> Hi Andrew,
> 
> We're waiting for the DPIO driver (which we depend on) to be moved
> out of staging first, it's currently under review:
> https://lkml.org/lkml/2018/3/27/1086

That's stalled on my side right now as the merge window is open and I
can't do any new stuff until after 4.17-rc1 is out.  So everyone please
be patient a bit...

thanks,

greg k-h

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Andrew Lunn]

> > Hi Andrew,
> > 
> > We're waiting for the DPIO driver (which we depend on) to be moved
> > out of staging first, it's currently under review:
> > https://lkml.org/lkml/2018/3/27/1086
> 
> That's stalled on my side right now as the merge window is open and I
> can't do any new stuff until after 4.17-rc1 is out.  So everyone please
> be patient a bit...

I took a quick look.

There are a few inline functions in .c files which is generally
frowned upon. Let the compiler decide.

e.g:

static inline struct dpaa2_io *service_select_by_cpu(struct dpaa2_io *d,	
                                                     int cpu)
static inline struct dpaa2_io *service_select(struct dpaa2_io *d)

dpaa2_io_down() seems to be too simple. dpaa2_io_create() sets up
interrupt triggers, notifications, and adds the new object to the
dpio_list. dpaa2_io_down() seems to just free the memory. Do
notifications need to be disabled, the object taken off the list?

dpaa2_io_store_create() allocates memory using kzalloc() and then uses
dma_map_single(,,DMA_FROM_DEVICE). The documentation says:

   DMA_FROM_DEVICE synchronisation must be done before the driver
   accesses data that may be changed by the device.  This memory
   should be treated as read-only by the driver.  If the driver needs
   to write to it at any point, it should be DMA_BIDIRECTIONAL (see
   below).

Since it has just been allocated, this seems questionable.

I'm also not sure where the correct call to
dma_map_single(,,DMA_FROM_DEVICE) is?  Should dpaa2_io_store_next()
doing this?

The DMA API usage might need a closer review.

    Andrew

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[gregkh]

On Thu, Apr 05, 2018 at 10:30:01AM +0000, Laurentiu Tudor wrote:
> Hello,
> 
> My 2c below.
> 
> On 04/04/2018 03:42 PM, Andrew Lunn wrote:
> >> I hear you.  It is more complicated this way...having all these individual
> >> objects vs just a single "bundle" of them that represents a NIC.  But, that's
> >> the way the DPAA2 hardware is, and we're implementing kernel support for
> >> the hardware as it is.
> >
> > Hi Stuart
> >
> > I see we are not making any progress here.
> >
> > So what i suggest is you post the kernel code and configuration tool
> > concept to netdev for a full review. You want reviews from David
> > Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc.
> >
> 
> I think that the discussion steered too much towards networking related 
> topics, while this ioctl doesn't have much to do with networking.
> It's just an ioctl for our mc-bus bus driver that is used to manage the 
> devices on this bus through userspace tools.
> In addition, I'd drop any mention of our reference user space app 
> (restool) to emphasize that this ioctl is not added just for a 
> particular user space app. I think Stuart also mentioned this.

I'm not going to take a "generic device configuration ioctl" patch
unless it is documented to all exactly what it does, and why it is
there.

thanks,

greg k-h

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[Laurentiu Tudor]

Hi Greg,

On 04/05/2018 03:30 PM, gregkh wrote:
> On Thu, Apr 05, 2018 at 10:30:01AM +0000, Laurentiu Tudor wrote:
>> Hello,
>>
>> My 2c below.
>>
>> On 04/04/2018 03:42 PM, Andrew Lunn wrote:
>>>> I hear you.  It is more complicated this way...having all these individual
>>>> objects vs just a single "bundle" of them that represents a NIC.  But, that's
>>>> the way the DPAA2 hardware is, and we're implementing kernel support for
>>>> the hardware as it is.
>>>
>>> Hi Stuart
>>>
>>> I see we are not making any progress here.
>>>
>>> So what i suggest is you post the kernel code and configuration tool
>>> concept to netdev for a full review. You want reviews from David
>>> Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc.
>>>
>>
>> I think that the discussion steered too much towards networking related
>> topics, while this ioctl doesn't have much to do with networking.
>> It's just an ioctl for our mc-bus bus driver that is used to manage the
>> devices on this bus through userspace tools.
>> In addition, I'd drop any mention of our reference user space app
>> (restool) to emphasize that this ioctl is not added just for a
>> particular user space app. I think Stuart also mentioned this.
>
> I'm not going to take a "generic device configuration ioctl" patch
> unless it is documented to all exactly what it does, and why it is
> there.

The ioctl() is just a simple pass-through interface to the firmware.
It passes commands to the firmware and returns the response back to the 
userspace. Thus the ABI used by the firmware applies for this ioctl() 
and it is documented in detail here:

https://www.nxp.com/docs/en/user-guide/DPAA2_UM.pdf

---
Best Regards, Laurentiu

Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support

[gregkh]

On Thu, Apr 05, 2018 at 02:09:47PM +0000, Laurentiu Tudor wrote:
> Hi Greg,
> 
> On 04/05/2018 03:30 PM, gregkh wrote:
> > On Thu, Apr 05, 2018 at 10:30:01AM +0000, Laurentiu Tudor wrote:
> >> Hello,
> >>
> >> My 2c below.
> >>
> >> On 04/04/2018 03:42 PM, Andrew Lunn wrote:
> >>>> I hear you.  It is more complicated this way...having all these individual
> >>>> objects vs just a single "bundle" of them that represents a NIC.  But, that's
> >>>> the way the DPAA2 hardware is, and we're implementing kernel support for
> >>>> the hardware as it is.
> >>>
> >>> Hi Stuart
> >>>
> >>> I see we are not making any progress here.
> >>>
> >>> So what i suggest is you post the kernel code and configuration tool
> >>> concept to netdev for a full review. You want reviews from David
> >>> Miller, Jiri Pirko, Jakub Kicinski, David Ahern, etc.
> >>>
> >>
> >> I think that the discussion steered too much towards networking related
> >> topics, while this ioctl doesn't have much to do with networking.
> >> It's just an ioctl for our mc-bus bus driver that is used to manage the
> >> devices on this bus through userspace tools.
> >> In addition, I'd drop any mention of our reference user space app
> >> (restool) to emphasize that this ioctl is not added just for a
> >> particular user space app. I think Stuart also mentioned this.
> >
> > I'm not going to take a "generic device configuration ioctl" patch
> > unless it is documented to all exactly what it does, and why it is
> > there.
> 
> The ioctl() is just a simple pass-through interface to the firmware.

Ah, so a new syscall?  :)

> It passes commands to the firmware and returns the response back to the 
> userspace. Thus the ABI used by the firmware applies for this ioctl() 
> and it is documented in detail here:
> 
> https://www.nxp.com/docs/en/user-guide/DPAA2_UM.pdf

Let's wait on this until people all agree that it's ok to expose this
directly.

thanks,

greg k-h

[PATCH v3 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:
  - added proper documentation in /Documentation/ABI/
  - updated the MAINTAINERS file
Changes in v3:
  - no change

 Documentation/ABI/stable/sysfs-bus-fsl-mc |  6 ++++++
 MAINTAINERS                               |  1 +
 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 +++
 5 files changed, 40 insertions(+), 2 deletions(-)
 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..e530e8c
--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-bus-fsl-mc
@@ -0,0 +1,6 @@
+What:		/sys/bus/fsl-mc/devices/dprc.*/rescan
+Date:		March. 2018
+KernelVersion:	4.16
+Contact:	Ioana Ciornei <ioana.ciornei@nxp.com>
+Description:	Root dprc rescan attribute
+Users:		Userspace drivers and management tools
diff --git a/MAINTAINERS b/MAINTAINERS
index 861cf1d..e179dc6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11486,6 +11486,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 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 0ade415..9d02984 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 00cca7d..c1fc80e 100644
--- a/drivers/bus/fsl-mc/fsl-mc-private.h
+++ b/drivers/bus/fsl-mc/fsl-mc-private.h
@@ -545,6 +545,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 v3 3/4] bus: fsl-mc: add root dprc rescan attribute

[Greg KH]

On Fri, Mar 23, 2018 at 10:38:57AM -0500, 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>
> ---
> Changes in v2:
>   - added proper documentation in /Documentation/ABI/
>   - updated the MAINTAINERS file
> Changes in v3:
>   - no change
> 
>  Documentation/ABI/stable/sysfs-bus-fsl-mc |  6 ++++++
>  MAINTAINERS                               |  1 +
>  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 +++
>  5 files changed, 40 insertions(+), 2 deletions(-)
>  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..e530e8c
> --- /dev/null
> +++ b/Documentation/ABI/stable/sysfs-bus-fsl-mc
> @@ -0,0 +1,6 @@
> +What:		/sys/bus/fsl-mc/devices/dprc.*/rescan
> +Date:		March. 2018

"."?

> +KernelVersion:	4.16

4.17 :)

> +Contact:	Ioana Ciornei <ioana.ciornei@nxp.com>
> +Description:	Root dprc rescan attribute

What does this do?  A bit more description please.

thanks,

greg k-h

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

[Ioana Ciornei]

> On Fri, Mar 23, 2018 at 10:38:57AM -0500, 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>
> > ---
> > Changes in v2:
> >   - added proper documentation in /Documentation/ABI/
> >   - updated the MAINTAINERS file
> > Changes in v3:
> >   - no change
> >
> >  Documentation/ABI/stable/sysfs-bus-fsl-mc |  6 ++++++
> >  MAINTAINERS                               |  1 +
> >  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 +++
> >  5 files changed, 40 insertions(+), 2 deletions(-)  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..e530e8c
> > --- /dev/null
> > +++ b/Documentation/ABI/stable/sysfs-bus-fsl-mc
> > @@ -0,0 +1,6 @@
> > +What:		/sys/bus/fsl-mc/devices/dprc.*/rescan
> > +Date:		March. 2018
> 
> "."?
> 
> > +KernelVersion:	4.16
> 
> 4.17 :)

Sorry for this. Will fix.

> 
> > +Contact:	Ioana Ciornei <ioana.ciornei@nxp.com>
> > +Description:	Root dprc rescan attribute
> 
> What does this do?  A bit more description please.

Will update the description in the next version for both attributes.
> 
> thanks,
> 
> greg k-h

[PATCH v3 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:
  - added proper documentation in /Documentation/ABI/
  - updated the MAINTAINERS file
Changes in v3:
  - no change

 Documentation/ABI/stable/sysfs-bus-fsl-mc |  7 +++++
 drivers/bus/fsl-mc/fsl-mc-bus.c           | 48 +++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

diff --git a/Documentation/ABI/stable/sysfs-bus-fsl-mc b/Documentation/ABI/stable/sysfs-bus-fsl-mc
index e530e8c..0663fbd 100644
--- a/Documentation/ABI/stable/sysfs-bus-fsl-mc
+++ b/Documentation/ABI/stable/sysfs-bus-fsl-mc
@@ -4,3 +4,10 @@ KernelVersion:	4.16
 Contact:	Ioana Ciornei <ioana.ciornei@nxp.com>
 Description:	Root dprc rescan attribute
 Users:		Userspace drivers and management tools
+
+What:		/sys/bus/fsl-mc/rescan
+Date:		March. 2018
+KernelVersion:	4.16
+Contact:	Ioana Ciornei <ioana.ciornei@nxp.com>
+Description:	Bus rescan attribute
+Users:		Userspace drivers and management tools
diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index 9d02984..80010d1 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

Re: [PATCH v3 4/4] bus: fsl-mc: add bus rescan attribute

[Greg KH]

On Fri, Mar 23, 2018 at 10:38:58AM -0500, Ioana Ciornei wrote:
> Introduce the rescan attribute as a bus attribute to
> synchronize the fsl-mc bus objects and the MC firmware.
> 
> To rescan the fsl-mc bus, e.g.,
> echo 1 > /sys/bus/fsl-mc/rescan
> 
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> ---
> Changes in v2:
>   - added proper documentation in /Documentation/ABI/
>   - updated the MAINTAINERS file
> Changes in v3:
>   - no change
> 
>  Documentation/ABI/stable/sysfs-bus-fsl-mc |  7 +++++
>  drivers/bus/fsl-mc/fsl-mc-bus.c           | 48 +++++++++++++++++++++++++++++++
>  2 files changed, 55 insertions(+)
> 
> diff --git a/Documentation/ABI/stable/sysfs-bus-fsl-mc b/Documentation/ABI/stable/sysfs-bus-fsl-mc
> index e530e8c..0663fbd 100644
> --- a/Documentation/ABI/stable/sysfs-bus-fsl-mc
> +++ b/Documentation/ABI/stable/sysfs-bus-fsl-mc
> @@ -4,3 +4,10 @@ KernelVersion:	4.16
>  Contact:	Ioana Ciornei <ioana.ciornei@nxp.com>
>  Description:	Root dprc rescan attribute
>  Users:		Userspace drivers and management tools
> +
> +What:		/sys/bus/fsl-mc/rescan
> +Date:		March. 2018
> +KernelVersion:	4.16

Same comments as previous review.

> +Contact:	Ioana Ciornei <ioana.ciornei@nxp.com>
> +Description:	Bus rescan attribute

Again, describe this better please.

> +Users:		Userspace drivers and management tools
> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
> index 9d02984..80010d1 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);

BUS_ATTR_RO()?



> +
> +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,
> +};

ATTRIBUTE_GROUPS()?

thanks,

greg k-h

RE: [PATCH v3 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:
> >   - added proper documentation in /Documentation/ABI/
> >   - updated the MAINTAINERS file
> > Changes in v3:
> >   - no change
> >
> >  Documentation/ABI/stable/sysfs-bus-fsl-mc |  7 +++++
> >  drivers/bus/fsl-mc/fsl-mc-bus.c           | 48
> +++++++++++++++++++++++++++++++
> >  2 files changed, 55 insertions(+)
> >
> > diff --git a/Documentation/ABI/stable/sysfs-bus-fsl-mc
> > b/Documentation/ABI/stable/sysfs-bus-fsl-mc
> > index e530e8c..0663fbd 100644
> > --- a/Documentation/ABI/stable/sysfs-bus-fsl-mc
> > +++ b/Documentation/ABI/stable/sysfs-bus-fsl-mc
> > @@ -4,3 +4,10 @@ KernelVersion:	4.16
> >  Contact:	Ioana Ciornei <ioana.ciornei@nxp.com>
> >  Description:	Root dprc rescan attribute
> >  Users:		Userspace drivers and management tools
> > +
> > +What:		/sys/bus/fsl-mc/rescan
> > +Date:		March. 2018
> > +KernelVersion:	4.16
> 
> Same comments as previous review.
> 
> > +Contact:	Ioana Ciornei <ioana.ciornei@nxp.com>
> > +Description:	Bus rescan attribute
> 
> Again, describe this better please.
> 
> > +Users:		Userspace drivers and management tools
> > diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c
> > b/drivers/bus/fsl-mc/fsl-mc-bus.c index 9d02984..80010d1 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);
> 
> BUS_ATTR_RO()?
> 
> 

Since this is a write-only attribute, a BUS_ATTR_WO would be needed but there is no WO macro defined.

Ioana

> 
> > +
> > +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,
> > +};
> 
> ATTRIBUTE_GROUPS()?
> 
> thanks,
> 
> greg k-h

Re: [PATCH v3 4/4] bus: fsl-mc: add bus rescan attribute

[Greg KH]

On Mon, Apr 02, 2018 at 01:46:13PM +0000, Ioana Ciornei wrote:
> > > Introduce the rescan attribute as a bus attribute to synchronize the
> > > fsl-mc bus objects and the MC firmware.
> > >
> > > To rescan the fsl-mc bus, e.g.,
> > > echo 1 > /sys/bus/fsl-mc/rescan
> > >
> > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > > ---
> > > Changes in v2:
> > >   - added proper documentation in /Documentation/ABI/
> > >   - updated the MAINTAINERS file
> > > Changes in v3:
> > >   - no change
> > >
> > >  Documentation/ABI/stable/sysfs-bus-fsl-mc |  7 +++++
> > >  drivers/bus/fsl-mc/fsl-mc-bus.c           | 48
> > +++++++++++++++++++++++++++++++
> > >  2 files changed, 55 insertions(+)
> > >
> > > diff --git a/Documentation/ABI/stable/sysfs-bus-fsl-mc
> > > b/Documentation/ABI/stable/sysfs-bus-fsl-mc
> > > index e530e8c..0663fbd 100644
> > > --- a/Documentation/ABI/stable/sysfs-bus-fsl-mc
> > > +++ b/Documentation/ABI/stable/sysfs-bus-fsl-mc
> > > @@ -4,3 +4,10 @@ KernelVersion:	4.16
> > >  Contact:	Ioana Ciornei <ioana.ciornei@nxp.com>
> > >  Description:	Root dprc rescan attribute
> > >  Users:		Userspace drivers and management tools
> > > +
> > > +What:		/sys/bus/fsl-mc/rescan
> > > +Date:		March. 2018
> > > +KernelVersion:	4.16
> > 
> > Same comments as previous review.
> > 
> > > +Contact:	Ioana Ciornei <ioana.ciornei@nxp.com>
> > > +Description:	Bus rescan attribute
> > 
> > Again, describe this better please.
> > 
> > > +Users:		Userspace drivers and management tools
> > > diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c
> > > b/drivers/bus/fsl-mc/fsl-mc-bus.c index 9d02984..80010d1 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);
> > 
> > BUS_ATTR_RO()?
> > 
> > 
> 
> Since this is a write-only attribute, a BUS_ATTR_WO would be needed but there is no WO macro defined.

Oops, yes.  We can easily fix that :)

thanks,

greg k-h