[PATCH v3 0/5] Introduce i3c device userspace interface

[PATCH v3 0/5] 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/bus/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 v3:
  Use the xfer_lock to prevent device detach during ioctl call
  Expose i3cdev under /dev/bus/i3c/ folder like usb does
  Change NOTIFY_BOUND to NOTIFY_BIND, this allows the device detach occur
  before driver->probe call
  Avoid use of IS_ERR_OR_NULL
  Use u64_to_user_ptr instead of (void __user *)(uintptr_t) cast
  Allocate k_xfer and data_ptrs at once and eliminate double allocation
  check
  Pass i3cdev to dev->driver_data
  Make all minors available
  Add API documentation

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

Vitor Soares (5):
  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
  Documentation: userspac-api: add i3cdev documentation

 Documentation/userspace-api/i3c/i3cdev.rst | 116 ++++++++
 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 +++
 7 files changed, 616 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/userspace-api/i3c/i3cdev.rst
 create mode 100644 drivers/i3c/i3cdev.c
 create mode 100644 include/uapi/linux/i3c/i3cdev.h

-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH v3 1/5] 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


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH v3 2/5] 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


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH v3 3/5] 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


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH v3 4/5] 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>
---
Changes in v3:
  Use the xfer_lock to prevent device detach during ioctl call
  Expose i3cdev under /dev/bus/i3c/ folder like usb does
  Change NOTIFY_BOUND to NOTIFY_BIND, this allows the device detach occur
  before driver->probe
  Avoid use of IS_ERR_OR_NULL
  Use u64_to_user_ptr instead of (void __user *)(uintptr_t) cast
  Allocate k_xfer and data_ptrs at once to eliminate duble allocation check
  Pass i3cdev to dev->driver_data
  Make all minors available

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

 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..1a6bd35
--- /dev/null
+++ b/drivers/i3c/i3cdev.c
@@ -0,0 +1,429 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020 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/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 i3c_device *i3c;
+	struct device *dev;
+	struct mutex xfer_lock; /* prevent detach while transferring */
+	struct cdev cdev;
+	int id;
+};
+
+static DEFINE_IDA(i3cdev_ida);
+static dev_t i3cdev_number;
+#define I3C_MINORS (MINORMASK + 1)
+
+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;
+	i3cdev_set_drvdata(i3c, i3cdev);
+
+	return i3cdev;
+}
+
+static void put_i3cdev(struct i3cdev_data *i3cdev)
+{
+	i3cdev_set_drvdata(i3cdev->i3c, NULL);
+	kfree(i3cdev);
+}
+
+static ssize_t
+i3cdev_read(struct file *file, char __user *buf, size_t count, loff_t *f_pos)
+{
+	struct i3cdev_data *i3cdev = file->private_data;
+	struct i3c_device *i3c = i3cdev->i3c;
+	struct i3c_priv_xfer xfers = {
+		.rnw = true,
+		.len = count,
+	};
+	int ret = -EACCES;
+	char *tmp;
+
+	mutex_lock(&i3cdev->xfer_lock);
+	if (i3c->dev.driver)
+		goto err_out;
+
+	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, xfers.len) ? -EFAULT : xfers.len;
+
+	kfree(tmp);
+
+err_out:
+	mutex_unlock(&i3cdev->xfer_lock);
+	return ret;
+}
+
+static ssize_t
+i3cdev_write(struct file *file, const char __user *buf, size_t count,
+	     loff_t *f_pos)
+{
+	struct i3cdev_data *i3cdev = file->private_data;
+	struct i3c_device *i3c = i3cdev->i3c;
+	struct i3c_priv_xfer xfers = {
+		.rnw = false,
+		.len = count,
+	};
+	int ret = -EACCES;
+	char *tmp;
+
+	mutex_lock(&i3cdev->xfer_lock);
+	if (i3c->dev.driver)
+		goto err_out;
+
+	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);
+
+err_out:
+	mutex_unlock(&i3cdev->xfer_lock);
+	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;
+
+	/* Since we have nxfers we may allocate k_xfer + *data_ptrs together */
+	k_xfers = kcalloc(nxfers, sizeof(*k_xfers) + sizeof(*data_ptrs),
+			  GFP_KERNEL);
+	if (!k_xfers)
+		return -ENOMEM;
+
+	/* set data_ptrs to be after nxfers * i3c_priv_xfer */
+	data_ptrs = (void *)k_xfers + (nxfers * sizeof(*k_xfers));
+
+	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(u64_to_user_ptr(xfers[i].data),
+					 data_ptrs[i], xfers[i].len))
+				ret = -EFAULT;
+		}
+	}
+
+err_free_mem:
+	for (; i >= 0; i--)
+		kfree(data_ptrs[i]);
+	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 ERR_PTR(-EINVAL);
+
+	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(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 i3cdev_data *i3cdev = file->private_data;
+	struct i3c_device *i3c = i3cdev->i3c;
+	int ret = -EACCES;
+
+	dev_dbg(&i3c->dev, "ioctl, cmd=0x%02x, arg=0x%02lx\n", cmd, arg);
+
+	if (_IOC_TYPE(cmd) != I3C_DEV_IOC_MAGIC)
+		return -ENOTTY;
+
+	/* Use the xfer_lock to prevent device detach during ioctl call */
+	mutex_lock(&i3cdev->xfer_lock);
+	if (i3c->dev.driver)
+		goto err_no_dev;
+
+	/* Check command number and direction */
+	if (_IOC_NR(cmd) == _IOC_NR(I3C_IOC_PRIV_XFER(0)) &&
+	    _IOC_DIR(cmd) == (_IOC_READ | _IOC_WRITE))
+		ret = i3cdev_ioc_priv_xfer(i3c, cmd,
+					(struct i3c_ioc_priv_xfer __user *)arg);
+
+err_no_dev:
+	mutex_unlock(&i3cdev->xfer_lock);
+	return ret;
+}
+
+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;
+
+	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);
+
+	mutex_init(&i3cdev->xfer_lock);
+	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, "bus!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_drvdata(i3c);
+	if (!i3cdev)
+		return 0;
+
+	/* Prevent transfers while cdev removal */
+	mutex_lock(&i3cdev->xfer_lock);
+	cdev_del(&i3cdev->cdev);
+	device_destroy(i3cdev_class, MKDEV(MAJOR(i3cdev_number), i3cdev->id));
+	mutex_unlock(&i3cdev->xfer_lock);
+
+	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_BIND_DRIVER:
+		return i3cdev_detach(dev, NULL);
+	}
+
+	return 0;
+}
+
+static struct notifier_block i3cdev_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, &i3cdev_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, &i3cdev_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..656a686
--- /dev/null
+++ b/include/uapi/linux/i3c/i3cdev.h
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Copyright (c) 2020 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


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH v3 5/5] add i3cdev documentation

[Vitor Soares]

This patch add documentation for the userspace API of i3cdev module.

Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
---
 Documentation/userspace-api/i3c/i3cdev.rst | 116 +++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)
 create mode 100644 Documentation/userspace-api/i3c/i3cdev.rst

diff --git a/Documentation/userspace-api/i3c/i3cdev.rst b/Documentation/userspace-api/i3c/i3cdev.rst
new file mode 100644
index 0000000..ada269f
--- /dev/null
+++ b/Documentation/userspace-api/i3c/i3cdev.rst
@@ -0,0 +1,116 @@
+====================
+I3C Device Interface
+====================
+
+I3C devices have the flexibility of being accessed from userspace, as well
+through the conventional use of kernel drivers. Userspace access, although
+limited to private SDR I3C transfers, provides the advantage of simplifying
+the implementation of straightforward communication protocols, applicable to
+scenarios where transfers are dedicated such for sensor bring-up scenarios
+(prototyping environments) or for microcontroller slave communication
+implementation.
+
+The major device number is dynamically attributed and it's all reserved for
+the i3c devices. By default, the i3cdev module only exposes the i3c devices
+without device driver bind and aren't of master type in sort of character
+device file under /dev/bus/i3c/ folder. They are identified through its
+<bus id>-<Provisional ID> same way they can be found in /sys/bus/i3c/devices/.
+::
+
+# ls -l /dev/bus/i3c/
+total 0
+crw-------    1 root     root      248,   0 Jan  1 00:22 0-6072303904d2
+crw-------    1 root     root      248,   1 Jan  1 00:22 0-b7405ba00929
+
+The simplest way to use this interface is to not have an I3C device bound to
+a kernel driver, this can be achieved by not have the kernel driver loaded or
+using the Sysfs to unbind the kernel driver from the device.
+
+BASIC CHARACTER DEVICE API
+===============================
+For now, the API has only support private SDR read and write transfers.
+Those transaction can be achieved by the following:
+
+``read(file, buffer, sizeof(buffer))``
+  The standard read() operation will work as a simple transaction of private
+  SDR read data followed a stop.
+  Return the number of bytes read on success, and a negative error otherwise.
+
+``write(file, buffer, sizeof(buffer))``
+  The standard write() operation will work as a simple transaction of private
+  SDR write data followed a stop.
+  Return the number of bytes written on success, and a negative error otherwise.
+
+``ioctl(file, I3C_IOC_PRIV_XFER(nxfers), struct i3c_ioc_priv_xfer *xfers)``
+  It combines read/write transactions without a stop in between.
+  Return 0 on success, and a negative error otherwise.
+
+NOTES:
+  - According to the MIPI I3C Protocol is the I3C slave that terminates the read
+    transaction otherwise Master can abort early on ninth (T) data bit of each
+    SDR data word.
+
+  - Normal open() and close() operations on /dev/bus/i3c/<bus>-<provisional id>
+    files work as you would expect.
+
+  - As documented in cdev_del() if a device was already open during
+    i3cdev_detach, the read(), write() and ioctl() fops will still be callable
+    yet they will return -EACCES.
+
+C EXAMPLE
+=========
+Working with I3C devices is much like working with files. You will need to open
+a file descriptor, do some I/O operations with it, and then close it.
+
+The following header files should be included in an I3C program::
+
+#include <fcntl.h>
+#include <unistd.h>
+#include <sys/ioctl.h>
+#include <linux/types.h>
+#include <linux/i3c/i3cdev.h>
+
+To work with an I3C device, the application must open the driver, made
+available at the device node::
+
+  int file;
+
+  file = open("/dev/bus/i3c/0-6072303904d2", O_RDWR);
+  if (file < 0)
+  exit(1);
+
+Now the file is opened, we can perform the operations available::
+
+  /* Write function */
+  uint_t8  buf[] = {0x00, 0xde, 0xad, 0xbe, 0xef}
+  if (write(file, buf, 5) != 5) {
+    /* ERROR HANDLING: I3C transaction failed */
+  }
+
+  /*  Read function */
+  ret = read(file, buf, 5);
+  If (ret < 0) {
+    /* ERROR HANDLING: I3C transaction failed */
+  } else {
+    /* Iterate over buf[] to get the read data */
+  }
+
+  /* IOCTL function */
+  struct i3c_ioc_priv_xfer xfers[2];
+
+  uint8_t tx_buf[] = {0x00, 0xde, 0xad, 0xbe, 0xef};
+  uint8_t rx_buf[10];
+
+  xfers[0].data = (uintptr_t)tx_buf;
+  xfers[0].len = 5;
+  xfers[0].rnw = 0;
+  xfers[1].data = (uintptr_t)rx_buf;
+  xfers[1].len = 10;
+  xfers[1].rnw = 1;
+
+  if (ioctl(file, I3C_IOC_PRIV_XFER(2), xfers) < 0)
+    /* ERROR HANDLING: I3C transaction failed */
+
+The device can be closed when the open file descriptor is no longer required::
+
+  close(file);
\ No newline at end of file
-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH v3 0/5] Introduce i3c device userspace interface

[Vitor Soares]

Hi Boris,

From: Vitor Soares <vitor.soares@synopsys.com>
Date: Wed, Feb 19, 2020 at 00:20:38

> 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/bus/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 v3:
>   Use the xfer_lock to prevent device detach during ioctl call
>   Expose i3cdev under /dev/bus/i3c/ folder like usb does
>   Change NOTIFY_BOUND to NOTIFY_BIND, this allows the device detach occur
>   before driver->probe call
>   Avoid use of IS_ERR_OR_NULL
>   Use u64_to_user_ptr instead of (void __user *)(uintptr_t) cast
>   Allocate k_xfer and data_ptrs at once and eliminate double allocation
>   check
>   Pass i3cdev to dev->driver_data
>   Make all minors available
>   Add API documentation
> 
> Changes in v2:
>   Use IDR api for minor numbering
>   Modify ioctl struct
>   Fix SPDX license
> 
> Vitor Soares (5):
>   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
>   userspace-api: add i3cdev documentation
> 
>  Documentation/userspace-api/i3c/i3cdev.rst | 116 ++++++++
>  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 +++
>  7 files changed, 616 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/userspace-api/i3c/i3cdev.rst
>  create mode 100644 drivers/i3c/i3cdev.c
>  create mode 100644 include/uapi/linux/i3c/i3cdev.h
> 
> -- 
> 2.7.4

I want to make you know that none of your previous comments was ignored 
and  I would like to start the discussion from this point.

Best regards,
Vitor Soares

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH v3 5/5] add i3cdev documentation

[Vitor Soares]

Hi Jonathan,


From: Vitor Soares <vitor.soares@synopsys.com>
Date: Wed, Feb 19, 2020 at 00:20:43

> This patch add documentation for the userspace API of i3cdev module.
> 

I just realize now that I missed the commit message, it should be:
  userspace-api: i3c: add i3cdev documentation

I will do next time.

> Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> ---
>  Documentation/userspace-api/i3c/i3cdev.rst | 116 +++++++++++++++++++++++++++++
>  1 file changed, 116 insertions(+)
>  create mode 100644 Documentation/userspace-api/i3c/i3cdev.rst
> 
> diff --git a/Documentation/userspace-api/i3c/i3cdev.rst b/Documentation/userspace-api/i3c/i3cdev.rst
> new file mode 100644
> index 0000000..ada269f
> --- /dev/null
> +++ b/Documentation/userspace-api/i3c/i3cdev.rst
> @@ -0,0 +1,116 @@
> +====================
> +I3C Device Interface
> +====================
> +
> +I3C devices have the flexibility of being accessed from userspace, as well
> +through the conventional use of kernel drivers. Userspace access, although
> +limited to private SDR I3C transfers, provides the advantage of simplifying
> +the implementation of straightforward communication protocols, applicable to
> +scenarios where transfers are dedicated such for sensor bring-up scenarios
> +(prototyping environments) or for microcontroller slave communication
> +implementation.
> +
> +The major device number is dynamically attributed and it's all reserved for
> +the i3c devices. By default, the i3cdev module only exposes the i3c devices
> +without device driver bind and aren't of master type in sort of character
> +device file under /dev/bus/i3c/ folder. They are identified through its
> +<bus id>-<Provisional ID> same way they can be found in /sys/bus/i3c/devices/.
> +::
> +
> +# ls -l /dev/bus/i3c/
> +total 0
> +crw-------    1 root     root      248,   0 Jan  1 00:22 0-6072303904d2
> +crw-------    1 root     root      248,   1 Jan  1 00:22 0-b7405ba00929
> +
> +The simplest way to use this interface is to not have an I3C device bound to
> +a kernel driver, this can be achieved by not have the kernel driver loaded or
> +using the Sysfs to unbind the kernel driver from the device.
> +
> +BASIC CHARACTER DEVICE API
> +===============================
> +For now, the API has only support private SDR read and write transfers.
> +Those transaction can be achieved by the following:
> +
> +``read(file, buffer, sizeof(buffer))``
> +  The standard read() operation will work as a simple transaction of private
> +  SDR read data followed a stop.
> +  Return the number of bytes read on success, and a negative error otherwise.
> +
> +``write(file, buffer, sizeof(buffer))``
> +  The standard write() operation will work as a simple transaction of private
> +  SDR write data followed a stop.
> +  Return the number of bytes written on success, and a negative error otherwise.
> +
> +``ioctl(file, I3C_IOC_PRIV_XFER(nxfers), struct i3c_ioc_priv_xfer *xfers)``
> +  It combines read/write transactions without a stop in between.
> +  Return 0 on success, and a negative error otherwise.
> +
> +NOTES:
> +  - According to the MIPI I3C Protocol is the I3C slave that terminates the read
> +    transaction otherwise Master can abort early on ninth (T) data bit of each
> +    SDR data word.
> +
> +  - Normal open() and close() operations on /dev/bus/i3c/<bus>-<provisional id>
> +    files work as you would expect.
> +
> +  - As documented in cdev_del() if a device was already open during
> +    i3cdev_detach, the read(), write() and ioctl() fops will still be callable
> +    yet they will return -EACCES.
> +
> +C EXAMPLE
> +=========
> +Working with I3C devices is much like working with files. You will need to open
> +a file descriptor, do some I/O operations with it, and then close it.
> +
> +The following header files should be included in an I3C program::
> +
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <sys/ioctl.h>
> +#include <linux/types.h>
> +#include <linux/i3c/i3cdev.h>
> +
> +To work with an I3C device, the application must open the driver, made
> +available at the device node::
> +
> +  int file;
> +
> +  file = open("/dev/bus/i3c/0-6072303904d2", O_RDWR);
> +  if (file < 0)
> +  exit(1);
> +
> +Now the file is opened, we can perform the operations available::
> +
> +  /* Write function */
> +  uint_t8  buf[] = {0x00, 0xde, 0xad, 0xbe, 0xef}
> +  if (write(file, buf, 5) != 5) {
> +    /* ERROR HANDLING: I3C transaction failed */
> +  }
> +
> +  /*  Read function */
> +  ret = read(file, buf, 5);
> +  If (ret < 0) {
> +    /* ERROR HANDLING: I3C transaction failed */
> +  } else {
> +    /* Iterate over buf[] to get the read data */
> +  }
> +
> +  /* IOCTL function */
> +  struct i3c_ioc_priv_xfer xfers[2];
> +
> +  uint8_t tx_buf[] = {0x00, 0xde, 0xad, 0xbe, 0xef};
> +  uint8_t rx_buf[10];
> +
> +  xfers[0].data = (uintptr_t)tx_buf;
> +  xfers[0].len = 5;
> +  xfers[0].rnw = 0;
> +  xfers[1].data = (uintptr_t)rx_buf;
> +  xfers[1].len = 10;
> +  xfers[1].rnw = 1;
> +
> +  if (ioctl(file, I3C_IOC_PRIV_XFER(2), xfers) < 0)
> +    /* ERROR HANDLING: I3C transaction failed */
> +
> +The device can be closed when the open file descriptor is no longer required::
> +
> +  close(file);
> \ No newline at end of file
> -- 
> 2.7.4

Best regards,
Vitor Soares

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v3 5/5] add i3cdev documentation

[Randy Dunlap]

On 2/18/20 4:20 PM, Vitor Soares wrote:
> This patch add documentation for the userspace API of i3cdev module.
> 
> Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> ---
>  Documentation/userspace-api/i3c/i3cdev.rst | 116 +++++++++++++++++++++++++++++
>  1 file changed, 116 insertions(+)
>  create mode 100644 Documentation/userspace-api/i3c/i3cdev.rst
> 
> diff --git a/Documentation/userspace-api/i3c/i3cdev.rst b/Documentation/userspace-api/i3c/i3cdev.rst
> new file mode 100644
> index 0000000..ada269f
> --- /dev/null
> +++ b/Documentation/userspace-api/i3c/i3cdev.rst
> @@ -0,0 +1,116 @@
> +====================
> +I3C Device Interface
> +====================
> +
> +I3C devices have the flexibility of being accessed from userspace, as well
> +through the conventional use of kernel drivers. Userspace access, although
> +limited to private SDR I3C transfers, provides the advantage of simplifying
> +the implementation of straightforward communication protocols, applicable to
> +scenarios where transfers are dedicated such for sensor bring-up scenarios
> +(prototyping environments) or for microcontroller slave communication
> +implementation.
> +
> +The major device number is dynamically attributed and it's all reserved for

                                          allocated (?)

> +the i3c devices. By default, the i3cdev module only exposes the i3c devices

       I3C                                                         I3C

> +without device driver bind and aren't of master type in sort of character
> +device file under /dev/bus/i3c/ folder. They are identified through its

IMO:                              s/folder/directory/ or sub-directory

> +<bus id>-<Provisional ID> same way they can be found in /sys/bus/i3c/devices/.

                             in the same way

> +::
> +
> +# ls -l /dev/bus/i3c/
> +total 0
> +crw-------    1 root     root      248,   0 Jan  1 00:22 0-6072303904d2
> +crw-------    1 root     root      248,   1 Jan  1 00:22 0-b7405ba00929
> +
> +The simplest way to use this interface is to not have an I3C device bound to
> +a kernel driver, this can be achieved by not have the kernel driver loaded or

            driver. This                 by not having

> +using the Sysfs to unbind the kernel driver from the device.

         the sysfs interface to unbind

> +
> +BASIC CHARACTER DEVICE API
> +===============================
> +For now, the API has only support private SDR read and write transfers.

                        only support for private

For the unfamiliar, what is this "SDR"?  (thanks)

> +Those transaction can be achieved by the following:
> +
> +``read(file, buffer, sizeof(buffer))``
> +  The standard read() operation will work as a simple transaction of private
> +  SDR read data followed a stop.
> +  Return the number of bytes read on success, and a negative error otherwise.
> +
> +``write(file, buffer, sizeof(buffer))``
> +  The standard write() operation will work as a simple transaction of private
> +  SDR write data followed a stop.
> +  Return the number of bytes written on success, and a negative error otherwise.
> +
> +``ioctl(file, I3C_IOC_PRIV_XFER(nxfers), struct i3c_ioc_priv_xfer *xfers)``
> +  It combines read/write transactions without a stop in between.
> +  Return 0 on success, and a negative error otherwise.
> +
> +NOTES:
> +  - According to the MIPI I3C Protocol is the I3C slave that terminates the read

                                          it is the I3C slave

> +    transaction otherwise Master can abort early on ninth (T) data bit of each
> +    SDR data word.
> +
> +  - Normal open() and close() operations on /dev/bus/i3c/<bus>-<provisional id>
> +    files work as you would expect.
> +
> +  - As documented in cdev_del() if a device was already open during
> +    i3cdev_detach, the read(), write() and ioctl() fops will still be callable
> +    yet they will return -EACCES.
> +
> +C EXAMPLE
> +=========
> +Working with I3C devices is much like working with files. You will need to open
> +a file descriptor, do some I/O operations with it, and then close it.
> +
> +The following header files should be included in an I3C program::
> +
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <sys/ioctl.h>
> +#include <linux/types.h>
> +#include <linux/i3c/i3cdev.h>
> +
> +To work with an I3C device, the application must open the driver, made
> +available at the device node::
> +
> +  int file;
> +
> +  file = open("/dev/bus/i3c/0-6072303904d2", O_RDWR);
> +  if (file < 0)
> +  exit(1);

better indentation?

> +
> +Now the file is opened, we can perform the operations available::
> +
> +  /* Write function */
> +  uint_t8  buf[] = {0x00, 0xde, 0xad, 0xbe, 0xef}

I can't find uint_t8.  Where is it located?
and the braces should end with a ';'.

> +  if (write(file, buf, 5) != 5) {
> +    /* ERROR HANDLING: I3C transaction failed */
> +  }
> +
> +  /*  Read function */
> +  ret = read(file, buf, 5);
> +  If (ret < 0) {
> +    /* ERROR HANDLING: I3C transaction failed */
> +  } else {
> +    /* Iterate over buf[] to get the read data */
> +  }
> +
> +  /* IOCTL function */
> +  struct i3c_ioc_priv_xfer xfers[2];
> +
> +  uint8_t tx_buf[] = {0x00, 0xde, 0xad, 0xbe, 0xef};
> +  uint8_t rx_buf[10];
> +
> +  xfers[0].data = (uintptr_t)tx_buf;
> +  xfers[0].len = 5;
> +  xfers[0].rnw = 0;
> +  xfers[1].data = (uintptr_t)rx_buf;
> +  xfers[1].len = 10;
> +  xfers[1].rnw = 1;
> +
> +  if (ioctl(file, I3C_IOC_PRIV_XFER(2), xfers) < 0)
> +    /* ERROR HANDLING: I3C transaction failed */
> +
> +The device can be closed when the open file descriptor is no longer required::
> +
> +  close(file);
> \ No newline at end of file

Please fix that warning. ^^^^^


-- 
~Randy


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v3 3/5] i3c: master: add i3c_for_each_dev helper

[Greg KH]

On Wed, Feb 19, 2020 at 01:20:41AM +0100, Vitor Soares wrote:
> 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);

Ick, why the lock?  Are you _sure_ you need that?  The core should
handle any list locking issues here, right?

I don't see bus-specific-locks around other subsystem functions that do
this (like usb_for_each_dev).

thanks,

greg k-h

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v3 4/5] i3c: add i3cdev module to expose i3c dev in /dev

[Greg KH]

On Wed, Feb 19, 2020 at 01:20:42AM +0100, Vitor Soares wrote:
> +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.

Hah, no, we don't get to say stuff like that.  It's hard to write good
apis, but once this is merged, we need to support what is there, right?

kernel programming is hard :(


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v3 4/5] i3c: add i3cdev module to expose i3c dev in /dev

[Greg KH]

On Wed, Feb 19, 2020 at 01:20:42AM +0100, Vitor Soares wrote:
> +
> +static DEFINE_IDA(i3cdev_ida);

You never destroy this when the code is unloaded :(


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v3 0/5] Introduce i3c device userspace interface

[Boris Brezillon]

On Wed, 19 Feb 2020 00:39:31 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> Hi Boris,
> 
> From: Vitor Soares <vitor.soares@synopsys.com>
> Date: Wed, Feb 19, 2020 at 00:20:38
> 
> > 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/bus/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 v3:
> >   Use the xfer_lock to prevent device detach during ioctl call
> >   Expose i3cdev under /dev/bus/i3c/ folder like usb does
> >   Change NOTIFY_BOUND to NOTIFY_BIND, this allows the device detach occur
> >   before driver->probe call
> >   Avoid use of IS_ERR_OR_NULL
> >   Use u64_to_user_ptr instead of (void __user *)(uintptr_t) cast
> >   Allocate k_xfer and data_ptrs at once and eliminate double allocation
> >   check
> >   Pass i3cdev to dev->driver_data
> >   Make all minors available
> >   Add API documentation
> > 
> > Changes in v2:
> >   Use IDR api for minor numbering
> >   Modify ioctl struct
> >   Fix SPDX license
> > 
> > Vitor Soares (5):
> >   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
> >   userspace-api: add i3cdev documentation
> > 
> >  Documentation/userspace-api/i3c/i3cdev.rst | 116 ++++++++
> >  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 +++
> >  7 files changed, 616 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/userspace-api/i3c/i3cdev.rst
> >  create mode 100644 drivers/i3c/i3cdev.c
> >  create mode 100644 include/uapi/linux/i3c/i3cdev.h
> > 
> > -- 
> > 2.7.4  
> 
> I want to make you know that none of your previous comments was ignored 
> and  I would like to start the discussion from this point.

Sure, np. I'll probably wait for a v4 exploring the option I proposed
then.

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v3 4/5] i3c: add i3cdev module to expose i3c dev in /dev

[Greg KH]

On Wed, Feb 19, 2020 at 01:20:42AM +0100, Vitor Soares wrote:
> 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>
> ---
> Changes in v3:
>   Use the xfer_lock to prevent device detach during ioctl call
>   Expose i3cdev under /dev/bus/i3c/ folder like usb does
>   Change NOTIFY_BOUND to NOTIFY_BIND, this allows the device detach occur
>   before driver->probe
>   Avoid use of IS_ERR_OR_NULL
>   Use u64_to_user_ptr instead of (void __user *)(uintptr_t) cast
>   Allocate k_xfer and data_ptrs at once to eliminate duble allocation check
>   Pass i3cdev to dev->driver_data
>   Make all minors available
> 
> Changes in v2:
>   Use IDR api for minor numbering
>   Modify ioctl struct
>   Fix SPDX license
> 
>  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..1a6bd35
> --- /dev/null
> +++ b/drivers/i3c/i3cdev.c
> @@ -0,0 +1,429 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020 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/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 i3c_device *i3c;
> +	struct device *dev;
> +	struct mutex xfer_lock; /* prevent detach while transferring */
> +	struct cdev cdev;
> +	int id;
> +};
> +
> +static DEFINE_IDA(i3cdev_ida);
> +static dev_t i3cdev_number;
> +#define I3C_MINORS (MINORMASK + 1)
> +
> +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;
> +	i3cdev_set_drvdata(i3c, i3cdev);
> +
> +	return i3cdev;
> +}
> +
> +static void put_i3cdev(struct i3cdev_data *i3cdev)
> +{
> +	i3cdev_set_drvdata(i3cdev->i3c, NULL);
> +	kfree(i3cdev);
> +}
> +
> +static ssize_t
> +i3cdev_read(struct file *file, char __user *buf, size_t count, loff_t *f_pos)
> +{
> +	struct i3cdev_data *i3cdev = file->private_data;
> +	struct i3c_device *i3c = i3cdev->i3c;
> +	struct i3c_priv_xfer xfers = {
> +		.rnw = true,
> +		.len = count,
> +	};
> +	int ret = -EACCES;
> +	char *tmp;
> +
> +	mutex_lock(&i3cdev->xfer_lock);
> +	if (i3c->dev.driver)
> +		goto err_out;
> +
> +	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, xfers.len) ? -EFAULT : xfers.len;
> +
> +	kfree(tmp);
> +
> +err_out:
> +	mutex_unlock(&i3cdev->xfer_lock);
> +	return ret;
> +}
> +
> +static ssize_t
> +i3cdev_write(struct file *file, const char __user *buf, size_t count,
> +	     loff_t *f_pos)
> +{
> +	struct i3cdev_data *i3cdev = file->private_data;
> +	struct i3c_device *i3c = i3cdev->i3c;
> +	struct i3c_priv_xfer xfers = {
> +		.rnw = false,
> +		.len = count,

No error checking of count?

> +	};

You can have this structure on the stack?

> +	int ret = -EACCES;
> +	char *tmp;
> +
> +	mutex_lock(&i3cdev->xfer_lock);
> +	if (i3c->dev.driver)
> +		goto err_out;
> +
> +	tmp = memdup_user(buf, count);

So userspace can allocate any amount of memory they want to?

Not good :(

> +	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);
> +
> +err_out:
> +	mutex_unlock(&i3cdev->xfer_lock);
> +	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;
> +
> +	/* Since we have nxfers we may allocate k_xfer + *data_ptrs together */
> +	k_xfers = kcalloc(nxfers, sizeof(*k_xfers) + sizeof(*data_ptrs),
> +			  GFP_KERNEL);
> +	if (!k_xfers)
> +		return -ENOMEM;
> +
> +	/* set data_ptrs to be after nxfers * i3c_priv_xfer */
> +	data_ptrs = (void *)k_xfers + (nxfers * sizeof(*k_xfers));
> +
> +	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(u64_to_user_ptr(xfers[i].data),
> +					 data_ptrs[i], xfers[i].len))
> +				ret = -EFAULT;

You should bail out of the loop here.

> +		}
> +	}
> +
> +err_free_mem:
> +	for (; i >= 0; i--)
> +		kfree(data_ptrs[i]);
> +	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 ERR_PTR(-EINVAL);

But no error checking for too many of these?

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

You lost your __user marking of this pointer :(

Please fix that up, have you run sparse on this?

> +{
> +	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(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 i3cdev_data *i3cdev = file->private_data;
> +	struct i3c_device *i3c = i3cdev->i3c;
> +	int ret = -EACCES;
> +
> +	dev_dbg(&i3c->dev, "ioctl, cmd=0x%02x, arg=0x%02lx\n", cmd, arg);
> +
> +	if (_IOC_TYPE(cmd) != I3C_DEV_IOC_MAGIC)
> +		return -ENOTTY;
> +
> +	/* Use the xfer_lock to prevent device detach during ioctl call */
> +	mutex_lock(&i3cdev->xfer_lock);
> +	if (i3c->dev.driver)
> +		goto err_no_dev;
> +
> +	/* Check command number and direction */
> +	if (_IOC_NR(cmd) == _IOC_NR(I3C_IOC_PRIV_XFER(0)) &&
> +	    _IOC_DIR(cmd) == (_IOC_READ | _IOC_WRITE))

Shouldn't the ioctl core already handle this for you?

> +		ret = i3cdev_ioc_priv_xfer(i3c, cmd,
> +					(struct i3c_ioc_priv_xfer __user *)arg);
> +
> +err_no_dev:
> +	mutex_unlock(&i3cdev->xfer_lock);
> +	return ret;
> +}
> +
> +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;
> +
> +	return 0;
> +}
> +
> +static int i3cdev_release(struct inode *inode, struct file *file)
> +{
> +	file->private_data = NULL;

Why does this matter?

> +
> +	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);
> +
> +	mutex_init(&i3cdev->xfer_lock);
> +	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, "bus!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);

dev_dbg()?

> +	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_drvdata(i3c);
> +	if (!i3cdev)
> +		return 0;
> +
> +	/* Prevent transfers while cdev removal */
> +	mutex_lock(&i3cdev->xfer_lock);
> +	cdev_del(&i3cdev->cdev);
> +	device_destroy(i3cdev_class, MKDEV(MAJOR(i3cdev_number), i3cdev->id));
> +	mutex_unlock(&i3cdev->xfer_lock);
> +
> +	ida_simple_remove(&i3cdev_ida, i3cdev->id);
> +	put_i3cdev(i3cdev);
> +
> +	pr_debug("i3cdev: device [%s] unregistered\n", dev_name(&i3c->dev));

dev_dbg()?

> +
> +	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_BIND_DRIVER:
> +		return i3cdev_detach(dev, NULL);
> +	}
> +
> +	return 0;
> +}
> +
> +static struct notifier_block i3cdev_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, &i3cdev_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__);

spelling?

> +	return res;
> +}
> +
> +static void __exit i3cdev_exit(void)
> +{
> +	bus_unregister_notifier(&i3c_bus_type, &i3cdev_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..656a686
> --- /dev/null
> +++ b/include/uapi/linux/i3c/i3cdev.h
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Copyright (c) 2020 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;

Shouldn't this be an enum type?  "true" is not a __u8 value :)

> +	__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))

_IOCWR()?

thanks,

greg k-h

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH v3 4/5] i3c: add i3cdev module to expose i3c dev in /dev

[Vitor Soares]

Hi Greg,

From: Greg KH <gregkh@linuxfoundation.org>
Date: Wed, Feb 19, 2020 at 07:37:09

> On Wed, Feb 19, 2020 at 01:20:42AM +0100, Vitor Soares wrote:
> > +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.
> 
> Hah, no, we don't get to say stuff like that.  It's hard to write good
> apis, but once this is merged, we need to support what is there, right?
> 
> kernel programming is hard :(

I will remove this part.

Best regards,
Vitor Soares

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH v3 5/5] add i3cdev documentation

[Vitor Soares]

Hi Randy,

From: Randy Dunlap <rdunlap@infradead.org>
Date: Wed, Feb 19, 2020 at 04:34:00

> On 2/18/20 4:20 PM, Vitor Soares wrote:
> > This patch add documentation for the userspace API of i3cdev module.
> > 
> > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > ---
> >  Documentation/userspace-api/i3c/i3cdev.rst | 116 +++++++++++++++++++++++++++++
> >  1 file changed, 116 insertions(+)
> >  create mode 100644 Documentation/userspace-api/i3c/i3cdev.rst
> > 
> > diff --git a/Documentation/userspace-api/i3c/i3cdev.rst b/Documentation/userspace-api/i3c/i3cdev.rst
> > new file mode 100644
> > index 0000000..ada269f
> > --- /dev/null
> > +++ b/Documentation/userspace-api/i3c/i3cdev.rst
> > @@ -0,0 +1,116 @@
> > +====================
> > +I3C Device Interface
> > +====================
> > +
> > +I3C devices have the flexibility of being accessed from userspace, as well
> > +through the conventional use of kernel drivers. Userspace access, although
> > +limited to private SDR I3C transfers, provides the advantage of simplifying
> > +the implementation of straightforward communication protocols, applicable to
> > +scenarios where transfers are dedicated such for sensor bring-up scenarios
> > +(prototyping environments) or for microcontroller slave communication
> > +implementation.
> > +
> > +The major device number is dynamically attributed and it's all reserved for
> 
>                                           allocated (?)
> 
> > +the i3c devices. By default, the i3cdev module only exposes the i3c devices
> 
>        I3C                                                         I3C
> 
> > +without device driver bind and aren't of master type in sort of character
> > +device file under /dev/bus/i3c/ folder. They are identified through its
> 
> IMO:                              s/folder/directory/ or sub-directory
> 
> > +<bus id>-<Provisional ID> same way they can be found in /sys/bus/i3c/devices/.
> 
>                              in the same way
> 
> > +::
> > +
> > +# ls -l /dev/bus/i3c/
> > +total 0
> > +crw-------    1 root     root      248,   0 Jan  1 00:22 0-6072303904d2
> > +crw-------    1 root     root      248,   1 Jan  1 00:22 0-b7405ba00929
> > +
> > +The simplest way to use this interface is to not have an I3C device bound to
> > +a kernel driver, this can be achieved by not have the kernel driver loaded or
> 
>             driver. This                 by not having
> 
> > +using the Sysfs to unbind the kernel driver from the device.
> 
>          the sysfs interface to unbind
> 
> > +
> > +BASIC CHARACTER DEVICE API
> > +===============================
> > +For now, the API has only support private SDR read and write transfers.
> 
>                         only support for private
> 
> For the unfamiliar, what is this "SDR"?  (thanks)

SDR stands for Single Data Rate. In I3C we can also have High Data Rate 
(HDR) modes:
  - Double Data Rate (HDR-DDR);
  - Ternary Symbol Legacy (HDR-TSL)
  - Ternary Symbol for Pure Bus (no I2C devices present on the bus)

Should I use Single Data Rate instead SDR  for the first time?

> 
> > +Those transaction can be achieved by the following:
> > +
> > +``read(file, buffer, sizeof(buffer))``
> > +  The standard read() operation will work as a simple transaction of private
> > +  SDR read data followed a stop.
> > +  Return the number of bytes read on success, and a negative error otherwise.
> > +
> > +``write(file, buffer, sizeof(buffer))``
> > +  The standard write() operation will work as a simple transaction of private
> > +  SDR write data followed a stop.
> > +  Return the number of bytes written on success, and a negative error otherwise.
> > +
> > +``ioctl(file, I3C_IOC_PRIV_XFER(nxfers), struct i3c_ioc_priv_xfer *xfers)``
> > +  It combines read/write transactions without a stop in between.
> > +  Return 0 on success, and a negative error otherwise.
> > +
> > +NOTES:
> > +  - According to the MIPI I3C Protocol is the I3C slave that terminates the read
> 
>                                           it is the I3C slave
> 
> > +    transaction otherwise Master can abort early on ninth (T) data bit of each
> > +    SDR data word.
> > +
> > +  - Normal open() and close() operations on /dev/bus/i3c/<bus>-<provisional id>
> > +    files work as you would expect.
> > +
> > +  - As documented in cdev_del() if a device was already open during
> > +    i3cdev_detach, the read(), write() and ioctl() fops will still be callable
> > +    yet they will return -EACCES.
> > +
> > +C EXAMPLE
> > +=========
> > +Working with I3C devices is much like working with files. You will need to open
> > +a file descriptor, do some I/O operations with it, and then close it.
> > +
> > +The following header files should be included in an I3C program::
> > +
> > +#include <fcntl.h>
> > +#include <unistd.h>
> > +#include <sys/ioctl.h>
> > +#include <linux/types.h>
> > +#include <linux/i3c/i3cdev.h>
> > +
> > +To work with an I3C device, the application must open the driver, made
> > +available at the device node::
> > +
> > +  int file;
> > +
> > +  file = open("/dev/bus/i3c/0-6072303904d2", O_RDWR);
> > +  if (file < 0)
> > +  exit(1);
> 
> better indentation?
> 
> > +
> > +Now the file is opened, we can perform the operations available::
> > +
> > +  /* Write function */
> > +  uint_t8  buf[] = {0x00, 0xde, 0xad, 0xbe, 0xef}
> 
> I can't find uint_t8.  Where is it located?

Typo ☹.

> and the braces should end with a ';'.
> 
> > +  if (write(file, buf, 5) != 5) {
> > +    /* ERROR HANDLING: I3C transaction failed */
> > +  }
> > +
> > +  /*  Read function */
> > +  ret = read(file, buf, 5);
> > +  If (ret < 0) {
> > +    /* ERROR HANDLING: I3C transaction failed */
> > +  } else {
> > +    /* Iterate over buf[] to get the read data */
> > +  }
> > +
> > +  /* IOCTL function */
> > +  struct i3c_ioc_priv_xfer xfers[2];
> > +
> > +  uint8_t tx_buf[] = {0x00, 0xde, 0xad, 0xbe, 0xef};
> > +  uint8_t rx_buf[10];
> > +
> > +  xfers[0].data = (uintptr_t)tx_buf;
> > +  xfers[0].len = 5;
> > +  xfers[0].rnw = 0;
> > +  xfers[1].data = (uintptr_t)rx_buf;
> > +  xfers[1].len = 10;
> > +  xfers[1].rnw = 1;
> > +
> > +  if (ioctl(file, I3C_IOC_PRIV_XFER(2), xfers) < 0)
> > +    /* ERROR HANDLING: I3C transaction failed */
> > +
> > +The device can be closed when the open file descriptor is no longer required::
> > +
> > +  close(file);
> > \ No newline at end of file
> 
> Please fix that warning. ^^^^^
> 
> 
> -- 
> ~Randy

Thanks for your feedback. I will address them next version.


Best regards,
Vitor Soares


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH v3 3/5] i3c: master: add i3c_for_each_dev helper

[Vitor Soares]

Hi Greg,

From: Greg KH <gregkh@linuxfoundation.org>
Date: Wed, Feb 19, 2020 at 07:35:48

> On Wed, Feb 19, 2020 at 01:20:41AM +0100, Vitor Soares wrote:
> > 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);
> 
> Ick, why the lock?  Are you _sure_ you need that?  The core should
> handle any list locking issues here, right?

I want to make sure that no new devices (eg: Hot-Join capable device) are 
added during this iteration and after this call, each new device will 
release a bus notification.

> 
> I don't see bus-specific-locks around other subsystem functions that do
> this (like usb_for_each_dev).

I based in I2C use case.

> 
> thanks,
> 
> greg k-h
> 
> _______________________________________________
> 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=tlMumonboh0P8ljWml7h8q161eLTzDbo5QF77YPi2t8&s=RYYk4dJ4Ujse2MjdOBsoEwEOkxXDStTffqzlFZhwlXQ&e= 

Best regards,
Vitor Soares

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH v3 4/5] i3c: add i3cdev module to expose i3c dev in /dev

[Vitor Soares]

From: Greg KH <gregkh@linuxfoundation.org>
Date: Wed, Feb 19, 2020 at 07:39:01

> On Wed, Feb 19, 2020 at 01:20:42AM +0100, Vitor Soares wrote:
> > +
> > +static DEFINE_IDA(i3cdev_ida);
> 
> You never destroy this when the code is unloaded :(

You are right, I need to destroy it.

Thanks,
Vitor Soares

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v3 3/5] i3c: master: add i3c_for_each_dev helper

[Greg KH]

On Fri, Feb 21, 2020 at 11:47:22AM +0000, Vitor Soares wrote:
> Hi Greg,
> 
> From: Greg KH <gregkh@linuxfoundation.org>
> Date: Wed, Feb 19, 2020 at 07:35:48
> 
> > On Wed, Feb 19, 2020 at 01:20:41AM +0100, Vitor Soares wrote:
> > > 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);
> > 
> > Ick, why the lock?  Are you _sure_ you need that?  The core should
> > handle any list locking issues here, right?
> 
> I want to make sure that no new devices (eg: Hot-Join capable device) are 
> added during this iteration and after this call, each new device will 
> release a bus notification.
> 
> > 
> > I don't see bus-specific-locks around other subsystem functions that do
> > this (like usb_for_each_dev).
> 
> I based in I2C use case.

Check to see if this is really needed, for some reason I doubt it...

thanks,

greg k-h

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v3 3/5] i3c: master: add i3c_for_each_dev helper

[Boris Brezillon]

On Fri, 21 Feb 2020 12:52:29 +0100
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Fri, Feb 21, 2020 at 11:47:22AM +0000, Vitor Soares wrote:
> > Hi Greg,
> > 
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Date: Wed, Feb 19, 2020 at 07:35:48
> >   
> > > On Wed, Feb 19, 2020 at 01:20:41AM +0100, Vitor Soares wrote:  
> > > > 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);  
> > > 
> > > Ick, why the lock?  Are you _sure_ you need that?  The core should
> > > handle any list locking issues here, right?  
> > 
> > I want to make sure that no new devices (eg: Hot-Join capable device) are 
> > added during this iteration and after this call, each new device will 
> > release a bus notification.
> >   
> > > 
> > > I don't see bus-specific-locks around other subsystem functions that do
> > > this (like usb_for_each_dev).  
> > 
> > I based in I2C use case.  
> 
> Check to see if this is really needed, for some reason I doubt it...

Can we please try the spidev approach before fixing those problems. None
of that would be needed if we declare the i3cdev driver as a regular
i3c_device_driver and let user space bind devices it wants to expose
through the sysfs interface. As I said earlier, we even have all the
pieces we need to automate that using a udev rule, and the resulting
patchset would be 'less invasive'/simpler for pretty much the same
result.

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v3 5/5] add i3cdev documentation

[Randy Dunlap]

On 2/21/20 2:31 AM, Vitor Soares wrote:
>>> +BASIC CHARACTER DEVICE API
>>> +===============================
>>> +For now, the API has only support private SDR read and write transfers.
>>                         only support for private
>>
>> For the unfamiliar, what is this "SDR"?  (thanks)
> SDR stands for Single Data Rate. In I3C we can also have High Data Rate 
> (HDR) modes:
>   - Double Data Rate (HDR-DDR);
>   - Ternary Symbol Legacy (HDR-TSL)
>   - Ternary Symbol for Pure Bus (no I2C devices present on the bus)
> 
> Should I use Single Data Rate instead SDR  for the first time?

Yes, please.

thanks.
-- 
~Randy


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v3 3/5] i3c: master: add i3c_for_each_dev helper

[Boris Brezillon]

On Fri, 21 Feb 2020 13:59:11 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Fri, 21 Feb 2020 12:52:29 +0100
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Fri, Feb 21, 2020 at 11:47:22AM +0000, Vitor Soares wrote:  
> > > Hi Greg,
> > > 
> > > From: Greg KH <gregkh@linuxfoundation.org>
> > > Date: Wed, Feb 19, 2020 at 07:35:48
> > >     
> > > > On Wed, Feb 19, 2020 at 01:20:41AM +0100, Vitor Soares wrote:    
> > > > > 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);    
> > > > 
> > > > Ick, why the lock?  Are you _sure_ you need that?  The core should
> > > > handle any list locking issues here, right?    
> > > 
> > > I want to make sure that no new devices (eg: Hot-Join capable device) are 
> > > added during this iteration and after this call, each new device will 
> > > release a bus notification.
> > >     
> > > > 
> > > > I don't see bus-specific-locks around other subsystem functions that do
> > > > this (like usb_for_each_dev).    
> > > 
> > > I based in I2C use case.    
> > 
> > Check to see if this is really needed, for some reason I doubt it...  
> 
> Can we please try the spidev approach before fixing those problems. None
> of that would be needed if we declare the i3cdev driver as a regular
> i3c_device_driver and let user space bind devices it wants to expose
> through the sysfs interface. As I said earlier, we even have all the
> pieces we need to automate that using a udev rule, and the resulting
> patchset would be 'less invasive'/simpler for pretty much the same
> result.

So, I went ahead and implemented it the way I suggest. The diffstat is
not representative here (though it's still in favor of this new version)
since I also changed the way we expose/handle SDR transfers. What's
most important IMO is the fact that

* we no longer need to access the internal I3C API
* we no longer need to care about transitions between i3cdev and
  other drivers (the core guarantees that a device is always bound to at
  most one driver)
* the registration/unregistration procedure is simplified

Not all problems have been addressed (we still need to put a limit on
the number of xfers and the max size per transfer we allow, and
probably plenty of other things pointed by Greg, Arnd and others), but
I'd really like to start from there for the next version.

--->8---
From a344e3c51db4e414e4ec6007c001c32afa954a82 Mon Sep 17 00:00:00 2001
From: Vitor Soares <Vitor.Soares@synopsys.com>
Date: Wed, 19 Feb 2020 01:20:42 +0100
Subject: [PATCH] i3c: add i3cdev module to expose i3c dev in /dev

This patch adds user mode support to I3C SDR transfers.

The module is based on spidev with the follow features:
  - Dynamically allocate the char device Major number.
  - Expose SDR (Single Data Rate) transfers

Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
---
 drivers/i3c/Kconfig             |  15 ++
 drivers/i3c/Makefile            |   1 +
 drivers/i3c/i3cdev.c            | 324 ++++++++++++++++++++++++++++++++
 include/uapi/linux/i3c/i3cdev.h |  38 ++++
 4 files changed, 378 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 30a441506f61..01642768ab5f 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 11982efbc6d9..606d422841b2 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 000000000000..2b408a446ab6
--- /dev/null
+++ b/drivers/i3c/i3cdev.c
@@ -0,0 +1,324 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020 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/i3c/device.h>
+#include <linux/init.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+
+#include <uapi/linux/i3c/i3cdev.h>
+
+#define MAX_I3CDEV_DEVS (MINORMASK + 1)
+
+struct i3cdev_data {
+	struct i3c_device *i3c;
+	struct device *dev;
+	struct cdev cdev;
+	dev_t devt;
+};
+
+static DEFINE_IDA(i3cdev_ida);
+static dev_t i3cdev_first_chrdev;
+
+static ssize_t
+i3cdev_read(struct file *file, char __user *buf, size_t count, loff_t *f_pos)
+{
+	struct i3cdev_data *i3cdev = file->private_data;
+	struct i3c_device *i3c = i3cdev->i3c;
+	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(i3cdev->dev, "Reading %zu bytes.\n", count);
+
+	ret = i3c_device_do_priv_xfers(i3c, &xfers, 1);
+	if (!ret)
+		ret = copy_to_user(buf, tmp, xfers.len) ? -EFAULT : xfers.len;
+
+	kfree(tmp);
+	return ret;
+}
+
+static ssize_t
+i3cdev_write(struct file *file, const char __user *buf, size_t count,
+	     loff_t *f_pos)
+{
+	struct i3cdev_data *i3cdev = file->private_data;
+	struct i3c_device *i3c = i3cdev->i3c;
+	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(i3cdev->dev, "Writing %zu bytes.\n", count);
+
+	ret = i3c_device_do_priv_xfers(i3c, &xfers, 1);
+	kfree(tmp);
+
+	return !ret ? count : ret;
+}
+
+static int
+i3cdev_ioctl_priv_xfers(struct i3c_device *i3c,
+			struct i3cdev_priv_xfers __user *uxfers)
+{
+	struct i3cdev_priv_xfers xfers;
+	struct i3cdev_priv_xfer *uxfer_array;
+	struct i3c_priv_xfer *xfer_array;
+	unsigned int i;
+	int ret;
+
+	if (copy_from_user(&xfers, uxfers, sizeof(xfers)))
+		return -EFAULT;
+
+	/* TODO: Add a limit for xfers.nxfers. */
+	uxfer_array = memdup_user(u64_to_user_ptr(xfers.xfers),
+				  xfers.nxfers * sizeof(*uxfer_array));
+	if (!uxfer_array)
+		return -ENOMEM;
+
+	xfer_array = kmalloc_array(xfers.nxfers, sizeof(*xfer_array),
+				   GFP_KERNEL);
+	if (!xfer_array) {
+		ret = -ENOMEM;
+		goto out_free_uxfer_array;
+	}
+
+	/* TODO Add a limit for max in/out size. */
+	for (i = 0; i < xfers.nxfers; i++) {
+		void __user *udata = u64_to_user_ptr(uxfer_array[i].data);
+		void *data;
+
+		xfer_array[i].rnw = uxfer_array[i].rnw;
+		xfer_array[i].len = uxfer_array[i].len;
+
+		if (!xfer_array[i].rnw) {
+			data = memdup_user(udata, xfer_array[i].len);
+		} else {
+			data = kzalloc(xfer_array[i].len, GFP_KERNEL);
+			if (data)
+				data = ERR_PTR(-ENOMEM);
+		}
+
+		if (IS_ERR(data)) {
+			ret = PTR_ERR(xfer_array[i].data.in);
+			goto out_free_xfer_array;
+		}
+
+		xfer_array[i].data.in = data;
+	}
+
+	ret = i3c_device_do_priv_xfers(i3c, xfer_array, xfers.nxfers);
+
+	if (!ret) {
+		for (i = 0; i < xfers.nxfers; i++) {
+			void __user *udata = u64_to_user_ptr(uxfer_array[i].data);
+			void *data = xfer_array[i].data.in;
+
+			if (!xfer_array[i].rnw)
+				continue;
+
+			if (copy_to_user(udata, data, xfer_array[i].len)) {
+				ret = -EFAULT;
+				goto out_free_xfer_array;
+			}
+		}
+	}
+
+out_free_xfer_array:
+	for (i = 0; i < xfers.nxfers; i++)
+		kfree(xfer_array[i].data.in);
+
+	kfree(xfer_array);
+
+out_free_uxfer_array:
+	kfree(uxfer_array);
+	return ret;
+}
+
+static long
+i3cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct i3cdev_data *i3cdev = file->private_data;
+	void __user *udata = (void __user *)arg;
+	int ret = -EACCES;
+
+	dev_dbg(i3cdev->dev, "ioctl, cmd=0x%02x, arg=0x%02lx\n", cmd, arg);
+
+	switch (cmd) {
+	case I3CDEV_PRIV_XFER:
+		ret = i3cdev_ioctl_priv_xfers(i3cdev->i3c, udata);
+		break;
+	default:
+		return -ENOTTY;
+	}
+
+	return ret;
+}
+
+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;
+
+	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_probe(struct i3c_device *i3c)
+{
+	struct device *dev = i3cdev_to_dev(i3c);
+	struct i3cdev_data *i3cdev;
+	int ret;
+	int id;
+
+	i3cdev = devm_kzalloc(dev, sizeof(*i3cdev), GFP_KERNEL);
+	if (!i3cdev)
+		return -ENOMEM;
+
+	ret = ida_simple_get(&i3cdev_ida, 0, MAX_I3CDEV_DEVS, GFP_KERNEL);
+	if (ret < 0) {
+		dev_err(dev, "no minor number available!\n");
+		return ret;
+	}
+
+	i3cdev->i3c = i3c;
+	i3cdev->devt = MKDEV(MAJOR(i3cdev_first_chrdev),
+			     MINOR(i3cdev_first_chrdev) + ret);
+	i3cdev_set_drvdata(i3c, i3cdev);
+	cdev_init(&i3cdev->cdev, &i3cdev_fops);
+	i3cdev->cdev.owner = THIS_MODULE;
+	ret = cdev_add(&i3cdev->cdev, i3cdev->devt, 1);
+	if (ret)
+		goto error_free_id;
+
+	i3cdev->dev = device_create(i3cdev_class, dev, i3cdev->devt,
+				    NULL, "i3cdev%d", id);
+	if (IS_ERR(i3cdev->dev)) {
+		ret = PTR_ERR(i3cdev->dev);
+		goto error_free_cdev;
+	}
+
+	return 0;
+
+error_free_cdev:
+	cdev_del(&i3cdev->cdev);
+error_free_id:
+	ida_free(&i3cdev_ida,
+		 MINOR(i3cdev->devt) - MINOR(i3cdev_first_chrdev));
+	return ret;
+}
+
+static int i3cdev_remove(struct i3c_device *i3c)
+{
+	struct i3cdev_data *i3cdev;
+
+	i3cdev = i3cdev_get_drvdata(i3c);
+
+	device_destroy(i3cdev_class, i3cdev->devt);
+	cdev_del(&i3cdev->cdev);
+	ida_simple_remove(&i3cdev_ida,
+			  MINOR(i3cdev->devt) - MINOR(i3cdev_first_chrdev));
+
+	return 0;
+}
+
+static const struct i3c_device_id i3cdev_ids[] = {
+	{ /* Sentinel */ },
+};
+
+static struct i3c_driver i3cdev_driver = {
+	.probe = i3cdev_probe,
+	.remove = i3cdev_remove,
+	.id_table = i3cdev_ids,
+};
+
+static int __init i3cdev_init(void)
+{
+	int ret;
+
+	ret = alloc_chrdev_region(&i3cdev_first_chrdev, 0, MAX_I3CDEV_DEVS,
+				  "i3cdev");
+	if (ret)
+		return ret;
+
+	i3cdev_class = class_create(THIS_MODULE, "i3cdev");
+	if (IS_ERR(i3cdev_class))
+		goto err_unreg_chrdev_region;
+
+	ret = i3c_driver_register(&i3cdev_driver);
+	if (ret)
+		goto err_destroy_class;
+
+	return 0;
+
+err_destroy_class:
+	class_destroy(i3cdev_class);
+err_unreg_chrdev_region:
+	unregister_chrdev_region(i3cdev_first_chrdev, MAX_I3CDEV_DEVS);
+	return ret;
+}
+module_init(i3cdev_init);
+
+static void __exit i3cdev_exit(void)
+{
+	i3c_driver_unregister(&i3cdev_driver);
+	class_destroy(i3cdev_class);
+	unregister_chrdev_region(i3cdev_first_chrdev, MAX_I3CDEV_DEVS);
+}
+module_exit(i3cdev_exit);
+
+MODULE_AUTHOR("Vitor Soares <soares@synopsys.com>");
+MODULE_DESCRIPTION("I3C /dev entries driver");
+MODULE_LICENSE("GPL");
diff --git a/include/uapi/linux/i3c/i3cdev.h b/include/uapi/linux/i3c/i3cdev.h
new file mode 100644
index 000000000000..305dc029ca2d
--- /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) 2020 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 i3cdev_priv_xfer {
+	__u64 data;
+	__u16 len;
+	__u8 rnw;
+	__u8 pad[5];
+};
+
+struct i3cdev_priv_xfers {
+	__u64 nxfers;
+	__u64 xfers;
+};
+
+#define I3CDEV_PRIV_XFER \
+	_IOWR(I3C_DEV_IOC_MAGIC, 0, struct i3cdev_priv_xfers)
+
+#endif
-- 
2.24.1


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v3 3/5] i3c: master: add i3c_for_each_dev helper

[Boris Brezillon]

On Fri, 21 Feb 2020 17:44:28 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Fri, 21 Feb 2020 13:59:11 +0100
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> > On Fri, 21 Feb 2020 12:52:29 +0100
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> >   
> > > On Fri, Feb 21, 2020 at 11:47:22AM +0000, Vitor Soares wrote:    
> > > > Hi Greg,
> > > > 
> > > > From: Greg KH <gregkh@linuxfoundation.org>
> > > > Date: Wed, Feb 19, 2020 at 07:35:48
> > > >       
> > > > > On Wed, Feb 19, 2020 at 01:20:41AM +0100, Vitor Soares wrote:      
> > > > > > 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);      
> > > > > 
> > > > > Ick, why the lock?  Are you _sure_ you need that?  The core should
> > > > > handle any list locking issues here, right?      
> > > > 
> > > > I want to make sure that no new devices (eg: Hot-Join capable device) are 
> > > > added during this iteration and after this call, each new device will 
> > > > release a bus notification.
> > > >       
> > > > > 
> > > > > I don't see bus-specific-locks around other subsystem functions that do
> > > > > this (like usb_for_each_dev).      
> > > > 
> > > > I based in I2C use case.      
> > > 
> > > Check to see if this is really needed, for some reason I doubt it...    
> > 
> > Can we please try the spidev approach before fixing those problems. None
> > of that would be needed if we declare the i3cdev driver as a regular
> > i3c_device_driver and let user space bind devices it wants to expose
> > through the sysfs interface. As I said earlier, we even have all the
> > pieces we need to automate that using a udev rule, and the resulting
> > patchset would be 'less invasive'/simpler for pretty much the same
> > result.  
> 
> So, I went ahead and implemented it the way I suggest. The diffstat is
> not representative here (though it's still in favor of this new version)
> since I also changed the way we expose/handle SDR transfers. What's
> most important IMO is the fact that
> 
> * we no longer need to access the internal I3C API
> * we no longer need to care about transitions between i3cdev and
>   other drivers (the core guarantees that a device is always bound to at
>   most one driver)
> * the registration/unregistration procedure is simplified

Forgot to mention that

* Patch 1-3 are no longer required

> 
> Not all problems have been addressed (we still need to put a limit on
> the number of xfers and the max size per transfer we allow, and
> probably plenty of other things pointed by Greg, Arnd and others), but
> I'd really like to start from there for the next version.
> 
> --->8---  
> From a344e3c51db4e414e4ec6007c001c32afa954a82 Mon Sep 17 00:00:00 2001
> From: Vitor Soares <Vitor.Soares@synopsys.com>
> Date: Wed, 19 Feb 2020 01:20:42 +0100
> Subject: [PATCH] i3c: add i3cdev module to expose i3c dev in /dev
> 
> This patch adds user mode support to I3C SDR transfers.
> 
> The module is based on spidev with the follow features:
>   - Dynamically allocate the char device Major number.
>   - Expose SDR (Single Data Rate) transfers
> 
> Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> ---
>  drivers/i3c/Kconfig             |  15 ++
>  drivers/i3c/Makefile            |   1 +
>  drivers/i3c/i3cdev.c            | 324 ++++++++++++++++++++++++++++++++
>  include/uapi/linux/i3c/i3cdev.h |  38 ++++
>  4 files changed, 378 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 30a441506f61..01642768ab5f 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 11982efbc6d9..606d422841b2 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 000000000000..2b408a446ab6
> --- /dev/null
> +++ b/drivers/i3c/i3cdev.c
> @@ -0,0 +1,324 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020 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/i3c/device.h>
> +#include <linux/init.h>
> +#include <linux/jiffies.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/notifier.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +
> +#include <uapi/linux/i3c/i3cdev.h>
> +
> +#define MAX_I3CDEV_DEVS (MINORMASK + 1)
> +
> +struct i3cdev_data {
> +	struct i3c_device *i3c;
> +	struct device *dev;
> +	struct cdev cdev;
> +	dev_t devt;
> +};
> +
> +static DEFINE_IDA(i3cdev_ida);
> +static dev_t i3cdev_first_chrdev;
> +
> +static ssize_t
> +i3cdev_read(struct file *file, char __user *buf, size_t count, loff_t *f_pos)
> +{
> +	struct i3cdev_data *i3cdev = file->private_data;
> +	struct i3c_device *i3c = i3cdev->i3c;
> +	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(i3cdev->dev, "Reading %zu bytes.\n", count);
> +
> +	ret = i3c_device_do_priv_xfers(i3c, &xfers, 1);
> +	if (!ret)
> +		ret = copy_to_user(buf, tmp, xfers.len) ? -EFAULT : xfers.len;
> +
> +	kfree(tmp);
> +	return ret;
> +}
> +
> +static ssize_t
> +i3cdev_write(struct file *file, const char __user *buf, size_t count,
> +	     loff_t *f_pos)
> +{
> +	struct i3cdev_data *i3cdev = file->private_data;
> +	struct i3c_device *i3c = i3cdev->i3c;
> +	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(i3cdev->dev, "Writing %zu bytes.\n", count);
> +
> +	ret = i3c_device_do_priv_xfers(i3c, &xfers, 1);
> +	kfree(tmp);
> +
> +	return !ret ? count : ret;
> +}
> +
> +static int
> +i3cdev_ioctl_priv_xfers(struct i3c_device *i3c,
> +			struct i3cdev_priv_xfers __user *uxfers)
> +{
> +	struct i3cdev_priv_xfers xfers;
> +	struct i3cdev_priv_xfer *uxfer_array;
> +	struct i3c_priv_xfer *xfer_array;
> +	unsigned int i;
> +	int ret;
> +
> +	if (copy_from_user(&xfers, uxfers, sizeof(xfers)))
> +		return -EFAULT;
> +
> +	/* TODO: Add a limit for xfers.nxfers. */
> +	uxfer_array = memdup_user(u64_to_user_ptr(xfers.xfers),
> +				  xfers.nxfers * sizeof(*uxfer_array));
> +	if (!uxfer_array)
> +		return -ENOMEM;
> +
> +	xfer_array = kmalloc_array(xfers.nxfers, sizeof(*xfer_array),
> +				   GFP_KERNEL);
> +	if (!xfer_array) {
> +		ret = -ENOMEM;
> +		goto out_free_uxfer_array;
> +	}
> +
> +	/* TODO Add a limit for max in/out size. */
> +	for (i = 0; i < xfers.nxfers; i++) {
> +		void __user *udata = u64_to_user_ptr(uxfer_array[i].data);
> +		void *data;
> +
> +		xfer_array[i].rnw = uxfer_array[i].rnw;
> +		xfer_array[i].len = uxfer_array[i].len;
> +
> +		if (!xfer_array[i].rnw) {
> +			data = memdup_user(udata, xfer_array[i].len);
> +		} else {
> +			data = kzalloc(xfer_array[i].len, GFP_KERNEL);
> +			if (data)
> +				data = ERR_PTR(-ENOMEM);
> +		}
> +
> +		if (IS_ERR(data)) {
> +			ret = PTR_ERR(xfer_array[i].data.in);
> +			goto out_free_xfer_array;
> +		}
> +
> +		xfer_array[i].data.in = data;
> +	}
> +
> +	ret = i3c_device_do_priv_xfers(i3c, xfer_array, xfers.nxfers);
> +
> +	if (!ret) {
> +		for (i = 0; i < xfers.nxfers; i++) {
> +			void __user *udata = u64_to_user_ptr(uxfer_array[i].data);
> +			void *data = xfer_array[i].data.in;
> +
> +			if (!xfer_array[i].rnw)
> +				continue;
> +
> +			if (copy_to_user(udata, data, xfer_array[i].len)) {
> +				ret = -EFAULT;
> +				goto out_free_xfer_array;
> +			}
> +		}
> +	}
> +
> +out_free_xfer_array:
> +	for (i = 0; i < xfers.nxfers; i++)
> +		kfree(xfer_array[i].data.in);
> +
> +	kfree(xfer_array);
> +
> +out_free_uxfer_array:
> +	kfree(uxfer_array);
> +	return ret;
> +}
> +
> +static long
> +i3cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	struct i3cdev_data *i3cdev = file->private_data;
> +	void __user *udata = (void __user *)arg;
> +	int ret = -EACCES;
> +
> +	dev_dbg(i3cdev->dev, "ioctl, cmd=0x%02x, arg=0x%02lx\n", cmd, arg);
> +
> +	switch (cmd) {
> +	case I3CDEV_PRIV_XFER:
> +		ret = i3cdev_ioctl_priv_xfers(i3cdev->i3c, udata);
> +		break;
> +	default:
> +		return -ENOTTY;
> +	}
> +
> +	return ret;
> +}
> +
> +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;
> +
> +	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_probe(struct i3c_device *i3c)
> +{
> +	struct device *dev = i3cdev_to_dev(i3c);
> +	struct i3cdev_data *i3cdev;
> +	int ret;
> +	int id;
> +
> +	i3cdev = devm_kzalloc(dev, sizeof(*i3cdev), GFP_KERNEL);
> +	if (!i3cdev)
> +		return -ENOMEM;
> +
> +	ret = ida_simple_get(&i3cdev_ida, 0, MAX_I3CDEV_DEVS, GFP_KERNEL);
> +	if (ret < 0) {
> +		dev_err(dev, "no minor number available!\n");
> +		return ret;
> +	}
> +
> +	i3cdev->i3c = i3c;
> +	i3cdev->devt = MKDEV(MAJOR(i3cdev_first_chrdev),
> +			     MINOR(i3cdev_first_chrdev) + ret);
> +	i3cdev_set_drvdata(i3c, i3cdev);
> +	cdev_init(&i3cdev->cdev, &i3cdev_fops);
> +	i3cdev->cdev.owner = THIS_MODULE;
> +	ret = cdev_add(&i3cdev->cdev, i3cdev->devt, 1);
> +	if (ret)
> +		goto error_free_id;
> +
> +	i3cdev->dev = device_create(i3cdev_class, dev, i3cdev->devt,
> +				    NULL, "i3cdev%d", id);
> +	if (IS_ERR(i3cdev->dev)) {
> +		ret = PTR_ERR(i3cdev->dev);
> +		goto error_free_cdev;
> +	}
> +
> +	return 0;
> +
> +error_free_cdev:
> +	cdev_del(&i3cdev->cdev);
> +error_free_id:
> +	ida_free(&i3cdev_ida,
> +		 MINOR(i3cdev->devt) - MINOR(i3cdev_first_chrdev));
> +	return ret;
> +}
> +
> +static int i3cdev_remove(struct i3c_device *i3c)
> +{
> +	struct i3cdev_data *i3cdev;
> +
> +	i3cdev = i3cdev_get_drvdata(i3c);
> +
> +	device_destroy(i3cdev_class, i3cdev->devt);
> +	cdev_del(&i3cdev->cdev);
> +	ida_simple_remove(&i3cdev_ida,
> +			  MINOR(i3cdev->devt) - MINOR(i3cdev_first_chrdev));
> +
> +	return 0;
> +}
> +
> +static const struct i3c_device_id i3cdev_ids[] = {
> +	{ /* Sentinel */ },
> +};
> +
> +static struct i3c_driver i3cdev_driver = {
> +	.probe = i3cdev_probe,
> +	.remove = i3cdev_remove,
> +	.id_table = i3cdev_ids,
> +};
> +
> +static int __init i3cdev_init(void)
> +{
> +	int ret;
> +
> +	ret = alloc_chrdev_region(&i3cdev_first_chrdev, 0, MAX_I3CDEV_DEVS,
> +				  "i3cdev");
> +	if (ret)
> +		return ret;
> +
> +	i3cdev_class = class_create(THIS_MODULE, "i3cdev");
> +	if (IS_ERR(i3cdev_class))
> +		goto err_unreg_chrdev_region;
> +
> +	ret = i3c_driver_register(&i3cdev_driver);
> +	if (ret)
> +		goto err_destroy_class;
> +
> +	return 0;
> +
> +err_destroy_class:
> +	class_destroy(i3cdev_class);
> +err_unreg_chrdev_region:
> +	unregister_chrdev_region(i3cdev_first_chrdev, MAX_I3CDEV_DEVS);
> +	return ret;
> +}
> +module_init(i3cdev_init);
> +
> +static void __exit i3cdev_exit(void)
> +{
> +	i3c_driver_unregister(&i3cdev_driver);
> +	class_destroy(i3cdev_class);
> +	unregister_chrdev_region(i3cdev_first_chrdev, MAX_I3CDEV_DEVS);
> +}
> +module_exit(i3cdev_exit);
> +
> +MODULE_AUTHOR("Vitor Soares <soares@synopsys.com>");
> +MODULE_DESCRIPTION("I3C /dev entries driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/uapi/linux/i3c/i3cdev.h b/include/uapi/linux/i3c/i3cdev.h
> new file mode 100644
> index 000000000000..305dc029ca2d
> --- /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) 2020 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 i3cdev_priv_xfer {
> +	__u64 data;
> +	__u16 len;
> +	__u8 rnw;
> +	__u8 pad[5];
> +};
> +
> +struct i3cdev_priv_xfers {
> +	__u64 nxfers;
> +	__u64 xfers;
> +};
> +
> +#define I3CDEV_PRIV_XFER \
> +	_IOWR(I3C_DEV_IOC_MAGIC, 0, struct i3cdev_priv_xfers)
> +
> +#endif


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH v3 0/5] Introduce i3c device userspace interface

[Vitor Soares]

Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Wed, Feb 19, 2020 at 08:16:58

> On Wed, 19 Feb 2020 00:39:31 +0000
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > Hi Boris,
> > 
> > From: Vitor Soares <vitor.soares@synopsys.com>
> > Date: Wed, Feb 19, 2020 at 00:20:38
> > 
> > > 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/bus/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://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2018_11_15_853&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=1N2OHEqhXNvZn7HIjf_EmPX5N9wwBWLsUakJ9a1zno0&s=RJ3Lr0SIvJYiKxLCFaV0_pNArfUJj7TxsJecNgYbTQA&e= 
> > > [2] https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_vitor-2Dsoares-2Dsnps_i3c-2Dtools.git&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=1N2OHEqhXNvZn7HIjf_EmPX5N9wwBWLsUakJ9a1zno0&s=q4Oi7UD-dZ1YMlTN9prqycS1G1QvEUuBFnsABMGRuBk&e= 
> > > 
> > > Changes in v3:
> > >   Use the xfer_lock to prevent device detach during ioctl call
> > >   Expose i3cdev under /dev/bus/i3c/ folder like usb does
> > >   Change NOTIFY_BOUND to NOTIFY_BIND, this allows the device detach occur
> > >   before driver->probe call
> > >   Avoid use of IS_ERR_OR_NULL
> > >   Use u64_to_user_ptr instead of (void __user *)(uintptr_t) cast
> > >   Allocate k_xfer and data_ptrs at once and eliminate double allocation
> > >   check
> > >   Pass i3cdev to dev->driver_data
> > >   Make all minors available
> > >   Add API documentation
> > > 
> > > Changes in v2:
> > >   Use IDR api for minor numbering
> > >   Modify ioctl struct
> > >   Fix SPDX license
> > > 
> > > Vitor Soares (5):
> > >   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
> > >   userspace-api: add i3cdev documentation
> > > 
> > >  Documentation/userspace-api/i3c/i3cdev.rst | 116 ++++++++
> > >  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 +++
> > >  7 files changed, 616 insertions(+), 1 deletion(-)
> > >  create mode 100644 Documentation/userspace-api/i3c/i3cdev.rst
> > >  create mode 100644 drivers/i3c/i3cdev.c
> > >  create mode 100644 include/uapi/linux/i3c/i3cdev.h
> > > 
> > > -- 
> > > 2.7.4  
> > 
> > I want to make you know that none of your previous comments was ignored 
> > and  I would like to start the discussion from this point.
> 
> Sure, np. I'll probably wait for a v4 exploring the option I proposed
> then.

I would like to check with you:
  - How can we prioritize the device driver over the i3cdev driver if the 
driver is loaded after i3cdev? Currently, this is done automatically 
without any command, and for me, this is a requirement.
  - For the ioctl command structure, there is no rule about the way I did 
or what you proposed, both are currently used in the kernel. For me it is 
one more structure to deal with, can you point the advantages of your 
purpose?
  - Regarding the ioctl codes, I tried to use those after I2C.



Best regards,
Vitor Soares

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH v3 3/5] i3c: master: add i3c_for_each_dev helper

[Vitor Soares]

Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Fri, Feb 21, 2020 at 16:44:28

> On Fri, 21 Feb 2020 13:59:11 +0100
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> > On Fri, 21 Feb 2020 12:52:29 +0100
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> > 
> > > On Fri, Feb 21, 2020 at 11:47:22AM +0000, Vitor Soares wrote:  
> > > > Hi Greg,
> > > > 
> > > > From: Greg KH <gregkh@linuxfoundation.org>
> > > > Date: Wed, Feb 19, 2020 at 07:35:48
> > > >     
> > > > > On Wed, Feb 19, 2020 at 01:20:41AM +0100, Vitor Soares wrote:    
> > > > > > 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);    
> > > > > 
> > > > > Ick, why the lock?  Are you _sure_ you need that?  The core should
> > > > > handle any list locking issues here, right?    
> > > > 
> > > > I want to make sure that no new devices (eg: Hot-Join capable device) are 
> > > > added during this iteration and after this call, each new device will 
> > > > release a bus notification.
> > > >     
> > > > > 
> > > > > I don't see bus-specific-locks around other subsystem functions that do
> > > > > this (like usb_for_each_dev).    
> > > > 
> > > > I based in I2C use case.    
> > > 
> > > Check to see if this is really needed, for some reason I doubt it...  
> > 
> > Can we please try the spidev approach before fixing those problems. None
> > of that would be needed if we declare the i3cdev driver as a regular
> > i3c_device_driver and let user space bind devices it wants to expose
> > through the sysfs interface. As I said earlier, we even have all the
> > pieces we need to automate that using a udev rule, and the resulting
> > patchset would be 'less invasive'/simpler for pretty much the same
> > result.
> 
> So, I went ahead and implemented it the way I suggest. The diffstat is
> not representative here (though it's still in favor of this new version)
> since I also changed the way we expose/handle SDR transfers. What's
> most important IMO is the fact that
> 
> * we no longer need to access the internal I3C API
> * we no longer need to care about transitions between i3cdev and
>   other drivers (the core guarantees that a device is always bound to at
>   most one driver)
> * the registration/unregistration procedure is simplified
> 
> Not all problems have been addressed (we still need to put a limit on
> the number of xfers and the max size per transfer we allow, and
> probably plenty of other things pointed by Greg, Arnd and others), but
> I'd really like to start from there for the next version.

Ohh, I send the other email without see this one ☹.

Very much appreciated 😊. I will test and change the tool and let you 
know.

Best regards,
Vitor Soares
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v3 0/5] Introduce i3c device userspace interface

[Boris Brezillon]

On Fri, 21 Feb 2020 17:08:17 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> Hi Boris,
> 
> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Wed, Feb 19, 2020 at 08:16:58
> 
> > On Wed, 19 Feb 2020 00:39:31 +0000
> > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> >   
> > > Hi Boris,
> > > 
> > > From: Vitor Soares <vitor.soares@synopsys.com>
> > > Date: Wed, Feb 19, 2020 at 00:20:38
> > >   
> > > > 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/bus/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://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2018_11_15_853&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=1N2OHEqhXNvZn7HIjf_EmPX5N9wwBWLsUakJ9a1zno0&s=RJ3Lr0SIvJYiKxLCFaV0_pNArfUJj7TxsJecNgYbTQA&e= 
> > > > [2] https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_vitor-2Dsoares-2Dsnps_i3c-2Dtools.git&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=1N2OHEqhXNvZn7HIjf_EmPX5N9wwBWLsUakJ9a1zno0&s=q4Oi7UD-dZ1YMlTN9prqycS1G1QvEUuBFnsABMGRuBk&e= 
> > > > 
> > > > Changes in v3:
> > > >   Use the xfer_lock to prevent device detach during ioctl call
> > > >   Expose i3cdev under /dev/bus/i3c/ folder like usb does
> > > >   Change NOTIFY_BOUND to NOTIFY_BIND, this allows the device detach occur
> > > >   before driver->probe call
> > > >   Avoid use of IS_ERR_OR_NULL
> > > >   Use u64_to_user_ptr instead of (void __user *)(uintptr_t) cast
> > > >   Allocate k_xfer and data_ptrs at once and eliminate double allocation
> > > >   check
> > > >   Pass i3cdev to dev->driver_data
> > > >   Make all minors available
> > > >   Add API documentation
> > > > 
> > > > Changes in v2:
> > > >   Use IDR api for minor numbering
> > > >   Modify ioctl struct
> > > >   Fix SPDX license
> > > > 
> > > > Vitor Soares (5):
> > > >   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
> > > >   userspace-api: add i3cdev documentation
> > > > 
> > > >  Documentation/userspace-api/i3c/i3cdev.rst | 116 ++++++++
> > > >  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 +++
> > > >  7 files changed, 616 insertions(+), 1 deletion(-)
> > > >  create mode 100644 Documentation/userspace-api/i3c/i3cdev.rst
> > > >  create mode 100644 drivers/i3c/i3cdev.c
> > > >  create mode 100644 include/uapi/linux/i3c/i3cdev.h
> > > > 
> > > > -- 
> > > > 2.7.4    
> > > 
> > > I want to make you know that none of your previous comments was ignored 
> > > and  I would like to start the discussion from this point.  
> > 
> > Sure, np. I'll probably wait for a v4 exploring the option I proposed
> > then.  
> 
> I would like to check with you:
>   - How can we prioritize the device driver over the i3cdev driver if the 
> driver is loaded after i3cdev? Currently, this is done automatically 
> without any command, and for me, this is a requirement.

No devs would be bound to the i3cdev driver by default, it would have
to be done explicitly through a sysfs knob. Which makes me realize
we can't use the generic bind knob since it doesn't let the subsystem
know that it's a manual bind. I thought there was a way to distinguish
between manual and auto-bind.

>   - For the ioctl command structure, there is no rule about the way I did 
> or what you proposed, both are currently used in the kernel. For me it is 
> one more structure to deal with, can you point the advantages of your 
> purpose?

I don't have a strong opinion on that one, though I find it a bit
easier to follow when the number of xfers is encoded in a separate
struct rather than extracted from the data size passed through the cmd
argument.

>   - Regarding the ioctl codes, I tried to use those after I2C.

Why start from 0x30? It doesn't make sense to me. Just because you base
your code on something that already exists doesn't mean you have to
copy all of it.

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v3 4/5] i3c: add i3cdev module to expose i3c dev in /dev

[Boris Brezillon]

On Wed, 19 Feb 2020 01:20:42 +0100
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> +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_drvdata(i3c);
> +	if (!i3cdev)
> +		return 0;
> +
> +	/* Prevent transfers while cdev removal */
> +	mutex_lock(&i3cdev->xfer_lock);
> +	cdev_del(&i3cdev->cdev);

When cdev_del() returns there might be opened FDs pointing to your
i3cdev [1] ...

> +	device_destroy(i3cdev_class, MKDEV(MAJOR(i3cdev_number), i3cdev->id));
> +	mutex_unlock(&i3cdev->xfer_lock);
> +
> +	ida_simple_remove(&i3cdev_ida, i3cdev->id);
> +	put_i3cdev(i3cdev);

... and you call put_i3cdev() here which frees the i3cdev object,
leading to potential use-after-free if any of the fops (ioctl, read,
write) are called on those dangling FDs. That's exactly the kind of
nightmare I'd like to avoid.

> +
> +	pr_debug("i3cdev: device [%s] unregistered\n", dev_name(&i3c->dev));
> +
> +	return 0;
> +}
> +

[1]https://elixir.bootlin.com/linux/latest/source/fs/char_dev.c#L587

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v3 3/5] i3c: master: add i3c_for_each_dev helper

[Boris Brezillon]

On Fri, 21 Feb 2020 17:44:28 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Fri, 21 Feb 2020 13:59:11 +0100
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> > On Fri, 21 Feb 2020 12:52:29 +0100
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> >   
> > > On Fri, Feb 21, 2020 at 11:47:22AM +0000, Vitor Soares wrote:    
> > > > Hi Greg,
> > > > 
> > > > From: Greg KH <gregkh@linuxfoundation.org>
> > > > Date: Wed, Feb 19, 2020 at 07:35:48
> > > >       
> > > > > On Wed, Feb 19, 2020 at 01:20:41AM +0100, Vitor Soares wrote:      
> > > > > > 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);      
> > > > > 
> > > > > Ick, why the lock?  Are you _sure_ you need that?  The core should
> > > > > handle any list locking issues here, right?      
> > > > 
> > > > I want to make sure that no new devices (eg: Hot-Join capable device) are 
> > > > added during this iteration and after this call, each new device will 
> > > > release a bus notification.
> > > >       
> > > > > 
> > > > > I don't see bus-specific-locks around other subsystem functions that do
> > > > > this (like usb_for_each_dev).      
> > > > 
> > > > I based in I2C use case.      
> > > 
> > > Check to see if this is really needed, for some reason I doubt it...    
> > 
> > Can we please try the spidev approach before fixing those problems. None
> > of that would be needed if we declare the i3cdev driver as a regular
> > i3c_device_driver and let user space bind devices it wants to expose
> > through the sysfs interface. As I said earlier, we even have all the
> > pieces we need to automate that using a udev rule, and the resulting
> > patchset would be 'less invasive'/simpler for pretty much the same
> > result.  
> 
> So, I went ahead and implemented it the way I suggest. The diffstat is
> not representative here (though it's still in favor of this new version)
> since I also changed the way we expose/handle SDR transfers. What's
> most important IMO is the fact that
> 
> * we no longer need to access the internal I3C API
> * we no longer need to care about transitions between i3cdev and
>   other drivers (the core guarantees that a device is always bound to at
>   most one driver)
> * the registration/unregistration procedure is simplified

Looks like I was wrong, there's no way to disable auto-binding and keep
manual binding possible (they both use the bus->match() hook to know if
a dev can be attached to a driver, and the type of binding is not
specified here). There are of course other options to do this:

* add a i3c_driver->match() hook and expose driver-specific sysfs knobs
  to explicitly add devices to attach to this driver
* make the i3cdev module register one driver per-device, each time with
  its own i3c_device_id table and add sysfs knobs to trigger this
  registration

but none of these option sounds reasonable to me, so let's pursue with
your approach.

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH v3 0/5] Introduce i3c device userspace interface

[Vitor Soares]

Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Fri, Feb 21, 2020 at 17:41:16

> > > > 
> > > > I want to make you know that none of your previous comments was ignored 
> > > > and  I would like to start the discussion from this point.  
> > > 
> > > Sure, np. I'll probably wait for a v4 exploring the option I proposed
> > > then.  
> > 
> > I would like to check with you:
> >   - How can we prioritize the device driver over the i3cdev driver if the 
> > driver is loaded after i3cdev? Currently, this is done automatically 
> > without any command, and for me, this is a requirement.
> 
> No devs would be bound to the i3cdev driver by default, it would have
> to be done explicitly through a sysfs knob. Which makes me realize
> we can't use the generic bind knob since it doesn't let the subsystem
> know that it's a manual bind. I thought there was a way to distinguish
> between manual and auto-bind.
> 
> >   - For the ioctl command structure, there is no rule about the way I did 
> > or what you proposed, both are currently used in the kernel. For me it is 
> > one more structure to deal with, can you point the advantages of your 
> > purpose?
> 
> I don't have a strong opinion on that one, though I find it a bit
> easier to follow when the number of xfers is encoded in a separate
> struct rather than extracted from the data size passed through the cmd
> argument.

I will change it then. Do you have any suggestion for the naming to keep 
it short?

> 
> >   - Regarding the ioctl codes, I tried to use those after I2C.
> 
> Why start from 0x30? It doesn't make sense to me. Just because you base
> your code on something that already exists doesn't mean you have to
> copy all of it.

I might be wrong but last I2C command is 0x20 and I tried to let some 
free space in case they need.
Also I think that make sense I2C and I3C share the same 'magic number'.

BTW, in ioctl-numbers documentation there is no reference for code 0x07.

Best regards,
Vitor Soares

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH v3 4/5] i3c: add i3cdev module to expose i3c dev in /dev

[Vitor Soares]

Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Fri, Feb 21, 2020 at 22:32:16

> On Wed, 19 Feb 2020 01:20:42 +0100
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > +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_drvdata(i3c);
> > +	if (!i3cdev)
> > +		return 0;
> > +
> > +	/* Prevent transfers while cdev removal */
> > +	mutex_lock(&i3cdev->xfer_lock);
> > +	cdev_del(&i3cdev->cdev);
> 
> When cdev_del() returns there might be opened FDs pointing to your
> i3cdev [1] ...

Yes, I know. I protected the driver part but I missed the 
file->private_data.

> 
> > +	device_destroy(i3cdev_class, MKDEV(MAJOR(i3cdev_number), i3cdev->id));
> > +	mutex_unlock(&i3cdev->xfer_lock);
> > +
> > +	ida_simple_remove(&i3cdev_ida, i3cdev->id);
> > +	put_i3cdev(i3cdev);
> 
> ... and you call put_i3cdev() here which frees the i3cdev object,
> leading to potential use-after-free if any of the fops (ioctl, read,
> write) are called on those dangling FDs. That's exactly the kind of
> nightmare I'd like to avoid.
> 
> > +
> > +	pr_debug("i3cdev: device [%s] unregistered\n", dev_name(&i3c->dev));
> > +
> > +	return 0;
> > +}
> > +
> 
> [1]https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_latest_source_fs_char-5Fdev.c-23L587&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=tn0F5CeiGtvih3B93GkstU1b2CzoxKklNq4vIvshH8I&s=FwSMjJ-YZFPUB6WCQM0KnFqFSVKp5KdKHdqSiu_0ycU&e= 



_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v3 4/5] i3c: add i3cdev module to expose i3c dev in /dev

[Boris Brezillon]

On Mon, 24 Feb 2020 11:04:50 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> Hi Boris,
> 
> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Fri, Feb 21, 2020 at 22:32:16
> 
> > On Wed, 19 Feb 2020 01:20:42 +0100
> > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> >   
> > > +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_drvdata(i3c);
> > > +	if (!i3cdev)
> > > +		return 0;
> > > +
> > > +	/* Prevent transfers while cdev removal */
> > > +	mutex_lock(&i3cdev->xfer_lock);
> > > +	cdev_del(&i3cdev->cdev);  
> > 
> > When cdev_del() returns there might be opened FDs pointing to your
> > i3cdev [1] ...  
> 
> Yes, I know. I protected the driver part but I missed the 
> file->private_data.

Not sure what you mean by protection, but if you meant locking, then
it's not enough: you need to refcnt the struct if you want to prevent
use-after-free situations.

BTW, I had a closer look at the usbdev implementation, and maybe you
should base yours on usb instead of i2c. They seem to register a cdev at
module init time, and add a dev_t per device at device registration
time. Not sure how they handle the userspace-driver vs kernel-driver
concurrency, but maybe returning EACCES (or EBUSY) instead of
detaching/attaching the i3cdev everytime a device is bound/unbound
would be simpler.

Also, I think Arnd was right, it'd be simpler if i3cdev support was
integrated to the core (still left as a option so it can be disabled,
but with a dedicated i3cdev field in i3c_device instead hijacking the
driver private field).


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v3 0/5] Introduce i3c device userspace interface

[Boris Brezillon]

On Mon, 24 Feb 2020 10:53:25 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> Hi Boris,
> 
> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Fri, Feb 21, 2020 at 17:41:16
> 
> > > > > 
> > > > > I want to make you know that none of your previous comments was ignored 
> > > > > and  I would like to start the discussion from this point.    
> > > > 
> > > > Sure, np. I'll probably wait for a v4 exploring the option I proposed
> > > > then.    
> > > 
> > > I would like to check with you:
> > >   - How can we prioritize the device driver over the i3cdev driver if the 
> > > driver is loaded after i3cdev? Currently, this is done automatically 
> > > without any command, and for me, this is a requirement.  
> > 
> > No devs would be bound to the i3cdev driver by default, it would have
> > to be done explicitly through a sysfs knob. Which makes me realize
> > we can't use the generic bind knob since it doesn't let the subsystem
> > know that it's a manual bind. I thought there was a way to distinguish
> > between manual and auto-bind.
> >   
> > >   - For the ioctl command structure, there is no rule about the way I did 
> > > or what you proposed, both are currently used in the kernel. For me it is 
> > > one more structure to deal with, can you point the advantages of your 
> > > purpose?  
> > 
> > I don't have a strong opinion on that one, though I find it a bit
> > easier to follow when the number of xfers is encoded in a separate
> > struct rather than extracted from the data size passed through the cmd
> > argument.  
> 
> I will change it then. Do you have any suggestion for the naming to keep 
> it short?

I named it i3cdev_priv_xfers in the patch I sent, but you can pick a
different name if you don't like this one.

> 
> >   
> > >   - Regarding the ioctl codes, I tried to use those after I2C.  
> > 
> > Why start from 0x30? It doesn't make sense to me. Just because you base
> > your code on something that already exists doesn't mean you have to
> > copy all of it.  
> 
> I might be wrong but last I2C command is 0x20 and I tried to let some 
> free space in case they need.
> Also I think that make sense I2C and I3C share the same 'magic number'.

Hm, I'm not sure that's a good idea. The set of ioctls for I2C and I3C
are likely to be completely different, so I'd recommend using a
separate namespace (AKA ioctl magic number). Wolfram, any opinion?

> 
> BTW, in ioctl-numbers documentation there is no reference for code 0x07.

Indeed, looks like it's not documented.

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c