* [PATCH v3 1/4] bus: fsl-mc: move fsl_mc_command struct in a uapi header
2018-11-20 15:39 [PATCH v3 0/4] bus: fsl-mc: enhance Management Complex userspace support Ioana Ciornei
@ 2018-11-20 15:39 ` Ioana Ciornei
2018-11-20 15:39 ` [PATCH v3 2/4] bus: fsl-mc: add fsl-mc userspace support Ioana Ciornei
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Ioana Ciornei @ 2018-11-20 15:39 UTC (permalink / raw)
To: gregkh, Laurentiu Tudor
Cc: linux-kernel, netdev-owner, Ioana Ciocoi Radulescu, Horia Geanta,
Leo Li, Ioana Ciornei
Define "struct fsl_mc_command" as a structure that can cross the
user/kernel boundary.
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v2:
- none
Changes in v3:
- none
MAINTAINERS | 1 +
include/linux/fsl/mc.h | 8 +-------
include/uapi/linux/fsl_mc.h | 25 +++++++++++++++++++++++++
3 files changed, 27 insertions(+), 7 deletions(-)
create mode 100644 include/uapi/linux/fsl_mc.h
diff --git a/MAINTAINERS b/MAINTAINERS
index f485597..e841cc4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12282,6 +12282,7 @@ S: Maintained
F: drivers/bus/fsl-mc/
F: Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
F: Documentation/networking/dpaa2/overview.rst
+F: include/uapi/linux/fsl_mc.h
QT1010 MEDIA DRIVER
M: Antti Palosaari <crope@iki.fi>
diff --git a/include/linux/fsl/mc.h b/include/linux/fsl/mc.h
index 741f567..64fc9f8 100644
--- a/include/linux/fsl/mc.h
+++ b/include/linux/fsl/mc.h
@@ -12,6 +12,7 @@
#include <linux/device.h>
#include <linux/mod_devicetable.h>
#include <linux/interrupt.h>
+#include <uapi/linux/fsl_mc.h>
#define FSL_MC_VENDOR_FREESCALE 0x1957
@@ -198,8 +199,6 @@ struct fsl_mc_device {
#define to_fsl_mc_device(_dev) \
container_of(_dev, struct fsl_mc_device, dev)
-#define MC_CMD_NUM_OF_PARAMS 7
-
struct mc_cmd_header {
u8 src_id;
u8 flags_hw;
@@ -209,11 +208,6 @@ struct mc_cmd_header {
__le16 cmd_id;
};
-struct fsl_mc_command {
- __le64 header;
- __le64 params[MC_CMD_NUM_OF_PARAMS];
-};
-
enum mc_cmd_status {
MC_CMD_STATUS_OK = 0x0, /* Completed successfully */
MC_CMD_STATUS_READY = 0x1, /* Ready to be processed */
diff --git a/include/uapi/linux/fsl_mc.h b/include/uapi/linux/fsl_mc.h
new file mode 100644
index 0000000..6c19cfc
--- /dev/null
+++ b/include/uapi/linux/fsl_mc.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Management Complex (MC) userspace public interface
+ *
+ * Copyright 2018 NXP
+ *
+ */
+#ifndef _UAPI_FSL_MC_H_
+#define _UAPI_FSL_MC_H_
+
+#include <linux/types.h>
+
+#define MC_CMD_NUM_OF_PARAMS 7
+
+/**
+ * struct fsl_mc_command - Management Complex (MC) command structure
+ * @header: MC command header
+ * @params: MC command parameters
+ */
+struct fsl_mc_command {
+ __le64 header;
+ __le64 params[MC_CMD_NUM_OF_PARAMS];
+};
+
+#endif /* _UAPI_FSL_MC_H_ */
--
1.9.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v3 2/4] bus: fsl-mc: add fsl-mc userspace support
2018-11-20 15:39 [PATCH v3 0/4] bus: fsl-mc: enhance Management Complex userspace support Ioana Ciornei
2018-11-20 15:39 ` [PATCH v3 1/4] bus: fsl-mc: move fsl_mc_command struct in a uapi header Ioana Ciornei
@ 2018-11-20 15:39 ` Ioana Ciornei
2018-11-20 16:44 ` gregkh
2018-11-20 16:48 ` gregkh
2018-11-20 15:39 ` [PATCH v3 3/4] bus: fsl-mc: add root dprc rescan attribute Ioana Ciornei
2018-11-20 15:39 ` [PATCH v3 4/4] bus: fsl-mc: add bus " Ioana Ciornei
3 siblings, 2 replies; 12+ messages in thread
From: Ioana Ciornei @ 2018-11-20 15:39 UTC (permalink / raw)
To: gregkh, Laurentiu Tudor
Cc: linux-kernel, netdev-owner, Ioana Ciocoi Radulescu, Horia Geanta,
Leo Li, Ioana Ciornei
Adding userspace support for the MC (Management Complex) means exporting
an ioctl capable device file representing the root resource container.
This new functionality in the fsl-mc bus driver intends to provide
userspace applications an interface to interact with the MC firmware.
Commands that are composed in userspace are sent to the MC firmware
through the FSL_MC_SEND_MC_COMMAND ioctl. By default the implicit MC
I/O portal is used for this operation, but if the implicit one is busy,
a dynamic portal is allocated and then freed upon execution.
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v2:
- use root dprc MC portal by default
- create the uapi misc device from the root dprc probe function
Changes in v3:
- use dev_ instead of pr_
- remove dynamic_instance_count and uapi_created variables
- do not assign true to a u32
- return the actual error code (the decision to defer is for the caller)
Documentation/ioctl/ioctl-number.txt | 1 +
drivers/bus/fsl-mc/Kconfig | 7 ++
drivers/bus/fsl-mc/Makefile | 3 +
drivers/bus/fsl-mc/dprc-driver.c | 14 ++-
drivers/bus/fsl-mc/fsl-mc-private.h | 39 ++++++++
drivers/bus/fsl-mc/fsl-mc-uapi.c | 168 +++++++++++++++++++++++++++++++++++
include/uapi/linux/fsl_mc.h | 9 ++
7 files changed, 240 insertions(+), 1 deletion(-)
create mode 100644 drivers/bus/fsl-mc/fsl-mc-uapi.c
diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index af6f6ba..eaae7bf 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -171,6 +171,7 @@ Code Seq#(hex) Include File Comments
'R' 00-1F linux/random.h conflict!
'R' 01 linux/rfkill.h conflict!
'R' C0-DF net/bluetooth/rfcomm.h
+'R' E0 uapi/linux/fsl_mc.h
'S' all linux/cdrom.h conflict!
'S' 80-81 scsi/scsi_ioctl.h conflict!
'S' 82-FF scsi/scsi.h conflict!
diff --git a/drivers/bus/fsl-mc/Kconfig b/drivers/bus/fsl-mc/Kconfig
index c23c77c..cde6f40 100644
--- a/drivers/bus/fsl-mc/Kconfig
+++ b/drivers/bus/fsl-mc/Kconfig
@@ -14,3 +14,10 @@ config FSL_MC_BUS
architecture. The fsl-mc bus driver handles discovery of
DPAA2 objects (which are represented as Linux devices) and
binding objects to drivers.
+
+config FSL_MC_UAPI_SUPPORT
+ bool "Management Complex (MC) userspace support"
+ depends on FSL_MC_BUS
+ help
+ Provides userspace support for creating/destroying/configuring
+ DPAA2 objects in the Management Complex.
diff --git a/drivers/bus/fsl-mc/Makefile b/drivers/bus/fsl-mc/Makefile
index 3c518c7..4ae292a 100644
--- a/drivers/bus/fsl-mc/Makefile
+++ b/drivers/bus/fsl-mc/Makefile
@@ -16,3 +16,6 @@ mc-bus-driver-objs := fsl-mc-bus.o \
fsl-mc-allocator.o \
fsl-mc-msi.o \
dpmcp.o
+
+# MC userspace support
+obj-$(CONFIG_FSL_MC_UAPI_SUPPORT) += fsl-mc-uapi.o
diff --git a/drivers/bus/fsl-mc/dprc-driver.c b/drivers/bus/fsl-mc/dprc-driver.c
index 52c7e15..bab0d5a 100644
--- a/drivers/bus/fsl-mc/dprc-driver.c
+++ b/drivers/bus/fsl-mc/dprc-driver.c
@@ -647,6 +647,12 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
} else {
dev_set_msi_domain(&mc_dev->dev, mc_msi_domain);
msi_domain_set = true;
+
+ error = fsl_mc_uapi_create_device_file(mc_bus);
+ if (error < 0) {
+ error = -EPROBE_DEFER;
+ goto error_cleanup_msi_domain;
+ }
}
}
@@ -654,7 +660,7 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
&mc_dev->mc_handle);
if (error < 0) {
dev_err(&mc_dev->dev, "dprc_open() failed: %d\n", error);
- goto error_cleanup_msi_domain;
+ goto error_cleanup_uapi;
}
error = dprc_get_attributes(mc_dev->mc_io, 0, mc_dev->mc_handle,
@@ -706,6 +712,10 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
error_cleanup_open:
(void)dprc_close(mc_dev->mc_io, 0, mc_dev->mc_handle);
+error_cleanup_uapi:
+ if (fsl_mc_is_root_dprc(&mc_dev->dev))
+ fsl_mc_uapi_remove_device_file(mc_bus);
+
error_cleanup_msi_domain:
if (msi_domain_set)
dev_set_msi_domain(&mc_dev->dev, NULL);
@@ -774,6 +784,8 @@ static int dprc_remove(struct fsl_mc_device *mc_dev)
if (!fsl_mc_is_root_dprc(&mc_dev->dev)) {
fsl_destroy_mc_io(mc_dev->mc_io);
mc_dev->mc_io = NULL;
+ } else {
+ fsl_mc_uapi_remove_device_file(mc_bus);
}
dev_info(&mc_dev->dev, "DPRC device unbound from driver");
diff --git a/drivers/bus/fsl-mc/fsl-mc-private.h b/drivers/bus/fsl-mc/fsl-mc-private.h
index ea11b4f..07f00f8 100644
--- a/drivers/bus/fsl-mc/fsl-mc-private.h
+++ b/drivers/bus/fsl-mc/fsl-mc-private.h
@@ -10,6 +10,8 @@
#include <linux/fsl/mc.h>
#include <linux/mutex.h>
+#include <linux/ioctl.h>
+#include <linux/miscdevice.h>
/*
* Data Path Management Complex (DPMNG) General API
@@ -492,6 +494,22 @@ struct fsl_mc_resource_pool {
};
/**
+ * struct fsl_mc_uapi - information associated with a device file
+ * @misc: struct miscdevice linked to the root dprc
+ * @device: newly created device in /dev
+ * @mutex: mutex lock to serialize the open/release operations
+ * @local_instance_in_use: local MC I/O instance in use or not
+ * @static_mc_io: pointer to the static MC I/O object
+ */
+struct fsl_mc_uapi {
+ struct miscdevice misc;
+ struct device *device;
+ struct mutex mutex; /* serialize open/release operations */
+ u32 local_instance_in_use;
+ struct fsl_mc_io *static_mc_io;
+};
+
+/**
* struct fsl_mc_bus - logical bus that corresponds to a physical DPRC
* @mc_dev: fsl-mc device for the bus device itself.
* @resource_pools: array of resource pools (one pool per resource type)
@@ -500,6 +518,7 @@ struct fsl_mc_resource_pool {
* @irq_resources: Pointer to array of IRQ objects for the IRQ pool
* @scan_mutex: Serializes bus scanning
* @dprc_attr: DPRC attributes
+ * @uapi_misc: struct that abstracts the interaction with userspace
*/
struct fsl_mc_bus {
struct fsl_mc_device mc_dev;
@@ -507,6 +526,7 @@ struct fsl_mc_bus {
struct fsl_mc_device_irq *irq_resources;
struct mutex scan_mutex; /* serializes bus scanning */
struct dprc_attributes dprc_attr;
+ struct fsl_mc_uapi uapi_misc;
};
#define to_fsl_mc_bus(_mc_dev) \
@@ -561,4 +581,23 @@ int __must_check fsl_create_mc_io(struct device *dev,
bool fsl_mc_is_root_dprc(struct device *dev);
+#ifdef CONFIG_FSL_MC_UAPI_SUPPORT
+
+int fsl_mc_uapi_create_device_file(struct fsl_mc_bus *mc_bus);
+
+void fsl_mc_uapi_remove_device_file(struct fsl_mc_bus *mc_bus);
+
+#else
+
+static inline int fsl_mc_uapi_create_device_file(struct fsl_mc_bus *mc_bus)
+{
+ return 0;
+}
+
+static inline void fsl_mc_uapi_remove_device_file(struct fsl_mc_bus *mc_bus)
+{
+}
+
+#endif
+
#endif /* _FSL_MC_PRIVATE_H_ */
diff --git a/drivers/bus/fsl-mc/fsl-mc-uapi.c b/drivers/bus/fsl-mc/fsl-mc-uapi.c
new file mode 100644
index 0000000..71ff201
--- /dev/null
+++ b/drivers/bus/fsl-mc/fsl-mc-uapi.c
@@ -0,0 +1,168 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Management Complex (MC) userspace support
+ *
+ * Copyright 2018 NXP
+ *
+ */
+
+#include <linux/slab.h>
+#include <linux/fs.h>
+#include <linux/uaccess.h>
+#include <linux/miscdevice.h>
+
+#include "fsl-mc-private.h"
+
+struct uapi_priv_data {
+ struct fsl_mc_uapi *uapi;
+ struct fsl_mc_io *mc_io;
+};
+
+static int fsl_mc_uapi_send_command(unsigned long arg,
+ struct fsl_mc_io *mc_io)
+{
+ struct fsl_mc_command mc_cmd;
+ int error;
+
+ error = copy_from_user(&mc_cmd, (void __user *)arg, sizeof(mc_cmd));
+ if (error)
+ return -EFAULT;
+
+ error = mc_send_command(mc_io, &mc_cmd);
+ if (error)
+ return error;
+
+ error = copy_to_user((void __user *)arg, &mc_cmd, sizeof(mc_cmd));
+ if (error)
+ return -EFAULT;
+
+ return 0;
+}
+
+static int fsl_mc_uapi_dev_open(struct inode *inode, struct file *filep)
+{
+ struct fsl_mc_device *root_mc_device;
+ struct uapi_priv_data *priv_data;
+ struct fsl_mc_io *dynamic_mc_io;
+ struct fsl_mc_uapi *mc_uapi;
+ struct fsl_mc_bus *mc_bus;
+ int error;
+
+ priv_data = kzalloc(sizeof(*priv_data), GFP_KERNEL);
+ if (!priv_data)
+ return -ENOMEM;
+
+ mc_uapi = container_of(filep->private_data, struct fsl_mc_uapi, misc);
+ mc_bus = container_of(mc_uapi, struct fsl_mc_bus, uapi_misc);
+ root_mc_device = &mc_bus->mc_dev;
+
+ mutex_lock(&mc_uapi->mutex);
+
+ if (!mc_uapi->local_instance_in_use) {
+ priv_data->mc_io = mc_uapi->static_mc_io;
+ mc_uapi->local_instance_in_use = 1;
+ } else {
+ error = fsl_mc_portal_allocate(root_mc_device, 0,
+ &dynamic_mc_io);
+ if (error) {
+ dev_dbg(&root_mc_device->dev,
+ "Could not allocate MC portal\n");
+ goto error_portal_allocate;
+ }
+
+ priv_data->mc_io = dynamic_mc_io;
+ }
+ priv_data->uapi = mc_uapi;
+ filep->private_data = priv_data;
+
+ mutex_unlock(&mc_uapi->mutex);
+
+ return 0;
+
+error_portal_allocate:
+ mutex_unlock(&mc_uapi->mutex);
+
+ return error;
+}
+
+static int fsl_mc_uapi_dev_release(struct inode *inode, struct file *filep)
+{
+ struct uapi_priv_data *priv_data;
+ struct fsl_mc_uapi *mc_uapi;
+ struct fsl_mc_io *mc_io;
+
+ priv_data = filep->private_data;
+ mc_uapi = priv_data->uapi;
+ mc_io = priv_data->mc_io;
+
+ mutex_lock(&mc_uapi->mutex);
+
+ if (mc_io == mc_uapi->static_mc_io)
+ mc_uapi->local_instance_in_use = 0;
+ else
+ fsl_mc_portal_free(mc_io);
+
+ kfree(filep->private_data);
+ filep->private_data = NULL;
+
+ mutex_unlock(&mc_uapi->mutex);
+
+ return 0;
+}
+
+static long fsl_mc_uapi_dev_ioctl(struct file *file,
+ unsigned int cmd,
+ unsigned long arg)
+{
+ struct uapi_priv_data *priv_data = file->private_data;
+ struct fsl_mc_device *root_mc_device;
+ struct fsl_mc_bus *mc_bus;
+ int error;
+
+ mc_bus = container_of(priv_data->uapi, struct fsl_mc_bus, uapi_misc);
+ root_mc_device = &mc_bus->mc_dev;
+
+ switch (cmd) {
+ case FSL_MC_SEND_MC_COMMAND:
+ error = fsl_mc_uapi_send_command(arg, priv_data->mc_io);
+ break;
+ default:
+ dev_dbg(&root_mc_device->dev, "unexpected ioctl call number\n");
+ error = -EINVAL;
+ }
+
+ return error;
+}
+
+static const struct file_operations fsl_mc_uapi_dev_fops = {
+ .owner = THIS_MODULE,
+ .open = fsl_mc_uapi_dev_open,
+ .release = fsl_mc_uapi_dev_release,
+ .unlocked_ioctl = fsl_mc_uapi_dev_ioctl,
+};
+
+int fsl_mc_uapi_create_device_file(struct fsl_mc_bus *mc_bus)
+{
+ struct fsl_mc_device *mc_dev = &mc_bus->mc_dev;
+ struct fsl_mc_uapi *mc_uapi = &mc_bus->uapi_misc;
+ int error;
+
+ mc_uapi->misc.minor = MISC_DYNAMIC_MINOR;
+ mc_uapi->misc.name = dev_name(&mc_dev->dev);
+ mc_uapi->misc.fops = &fsl_mc_uapi_dev_fops;
+
+ error = misc_register(&mc_uapi->misc);
+ if (error)
+ return error;
+
+ mc_uapi->static_mc_io = mc_bus->mc_dev.mc_io;
+
+ mutex_init(&mc_uapi->mutex);
+
+ return 0;
+}
+
+void fsl_mc_uapi_remove_device_file(struct fsl_mc_bus *mc_bus)
+{
+ misc_deregister(&mc_bus->uapi_misc.misc);
+}
diff --git a/include/uapi/linux/fsl_mc.h b/include/uapi/linux/fsl_mc.h
index 6c19cfc..4989c1b 100644
--- a/include/uapi/linux/fsl_mc.h
+++ b/include/uapi/linux/fsl_mc.h
@@ -16,10 +16,19 @@
* struct fsl_mc_command - Management Complex (MC) command structure
* @header: MC command header
* @params: MC command parameters
+ *
+ * Used by FSL_MC_SEND_MC_COMMAND
*/
struct fsl_mc_command {
__le64 header;
__le64 params[MC_CMD_NUM_OF_PARAMS];
};
+#define FSL_MC_SEND_CMD_IOCTL_TYPE 'R'
+#define FSL_MC_SEND_CMD_IOCTL_SEQ 0xE0
+
+#define FSL_MC_SEND_MC_COMMAND \
+ _IOWR(FSL_MC_SEND_CMD_IOCTL_TYPE, FSL_MC_SEND_CMD_IOCTL_SEQ, \
+ struct fsl_mc_command)
+
#endif /* _UAPI_FSL_MC_H_ */
--
1.9.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3 2/4] bus: fsl-mc: add fsl-mc userspace support
2018-11-20 15:39 ` [PATCH v3 2/4] bus: fsl-mc: add fsl-mc userspace support Ioana Ciornei
@ 2018-11-20 16:44 ` gregkh
2018-11-20 16:51 ` Ioana Ciornei
2018-11-20 16:48 ` gregkh
1 sibling, 1 reply; 12+ messages in thread
From: gregkh @ 2018-11-20 16:44 UTC (permalink / raw)
To: Ioana Ciornei
Cc: Laurentiu Tudor, linux-kernel, netdev-owner,
Ioana Ciocoi Radulescu, Horia Geanta, Leo Li
On Tue, Nov 20, 2018 at 03:39:45PM +0000, Ioana Ciornei wrote:
> +
> + error = fsl_mc_uapi_create_device_file(mc_bus);
> + if (error < 0) {
> + error = -EPROBE_DEFER;
> + goto error_cleanup_msi_domain;
> + }
What error could occur here that would be somehow fixed if you defer and
try this all again? Shouldn't you just return the error given (as
something really went wrong here, it's not a "something else needs to
start up first issue), and abort?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread* RE: [PATCH v3 2/4] bus: fsl-mc: add fsl-mc userspace support
2018-11-20 16:44 ` gregkh
@ 2018-11-20 16:51 ` Ioana Ciornei
2018-11-20 16:58 ` gregkh
0 siblings, 1 reply; 12+ messages in thread
From: Ioana Ciornei @ 2018-11-20 16:51 UTC (permalink / raw)
To: gregkh
Cc: Laurentiu Tudor, linux-kernel, netdev-owner,
Ioana Ciocoi Radulescu, Horia Geanta, Leo Li
> On Tue, Nov 20, 2018 at 03:39:45PM +0000, Ioana Ciornei wrote:
> > +
> > + error = fsl_mc_uapi_create_device_file(mc_bus);
> > + if (error < 0) {
> > + error = -EPROBE_DEFER;
> > + goto error_cleanup_msi_domain;
> > + }
>
> What error could occur here that would be somehow fixed if you defer and try
> this all again? Shouldn't you just return the error given (as something really went
> wrong here, it's not a "something else needs to start up first issue), and abort?
>
It's really a "something else needs to start up first" type of issue. The fsl_mc_uapi_create_device_file fails only if the misc_register fails and this could really happen when the misc char driver is not yet initialized.
Initially, I have used the register_chrdev API and you suggested to use a misc device and just defer probe if it's not yet initialized.
Ioana C
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v3 2/4] bus: fsl-mc: add fsl-mc userspace support
2018-11-20 16:51 ` Ioana Ciornei
@ 2018-11-20 16:58 ` gregkh
0 siblings, 0 replies; 12+ messages in thread
From: gregkh @ 2018-11-20 16:58 UTC (permalink / raw)
To: Ioana Ciornei
Cc: Laurentiu Tudor, linux-kernel, netdev-owner,
Ioana Ciocoi Radulescu, Horia Geanta, Leo Li
On Tue, Nov 20, 2018 at 04:51:07PM +0000, Ioana Ciornei wrote:
> > On Tue, Nov 20, 2018 at 03:39:45PM +0000, Ioana Ciornei wrote:
> > > +
> > > + error = fsl_mc_uapi_create_device_file(mc_bus);
> > > + if (error < 0) {
> > > + error = -EPROBE_DEFER;
> > > + goto error_cleanup_msi_domain;
> > > + }
> >
> > What error could occur here that would be somehow fixed if you defer and try
> > this all again? Shouldn't you just return the error given (as something really went
> > wrong here, it's not a "something else needs to start up first issue), and abort?
> >
>
> It's really a "something else needs to start up first" type of issue. The fsl_mc_uapi_create_device_file fails only if the misc_register fails and this could really happen when the misc char driver is not yet initialized.
>
> Initially, I have used the register_chrdev API and you suggested to use a misc device and just defer probe if it's not yet initialized.
Ok, fair enough, looks good to me, thanks for answering.
greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/4] bus: fsl-mc: add fsl-mc userspace support
2018-11-20 15:39 ` [PATCH v3 2/4] bus: fsl-mc: add fsl-mc userspace support Ioana Ciornei
2018-11-20 16:44 ` gregkh
@ 2018-11-20 16:48 ` gregkh
2018-11-20 17:59 ` Ioana Ciornei
1 sibling, 1 reply; 12+ messages in thread
From: gregkh @ 2018-11-20 16:48 UTC (permalink / raw)
To: Ioana Ciornei
Cc: Laurentiu Tudor, linux-kernel, netdev-owner,
Ioana Ciocoi Radulescu, Horia Geanta, Leo Li
On Tue, Nov 20, 2018 at 03:39:45PM +0000, Ioana Ciornei wrote:
> +static int fsl_mc_uapi_send_command(unsigned long arg,
> + struct fsl_mc_io *mc_io)
> +{
> + struct fsl_mc_command mc_cmd;
> + int error;
> +
> + error = copy_from_user(&mc_cmd, (void __user *)arg, sizeof(mc_cmd));
> + if (error)
> + return -EFAULT;
> +
> + error = mc_send_command(mc_io, &mc_cmd);
> + if (error)
> + return error;
> +
> + error = copy_to_user((void __user *)arg, &mc_cmd, sizeof(mc_cmd));
> + if (error)
> + return -EFAULT;
> +
> + return 0;
> +}
I know you said that "the firmware will properly verify the command"
already, but given that I used to be a firmware developer a long time
ago, I can almost guarantee that this will cause problems in the future.
Want to make a friendly bet about this?
What is the odds that your firmware api/interface has been properly
fuzzed such that all possible combinations of bad commands will really
not do horrible things to the hardware/system?
Are you all willing to bet the system intregrity on this? If so, ok,
it's your systems :)
Personally, I think you need to add a "known whitelist" and do some sort
of sanity checking here.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread* RE: [PATCH v3 2/4] bus: fsl-mc: add fsl-mc userspace support
2018-11-20 16:48 ` gregkh
@ 2018-11-20 17:59 ` Ioana Ciornei
2018-11-21 8:14 ` gregkh
0 siblings, 1 reply; 12+ messages in thread
From: Ioana Ciornei @ 2018-11-20 17:59 UTC (permalink / raw)
To: gregkh
Cc: Laurentiu Tudor, linux-kernel, netdev-owner,
Ioana Ciocoi Radulescu, Horia Geanta, Leo Li
> > +static int fsl_mc_uapi_send_command(unsigned long arg,
> > + struct fsl_mc_io *mc_io)
> > +{
> > + struct fsl_mc_command mc_cmd;
> > + int error;
> > +
> > + error = copy_from_user(&mc_cmd, (void __user *)arg, sizeof(mc_cmd));
> > + if (error)
> > + return -EFAULT;
> > +
> > + error = mc_send_command(mc_io, &mc_cmd);
> > + if (error)
> > + return error;
> > +
> > + error = copy_to_user((void __user *)arg, &mc_cmd, sizeof(mc_cmd));
> > + if (error)
> > + return -EFAULT;
> > +
> > + return 0;
> > +}
>
> I know you said that "the firmware will properly verify the command"
> already, but given that I used to be a firmware developer a long time ago, I can
> almost guarantee that this will cause problems in the future.
>
> Want to make a friendly bet about this?
>
> What is the odds that your firmware api/interface has been properly fuzzed such
> that all possible combinations of bad commands will really not do horrible things
> to the hardware/system?
>
> Are you all willing to bet the system intregrity on this? If so, ok, it's your systems
> :)
>
> Personally, I think you need to add a "known whitelist" and do some sort of
> sanity checking here.
>
I can add a whitelist on the command ids that can be received from userspace but
leave the parameter parsing to the firmware to process and interpret depending on the
current system settings.
Would that be a viable option from your point of view?
Ioana C
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v3 2/4] bus: fsl-mc: add fsl-mc userspace support
2018-11-20 17:59 ` Ioana Ciornei
@ 2018-11-21 8:14 ` gregkh
2018-11-29 16:55 ` Ioana Ciornei
0 siblings, 1 reply; 12+ messages in thread
From: gregkh @ 2018-11-21 8:14 UTC (permalink / raw)
To: Ioana Ciornei
Cc: Laurentiu Tudor, linux-kernel, netdev-owner,
Ioana Ciocoi Radulescu, Horia Geanta, Leo Li
On Tue, Nov 20, 2018 at 05:59:26PM +0000, Ioana Ciornei wrote:
> > > +static int fsl_mc_uapi_send_command(unsigned long arg,
> > > + struct fsl_mc_io *mc_io)
> > > +{
> > > + struct fsl_mc_command mc_cmd;
> > > + int error;
> > > +
> > > + error = copy_from_user(&mc_cmd, (void __user *)arg, sizeof(mc_cmd));
> > > + if (error)
> > > + return -EFAULT;
> > > +
> > > + error = mc_send_command(mc_io, &mc_cmd);
> > > + if (error)
> > > + return error;
> > > +
> > > + error = copy_to_user((void __user *)arg, &mc_cmd, sizeof(mc_cmd));
> > > + if (error)
> > > + return -EFAULT;
> > > +
> > > + return 0;
> > > +}
> >
> > I know you said that "the firmware will properly verify the command"
> > already, but given that I used to be a firmware developer a long time ago, I can
> > almost guarantee that this will cause problems in the future.
> >
> > Want to make a friendly bet about this?
> >
> > What is the odds that your firmware api/interface has been properly fuzzed such
> > that all possible combinations of bad commands will really not do horrible things
> > to the hardware/system?
> >
> > Are you all willing to bet the system intregrity on this? If so, ok, it's your systems
> > :)
> >
> > Personally, I think you need to add a "known whitelist" and do some sort of
> > sanity checking here.
> >
>
> I can add a whitelist on the command ids that can be received from userspace but
> leave the parameter parsing to the firmware to process and interpret depending on the
> current system settings.
>
> Would that be a viable option from your point of view?
That might be a good start. Again, how well have you tested this
firmware interface can handle invalid data? Are you willing to bet your
system integrity on it? :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread* RE: [PATCH v3 2/4] bus: fsl-mc: add fsl-mc userspace support
2018-11-21 8:14 ` gregkh
@ 2018-11-29 16:55 ` Ioana Ciornei
0 siblings, 0 replies; 12+ messages in thread
From: Ioana Ciornei @ 2018-11-29 16:55 UTC (permalink / raw)
To: gregkh
Cc: Laurentiu Tudor, linux-kernel, netdev-owner,
Ioana Ciocoi Radulescu, Horia Geanta, Leo Li
> Subject: Re: [PATCH v3 2/4] bus: fsl-mc: add fsl-mc userspace support
>
> On Tue, Nov 20, 2018 at 05:59:26PM +0000, Ioana Ciornei wrote:
> > > > +static int fsl_mc_uapi_send_command(unsigned long arg,
> > > > + struct fsl_mc_io *mc_io)
> > > > +{
> > > > + struct fsl_mc_command mc_cmd;
> > > > + int error;
> > > > +
> > > > + error = copy_from_user(&mc_cmd, (void __user *)arg, sizeof(mc_cmd));
> > > > + if (error)
> > > > + return -EFAULT;
> > > > +
> > > > + error = mc_send_command(mc_io, &mc_cmd);
> > > > + if (error)
> > > > + return error;
> > > > +
> > > > + error = copy_to_user((void __user *)arg, &mc_cmd, sizeof(mc_cmd));
> > > > + if (error)
> > > > + return -EFAULT;
> > > > +
> > > > + return 0;
> > > > +}
> > >
> > > I know you said that "the firmware will properly verify the command"
> > > already, but given that I used to be a firmware developer a long
> > > time ago, I can almost guarantee that this will cause problems in the future.
> > >
> > > Want to make a friendly bet about this?
> > >
> > > What is the odds that your firmware api/interface has been properly
> > > fuzzed such that all possible combinations of bad commands will
> > > really not do horrible things to the hardware/system?
> > >
> > > Are you all willing to bet the system intregrity on this? If so,
> > > ok, it's your systems
> > > :)
> > >
> > > Personally, I think you need to add a "known whitelist" and do some
> > > sort of sanity checking here.
> > >
> >
> > I can add a whitelist on the command ids that can be received from
> > userspace but leave the parameter parsing to the firmware to process
> > and interpret depending on the current system settings.
> >
> > Would that be a viable option from your point of view?
>
> That might be a good start. Again, how well have you tested this firmware
> interface can handle invalid data? Are you willing to bet your system integrity on
> it? :)
>
We understand your concerns regarding the integrity of the MC firmware.
Each command has validation checks inside the MC and we are currently doing extensive testing with fuzzed commands from user space.
We'll come back with a v4 on this once our tests are complete and we are confident our firmware is robust enough.
Ioana C
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 3/4] bus: fsl-mc: add root dprc rescan attribute
2018-11-20 15:39 [PATCH v3 0/4] bus: fsl-mc: enhance Management Complex userspace support Ioana Ciornei
2018-11-20 15:39 ` [PATCH v3 1/4] bus: fsl-mc: move fsl_mc_command struct in a uapi header Ioana Ciornei
2018-11-20 15:39 ` [PATCH v3 2/4] bus: fsl-mc: add fsl-mc userspace support Ioana Ciornei
@ 2018-11-20 15:39 ` Ioana Ciornei
2018-11-20 15:39 ` [PATCH v3 4/4] bus: fsl-mc: add bus " Ioana Ciornei
3 siblings, 0 replies; 12+ messages in thread
From: Ioana Ciornei @ 2018-11-20 15:39 UTC (permalink / raw)
To: gregkh, Laurentiu Tudor
Cc: linux-kernel, netdev-owner, Ioana Ciocoi Radulescu, Horia Geanta,
Leo Li, Ioana Ciornei
Introduce the rescan attribute as a device attribute to
synchronize the fsl-mc bus objects and the MC firmware.
To rescan the root dprc only, e.g.
echo 1 > /sys/bus/fsl-mc/devices/dprc.1/rescan
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v2:
- remove the rescan dev_attr on the remove path
Changes in v3:
- none
Documentation/ABI/stable/sysfs-bus-fsl-mc | 9 ++++++++
MAINTAINERS | 1 +
drivers/bus/fsl-mc/dprc-driver.c | 36 +++++++++++++++++++++++++++++++
3 files changed, 46 insertions(+)
create mode 100644 Documentation/ABI/stable/sysfs-bus-fsl-mc
diff --git a/Documentation/ABI/stable/sysfs-bus-fsl-mc b/Documentation/ABI/stable/sysfs-bus-fsl-mc
new file mode 100644
index 0000000..9f3a95e
--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-bus-fsl-mc
@@ -0,0 +1,9 @@
+What: /sys/bus/fsl-mc/devices/dprc.*/rescan
+Date: November 2018
+KernelVersion: 5.0
+Contact: Ioana Ciornei <ioana.ciornei@nxp.com>
+Description: Writing a non-zero value to this attribute will
+ force a rescan of dprc.X container in the system and
+ synchronize the objects under dprc.X and the
+ Management Complex firmware.
+Users: Userspace drivers and management tools
diff --git a/MAINTAINERS b/MAINTAINERS
index e841cc4..7b76a80 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12283,6 +12283,7 @@ F: drivers/bus/fsl-mc/
F: Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
F: Documentation/networking/dpaa2/overview.rst
F: include/uapi/linux/fsl_mc.h
+F: Documentation/ABI/stable/sysfs-bus-fsl-mc
QT1010 MEDIA DRIVER
M: Antti Palosaari <crope@iki.fi>
diff --git a/drivers/bus/fsl-mc/dprc-driver.c b/drivers/bus/fsl-mc/dprc-driver.c
index bab0d5a..2f94aba 100644
--- a/drivers/bus/fsl-mc/dprc-driver.c
+++ b/drivers/bus/fsl-mc/dprc-driver.c
@@ -354,6 +354,33 @@ static int dprc_scan_container(struct fsl_mc_device *mc_bus_dev)
return 0;
}
+static ssize_t rescan_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct fsl_mc_device *root_mc_dev;
+ struct fsl_mc_bus *root_mc_bus;
+ unsigned long val;
+
+ if (!fsl_mc_is_root_dprc(dev))
+ return -EINVAL;
+
+ root_mc_dev = to_fsl_mc_device(dev);
+ root_mc_bus = to_fsl_mc_bus(root_mc_dev);
+
+ if (kstrtoul(buf, 0, &val) < 0)
+ return -EINVAL;
+
+ if (val) {
+ mutex_lock(&root_mc_bus->scan_mutex);
+ dprc_scan_objects(root_mc_dev, NULL);
+ mutex_unlock(&root_mc_bus->scan_mutex);
+ }
+
+ return count;
+}
+static DEVICE_ATTR_WO(rescan);
+
/**
* dprc_irq0_handler - Regular ISR for DPRC interrupt 0
*
@@ -692,6 +719,13 @@ static int dprc_probe(struct fsl_mc_device *mc_dev)
mutex_init(&mc_bus->scan_mutex);
+ error = device_create_file(&mc_dev->dev, &dev_attr_rescan);
+ if (error < 0) {
+ dev_err(&mc_dev->dev, "device_create_file() failed: %d\n",
+ error);
+ goto error_cleanup_open;
+ }
+
/*
* Discover MC objects in DPRC object:
*/
@@ -788,6 +822,8 @@ static int dprc_remove(struct fsl_mc_device *mc_dev)
fsl_mc_uapi_remove_device_file(mc_bus);
}
+ device_remove_file(&mc_dev->dev, &dev_attr_rescan);
+
dev_info(&mc_dev->dev, "DPRC device unbound from driver");
return 0;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v3 4/4] bus: fsl-mc: add bus rescan attribute
2018-11-20 15:39 [PATCH v3 0/4] bus: fsl-mc: enhance Management Complex userspace support Ioana Ciornei
` (2 preceding siblings ...)
2018-11-20 15:39 ` [PATCH v3 3/4] bus: fsl-mc: add root dprc rescan attribute Ioana Ciornei
@ 2018-11-20 15:39 ` Ioana Ciornei
3 siblings, 0 replies; 12+ messages in thread
From: Ioana Ciornei @ 2018-11-20 15:39 UTC (permalink / raw)
To: gregkh, Laurentiu Tudor
Cc: linux-kernel, netdev-owner, Ioana Ciocoi Radulescu, Horia Geanta,
Leo Li, Ioana Ciornei
Introduce the rescan attribute as a bus attribute to
synchronize the fsl-mc bus objects and the MC firmware.
To rescan the fsl-mc bus, e.g.,
echo 1 > /sys/bus/fsl-mc/rescan
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v2:
- none
Changes in v3:
- none
Documentation/ABI/stable/sysfs-bus-fsl-mc | 10 ++++++++
drivers/bus/fsl-mc/dprc-driver.c | 4 +--
drivers/bus/fsl-mc/fsl-mc-bus.c | 41 +++++++++++++++++++++++++++++++
drivers/bus/fsl-mc/fsl-mc-private.h | 2 ++
4 files changed, 55 insertions(+), 2 deletions(-)
diff --git a/Documentation/ABI/stable/sysfs-bus-fsl-mc b/Documentation/ABI/stable/sysfs-bus-fsl-mc
index 9f3a95e..e5c7f7f 100644
--- a/Documentation/ABI/stable/sysfs-bus-fsl-mc
+++ b/Documentation/ABI/stable/sysfs-bus-fsl-mc
@@ -7,3 +7,13 @@ Description: Writing a non-zero value to this attribute will
synchronize the objects under dprc.X and the
Management Complex firmware.
Users: Userspace drivers and management tools
+
+What: /sys/bus/fsl-mc/rescan
+Date: November 2018
+KernelVersion: 5.0
+Contact: Ioana Ciornei <ioana.ciornei@nxp.com>
+Description: Writing a non-zero value to this attribute will
+ force a rescan of fsl-mc bus in the system and
+ synchronize the objects under fsl-mc bus and the
+ Management Complex firmware.
+Users: Userspace drivers and management tools
diff --git a/drivers/bus/fsl-mc/dprc-driver.c b/drivers/bus/fsl-mc/dprc-driver.c
index 2f94aba..cac2dfc 100644
--- a/drivers/bus/fsl-mc/dprc-driver.c
+++ b/drivers/bus/fsl-mc/dprc-driver.c
@@ -214,8 +214,8 @@ static void dprc_add_new_devices(struct fsl_mc_device *mc_bus_dev,
* populated before they can get allocation requests from probe callbacks
* of the device drivers for the non-allocatable devices.
*/
-static int dprc_scan_objects(struct fsl_mc_device *mc_bus_dev,
- unsigned int *total_irq_count)
+int dprc_scan_objects(struct fsl_mc_device *mc_bus_dev,
+ unsigned int *total_irq_count)
{
int num_child_objects;
int dprc_get_obj_failures;
diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index f0404c6..ea8991d 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -154,12 +154,53 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
ATTRIBUTE_GROUPS(fsl_mc_dev);
+static int scan_fsl_mc_bus(struct device *dev, void *data)
+{
+ struct fsl_mc_device *root_mc_dev;
+ struct fsl_mc_bus *root_mc_bus;
+
+ if (!fsl_mc_is_root_dprc(dev))
+ goto exit;
+
+ root_mc_dev = to_fsl_mc_device(dev);
+ root_mc_bus = to_fsl_mc_bus(root_mc_dev);
+ mutex_lock(&root_mc_bus->scan_mutex);
+ dprc_scan_objects(root_mc_dev, NULL);
+ mutex_unlock(&root_mc_bus->scan_mutex);
+
+exit:
+ return 0;
+}
+
+static ssize_t rescan_store(struct bus_type *bus,
+ const char *buf, size_t count)
+{
+ unsigned long val;
+
+ if (kstrtoul(buf, 0, &val) < 0)
+ return -EINVAL;
+
+ if (val)
+ bus_for_each_dev(bus, NULL, NULL, scan_fsl_mc_bus);
+
+ return count;
+}
+static BUS_ATTR_WO(rescan);
+
+static struct attribute *fsl_mc_bus_attrs[] = {
+ &bus_attr_rescan.attr,
+ NULL,
+};
+
+ATTRIBUTE_GROUPS(fsl_mc_bus);
+
struct bus_type fsl_mc_bus_type = {
.name = "fsl-mc",
.match = fsl_mc_bus_match,
.uevent = fsl_mc_bus_uevent,
.dma_configure = fsl_mc_dma_configure,
.dev_groups = fsl_mc_dev_groups,
+ .bus_groups = fsl_mc_bus_groups,
};
EXPORT_SYMBOL_GPL(fsl_mc_bus_type);
diff --git a/drivers/bus/fsl-mc/fsl-mc-private.h b/drivers/bus/fsl-mc/fsl-mc-private.h
index 07f00f8..ca71275 100644
--- a/drivers/bus/fsl-mc/fsl-mc-private.h
+++ b/drivers/bus/fsl-mc/fsl-mc-private.h
@@ -543,6 +543,8 @@ int __must_check fsl_mc_device_add(struct fsl_mc_obj_desc *obj_desc,
void dprc_driver_exit(void);
+int dprc_scan_objects(struct fsl_mc_device *mc_bus_dev,
+ unsigned int *total_irq_count);
int __init fsl_mc_allocator_driver_init(void);
void fsl_mc_allocator_driver_exit(void);
--
1.9.1
^ permalink raw reply related [flat|nested] 12+ messages in thread