[RFC v2 0/4] Introduce i3c device userspace interface

[RFC v2 0/4] Introduce i3c device userspace interface

[Vitor Soares]

For today there is no way to use i3c devices from user space and
the introduction of such API will help developers during the i3c device
or i3c host controllers development.

The i3cdev module is highly based on i2c-dev and yet I tried to address
the concerns raised in [1].

NOTES:
- The i3cdev dynamically request an unused major number.

- The i3c devices are dynamically exposed/removed from dev/ folder based
  on if they have a device driver bound to it.

- For now, the module exposes i3c devices without device driver on
  dev/i3c-<bus>-<pid>, but we can change the path to
  dev/bus/i3c/<bus>-<pid> or dev/i3c/<bus>-<pid>.

- As in the i2c subsystem, here it is exposed the i3c_priv_xfer to
  userspace. I tried to use a dedicated structure as in spidev but I don't
  see any obvious advantage.

- Since the i3c API only exposes i3c_priv_xfer to devices, for now, the
  module just makes use of one ioctl(). This can change in the future with
  the introduction hdr commands or by the need of exposing some CCC
  commands to the device API (private contract between master-slave).
  Regarding the i3c device info, some information is already available
  through sysfs. We can add more device attributes to expose more
  information or add a dedicated ioctl() request for that purpose or both.

- Similar to i2c, I have also created a tool that you can find in [2]
  for testing purposes. If you have some time available I would appreciate
  your feedback about it as well.

[1] https://lkml.org/lkml/2018/11/15/853
[2] https://github.com/vitor-soares-snps/i3c-tools.git

Changes in v2:
  Use IDR api for minor numbering
  Modify ioctl struct
  Fix SPDX license

Vitor Soares (4):
  i3c: master: export i3c_masterdev_type
  i3c: master: export i3c_bus_type symbol
  i3c: master: add i3c_for_each_dev helper
  i3c: add i3cdev module to expose i3c dev in /dev

 drivers/i3c/Kconfig             |  15 ++
 drivers/i3c/Makefile            |   1 +
 drivers/i3c/i3cdev.c            | 429 ++++++++++++++++++++++++++++++++++++++++
 drivers/i3c/internals.h         |   2 +
 drivers/i3c/master.c            |  16 +-
 include/uapi/linux/i3c/i3cdev.h |  38 ++++
 6 files changed, 500 insertions(+), 1 deletion(-)
 create mode 100644 drivers/i3c/i3cdev.c
 create mode 100644 include/uapi/linux/i3c/i3cdev.h

-- 
2.7.4

[RFC v2 1/4] i3c: master: export i3c_masterdev_type

[Vitor Soares]

Exporte i3c_masterdev_type so i3cdev module can verify if an i3c device
is a master.

Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
---
 drivers/i3c/internals.h | 1 +
 drivers/i3c/master.c    | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
index 86b7b44..bc062e8 100644
--- a/drivers/i3c/internals.h
+++ b/drivers/i3c/internals.h
@@ -11,6 +11,7 @@
 #include <linux/i3c/master.h>
 
 extern struct bus_type i3c_bus_type;
+extern const struct device_type i3c_masterdev_type;
 
 void i3c_bus_normaluse_lock(struct i3c_bus *bus);
 void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 7f8f896..8a0ba34 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -523,9 +523,10 @@ static void i3c_masterdev_release(struct device *dev)
 	of_node_put(dev->of_node);
 }
 
-static const struct device_type i3c_masterdev_type = {
+const struct device_type i3c_masterdev_type = {
 	.groups	= i3c_masterdev_groups,
 };
+EXPORT_SYMBOL_GPL(i3c_masterdev_type);
 
 static int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode,
 			    unsigned long max_i2c_scl_rate)
-- 
2.7.4

[RFC v2 2/4] i3c: master: export i3c_bus_type symbol

[Vitor Soares]

Export i3c_bus_type symbol so i3cdev can register a notifier chain
for i3c bus.

Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
---
 drivers/i3c/master.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 8a0ba34..21c4372 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -321,6 +321,7 @@ struct bus_type i3c_bus_type = {
 	.probe = i3c_device_probe,
 	.remove = i3c_device_remove,
 };
+EXPORT_SYMBOL_GPL(i3c_bus_type);
 
 static enum i3c_addr_slot_status
 i3c_bus_get_addr_slot_status(struct i3c_bus *bus, u16 addr)
-- 
2.7.4

[RFC v2 3/4] i3c: master: add i3c_for_each_dev helper

[Vitor Soares]

Introduce i3c_for_each_dev(), an i3c device iterator for use by i3cdev.

Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
---
 drivers/i3c/internals.h |  1 +
 drivers/i3c/master.c    | 12 ++++++++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
index bc062e8..a6deedf 100644
--- a/drivers/i3c/internals.h
+++ b/drivers/i3c/internals.h
@@ -24,4 +24,5 @@ int i3c_dev_enable_ibi_locked(struct i3c_dev_desc *dev);
 int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev,
 			       const struct i3c_ibi_setup *req);
 void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev);
+int i3c_for_each_dev(void *data, int (*fn)(struct device *, void *));
 #endif /* I3C_INTERNAL_H */
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 21c4372..8e22da2 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -2640,6 +2640,18 @@ void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
 	dev->ibi = NULL;
 }
 
+int i3c_for_each_dev(void *data, int (*fn)(struct device *, void *))
+{
+	int res;
+
+	mutex_lock(&i3c_core_lock);
+	res = bus_for_each_dev(&i3c_bus_type, NULL, data, fn);
+	mutex_unlock(&i3c_core_lock);
+
+	return res;
+}
+EXPORT_SYMBOL_GPL(i3c_for_each_dev);
+
 static int __init i3c_init(void)
 {
 	return bus_register(&i3c_bus_type);
-- 
2.7.4

[RFC v2 4/4] i3c: add i3cdev module to expose i3c dev in /dev

[Vitor Soares]

This patch adds user mode support to I3C SDR transfers.

The module is based on i2c-dev.c with the follow features:
  - expose on /dev the i3c devices dynamically based on if they have
    a device driver bound.
  - Dynamically allocate the char device Major number.

Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
---
 drivers/i3c/Kconfig             |  15 ++
 drivers/i3c/Makefile            |   1 +
 drivers/i3c/i3cdev.c            | 429 ++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/i3c/i3cdev.h |  38 ++++
 4 files changed, 483 insertions(+)
 create mode 100644 drivers/i3c/i3cdev.c
 create mode 100644 include/uapi/linux/i3c/i3cdev.h

diff --git a/drivers/i3c/Kconfig b/drivers/i3c/Kconfig
index 30a4415..0164276 100644
--- a/drivers/i3c/Kconfig
+++ b/drivers/i3c/Kconfig
@@ -20,5 +20,20 @@ menuconfig I3C
 	  will be called i3c.
 
 if I3C
+
+config I3CDEV
+	tristate "I3C device interface"
+	depends on I3C
+	help
+	  Say Y here to use i3c-* device files, usually found in the /dev
+	  directory on your system.  They make it possible to have user-space
+	  programs use the I3C devices.
+
+	  This support is also available as a module.  If so, the module
+	  will be called i3cdev.
+
+	  Note that this application programming interface is EXPERIMENTAL
+	  and hence SUBJECT TO CHANGE WITHOUT NOTICE while it stabilizes.
+
 source "drivers/i3c/master/Kconfig"
 endif # I3C
diff --git a/drivers/i3c/Makefile b/drivers/i3c/Makefile
index 11982ef..606d422 100644
--- a/drivers/i3c/Makefile
+++ b/drivers/i3c/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
 i3c-y				:= device.o master.o
 obj-$(CONFIG_I3C)		+= i3c.o
+obj-$(CONFIG_I3CDEV)		+= i3cdev.o
 obj-$(CONFIG_I3C)		+= master/
diff --git a/drivers/i3c/i3cdev.c b/drivers/i3c/i3cdev.c
new file mode 100644
index 0000000..f1140dc
--- /dev/null
+++ b/drivers/i3c/i3cdev.c
@@ -0,0 +1,429 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 Synopsys, Inc. and/or its affiliates.
+ *
+ * Author: Vitor Soares <soares@synopsys.com>
+ */
+
+#include <linux/cdev.h>
+#include <linux/compat.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+
+#include <linux/i3c/i3cdev.h>
+
+#include "internals.h"
+
+struct i3cdev_data {
+	struct list_head list;
+	struct i3c_device *i3c;
+	struct cdev cdev;
+	struct device *dev;
+	int id;
+};
+
+static DEFINE_IDA(i3cdev_ida);
+static dev_t i3cdev_number;
+#define I3C_MINORS 16 /* 16 I3C devices supported for now */
+
+static LIST_HEAD(i3cdev_list);
+static DEFINE_SPINLOCK(i3cdev_list_lock);
+
+static struct i3cdev_data *i3cdev_get_by_i3c(struct i3c_device *i3c)
+{
+	struct i3cdev_data *i3cdev;
+
+	spin_lock(&i3cdev_list_lock);
+	list_for_each_entry(i3cdev, &i3cdev_list, list) {
+		if (i3cdev->i3c == i3c)
+			goto found;
+	}
+
+	i3cdev = NULL;
+
+found:
+	spin_unlock(&i3cdev_list_lock);
+	return i3cdev;
+}
+
+static struct i3cdev_data *get_free_i3cdev(struct i3c_device *i3c)
+{
+	struct i3cdev_data *i3cdev;
+	int id;
+
+	id = ida_simple_get(&i3cdev_ida, 0, I3C_MINORS, GFP_KERNEL);
+	if (id < 0) {
+		pr_err("i3cdev: no minor number available!\n");
+		return ERR_PTR(id);
+	}
+
+	i3cdev = kzalloc(sizeof(*i3cdev), GFP_KERNEL);
+	if (!i3cdev) {
+		ida_simple_remove(&i3cdev_ida, id);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	i3cdev->i3c = i3c;
+	i3cdev->id = id;
+
+	spin_lock(&i3cdev_list_lock);
+	list_add_tail(&i3cdev->list, &i3cdev_list);
+	spin_unlock(&i3cdev_list_lock);
+
+	return i3cdev;
+}
+
+static void put_i3cdev(struct i3cdev_data *i3cdev)
+{
+	spin_lock(&i3cdev_list_lock);
+	list_del(&i3cdev->list);
+	spin_unlock(&i3cdev_list_lock);
+	kfree(i3cdev);
+}
+
+static ssize_t
+i3cdev_read(struct file *file, char __user *buf, size_t count, loff_t *f_pos)
+{
+	struct i3c_device *i3c = file->private_data;
+	struct i3c_priv_xfer xfers = {
+		.rnw = true,
+		.len = count,
+	};
+	char *tmp;
+	int ret;
+
+	tmp = kzalloc(count, GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;
+
+	xfers.data.in = tmp;
+
+	dev_dbg(&i3c->dev, "Reading %zu bytes.\n", count);
+
+	ret = i3c_device_do_priv_xfers(i3c, &xfers, 1);
+	if (!ret)
+		ret = copy_to_user(buf, tmp, count) ? -EFAULT : ret;
+
+	kfree(tmp);
+	return ret;
+}
+
+static ssize_t
+i3cdev_write(struct file *file, const char __user *buf, size_t count,
+	     loff_t *f_pos)
+{
+	struct i3c_device *i3c = file->private_data;
+	struct i3c_priv_xfer xfers = {
+		.rnw = false,
+		.len = count,
+	};
+	char *tmp;
+	int ret;
+
+	tmp = memdup_user(buf, count);
+	if (IS_ERR(tmp))
+		return PTR_ERR(tmp);
+
+	xfers.data.out = tmp;
+
+	dev_dbg(&i3c->dev, "Writing %zu bytes.\n", count);
+
+	ret = i3c_device_do_priv_xfers(i3c, &xfers, 1);
+	kfree(tmp);
+	return (!ret) ? count : ret;
+}
+
+static int
+i3cdev_do_priv_xfer(struct i3c_device *dev, struct i3c_ioc_priv_xfer *xfers,
+		    unsigned int nxfers)
+{
+	struct i3c_priv_xfer *k_xfers;
+	u8 **data_ptrs;
+	int i, ret = 0;
+
+	k_xfers = kcalloc(nxfers, sizeof(*k_xfers), GFP_KERNEL);
+	if (!k_xfers)
+		return -ENOMEM;
+
+	data_ptrs = kcalloc(nxfers, sizeof(*data_ptrs), GFP_KERNEL);
+	if (!data_ptrs) {
+		ret = -ENOMEM;
+		goto err_free_k_xfer;
+	}
+
+	for (i = 0; i < nxfers; i++) {
+		data_ptrs[i] = memdup_user((const u8 __user *)
+					   (uintptr_t)xfers[i].data,
+					   xfers[i].len);
+		if (IS_ERR(data_ptrs[i])) {
+			ret = PTR_ERR(data_ptrs[i]);
+			break;
+		}
+
+		k_xfers[i].len = xfers[i].len;
+		if (xfers[i].rnw) {
+			k_xfers[i].rnw = true;
+			k_xfers[i].data.in = data_ptrs[i];
+		} else {
+			k_xfers[i].rnw = false;
+			k_xfers[i].data.out = data_ptrs[i];
+		}
+	}
+
+	if (ret < 0) {
+		i--;
+		goto err_free_mem;
+	}
+
+	ret = i3c_device_do_priv_xfers(dev, k_xfers, nxfers);
+	if (ret)
+		goto err_free_mem;
+
+	for (i = 0; i < nxfers; i++) {
+		if (xfers[i].rnw) {
+			if (copy_to_user((void __user *)(uintptr_t)xfers[i].data,
+					 data_ptrs[i], xfers[i].len))
+				ret = -EFAULT;
+		}
+	}
+
+err_free_mem:
+	for (; i >= 0; i--)
+		kfree(data_ptrs[i]);
+	kfree(data_ptrs);
+err_free_k_xfer:
+	kfree(k_xfers);
+	return ret;
+}
+
+static struct i3c_ioc_priv_xfer *
+i3cdev_get_ioc_priv_xfer(unsigned int cmd, struct i3c_ioc_priv_xfer *u_xfers,
+			 unsigned int *nxfers)
+{
+	u32 tmp = _IOC_SIZE(cmd);
+
+	if ((tmp % sizeof(struct i3c_ioc_priv_xfer)) != 0)
+		return ERR_PTR(-EINVAL);
+
+	*nxfers = tmp / sizeof(struct i3c_ioc_priv_xfer);
+	if (*nxfers == 0)
+		return NULL;
+
+	return memdup_user(u_xfers, tmp);
+}
+
+static int
+i3cdev_ioc_priv_xfer(struct i3c_device *i3c, unsigned int cmd,
+		     struct i3c_ioc_priv_xfer *u_xfers)
+{
+	struct i3c_ioc_priv_xfer *k_xfers;
+	unsigned int nxfers;
+	int ret;
+
+	k_xfers = i3cdev_get_ioc_priv_xfer(cmd, u_xfers, &nxfers);
+	if (IS_ERR_OR_NULL(k_xfers))
+		return PTR_ERR(k_xfers);
+
+	ret = i3cdev_do_priv_xfer(i3c, k_xfers, nxfers);
+
+	kfree(k_xfers);
+
+	return ret;
+}
+
+static long
+i3cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct i3c_device *i3c = file->private_data;
+
+	dev_dbg(&i3c->dev, "ioctl, cmd=0x%02x, arg=0x%02lx\n", cmd, arg);
+
+	if (_IOC_TYPE(cmd) != I3C_DEV_IOC_MAGIC)
+		return -ENOTTY;
+
+	/* Check command number and direction */
+	if (_IOC_NR(cmd) == _IOC_NR(I3C_IOC_PRIV_XFER(0)) &&
+	    _IOC_DIR(cmd) == (_IOC_READ | _IOC_WRITE))
+		return i3cdev_ioc_priv_xfer(i3c, cmd,
+					(struct i3c_ioc_priv_xfer __user *)arg);
+
+	return 0;
+}
+
+static int i3cdev_open(struct inode *inode, struct file *file)
+{
+	struct i3cdev_data *i3cdev = container_of(inode->i_cdev,
+						  struct i3cdev_data,
+						  cdev);
+
+	file->private_data = i3cdev->i3c;
+
+	return 0;
+}
+
+static int i3cdev_release(struct inode *inode, struct file *file)
+{
+	file->private_data = NULL;
+
+	return 0;
+}
+
+static const struct file_operations i3cdev_fops = {
+	.owner		= THIS_MODULE,
+	.read		= i3cdev_read,
+	.write		= i3cdev_write,
+	.unlocked_ioctl	= i3cdev_ioctl,
+	.compat_ioctl	= compat_ptr_ioctl,
+	.open		= i3cdev_open,
+	.release	= i3cdev_release,
+};
+
+/* ------------------------------------------------------------------------- */
+
+static struct class *i3cdev_class;
+
+static int i3cdev_attach(struct device *dev, void *dummy)
+{
+	struct i3cdev_data *i3cdev;
+	struct i3c_device *i3c;
+	int res;
+
+	if (dev->type == &i3c_masterdev_type || dev->driver)
+		return 0;
+
+	i3c = dev_to_i3cdev(dev);
+
+	/* Get a device */
+	i3cdev = get_free_i3cdev(i3c);
+	if (IS_ERR(i3cdev))
+		return PTR_ERR(i3cdev);
+
+	cdev_init(&i3cdev->cdev, &i3cdev_fops);
+	i3cdev->cdev.owner = THIS_MODULE;
+	res = cdev_add(&i3cdev->cdev,
+		       MKDEV(MAJOR(i3cdev_number), i3cdev->id), 1);
+	if (res)
+		goto error_cdev;
+
+	/* register this i3c device with the driver core */
+	i3cdev->dev = device_create(i3cdev_class, &i3c->dev,
+				    MKDEV(MAJOR(i3cdev_number), i3cdev->id),
+				    NULL, "i3c-%s", dev_name(&i3c->dev));
+	if (IS_ERR(i3cdev->dev)) {
+		res = PTR_ERR(i3cdev->dev);
+		goto error;
+	}
+	pr_debug("i3cdev: I3C device [%s] registered as minor %d\n",
+		 dev_name(&i3c->dev), i3cdev->id);
+	return 0;
+
+error:
+	cdev_del(&i3cdev->cdev);
+error_cdev:
+	put_i3cdev(i3cdev);
+	return res;
+}
+
+static int i3cdev_detach(struct device *dev, void *dummy)
+{
+	struct i3cdev_data *i3cdev;
+	struct i3c_device *i3c;
+
+	if (dev->type == &i3c_masterdev_type)
+		return 0;
+
+	i3c = dev_to_i3cdev(dev);
+
+	i3cdev = i3cdev_get_by_i3c(i3c);
+	if (!i3cdev)
+		return 0;
+
+	cdev_del(&i3cdev->cdev);
+	device_destroy(i3cdev_class, MKDEV(MAJOR(i3cdev_number), i3cdev->id));
+	ida_simple_remove(&i3cdev_ida, i3cdev->id);
+	put_i3cdev(i3cdev);
+
+	pr_debug("i3cdev: device [%s] unregistered\n", dev_name(&i3c->dev));
+
+	return 0;
+}
+
+static int i3cdev_notifier_call(struct notifier_block *nb,
+				unsigned long action,
+				void *data)
+{
+	struct device *dev = data;
+
+	switch (action) {
+	case BUS_NOTIFY_ADD_DEVICE:
+	case BUS_NOTIFY_UNBOUND_DRIVER:
+		return i3cdev_attach(dev, NULL);
+	case BUS_NOTIFY_DEL_DEVICE:
+	case BUS_NOTIFY_BOUND_DRIVER:
+		return i3cdev_detach(dev, NULL);
+	}
+
+	return 0;
+}
+
+static struct notifier_block i3c_notifier = {
+	.notifier_call = i3cdev_notifier_call,
+};
+
+static int __init i3cdev_init(void)
+{
+	int res;
+
+	/* Dynamically request unused major number */
+	res = alloc_chrdev_region(&i3cdev_number, 0, I3C_MINORS, "i3c");
+	if (res)
+		goto out;
+
+	/* Create a classe to populate sysfs entries*/
+	i3cdev_class = class_create(THIS_MODULE, "i3cdev");
+	if (IS_ERR(i3cdev_class)) {
+		res = PTR_ERR(i3cdev_class);
+		goto out_unreg_chrdev;
+	}
+
+	/* Keep track of busses which have devices to add or remove later */
+	res = bus_register_notifier(&i3c_bus_type, &i3c_notifier);
+	if (res)
+		goto out_unreg_class;
+
+	/* Bind to already existing device without driver right away */
+	i3c_for_each_dev(NULL, i3cdev_attach);
+
+	return 0;
+
+out_unreg_class:
+	class_destroy(i3cdev_class);
+out_unreg_chrdev:
+	unregister_chrdev_region(i3cdev_number, I3C_MINORS);
+out:
+	pr_err("%s: Driver Initialisation failed\n", __FILE__);
+	return res;
+}
+
+static void __exit i3cdev_exit(void)
+{
+	bus_unregister_notifier(&i3c_bus_type, &i3c_notifier);
+	i3c_for_each_dev(NULL, i3cdev_detach);
+	class_destroy(i3cdev_class);
+	unregister_chrdev_region(i3cdev_number, I3C_MINORS);
+}
+
+MODULE_AUTHOR("Vitor Soares <soares@synopsys.com>");
+MODULE_DESCRIPTION("I3C /dev entries driver");
+MODULE_LICENSE("GPL");
+
+module_init(i3cdev_init);
+module_exit(i3cdev_exit);
diff --git a/include/uapi/linux/i3c/i3cdev.h b/include/uapi/linux/i3c/i3cdev.h
new file mode 100644
index 0000000..0897313
--- /dev/null
+++ b/include/uapi/linux/i3c/i3cdev.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Copyright (c) 2019 Synopsys, Inc. and/or its affiliates.
+ *
+ * Author: Vitor Soares <vitor.soares@synopsys.com>
+ */
+
+#ifndef _UAPI_I3C_DEV_H_
+#define _UAPI_I3C_DEV_H_
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+/* IOCTL commands */
+#define I3C_DEV_IOC_MAGIC	0x07
+
+/**
+ * struct i3c_ioc_priv_xfer - I3C SDR ioctl private transfer
+ * @data: Holds pointer to userspace buffer with transmit data.
+ * @len: Length of data buffer buffers, in bytes.
+ * @rnw: encodes the transfer direction. true for a read, false for a write
+ */
+struct i3c_ioc_priv_xfer {
+	__u64 data;
+	__u16 len;
+	__u8 rnw;
+	__u8 pad[5];
+};
+
+
+#define I3C_PRIV_XFER_SIZE(N)	\
+	((((sizeof(struct i3c_ioc_priv_xfer)) * (N)) < (1 << _IOC_SIZEBITS)) \
+	? ((sizeof(struct i3c_ioc_priv_xfer)) * (N)) : 0)
+
+#define I3C_IOC_PRIV_XFER(N)	\
+	_IOC(_IOC_READ|_IOC_WRITE, I3C_DEV_IOC_MAGIC, 30, I3C_PRIV_XFER_SIZE(N))
+
+#endif
-- 
2.7.4

Re: [RFC v2 4/4] i3c: add i3cdev module to expose i3c dev in /dev

[Arnd Bergmann]

On Wed, Jan 29, 2020 at 1:17 PM Vitor Soares <Vitor.Soares@synopsys.com> wrote:
>
> +
> +struct i3cdev_data {
> +       struct list_head list;
> +       struct i3c_device *i3c;
> +       struct cdev cdev;
> +       struct device *dev;
> +       int id;
> +};
> +
> +static DEFINE_IDA(i3cdev_ida);
> +static dev_t i3cdev_number;
> +#define I3C_MINORS 16 /* 16 I3C devices supported for now */
> +
> +static LIST_HEAD(i3cdev_list);
> +static DEFINE_SPINLOCK(i3cdev_list_lock);

Please try to avoid arbitrarily limiting the number of devices you support.

Searching through the list feels a little clumsy. If the i3c user interface is
supposed to become a standard feature of the subsystem, it would seem
appropriate to put a pointer into the device to simplify the lookup, or
just embed the cdev inside of i3c_device.

> +static int
> +i3cdev_do_priv_xfer(struct i3c_device *dev, struct i3c_ioc_priv_xfer *xfers,
> +                   unsigned int nxfers)
> +{
> +       struct i3c_priv_xfer *k_xfers;
> +       u8 **data_ptrs;
> +       int i, ret = 0;
> +
> +       k_xfers = kcalloc(nxfers, sizeof(*k_xfers), GFP_KERNEL);
> +       if (!k_xfers)
> +               return -ENOMEM;
> +
> +       data_ptrs = kcalloc(nxfers, sizeof(*data_ptrs), GFP_KERNEL);
> +       if (!data_ptrs) {
> +               ret = -ENOMEM;
> +               goto err_free_k_xfer;
> +       }

Maybe use a  combined allocation to simplify the error handling?

> +       for (i = 0; i < nxfers; i++) {
> +               data_ptrs[i] = memdup_user((const u8 __user *)
> +                                          (uintptr_t)xfers[i].data,
> +                                          xfers[i].len);

> +               if (xfers[i].rnw) {
> +                       if (copy_to_user((void __user *)(uintptr_t)xfers[i].data,
> +                                        data_ptrs[i], xfers[i].len))

Use u64_to_user_ptr() here.

> +
> +static struct i3c_ioc_priv_xfer *
> +i3cdev_get_ioc_priv_xfer(unsigned int cmd, struct i3c_ioc_priv_xfer *u_xfers,
> +                        unsigned int *nxfers)
> +{
> +       u32 tmp = _IOC_SIZE(cmd);
> +
> +       if ((tmp % sizeof(struct i3c_ioc_priv_xfer)) != 0)
> +               return ERR_PTR(-EINVAL);
> +
> +       *nxfers = tmp / sizeof(struct i3c_ioc_priv_xfer);
> +       if (*nxfers == 0)
> +               return NULL;
> +
> +       return memdup_user(u_xfers, tmp);
> +}
> +
> +static int
> +i3cdev_ioc_priv_xfer(struct i3c_device *i3c, unsigned int cmd,
> +                    struct i3c_ioc_priv_xfer *u_xfers)
> +{
> +       struct i3c_ioc_priv_xfer *k_xfers;
> +       unsigned int nxfers;
> +       int ret;
> +
> +       k_xfers = i3cdev_get_ioc_priv_xfer(cmd, u_xfers, &nxfers);
> +       if (IS_ERR_OR_NULL(k_xfers))
> +               return PTR_ERR(k_xfers);
> +
> +       ret = i3cdev_do_priv_xfer(i3c, k_xfers, nxfers);

The IS_ERR_OR_NULL() usage looks suspicious. It's generally
better to avoid interfaces that require this. What does it mean to
return NULL from i3cdev_get_ioc_priv_xfer() and turn that into
success? Could you handle this condition in the caller instead,
or turn it into an error?

> +       /* Keep track of busses which have devices to add or remove later */
> +       res = bus_register_notifier(&i3c_bus_type, &i3c_notifier);
> +       if (res)
> +               goto out_unreg_class;
> +
> +       /* Bind to already existing device without driver right away */
> +       i3c_for_each_dev(NULL, i3cdev_attach);

The combination of the notifier and searching through the devices
seems to be racy. What happens when a device appears just before
or during the i3c_for_each_dev() traversal?

What happens when a driver attaches to a device that is currently
transferring data on the user interface?

Is there any guarantee that the notifiers for attach and detach
are serialized?

> +/**
> + * struct i3c_ioc_priv_xfer - I3C SDR ioctl private transfer
> + * @data: Holds pointer to userspace buffer with transmit data.
> + * @len: Length of data buffer buffers, in bytes.
> + * @rnw: encodes the transfer direction. true for a read, false for a write
> + */
> +struct i3c_ioc_priv_xfer {
> +       __u64 data;
> +       __u16 len;
> +       __u8 rnw;
> +       __u8 pad[5];
> +};
> +
> +
> +#define I3C_PRIV_XFER_SIZE(N)  \
> +       ((((sizeof(struct i3c_ioc_priv_xfer)) * (N)) < (1 << _IOC_SIZEBITS)) \
> +       ? ((sizeof(struct i3c_ioc_priv_xfer)) * (N)) : 0)
> +
> +#define I3C_IOC_PRIV_XFER(N)   \
> +       _IOC(_IOC_READ|_IOC_WRITE, I3C_DEV_IOC_MAGIC, 30, I3C_PRIV_XFER_SIZE(N))

This looks like a reasonable ioctl definition, avoiding the usual problems
with compat mode etc.

      Arnd

RE: [RFC v2 4/4] i3c: add i3cdev module to expose i3c dev in /dev

[Vitor Soares]

Hi Arnd,

From: Arnd Bergmann <arnd@arndb.de>
Date: Wed, Jan 29, 2020 at 14:30:56

> On Wed, Jan 29, 2020 at 1:17 PM Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> >
> > +
> > +struct i3cdev_data {
> > +       struct list_head list;
> > +       struct i3c_device *i3c;
> > +       struct cdev cdev;
> > +       struct device *dev;
> > +       int id;
> > +};
> > +
> > +static DEFINE_IDA(i3cdev_ida);
> > +static dev_t i3cdev_number;
> > +#define I3C_MINORS 16 /* 16 I3C devices supported for now */
> > +
> > +static LIST_HEAD(i3cdev_list);
> > +static DEFINE_SPINLOCK(i3cdev_list_lock);
> 
> Please try to avoid arbitrarily limiting the number of devices you support.

Should I use all minors range instead?

> 
> Searching through the list feels a little clumsy. If the i3c user interface is
> supposed to become a standard feature of the subsystem, it would seem
> appropriate to put a pointer into the device to simplify the lookup, 

Do you mean i3c->dev ?

> or
> just embed the cdev inside of i3c_device.

I would prefer to have a pointer in i3c_device for i3cdev_data, but I see 
others using it in drvdata.

> 
> > +static int
> > +i3cdev_do_priv_xfer(struct i3c_device *dev, struct i3c_ioc_priv_xfer *xfers,
> > +                   unsigned int nxfers)
> > +{
> > +       struct i3c_priv_xfer *k_xfers;
> > +       u8 **data_ptrs;
> > +       int i, ret = 0;
> > +
> > +       k_xfers = kcalloc(nxfers, sizeof(*k_xfers), GFP_KERNEL);
> > +       if (!k_xfers)
> > +               return -ENOMEM;
> > +
> > +       data_ptrs = kcalloc(nxfers, sizeof(*data_ptrs), GFP_KERNEL);
> > +       if (!data_ptrs) {
> > +               ret = -ENOMEM;
> > +               goto err_free_k_xfer;
> > +       }
> 
> Maybe use a  combined allocation to simplify the error handling?

Could you please provide an example?

> 
> > +       for (i = 0; i < nxfers; i++) {
> > +               data_ptrs[i] = memdup_user((const u8 __user *)
> > +                                          (uintptr_t)xfers[i].data,
> > +                                          xfers[i].len);
> 
> > +               if (xfers[i].rnw) {
> > +                       if (copy_to_user((void __user *)(uintptr_t)xfers[i].data,
> > +                                        data_ptrs[i], xfers[i].len))
> 
> Use u64_to_user_ptr() here.

You are right, it wasn't available went I did it.

> 
> > +
> > +static struct i3c_ioc_priv_xfer *
> > +i3cdev_get_ioc_priv_xfer(unsigned int cmd, struct i3c_ioc_priv_xfer *u_xfers,
> > +                        unsigned int *nxfers)
> > +{
> > +       u32 tmp = _IOC_SIZE(cmd);
> > +
> > +       if ((tmp % sizeof(struct i3c_ioc_priv_xfer)) != 0)
> > +               return ERR_PTR(-EINVAL);
> > +
> > +       *nxfers = tmp / sizeof(struct i3c_ioc_priv_xfer);
> > +       if (*nxfers == 0)
> > +               return NULL;
> > +
> > +       return memdup_user(u_xfers, tmp);
> > +}
> > +
> > +static int
> > +i3cdev_ioc_priv_xfer(struct i3c_device *i3c, unsigned int cmd,
> > +                    struct i3c_ioc_priv_xfer *u_xfers)
> > +{
> > +       struct i3c_ioc_priv_xfer *k_xfers;
> > +       unsigned int nxfers;
> > +       int ret;
> > +
> > +       k_xfers = i3cdev_get_ioc_priv_xfer(cmd, u_xfers, &nxfers);
> > +       if (IS_ERR_OR_NULL(k_xfers))
> > +               return PTR_ERR(k_xfers);
> > +
> > +       ret = i3cdev_do_priv_xfer(i3c, k_xfers, nxfers);
> 
> The IS_ERR_OR_NULL() usage looks suspicious. It's generally
> better to avoid interfaces that require this. What does it mean to
> return NULL from i3cdev_get_ioc_priv_xfer() and turn that into
> success? Could you handle this condition in the caller instead,
> or turn it into an error?

In both cases something is not correct. I will turn both conditions to 
return ERR_PTR(-EINVAL) and them just check if (IS_ERR(k_xfer)).

> 
> > +       /* Keep track of busses which have devices to add or remove later */
> > +       res = bus_register_notifier(&i3c_bus_type, &i3c_notifier);
> > +       if (res)
> > +               goto out_unreg_class;
> > +
> > +       /* Bind to already existing device without driver right away */
> > +       i3c_for_each_dev(NULL, i3cdev_attach);
> 
> The combination of the notifier and searching through the devices
> seems to be racy. What happens when a device appears just before
> or during the i3c_for_each_dev() traversal?

The i3c core is locked during this phase.

> 
> What happens when a driver attaches to a device that is currently
> transferring data on the user interface?
> 

It may lost references for inode and file. I need to guarantee there no 
tranfer going on during the detach.
Do you have any suggestion?

> Is there any guarantee that the notifiers for attach and detach
> are serialized?
> 

Sorry I didn't get this part. 

> > +/**
> > + * struct i3c_ioc_priv_xfer - I3C SDR ioctl private transfer
> > + * @data: Holds pointer to userspace buffer with transmit data.
> > + * @len: Length of data buffer buffers, in bytes.
> > + * @rnw: encodes the transfer direction. true for a read, false for a write
> > + */
> > +struct i3c_ioc_priv_xfer {
> > +       __u64 data;
> > +       __u16 len;
> > +       __u8 rnw;
> > +       __u8 pad[5];
> > +};
> > +
> > +
> > +#define I3C_PRIV_XFER_SIZE(N)  \
> > +       ((((sizeof(struct i3c_ioc_priv_xfer)) * (N)) < (1 << _IOC_SIZEBITS)) \
> > +       ? ((sizeof(struct i3c_ioc_priv_xfer)) * (N)) : 0)
> > +
> > +#define I3C_IOC_PRIV_XFER(N)   \
> > +       _IOC(_IOC_READ|_IOC_WRITE, I3C_DEV_IOC_MAGIC, 30, I3C_PRIV_XFER_SIZE(N))
> 
> This looks like a reasonable ioctl definition, avoiding the usual problems
> with compat mode etc.

Do you think I should add more reserved fields for future?

> 
>       Arnd
> 
> _______________________________________________
> linux-i3c mailing list
> linux-i3c@lists.infradead.org
> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Di3c&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=pv8xU_wOpDLOkwdQiuBDso73EKvNPX2jXLtBHDVWRFo&s=S-Tesk8Hi3Ok6y9d_ysocHXGt2dmnn-WcM0BxurcDdQ&e= 

Thanks for your comments 😊

Best regards,
Vitor Soares

Re: [RFC v2 4/4] i3c: add i3cdev module to expose i3c dev in /dev

[Arnd Bergmann]

On Wed, Jan 29, 2020 at 6:00 PM Vitor Soares <Vitor.Soares@synopsys.com> wrote:
>
> Hi Arnd,
>
> From: Arnd Bergmann <arnd@arndb.de>
> Date: Wed, Jan 29, 2020 at 14:30:56
>
> > On Wed, Jan 29, 2020 at 1:17 PM Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > >
> > > +
> > > +struct i3cdev_data {
> > > +       struct list_head list;
> > > +       struct i3c_device *i3c;
> > > +       struct cdev cdev;
> > > +       struct device *dev;
> > > +       int id;
> > > +};
> > > +
> > > +static DEFINE_IDA(i3cdev_ida);
> > > +static dev_t i3cdev_number;
> > > +#define I3C_MINORS 16 /* 16 I3C devices supported for now */
> > > +
> > > +static LIST_HEAD(i3cdev_list);
> > > +static DEFINE_SPINLOCK(i3cdev_list_lock);
> >
> > Please try to avoid arbitrarily limiting the number of devices you support.
>
> Should I use all minors range instead?

Yes, I'm fairly sure that if you use a dynamic major number, there
is no downside in using all of them.

> > Searching through the list feels a little clumsy. If the i3c user interface is
> > supposed to become a standard feature of the subsystem, it would seem
> > appropriate to put a pointer into the device to simplify the lookup,
>
> Do you mean i3c->dev ?

I was thinking you could add another member in i3c_device, next to ->dev.

> > or
> > just embed the cdev inside of i3c_device.
>
> I would prefer to have a pointer in i3c_device for i3cdev_data, but I see
> others using it in drvdata.

Ok, I think drvdata should work, but you should check that this is
correct when the device goes back between being bound to a device
driver and used through the chardev.

> >
> > > +static int
> > > +i3cdev_do_priv_xfer(struct i3c_device *dev, struct i3c_ioc_priv_xfer *xfers,
> > > +                   unsigned int nxfers)
> > > +{
> > > +       struct i3c_priv_xfer *k_xfers;
> > > +       u8 **data_ptrs;
> > > +       int i, ret = 0;
> > > +
> > > +       k_xfers = kcalloc(nxfers, sizeof(*k_xfers), GFP_KERNEL);
> > > +       if (!k_xfers)
> > > +               return -ENOMEM;
> > > +
> > > +       data_ptrs = kcalloc(nxfers, sizeof(*data_ptrs), GFP_KERNEL);
> > > +       if (!data_ptrs) {
> > > +               ret = -ENOMEM;
> > > +               goto err_free_k_xfer;
> > > +       }
> >
> > Maybe use a  combined allocation to simplify the error handling?
>
> Could you please provide an example?

Something like

       k_xfers = kcalloc(nxfers, sizeof(*k_xfers) +
sizeof(*data_ptrs), GFP_KERNEL);
       data_ptrs = (void *)k_xfers + (nxfers, sizeof(*k_xfers));

This would need a comment to explain the pointer math, but the resulting
object code is slightly simpler.

> > > +       /* Keep track of busses which have devices to add or remove later */
> > > +       res = bus_register_notifier(&i3c_bus_type, &i3c_notifier);
> > > +       if (res)
> > > +               goto out_unreg_class;
> > > +
> > > +       /* Bind to already existing device without driver right away */
> > > +       i3c_for_each_dev(NULL, i3cdev_attach);
> >
> > The combination of the notifier and searching through the devices
> > seems to be racy. What happens when a device appears just before
> > or during the i3c_for_each_dev() traversal?
>
> The i3c core is locked during this phase.

Ok.

> > What happens when a driver attaches to a device that is currently
> > transferring data on the user interface?
> >
>
> It may lost references for inode and file. I need to guarantee there no
> tranfer going on during the detach.
> Do you have any suggestion?

If the notifier is blocking, you could hold another mutex during the transfer
I think.

> > Is there any guarantee that the notifiers for attach and detach
> > are serialized?
> >
>
> Sorry I didn't get this part.

I think you answered this above: if the i3c code is locked while calling
the notifier, this cannot happen.

> > > +/**
> > > + * struct i3c_ioc_priv_xfer - I3C SDR ioctl private transfer
> > > + * @data: Holds pointer to userspace buffer with transmit data.
> > > + * @len: Length of data buffer buffers, in bytes.
> > > + * @rnw: encodes the transfer direction. true for a read, false for a write
> > > + */
> > > +struct i3c_ioc_priv_xfer {
> > > +       __u64 data;
> > > +       __u16 len;
> > > +       __u8 rnw;
> > > +       __u8 pad[5];
> > > +};
> > > +
> > > +
> > > +#define I3C_PRIV_XFER_SIZE(N)  \
> > > +       ((((sizeof(struct i3c_ioc_priv_xfer)) * (N)) < (1 << _IOC_SIZEBITS)) \
> > > +       ? ((sizeof(struct i3c_ioc_priv_xfer)) * (N)) : 0)
> > > +
> > > +#define I3C_IOC_PRIV_XFER(N)   \
> > > +       _IOC(_IOC_READ|_IOC_WRITE, I3C_DEV_IOC_MAGIC, 30, I3C_PRIV_XFER_SIZE(N))
> >
> > This looks like a reasonable ioctl definition, avoiding the usual problems
> > with compat mode etc.
>
> Do you think I should add more reserved fields for future?

No, what I meant is that I like it the way it is.

     Arnd

RE: [RFC v2 4/4] i3c: add i3cdev module to expose i3c dev in /dev

[Vitor Soares]

From: Arnd Bergmann <arnd@arndb.de>
Date: Wed, Jan 29, 2020 at 19:39:41

> On Wed, Jan 29, 2020 at 6:00 PM Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> >
> > Hi Arnd,
> >
> > From: Arnd Bergmann <arnd@arndb.de>
> > Date: Wed, Jan 29, 2020 at 14:30:56
> >
> > > On Wed, Jan 29, 2020 at 1:17 PM Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > >
> > > > +
> > > > +struct i3cdev_data {
> > > > +       struct list_head list;
> > > > +       struct i3c_device *i3c;
> > > > +       struct cdev cdev;
> > > > +       struct device *dev;
> > > > +       int id;
> > > > +};
> > > > +
> > > > +static DEFINE_IDA(i3cdev_ida);
> > > > +static dev_t i3cdev_number;
> > > > +#define I3C_MINORS 16 /* 16 I3C devices supported for now */
> > > > +
> > > > +static LIST_HEAD(i3cdev_list);
> > > > +static DEFINE_SPINLOCK(i3cdev_list_lock);
> > >
> > > Please try to avoid arbitrarily limiting the number of devices you support.
> >
> > Should I use all minors range instead?
> 
> Yes, I'm fairly sure that if you use a dynamic major number, there
> is no downside in using all of them.
> 
> > > Searching through the list feels a little clumsy. If the i3c user interface is
> > > supposed to become a standard feature of the subsystem, it would seem
> > > appropriate to put a pointer into the device to simplify the lookup,
> >
> > Do you mean i3c->dev ?
> 
> I was thinking you could add another member in i3c_device, next to ->dev.
> 
> > > or
> > > just embed the cdev inside of i3c_device.
> >
> > I would prefer to have a pointer in i3c_device for i3cdev_data, but I see
> > others using it in drvdata.
> 
> Ok, I think drvdata should work, but you should check that this is
> correct when the device goes back between being bound to a device
> driver and used through the chardev.

I changed the detach to be done in BUS_NOTIFY_BIND_DRIVER.

> 
> > >
> > > > +static int
> > > > +i3cdev_do_priv_xfer(struct i3c_device *dev, struct i3c_ioc_priv_xfer *xfers,
> > > > +                   unsigned int nxfers)
> > > > +{
> > > > +       struct i3c_priv_xfer *k_xfers;
> > > > +       u8 **data_ptrs;
> > > > +       int i, ret = 0;
> > > > +
> > > > +       k_xfers = kcalloc(nxfers, sizeof(*k_xfers), GFP_KERNEL);
> > > > +       if (!k_xfers)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       data_ptrs = kcalloc(nxfers, sizeof(*data_ptrs), GFP_KERNEL);
> > > > +       if (!data_ptrs) {
> > > > +               ret = -ENOMEM;
> > > > +               goto err_free_k_xfer;
> > > > +       }
> > >
> > > Maybe use a  combined allocation to simplify the error handling?
> >
> > Could you please provide an example?
> 
> Something like
> 
>        k_xfers = kcalloc(nxfers, sizeof(*k_xfers) +
> sizeof(*data_ptrs), GFP_KERNEL);
>        data_ptrs = (void *)k_xfers + (nxfers, sizeof(*k_xfers));
> 
> This would need a comment to explain the pointer math, but the resulting
> object code is slightly simpler.

As we have nferxs, there is no problem to allocate k_xfers more than 
needed, right?

> 
> > > > +       /* Keep track of busses which have devices to add or remove later */
> > > > +       res = bus_register_notifier(&i3c_bus_type, &i3c_notifier);
> > > > +       if (res)
> > > > +               goto out_unreg_class;
> > > > +
> > > > +       /* Bind to already existing device without driver right away */
> > > > +       i3c_for_each_dev(NULL, i3cdev_attach);
> > >
> > > The combination of the notifier and searching through the devices
> > > seems to be racy. What happens when a device appears just before
> > > or during the i3c_for_each_dev() traversal?
> >
> > The i3c core is locked during this phase.
> 
> Ok.
> 
> > > What happens when a driver attaches to a device that is currently
> > > transferring data on the user interface?
> > >
> >
> > It may lost references for inode and file. I need to guarantee there no
> > tranfer going on during the detach.
> > Do you have any suggestion?
> 
> If the notifier is blocking, you could hold another mutex during the transfer
> I think.

A mutex during the transfer will solve the detach issue, I doing some 
tests but even with the change to BUS_NOTIFY_BIND_DRIVER I'm not sure if 
it can race with driver probe function.

> 
> > > Is there any guarantee that the notifiers for attach and detach
> > > are serialized?
> > >
> >
> > Sorry I didn't get this part.
> 
> I think you answered this above: if the i3c code is locked while calling
> the notifier, this cannot happen.
> 

The i3c code is only locked during the i3c_for_each_dev().

> > > > +/**
> > > > + * struct i3c_ioc_priv_xfer - I3C SDR ioctl private transfer
> > > > + * @data: Holds pointer to userspace buffer with transmit data.
> > > > + * @len: Length of data buffer buffers, in bytes.
> > > > + * @rnw: encodes the transfer direction. true for a read, false for a write
> > > > + */
> > > > +struct i3c_ioc_priv_xfer {
> > > > +       __u64 data;
> > > > +       __u16 len;
> > > > +       __u8 rnw;
> > > > +       __u8 pad[5];
> > > > +};
> > > > +
> > > > +
> > > > +#define I3C_PRIV_XFER_SIZE(N)  \
> > > > +       ((((sizeof(struct i3c_ioc_priv_xfer)) * (N)) < (1 << _IOC_SIZEBITS)) \
> > > > +       ? ((sizeof(struct i3c_ioc_priv_xfer)) * (N)) : 0)
> > > > +
> > > > +#define I3C_IOC_PRIV_XFER(N)   \
> > > > +       _IOC(_IOC_READ|_IOC_WRITE, I3C_DEV_IOC_MAGIC, 30, I3C_PRIV_XFER_SIZE(N))
> > >
> > > This looks like a reasonable ioctl definition, avoiding the usual problems
> > > with compat mode etc.
> >
> > Do you think I should add more reserved fields for future?
> 
> No, what I meant is that I like it the way it is.
> 
>      Arnd

Best regards,
Vitor Soares

Re: [RFC v2 0/4] Introduce i3c device userspace interface

[Boris Brezillon]

Hello Vitor,

Sorry for taking so long to reply, and thanks for working on that topic.

On Wed, 29 Jan 2020 13:17:31 +0100
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> For today there is no way to use i3c devices from user space and
> the introduction of such API will help developers during the i3c device
> or i3c host controllers development.
> 
> The i3cdev module is highly based on i2c-dev and yet I tried to address
> the concerns raised in [1].
> 
> NOTES:
> - The i3cdev dynamically request an unused major number.
> 
> - The i3c devices are dynamically exposed/removed from dev/ folder based
>   on if they have a device driver bound to it.

May I ask why you need to automatically bind devices to the i3cdev
driver when they don't have a driver matching the device id
loaded/compiled-in? If we get the i3c subsystem to generate proper
uevents we should be able to load the i3cdev module and bind the device
to this driver using a udev rule.

Regards,

Boris

Re: [RFC v2 1/4] i3c: master: export i3c_masterdev_type

[Boris Brezillon]

On Wed, 29 Jan 2020 13:17:32 +0100
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> Exporte i3c_masterdev_type so i3cdev module can verify if an i3c device
> is a master.
> 
> Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> ---
>  drivers/i3c/internals.h | 1 +
>  drivers/i3c/master.c    | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> index 86b7b44..bc062e8 100644
> --- a/drivers/i3c/internals.h
> +++ b/drivers/i3c/internals.h
> @@ -11,6 +11,7 @@
>  #include <linux/i3c/master.h>
>  
>  extern struct bus_type i3c_bus_type;
> +extern const struct device_type i3c_masterdev_type;
>  
>  void i3c_bus_normaluse_lock(struct i3c_bus *bus);
>  void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 7f8f896..8a0ba34 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -523,9 +523,10 @@ static void i3c_masterdev_release(struct device *dev)
>  	of_node_put(dev->of_node);
>  }
>  
> -static const struct device_type i3c_masterdev_type = {
> +const struct device_type i3c_masterdev_type = {
>  	.groups	= i3c_masterdev_groups,
>  };
> +EXPORT_SYMBOL_GPL(i3c_masterdev_type);

No need to export the symbol, removing the static and adding the
definition to internal.h should work just fine (i3c.o contains
both master.o and device.o).

>  
>  static int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode,
>  			    unsigned long max_i2c_scl_rate)

Re: [RFC v2 1/4] i3c: master: export i3c_masterdev_type

[Boris Brezillon]

On Mon, 17 Feb 2020 15:56:23 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Wed, 29 Jan 2020 13:17:32 +0100
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > Exporte i3c_masterdev_type so i3cdev module can verify if an i3c device
> > is a master.
> > 
> > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > ---
> >  drivers/i3c/internals.h | 1 +
> >  drivers/i3c/master.c    | 3 ++-
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> > index 86b7b44..bc062e8 100644
> > --- a/drivers/i3c/internals.h
> > +++ b/drivers/i3c/internals.h
> > @@ -11,6 +11,7 @@
> >  #include <linux/i3c/master.h>
> >  
> >  extern struct bus_type i3c_bus_type;
> > +extern const struct device_type i3c_masterdev_type;
> >  
> >  void i3c_bus_normaluse_lock(struct i3c_bus *bus);
> >  void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index 7f8f896..8a0ba34 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -523,9 +523,10 @@ static void i3c_masterdev_release(struct device *dev)
> >  	of_node_put(dev->of_node);
> >  }
> >  
> > -static const struct device_type i3c_masterdev_type = {
> > +const struct device_type i3c_masterdev_type = {
> >  	.groups	= i3c_masterdev_groups,
> >  };
> > +EXPORT_SYMBOL_GPL(i3c_masterdev_type);  
> 
> No need to export the symbol, removing the static and adding the
> definition to internal.h should work just fine (i3c.o contains
> both master.o and device.o).

Hm, my bad. Looks like i3cdev is a separate module/driver. If that's
the case, it should not have direct access to internals.h. I see 2
options here:

1/ make the i3cdev logic part of the core
2/ provide helpers to find devices by type

But maybe none of that is needed if you let userspace bind i3c devices
to the i3cdev driver.

> 
> >  
> >  static int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode,
> >  			    unsigned long max_i2c_scl_rate)  
> 

Re: [RFC v2 0/4] Introduce i3c device userspace interface

[Arnd Bergmann]

On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
<boris.brezillon@collabora.com> wrote:
> Sorry for taking so long to reply, and thanks for working on that topic.
>
> On Wed, 29 Jan 2020 13:17:31 +0100
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
>
> > For today there is no way to use i3c devices from user space and
> > the introduction of such API will help developers during the i3c device
> > or i3c host controllers development.
> >
> > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > the concerns raised in [1].
> >
> > NOTES:
> > - The i3cdev dynamically request an unused major number.
> >
> > - The i3c devices are dynamically exposed/removed from dev/ folder based
> >   on if they have a device driver bound to it.
>
> May I ask why you need to automatically bind devices to the i3cdev
> driver when they don't have a driver matching the device id
> loaded/compiled-in? If we get the i3c subsystem to generate proper
> uevents we should be able to load the i3cdev module and bind the device
> to this driver using a udev rule.

I think that would require manual configuration to ensure that the correct
set of devices get bound to either the userspace driver or an in-kernel
driver. The method from the current patch series is more complicated,
but it means that any device can be accessed by the user space driver
as long as it's not already owned by a kernel driver.

     Arnd

Re: [RFC v2 4/4] i3c: add i3cdev module to expose i3c dev in /dev

[Boris Brezillon]

On Wed, 29 Jan 2020 13:17:35 +0100
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> diff --git a/include/uapi/linux/i3c/i3cdev.h b/include/uapi/linux/i3c/i3cdev.h
> new file mode 100644
> index 0000000..0897313
> --- /dev/null
> +++ b/include/uapi/linux/i3c/i3cdev.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Copyright (c) 2019 Synopsys, Inc. and/or its affiliates.
> + *
> + * Author: Vitor Soares <vitor.soares@synopsys.com>
> + */
> +
> +#ifndef _UAPI_I3C_DEV_H_
> +#define _UAPI_I3C_DEV_H_
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +/* IOCTL commands */
> +#define I3C_DEV_IOC_MAGIC	0x07

I guess you already made sure there was no collision with other magic
values.

> +
> +/**
> + * struct i3c_ioc_priv_xfer - I3C SDR ioctl private transfer
> + * @data: Holds pointer to userspace buffer with transmit data.
> + * @len: Length of data buffer buffers, in bytes.
> + * @rnw: encodes the transfer direction. true for a read, false for a write
> + */
> +struct i3c_ioc_priv_xfer {
> +	__u64 data;
> +	__u16 len;
> +	__u8 rnw;
> +	__u8 pad[5];
> +};
> +
> +
> +#define I3C_PRIV_XFER_SIZE(N)	\
> +	((((sizeof(struct i3c_ioc_priv_xfer)) * (N)) < (1 << _IOC_SIZEBITS)) \
> +	? ((sizeof(struct i3c_ioc_priv_xfer)) * (N)) : 0)
> +
> +#define I3C_IOC_PRIV_XFER(N)	\
> +	_IOC(_IOC_READ|_IOC_WRITE, I3C_DEV_IOC_MAGIC, 30, I3C_PRIV_XFER_SIZE(N))

Any reason for starting at 30 instead of 0x0 or 0x1?

Also, this ioctl definition is a bit unusual. Most of the time, when we
want to pass an array of elements we pass a struct that contains the
number of entries in this array, and a pointer to the array itself.

struct i3cdev_priv_xfers {
	__u64 nxfers;
	__u64 xfers; /*Use u64_to_user_ptr() to get the __user pointer*/
};

> +
> +#endif

RE: [RFC v2 0/4] Introduce i3c device userspace interface

[Vitor Soares]

Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Mon, Feb 17, 2020 at 14:51:41

> Hello Vitor,
> 
> Sorry for taking so long to reply, and thanks for working on that topic.
> 
> On Wed, 29 Jan 2020 13:17:31 +0100
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > For today there is no way to use i3c devices from user space and
> > the introduction of such API will help developers during the i3c device
> > or i3c host controllers development.
> > 
> > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > the concerns raised in [1].
> > 
> > NOTES:
> > - The i3cdev dynamically request an unused major number.
> > 
> > - The i3c devices are dynamically exposed/removed from dev/ folder based
> >   on if they have a device driver bound to it.
> 
> May I ask why you need to automatically bind devices to the i3cdev
> driver when they don't have a driver matching the device id
> loaded/compiled-in? If we get the i3c subsystem to generate proper
> uevents we should be able to load the i3cdev module and bind the device
> to this driver using a udev rule.

My idea was to expose every device to user-space by default so we can 
test them without a driver (more or less the i2c use case) but as we 
agreed during the i3c subsystem only expose devices that doesn't have 
device driver.
I considered to have a uevent but to expose the devices by default it 
would required something generic, what I didn't figure out and tend to 
follow the i2c-dev module.

With this current approach even if a device has a driver we can unbind it 
through the Sysfs and have access from user space which I found useful 
for debug.

> 
> Regards,
> 
> Boris

Re: [RFC v2 0/4] Introduce i3c device userspace interface

[Boris Brezillon]

On Mon, 17 Feb 2020 16:06:45 +0100
Arnd Bergmann <arnd@arndb.de> wrote:

> On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> > Sorry for taking so long to reply, and thanks for working on that topic.
> >
> > On Wed, 29 Jan 2020 13:17:31 +0100
> > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> >  
> > > For today there is no way to use i3c devices from user space and
> > > the introduction of such API will help developers during the i3c device
> > > or i3c host controllers development.
> > >
> > > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > > the concerns raised in [1].
> > >
> > > NOTES:
> > > - The i3cdev dynamically request an unused major number.
> > >
> > > - The i3c devices are dynamically exposed/removed from dev/ folder based
> > >   on if they have a device driver bound to it.  
> >
> > May I ask why you need to automatically bind devices to the i3cdev
> > driver when they don't have a driver matching the device id
> > loaded/compiled-in? If we get the i3c subsystem to generate proper
> > uevents we should be able to load the i3cdev module and bind the device
> > to this driver using a udev rule.  
> 
> I think that would require manual configuration to ensure that the correct
> set of devices get bound to either the userspace driver or an in-kernel
> driver.

Hm, isn't that what udev is supposed to do anyway? Remember that
I3C devices expose a manufacturer and part-id (which are similar to the
USB vendor and product ids), so deciding when an I3C device should be
bound to the i3cdev driver should be fairly easy, and that's a
per-device decision anyway.

> The method from the current patch series is more complicated,
> but it means that any device can be accessed by the user space driver
> as long as it's not already owned by a kernel driver.

Well, I'm more worried about the extra churn this auto-binding logic
might create for the common 'on-demand driver loading' use case. At
first, there's no driver matching a specific device, but userspace
might load one based on the uevents it receives. With the current
approach, that means we'd first have to unbind the device before
loading the driver. AFAICT, no other subsystem does that.

Re: [RFC v2 0/4] Introduce i3c device userspace interface

[Boris Brezillon]

On Mon, 17 Feb 2020 15:32:55 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> Hi Boris,
> 
> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Mon, Feb 17, 2020 at 14:51:41
> 
> > Hello Vitor,
> > 
> > Sorry for taking so long to reply, and thanks for working on that topic.
> > 
> > On Wed, 29 Jan 2020 13:17:31 +0100
> > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> >   
> > > For today there is no way to use i3c devices from user space and
> > > the introduction of such API will help developers during the i3c device
> > > or i3c host controllers development.
> > > 
> > > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > > the concerns raised in [1].
> > > 
> > > NOTES:
> > > - The i3cdev dynamically request an unused major number.
> > > 
> > > - The i3c devices are dynamically exposed/removed from dev/ folder based
> > >   on if they have a device driver bound to it.  
> > 
> > May I ask why you need to automatically bind devices to the i3cdev
> > driver when they don't have a driver matching the device id
> > loaded/compiled-in? If we get the i3c subsystem to generate proper
> > uevents we should be able to load the i3cdev module and bind the device
> > to this driver using a udev rule.  
> 
> My idea was to expose every device to user-space by default so we can 
> test them without a driver (more or less the i2c use case) but as we 
> agreed during the i3c subsystem only expose devices that doesn't have 
> device driver.

Those that do not have a driver *yet*. What if i3cdev is compiled-in
and other I3C drivers are enabled as modules? When the device is
discovered at boot time is will be automatically bound to the i3cdev
driver since no matching drivers are available at that point. But
the end user probably expects this device to be attached to the in
kernel driver.

> I considered to have a uevent but to expose the devices by default it 
> would required something generic, what I didn't figure out and tend to 
> follow the i2c-dev module.

Well, I3C and I2C/SPI are quite different in this regard. I2C dev
exposes the whole bus, and SPI devs don't have a standard way to
uniquely identify the device connected on the bus (unless it has a
dedicated compatible for DT-based boards). In that case it might make
sense to auto-bind all orphan devs to the default spidev driver, though
I'm not entirely sure it's really necessary since that's probably a
per-board decision, and having a udev rule matching the bus/CS would
make sense too.

> 
> With this current approach even if a device has a driver we can unbind it 
> through the Sysfs and have access from user space which I found useful 
> for debug.

That's my point. We're making the default 'on-demand driver loading'
use case more complicated to ease the less common 'userspace driver'
use case.

RE: [RFC v2 0/4] Introduce i3c device userspace interface

[Vitor Soares]

Hi,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Mon, Feb 17, 2020 at 15:36:22

> On Mon, 17 Feb 2020 16:06:45 +0100
> Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
> > <boris.brezillon@collabora.com> wrote:
> > > Sorry for taking so long to reply, and thanks for working on that topic.
> > >
> > > On Wed, 29 Jan 2020 13:17:31 +0100
> > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > >  
> > > > For today there is no way to use i3c devices from user space and
> > > > the introduction of such API will help developers during the i3c device
> > > > or i3c host controllers development.
> > > >
> > > > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > > > the concerns raised in [1].
> > > >
> > > > NOTES:
> > > > - The i3cdev dynamically request an unused major number.
> > > >
> > > > - The i3c devices are dynamically exposed/removed from dev/ folder based
> > > >   on if they have a device driver bound to it.  
> > >
> > > May I ask why you need to automatically bind devices to the i3cdev
> > > driver when they don't have a driver matching the device id
> > > loaded/compiled-in? If we get the i3c subsystem to generate proper
> > > uevents we should be able to load the i3cdev module and bind the device
> > > to this driver using a udev rule.  
> > 
> > I think that would require manual configuration to ensure that the correct
> > set of devices get bound to either the userspace driver or an in-kernel
> > driver.
> 
> Hm, isn't that what udev is supposed to do anyway? Remember that
> I3C devices expose a manufacturer and part-id (which are similar to the
> USB vendor and product ids), so deciding when an I3C device should be
> bound to the i3cdev driver should be fairly easy, and that's a
> per-device decision anyway.
> 
> > The method from the current patch series is more complicated,
> > but it means that any device can be accessed by the user space driver
> > as long as it's not already owned by a kernel driver.
> 
> Well, I'm more worried about the extra churn this auto-binding logic
> might create for the common 'on-demand driver loading' use case. At
> first, there's no driver matching a specific device, but userspace
> might load one based on the uevents it receives. With the current
> approach, that means we'd first have to unbind the device before
> loading the driver. AFAICT, no other subsystem does that.

I'm about to finish v3 (today or tomorrow) and I think I fixed all 
concerns rise during v2. I would like you to see that version before any 
change.

Best regards,
Vitor Soares

Re: [RFC v2 0/4] Introduce i3c device userspace interface

[gregkh]

On Mon, Feb 17, 2020 at 03:55:08PM +0000, Vitor Soares wrote:
> Hi,
> 
> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Mon, Feb 17, 2020 at 15:36:22
> 
> > On Mon, 17 Feb 2020 16:06:45 +0100
> > Arnd Bergmann <arnd@arndb.de> wrote:
> > 
> > > On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
> > > <boris.brezillon@collabora.com> wrote:
> > > > Sorry for taking so long to reply, and thanks for working on that topic.
> > > >
> > > > On Wed, 29 Jan 2020 13:17:31 +0100
> > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > >  
> > > > > For today there is no way to use i3c devices from user space and
> > > > > the introduction of such API will help developers during the i3c device
> > > > > or i3c host controllers development.
> > > > >
> > > > > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > > > > the concerns raised in [1].
> > > > >
> > > > > NOTES:
> > > > > - The i3cdev dynamically request an unused major number.
> > > > >
> > > > > - The i3c devices are dynamically exposed/removed from dev/ folder based
> > > > >   on if they have a device driver bound to it.  
> > > >
> > > > May I ask why you need to automatically bind devices to the i3cdev
> > > > driver when they don't have a driver matching the device id
> > > > loaded/compiled-in? If we get the i3c subsystem to generate proper
> > > > uevents we should be able to load the i3cdev module and bind the device
> > > > to this driver using a udev rule.  
> > > 
> > > I think that would require manual configuration to ensure that the correct
> > > set of devices get bound to either the userspace driver or an in-kernel
> > > driver.
> > 
> > Hm, isn't that what udev is supposed to do anyway? Remember that
> > I3C devices expose a manufacturer and part-id (which are similar to the
> > USB vendor and product ids), so deciding when an I3C device should be
> > bound to the i3cdev driver should be fairly easy, and that's a
> > per-device decision anyway.
> > 
> > > The method from the current patch series is more complicated,
> > > but it means that any device can be accessed by the user space driver
> > > as long as it's not already owned by a kernel driver.
> > 
> > Well, I'm more worried about the extra churn this auto-binding logic
> > might create for the common 'on-demand driver loading' use case. At
> > first, there's no driver matching a specific device, but userspace
> > might load one based on the uevents it receives. With the current
> > approach, that means we'd first have to unbind the device before
> > loading the driver. AFAICT, no other subsystem does that.
> 
> I'm about to finish v3 (today or tomorrow) and I think I fixed all 
> concerns rise during v2. I would like you to see that version before any 
> change.

Why are there so many "RFC" series here?  I treat "RFC" as "I don't
really like this patch but I'm throwing it out there for others to look
at if they care".  No RFC should ever go on beyond a v1 as obviously you
think this is good enough by now, right?

Also, I almost never review RFC patches, we have enough "real" patches
to review as it is :)

thanks,

greg k-h

RE: [RFC v2 0/4] Introduce i3c device userspace interface

[Vitor Soares]

From: gregkh <gregkh@linuxfoundation.org>
Date: Mon, Feb 17, 2020 at 16:03:45

> On Mon, Feb 17, 2020 at 03:55:08PM +0000, Vitor Soares wrote:
> > Hi,
> > 
> > From: Boris Brezillon <boris.brezillon@collabora.com>
> > Date: Mon, Feb 17, 2020 at 15:36:22
> > 
> > > On Mon, 17 Feb 2020 16:06:45 +0100
> > > Arnd Bergmann <arnd@arndb.de> wrote:
> > > 
> > > > On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
> > > > <boris.brezillon@collabora.com> wrote:
> > > > > Sorry for taking so long to reply, and thanks for working on that topic.
> > > > >
> > > > > On Wed, 29 Jan 2020 13:17:31 +0100
> > > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > > >  
> > > > > > For today there is no way to use i3c devices from user space and
> > > > > > the introduction of such API will help developers during the i3c device
> > > > > > or i3c host controllers development.
> > > > > >
> > > > > > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > > > > > the concerns raised in [1].
> > > > > >
> > > > > > NOTES:
> > > > > > - The i3cdev dynamically request an unused major number.
> > > > > >
> > > > > > - The i3c devices are dynamically exposed/removed from dev/ folder based
> > > > > >   on if they have a device driver bound to it.  
> > > > >
> > > > > May I ask why you need to automatically bind devices to the i3cdev
> > > > > driver when they don't have a driver matching the device id
> > > > > loaded/compiled-in? If we get the i3c subsystem to generate proper
> > > > > uevents we should be able to load the i3cdev module and bind the device
> > > > > to this driver using a udev rule.  
> > > > 
> > > > I think that would require manual configuration to ensure that the correct
> > > > set of devices get bound to either the userspace driver or an in-kernel
> > > > driver.
> > > 
> > > Hm, isn't that what udev is supposed to do anyway? Remember that
> > > I3C devices expose a manufacturer and part-id (which are similar to the
> > > USB vendor and product ids), so deciding when an I3C device should be
> > > bound to the i3cdev driver should be fairly easy, and that's a
> > > per-device decision anyway.
> > > 
> > > > The method from the current patch series is more complicated,
> > > > but it means that any device can be accessed by the user space driver
> > > > as long as it's not already owned by a kernel driver.
> > > 
> > > Well, I'm more worried about the extra churn this auto-binding logic
> > > might create for the common 'on-demand driver loading' use case. At
> > > first, there's no driver matching a specific device, but userspace
> > > might load one based on the uevents it receives. With the current
> > > approach, that means we'd first have to unbind the device before
> > > loading the driver. AFAICT, no other subsystem does that.
> > 
> > I'm about to finish v3 (today or tomorrow) and I think I fixed all 
> > concerns rise during v2. I would like you to see that version before any 
> > change.
> 
> Why are there so many "RFC" series here?  I treat "RFC" as "I don't
> really like this patch but I'm throwing it out there for others to look
> at if they care".  No RFC should ever go on beyond a v1 

My bad ☹.

> as obviously you
> think this is good enough by now, right?

Yes and I really appreciate the feedback that leads me to this v3.

> 
> Also, I almost never review RFC patches, we have enough "real" patches
> to review as it is :)
> 
> thanks,
> 
> greg k-h

Thanks for your advice.


Best regards,
Vitor Soares

Re: [RFC v2 0/4] Introduce i3c device userspace interface

[Arnd Bergmann]

On Mon, Feb 17, 2020 at 4:36 PM Boris Brezillon
<boris.brezillon@collabora.com> wrote:
> On Mon, 17 Feb 2020 16:06:45 +0100 Arnd Bergmann <arnd@arndb.de> wrote:
> > On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
> > <boris.brezillon@collabora.com> wrote:
> > > Sorry for taking so long to reply, and thanks for working on that topic.
> > >
> > > On Wed, 29 Jan 2020 13:17:31 +0100
> > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > >
> > > > For today there is no way to use i3c devices from user space and
> > > > the introduction of such API will help developers during the i3c device
> > > > or i3c host controllers development.
> > > >
> > > > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > > > the concerns raised in [1].
> > > >
> > > > NOTES:
> > > > - The i3cdev dynamically request an unused major number.
> > > >
> > > > - The i3c devices are dynamically exposed/removed from dev/ folder based
> > > >   on if they have a device driver bound to it.
> > >
> > > May I ask why you need to automatically bind devices to the i3cdev
> > > driver when they don't have a driver matching the device id
> > > loaded/compiled-in? If we get the i3c subsystem to generate proper
> > > uevents we should be able to load the i3cdev module and bind the device
> > > to this driver using a udev rule.
> >
> > I think that would require manual configuration to ensure that the correct
> > set of devices get bound to either the userspace driver or an in-kernel
> > driver.
>
> Hm, isn't that what udev is supposed to do anyway? Remember that
> I3C devices expose a manufacturer and part-id (which are similar to the
> USB vendor and product ids), so deciding when an I3C device should be
> bound to the i3cdev driver should be fairly easy, and that's a
> per-device decision anyway.
>
> > The method from the current patch series is more complicated,
> > but it means that any device can be accessed by the user space driver
> > as long as it's not already owned by a kernel driver.
>
> Well, I'm more worried about the extra churn this auto-binding logic
> might create for the common 'on-demand driver loading' use case. At
> first, there's no driver matching a specific device, but userspace
> might load one based on the uevents it receives. With the current
> approach, that means we'd first have to unbind the device before
> loading the driver. AFAICT, no other subsystem does that.

As I understand it, this is handled by the patches: when a new device
shows up, this triggers the creation of the userspace interface and
also the event that leads to the kernel driver to get loaded. If there
is a kernel driver for the device, that should still load and bind to the
device, at which point the user space interface will go away again.

This may waste CPU cycles for first creating and then destroying
the user space interface, but I don't see how it requires extra work.
If it does require manual configuration or unbinding, that would
indeed be a bad design.

      Arnd

Re: [RFC v2 0/4] Introduce i3c device userspace interface

[Boris Brezillon]

On Mon, 17 Feb 2020 15:55:08 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> Hi,
> 
> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Mon, Feb 17, 2020 at 15:36:22
> 
> > On Mon, 17 Feb 2020 16:06:45 +0100
> > Arnd Bergmann <arnd@arndb.de> wrote:
> >   
> > > On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
> > > <boris.brezillon@collabora.com> wrote:  
> > > > Sorry for taking so long to reply, and thanks for working on that topic.
> > > >
> > > > On Wed, 29 Jan 2020 13:17:31 +0100
> > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > >    
> > > > > For today there is no way to use i3c devices from user space and
> > > > > the introduction of such API will help developers during the i3c device
> > > > > or i3c host controllers development.
> > > > >
> > > > > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > > > > the concerns raised in [1].
> > > > >
> > > > > NOTES:
> > > > > - The i3cdev dynamically request an unused major number.
> > > > >
> > > > > - The i3c devices are dynamically exposed/removed from dev/ folder based
> > > > >   on if they have a device driver bound to it.    
> > > >
> > > > May I ask why you need to automatically bind devices to the i3cdev
> > > > driver when they don't have a driver matching the device id
> > > > loaded/compiled-in? If we get the i3c subsystem to generate proper
> > > > uevents we should be able to load the i3cdev module and bind the device
> > > > to this driver using a udev rule.    
> > > 
> > > I think that would require manual configuration to ensure that the correct
> > > set of devices get bound to either the userspace driver or an in-kernel
> > > driver.  
> > 
> > Hm, isn't that what udev is supposed to do anyway? Remember that
> > I3C devices expose a manufacturer and part-id (which are similar to the
> > USB vendor and product ids), so deciding when an I3C device should be
> > bound to the i3cdev driver should be fairly easy, and that's a
> > per-device decision anyway.
> >   
> > > The method from the current patch series is more complicated,
> > > but it means that any device can be accessed by the user space driver
> > > as long as it's not already owned by a kernel driver.  
> > 
> > Well, I'm more worried about the extra churn this auto-binding logic
> > might create for the common 'on-demand driver loading' use case. At
> > first, there's no driver matching a specific device, but userspace
> > might load one based on the uevents it receives. With the current
> > approach, that means we'd first have to unbind the device before
> > loading the driver. AFAICT, no other subsystem does that.  

Okay, I have clearly not read the code carefully enough. I thought you
were declaring a new i3c_device_driver and were manually binding all
orphan devices to this driver. Looks like the solution is more subtle
than that, and i3cdevs are actually subdevices that are automatically
created/removed when the I3C device is unbound/bound. That means the
'on-demand driver loading' logic is not impacted by this new layer. I'm
still not convinced this is needed (I expect i3cdev to be used mostly
for experiment, and in that case, having a udev rule, or manually
binding the device to the i3cdev driver shouldn't be a problem). I'm
also not sure what happens if the device is still used when
i3cdev_detach() is called, can transfers still be done after the device
is attached to its in-kernel driver?

Re: [RFC v2 0/4] Introduce i3c device userspace interface

[Arnd Bergmann]

On Mon, Feb 17, 2020 at 5:23 PM Boris Brezillon
<boris.brezillon@collabora.com> wrote:
> On Mon, 17 Feb 2020 15:55:08 +0000 Vitor Soares <Vitor.Soares@synopsys.com> wrote:
>
> Okay, I have clearly not read the code carefully enough. I thought you
> were declaring a new i3c_device_driver and were manually binding all
> orphan devices to this driver. Looks like the solution is more subtle
> than that, and i3cdevs are actually subdevices that are automatically
> created/removed when the I3C device is unbound/bound. That means the
> 'on-demand driver loading' logic is not impacted by this new layer. I'm
> still not convinced this is needed (I expect i3cdev to be used mostly
> for experiment, and in that case, having a udev rule, or manually
> binding the device to the i3cdev driver shouldn't be a problem).

I'm fairly sure it's not needed, other approaches could be used to
provide user space access, but it's not clear if any other way is
better either. It also took me a while to figure out what is going on
when I read the code.

One thought that I had was that this could be better integrated into
the core, with user space being there implicitly through sysfs rather
than a /dev file.

> I'm also not sure what happens if the device is still used when
> i3cdev_detach() is called, can transfers still be done after the device
> is attached to its in-kernel driver?

I think this is still an open issue that I also pointed out. The driver
binding/unbinding and user space access definitely needs to
be properly serialized, whichever method is used to implement the
user access.

       Arnd

Re: [RFC v2 0/4] Introduce i3c device userspace interface

[Boris Brezillon]

On Mon, 17 Feb 2020 17:19:57 +0100
Arnd Bergmann <arnd@arndb.de> wrote:

> On Mon, Feb 17, 2020 at 4:36 PM Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> > On Mon, 17 Feb 2020 16:06:45 +0100 Arnd Bergmann <arnd@arndb.de> wrote:  
> > > On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
> > > <boris.brezillon@collabora.com> wrote:  
> > > > Sorry for taking so long to reply, and thanks for working on that topic.
> > > >
> > > > On Wed, 29 Jan 2020 13:17:31 +0100
> > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > >  
> > > > > For today there is no way to use i3c devices from user space and
> > > > > the introduction of such API will help developers during the i3c device
> > > > > or i3c host controllers development.
> > > > >
> > > > > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > > > > the concerns raised in [1].
> > > > >
> > > > > NOTES:
> > > > > - The i3cdev dynamically request an unused major number.
> > > > >
> > > > > - The i3c devices are dynamically exposed/removed from dev/ folder based
> > > > >   on if they have a device driver bound to it.  
> > > >
> > > > May I ask why you need to automatically bind devices to the i3cdev
> > > > driver when they don't have a driver matching the device id
> > > > loaded/compiled-in? If we get the i3c subsystem to generate proper
> > > > uevents we should be able to load the i3cdev module and bind the device
> > > > to this driver using a udev rule.  
> > >
> > > I think that would require manual configuration to ensure that the correct
> > > set of devices get bound to either the userspace driver or an in-kernel
> > > driver.  
> >
> > Hm, isn't that what udev is supposed to do anyway? Remember that
> > I3C devices expose a manufacturer and part-id (which are similar to the
> > USB vendor and product ids), so deciding when an I3C device should be
> > bound to the i3cdev driver should be fairly easy, and that's a
> > per-device decision anyway.
> >  
> > > The method from the current patch series is more complicated,
> > > but it means that any device can be accessed by the user space driver
> > > as long as it's not already owned by a kernel driver.  
> >
> > Well, I'm more worried about the extra churn this auto-binding logic
> > might create for the common 'on-demand driver loading' use case. At
> > first, there's no driver matching a specific device, but userspace
> > might load one based on the uevents it receives. With the current
> > approach, that means we'd first have to unbind the device before
> > loading the driver. AFAICT, no other subsystem does that.  
> 
> As I understand it, this is handled by the patches: when a new device
> shows up, this triggers the creation of the userspace interface and
> also the event that leads to the kernel driver to get loaded. If there
> is a kernel driver for the device, that should still load and bind to the
> device, at which point the user space interface will go away again.

Yep, that's what I figured after having a closer look at the code.

> 
> This may waste CPU cycles for first creating and then destroying
> the user space interface, but I don't see how it requires extra work.
> If it does require manual configuration or unbinding, that would
> indeed be a bad design.

To be honest, I had something less invasive in mind. Something closer
to what spidev provides (a driver that can expose I3C devices to
userspace when explicitly requested). I see now that the USB subsystem
does something similar to what's done here, but I'm wondering if it's
really worth it in the I3C case. As I said in my previous reply, I
expect i3cdev to be used when experimenting or when kernel-space driver
is not an option (licensing/security issues).

Re: [RFC v2 0/4] Introduce i3c device userspace interface

[Boris Brezillon]

On Mon, 17 Feb 2020 17:31:08 +0100
Arnd Bergmann <arnd@arndb.de> wrote:

> On Mon, Feb 17, 2020 at 5:23 PM Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> > On Mon, 17 Feb 2020 15:55:08 +0000 Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> >
> > Okay, I have clearly not read the code carefully enough. I thought you
> > were declaring a new i3c_device_driver and were manually binding all
> > orphan devices to this driver. Looks like the solution is more subtle
> > than that, and i3cdevs are actually subdevices that are automatically
> > created/removed when the I3C device is unbound/bound. That means the
> > 'on-demand driver loading' logic is not impacted by this new layer. I'm
> > still not convinced this is needed (I expect i3cdev to be used mostly
> > for experiment, and in that case, having a udev rule, or manually
> > binding the device to the i3cdev driver shouldn't be a problem).  
> 
> I'm fairly sure it's not needed, other approaches could be used to
> provide user space access, but it's not clear if any other way is
> better either. It also took me a while to figure out what is going on
> when I read the code.
> 
> One thought that I had was that this could be better integrated into
> the core, with user space being there implicitly through sysfs rather
> than a /dev file.

Hm, doing I3C transfers through sysfs sounds a bit odd. sysfs entries
are most of the time exposing human readable/writable stuff (plain text
data, that is).

> 
> > I'm also not sure what happens if the device is still used when
> > i3cdev_detach() is called, can transfers still be done after the device
> > is attached to its in-kernel driver?  
> 
> I think this is still an open issue that I also pointed out. The driver
> binding/unbinding and user space access definitely needs to
> be properly serialized, whichever method is used to implement the
> user access.

Well, going for the spidev approach would solve that and make the
whole implementation more straightforward and less invasive (no
notifier, no need to expose internal device types to this i3cdev
driver, no concurrency issues, ...). We can always revisit this solution
if it proves to be to limited and we feel the need for a
kernel-driven i3cdev auto-binding logic, but I doubt we'll ever be
in that position since udev can handle that for us, and if we start
having actual userspace drivers (which is not the case yet), I expect
distros to add the relevant udev rules to take care of this auto-bind.

Re: [RFC v2 0/4] Introduce i3c device userspace interface

[Boris Brezillon]

On Mon, 17 Feb 2020 15:51:41 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> Hello Vitor,
> 
> Sorry for taking so long to reply, and thanks for working on that topic.
> 
> On Wed, 29 Jan 2020 13:17:31 +0100
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > For today there is no way to use i3c devices from user space and
> > the introduction of such API will help developers during the i3c device
> > or i3c host controllers development.
> > 
> > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > the concerns raised in [1].
> > 
> > NOTES:
> > - The i3cdev dynamically request an unused major number.
> > 
> > - The i3c devices are dynamically exposed/removed from dev/ folder based
> >   on if they have a device driver bound to it.  
> 
> May I ask why you need to automatically bind devices to the i3cdev
> driver when they don't have a driver matching the device id
> loaded/compiled-in? If we get the i3c subsystem to generate proper
> uevents we should be able to load the i3cdev module and bind the device
> to this driver using a udev rule.

Hm, looks like we already send a proper MODALIAS [1], so we can actually
implement this auto-bind to i3cdev in udev (fall back on i3cdev
load+bind if no matching module is found).

[1]https://elixir.bootlin.com/linux/latest/source/drivers/i3c/master.c#L269