[PATCH v6 0/2] staging: ion: get one device per heap

version 6:
- add an ION bus so heap are show as devices in /sys/bus/ion/
  instead of platform bus.
- split the patch in two: one for include reordering and one
  for per heap device change
- rebased on top of next-2017110 tag

version 5:
- create a configuration flag to keep legacy Ion misc device

version 4:
- add a configuration flag to switch between legacy Ion misc device
  and one device per heap version.
  This change has been suggested after Laura talks at XDC 2017.

version 3:
- change ion_device_add_heap prototype to return a possible error.

version 2:
- simplify ioctl check like propose by Dan
- make sure that we don't register more than ION_DEV_MAX heaps.

Until now all ion heaps are addressing using the same device "/dev/ion".
This way of working doesn't allow to give access rights (for example with
SElinux rules) per heap.
This series propose to have one device "/dev/ionX" per heap.
Query heaps informations will be possible on each device node but
allocation request will only be possible if heap_mask_id match with device minor number.

Using legacy Ion misc device is still by setting ION_LEGACY_DEVICE_API
configuration flag.


Benjamin Gaignard (2):
  staging: ion: reorder include
  staging: ion: create one device entry per heap

 drivers/staging/android/TODO            |  1 -
 drivers/staging/android/ion/Kconfig     |  7 +++
 drivers/staging/android/ion/ion-ioctl.c | 18 +++++++-
 drivers/staging/android/ion/ion.c       | 76 +++++++++++++++++++++++++++------
 drivers/staging/android/ion/ion.h       | 15 ++++++-
 5 files changed, 98 insertions(+), 19 deletions(-)

-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Put include in alphabetic order

Signed-off-by: Benjamin Gaignard <redacted>
---
 drivers/staging/android/ion/ion.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index a7d9b0e..fda9756 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c

@@ -15,28 +15,28 @@
  *
  */
 
+#include <linux/anon_inodes.h>
+#include <linux/debugfs.h>
 #include <linux/device.h>
+#include <linux/dma-buf.h>
 #include <linux/err.h>
+#include <linux/export.h>
 #include <linux/file.h>
 #include <linux/freezer.h>
 #include <linux/fs.h>
-#include <linux/anon_inodes.h>
+#include <linux/idr.h>
 #include <linux/kthread.h>
 #include <linux/list.h>
 #include <linux/memblock.h>
 #include <linux/miscdevice.h>
-#include <linux/export.h>
 #include <linux/mm.h>
 #include <linux/mm_types.h>
 #include <linux/rbtree.h>
-#include <linux/slab.h>
+#include <linux/sched/task.h>
 #include <linux/seq_file.h>
+#include <linux/slab.h>
 #include <linux/uaccess.h>
 #include <linux/vmalloc.h>
-#include <linux/debugfs.h>
-#include <linux/dma-buf.h>
-#include <linux/idr.h>
-#include <linux/sched/task.h>
 
 #include "ion.h"
 

-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Instead a getting only one common device "/dev/ion" for
all the heaps this patch allow to create one device
entry ("/dev/ionX") per heap.
Getting an entry per heap could allow to set security rules
per heap and global ones for all heaps.

Allocation requests will be only allowed if the mask_id
match with device minor.
Query request could be done on any of the devices.

Signed-off-by: Benjamin Gaignard <redacted>
---
 drivers/staging/android/TODO            |  1 -
 drivers/staging/android/ion/Kconfig     |  7 ++++
 drivers/staging/android/ion/ion-ioctl.c | 18 ++++++++--
 drivers/staging/android/ion/ion.c       | 62 +++++++++++++++++++++++++++++----
 drivers/staging/android/ion/ion.h       | 15 ++++++--
 5 files changed, 91 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
index 687e0ea..8a11931 100644
--- a/drivers/staging/android/TODO
+++ b/drivers/staging/android/TODO

@@ -8,7 +8,6 @@ TODO:
 ion/
  - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
    involve putting appropriate bindings in a memory node for Ion to find.
- - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
  - Better test framework (integration with VGEM was suggested)
 
 Please send patches to Greg Kroah-Hartman <greg@kroah.com> and Cc:

diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
index a517b2d..cb4666e 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig

@@ -10,6 +10,13 @@ menuconfig ION
 	  If you're not using Android its probably safe to
 	  say N here.
 
+config ION_LEGACY_DEVICE_API
+	bool "Keep using Ion legacy misc device API"
+	depends on ION
+	help
+	  Choose this option to keep using Ion legacy misc device API
+	  i.e. /dev/ion
+
 config ION_SYSTEM_HEAP
 	bool "Ion system heap"
 	depends on ION

diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index e26b786..bb5c77b 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c

@@ -25,7 +25,8 @@ union ion_ioctl_arg {
 	struct ion_heap_query query;
 };
 
-static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
+static int validate_ioctl_arg(struct file *filp,
+			      unsigned int cmd, union ion_ioctl_arg *arg)
 {
 	switch (cmd) {
 	case ION_IOC_HEAP_QUERY:

@@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
 		    arg->query.reserved2 )
 			return -EINVAL;
 		break;
+
+	case ION_IOC_ALLOC:
+	{
+		int mask = 1 << iminor(filp->f_inode);
+
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+		if (imajor(filp->f_inode) == MISC_MAJOR)
+			return 0;
+#endif
+		if (!(arg->allocation.heap_id_mask & mask))
+			return -EINVAL;
+		break;
+	}
 	default:
 		break;
 	}

@@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
 		return -EFAULT;
 
-	ret = validate_ioctl_arg(cmd, &data);
+	ret = validate_ioctl_arg(filp, cmd, &data);
 	if (WARN_ON_ONCE(ret))
 		return ret;
 

diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index fda9756..2c2568b 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c

@@ -40,6 +40,9 @@
 
 #include "ion.h"
 
+#define ION_DEV_MAX 32
+#define ION_NAME "ion"
+
 static struct ion_device *internal_dev;
 static int heap_id;
 

@@ -535,15 +538,38 @@ static int debug_shrink_get(void *data, u64 *val)
 DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
 			debug_shrink_set, "%llu\n");
 
-void ion_device_add_heap(struct ion_heap *heap)
+static struct device ion_bus = {
+	.init_name = ION_NAME,
+};
+
+static struct bus_type ion_bus_type = {
+	.name = ION_NAME,
+};
+
+int ion_device_add_heap(struct ion_heap *heap)
 {
 	struct dentry *debug_file;
 	struct ion_device *dev = internal_dev;
+	int ret = 0;
 
 	if (!heap->ops->allocate || !heap->ops->free)
 		pr_err("%s: can not add heap with invalid ops struct.\n",
 		       __func__);
 
+	if (heap_id >= ION_DEV_MAX)
+		return -EBUSY;
+
+	heap->ddev.parent = &ion_bus;
+	heap->ddev.bus = &ion_bus_type;
+	heap->ddev.devt = MKDEV(MAJOR(internal_dev->devt), heap_id);
+	dev_set_name(&heap->ddev, ION_NAME"%d", heap_id);
+	device_initialize(&heap->ddev);
+	cdev_init(&heap->chrdev, &ion_fops);
+	heap->chrdev.owner = THIS_MODULE;
+	ret = cdev_device_add(&heap->chrdev, &heap->ddev);
+	if (ret < 0)
+		return ret;
+
 	spin_lock_init(&heap->free_lock);
 	heap->free_list_size = 0;
 

@@ -581,6 +607,8 @@ void ion_device_add_heap(struct ion_heap *heap)
 
 	dev->heap_cnt++;
 	up_write(&dev->lock);
+
+	return ret;
 }
 EXPORT_SYMBOL(ion_device_add_heap);
 

@@ -593,8 +621,9 @@ static int ion_device_create(void)
 	if (!idev)
 		return -ENOMEM;
 
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
 	idev->dev.minor = MISC_DYNAMIC_MINOR;
-	idev->dev.name = "ion";
+	idev->dev.name = ION_NAME;
 	idev->dev.fops = &ion_fops;
 	idev->dev.parent = NULL;
 	ret = misc_register(&idev->dev);

@@ -603,19 +632,38 @@ static int ion_device_create(void)
 		kfree(idev);
 		return ret;
 	}
+#endif
 
-	idev->debug_root = debugfs_create_dir("ion", NULL);
-	if (!idev->debug_root) {
+	ret = device_register(&ion_bus);
+	if (ret)
+		goto clean_misc;
+
+	ret = bus_register(&ion_bus_type);
+	if (ret)
+		goto clean_device;
+
+	ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, ION_NAME);
+	if (ret)
+		goto clean_device;
+
+	idev->debug_root = debugfs_create_dir(ION_NAME, NULL);
+	if (!idev->debug_root)
 		pr_err("ion: failed to create debugfs root directory.\n");
-		goto debugfs_done;
-	}
 
-debugfs_done:
 	idev->buffers = RB_ROOT;
 	mutex_init(&idev->buffer_lock);
 	init_rwsem(&idev->lock);
 	plist_head_init(&idev->heaps);
 	internal_dev = idev;
 	return 0;
+
+clean_device:
+	device_unregister(&ion_bus);
+clean_misc:
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+	misc_deregister(&idev->dev);
+#endif
+	kfree(idev);
+	return ret;
 }
 subsys_initcall(ion_device_create);

diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
index f5f9cd6..4869e96 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h

@@ -17,16 +17,19 @@
 #ifndef _ION_H
 #define _ION_H
 
+#include <linux/cdev.h>
 #include <linux/device.h>
 #include <linux/dma-direction.h>
 #include <linux/kref.h>
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+#include <linux/miscdevice.h>
+#endif
 #include <linux/mm_types.h>
 #include <linux/mutex.h>
 #include <linux/rbtree.h>
 #include <linux/sched.h>
 #include <linux/shrinker.h>
 #include <linux/types.h>
-#include <linux/miscdevice.h>
 
 #include "../uapi/ion.h"
 

@@ -92,12 +95,16 @@ void ion_buffer_destroy(struct ion_buffer *buffer);
 /**
  * struct ion_device - the metadata of the ion device node
  * @dev:		the actual misc device
+ * @devt:		Ion device
  * @buffers:		an rb tree of all the existing buffers
  * @buffer_lock:	lock protecting the tree of buffers
  * @lock:		rwsem protecting the tree of heaps and clients
  */
 struct ion_device {
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
 	struct miscdevice dev;
+#endif
+	dev_t devt;
 	struct rb_root buffers;
 	struct mutex buffer_lock;
 	struct rw_semaphore lock;

@@ -153,6 +160,8 @@ struct ion_heap_ops {
  * struct ion_heap - represents a heap in the system
  * @node:		rb node to put the heap on the device's tree of heaps
  * @dev:		back pointer to the ion_device
+ * @ddev:		device structure
+ * @chrdev:		associated character device
  * @type:		type of heap
  * @ops:		ops struct as above
  * @flags:		flags

@@ -177,6 +186,8 @@ struct ion_heap_ops {
 struct ion_heap {
 	struct plist_node node;
 	struct ion_device *dev;
+	struct device ddev;
+	struct cdev chrdev;
 	enum ion_heap_type type;
 	struct ion_heap_ops *ops;
 	unsigned long flags;

@@ -213,7 +224,7 @@ bool ion_buffer_fault_user_mappings(struct ion_buffer *buffer);
  * ion_device_add_heap - adds a heap to the ion device
  * @heap:		the heap to add
  */
-void ion_device_add_heap(struct ion_heap *heap);
+int ion_device_add_heap(struct ion_heap *heap);
 
 /**
  * some helpers for common operations on buffers using the sg_table

-- 
2.7.4

On 11/06/2017 07:59 AM, Benjamin Gaignard wrote:

Put include in alphabetic order

Acked-by: Laura Abbott <redacted>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

On 11/06/2017 07:59 AM, Benjamin Gaignard wrote:

Instead a getting only one common device "/dev/ion" for
all the heaps this patch allow to create one device
entry ("/dev/ionX") per heap.
Getting an entry per heap could allow to set security rules
per heap and global ones for all heaps.

Allocation requests will be only allowed if the mask_id
match with device minor.
Query request could be done on any of the devices.

With this patch, sysfs looks like:

$ ls /sys/devices/
breakpoint ion platform software system virtual

 From an Ion perspective, you can have

Acked-by: Laura Abbott <redacted>

Another Ack for the device model stuff would be good but I'll
assume deafening silence means nobody hates it.

Thanks,
Laura

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

2017-11-09 22:17 GMT+01:00 Laura Abbott [off-list ref]:

On 11/06/2017 07:59 AM, Benjamin Gaignard wrote:

quoted

Instead a getting only one common device "/dev/ion" for
all the heaps this patch allow to create one device
entry ("/dev/ionX") per heap.
Getting an entry per heap could allow to set security rules
per heap and global ones for all heaps.

Allocation requests will be only allowed if the mask_id
match with device minor.
Query request could be done on any of the devices.

With this patch, sysfs looks like:

$ ls /sys/devices/
breakpoint ion platform software system virtual

From an Ion perspective, you can have

Acked-by: Laura Abbott <redacted>

Another Ack for the device model stuff would be good but I'll
assume deafening silence means nobody hates it.

Greg, can we get your point of view of this ?

Thanks,
Benjamin

Thanks,
Laura

quoted

Signed-off-by: Benjamin Gaignard <redacted>
---
  drivers/staging/android/TODO            |  1 -
  drivers/staging/android/ion/Kconfig     |  7 ++++
  drivers/staging/android/ion/ion-ioctl.c | 18 ++++++++--
  drivers/staging/android/ion/ion.c       | 62
+++++++++++++++++++++++++++++----
  drivers/staging/android/ion/ion.h       | 15 ++++++--
  5 files changed, 91 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
index 687e0ea..8a11931 100644
--- a/drivers/staging/android/TODO
+++ b/drivers/staging/android/TODO

@@ -8,7 +8,6 @@ TODO:
  ion/
   - Add dt-bindings for remaining heaps (chunk and carveout heaps). This

would
     involve putting appropriate bindings in a memory node for Ion to
find.
- - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
   - Better test framework (integration with VGEM was suggested)
    Please send patches to Greg Kroah-Hartman [off-list ref] and Cc:

diff --git a/drivers/staging/android/ion/Kconfig

b/drivers/staging/android/ion/Kconfig
index a517b2d..cb4666e 100644

--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig

@@ -10,6 +10,13 @@ menuconfig ION
          If you're not using Android its probably safe to
          say N here.
  +config ION_LEGACY_DEVICE_API
+       bool "Keep using Ion legacy misc device API"
+       depends on ION
+       help
+         Choose this option to keep using Ion legacy misc device API
+         i.e. /dev/ion
+
  config ION_SYSTEM_HEAP
        bool "Ion system heap"
        depends on ION

diff --git a/drivers/staging/android/ion/ion-ioctl.c

b/drivers/staging/android/ion/ion-ioctl.c
index e26b786..bb5c77b 100644

--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c

@@ -25,7 +25,8 @@ union ion_ioctl_arg {
        struct ion_heap_query query;
  };
  -static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg

*arg)
+static int validate_ioctl_arg(struct file *filp,
+                             unsigned int cmd, union ion_ioctl_arg *arg)
  {
        switch (cmd) {
        case ION_IOC_HEAP_QUERY:

@@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union

ion_ioctl_arg *arg)
                    arg->query.reserved2 )
                        return -EINVAL;
                break;
+
+       case ION_IOC_ALLOC:
+       {
+               int mask = 1 << iminor(filp->f_inode);
+
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+               if (imajor(filp->f_inode) == MISC_MAJOR)
+                       return 0;
+#endif
+               if (!(arg->allocation.heap_id_mask & mask))
+                       return -EINVAL;
+               break;
+       }
        default:
                break;
        }

@@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd,

unsigned long arg)
        if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
                return -EFAULT;
  -     ret = validate_ioctl_arg(cmd, &data);
+       ret = validate_ioctl_arg(filp, cmd, &data);
        if (WARN_ON_ONCE(ret))
                return ret;
  diff --git a/drivers/staging/android/ion/ion.c
b/drivers/staging/android/ion/ion.c
index fda9756..2c2568b 100644

--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c

@@ -40,6 +40,9 @@
    #include "ion.h"
  +#define ION_DEV_MAX 32
+#define ION_NAME "ion"
+
  static struct ion_device *internal_dev;
  static int heap_id;
  @@ -535,15 +538,38 @@ static int debug_shrink_get(void *data, u64 *val)
  DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
                        debug_shrink_set, "%llu\n");
  -void ion_device_add_heap(struct ion_heap *heap)
+static struct device ion_bus = {
+       .init_name = ION_NAME,
+};
+
+static struct bus_type ion_bus_type = {
+       .name = ION_NAME,
+};
+
+int ion_device_add_heap(struct ion_heap *heap)
  {
        struct dentry *debug_file;
        struct ion_device *dev = internal_dev;
+       int ret = 0;
        if (!heap->ops->allocate || !heap->ops->free)
                pr_err("%s: can not add heap with invalid ops struct.\n",
                       __func__);
  +     if (heap_id >= ION_DEV_MAX)
+               return -EBUSY;
+
+       heap->ddev.parent = &ion_bus;
+       heap->ddev.bus = &ion_bus_type;
+       heap->ddev.devt = MKDEV(MAJOR(internal_dev->devt), heap_id);
+       dev_set_name(&heap->ddev, ION_NAME"%d", heap_id);
+       device_initialize(&heap->ddev);
+       cdev_init(&heap->chrdev, &ion_fops);
+       heap->chrdev.owner = THIS_MODULE;
+       ret = cdev_device_add(&heap->chrdev, &heap->ddev);
+       if (ret < 0)
+               return ret;
+
        spin_lock_init(&heap->free_lock);
        heap->free_list_size = 0;
  @@ -581,6 +607,8 @@ void ion_device_add_heap(struct ion_heap *heap)
        dev->heap_cnt++;
        up_write(&dev->lock);
+
+       return ret;
  }
  EXPORT_SYMBOL(ion_device_add_heap);
  @@ -593,8 +621,9 @@ static int ion_device_create(void)
        if (!idev)
                return -ENOMEM;
  +#ifdef CONFIG_ION_LEGACY_DEVICE_API
        idev->dev.minor = MISC_DYNAMIC_MINOR;
-       idev->dev.name = "ion";
+       idev->dev.name = ION_NAME;
        idev->dev.fops = &ion_fops;
        idev->dev.parent = NULL;
        ret = misc_register(&idev->dev);

@@ -603,19 +632,38 @@ static int ion_device_create(void)
                kfree(idev);
                return ret;
        }
+#endif
  -     idev->debug_root = debugfs_create_dir("ion", NULL);
-       if (!idev->debug_root) {
+       ret = device_register(&ion_bus);
+       if (ret)
+               goto clean_misc;
+
+       ret = bus_register(&ion_bus_type);
+       if (ret)
+               goto clean_device;
+
+       ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, ION_NAME);
+       if (ret)
+               goto clean_device;
+
+       idev->debug_root = debugfs_create_dir(ION_NAME, NULL);
+       if (!idev->debug_root)
                pr_err("ion: failed to create debugfs root directory.\n");
-               goto debugfs_done;
-       }
  -debugfs_done:
        idev->buffers = RB_ROOT;
        mutex_init(&idev->buffer_lock);
        init_rwsem(&idev->lock);
        plist_head_init(&idev->heaps);
        internal_dev = idev;
        return 0;
+
+clean_device:
+       device_unregister(&ion_bus);
+clean_misc:
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+       misc_deregister(&idev->dev);
+#endif
+       kfree(idev);
+       return ret;
  }
  subsys_initcall(ion_device_create);

diff --git a/drivers/staging/android/ion/ion.h

b/drivers/staging/android/ion/ion.h
index f5f9cd6..4869e96 100644

--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h

@@ -17,16 +17,19 @@
  #ifndef _ION_H
  #define _ION_H
  +#include <linux/cdev.h>
  #include <linux/device.h>
  #include <linux/dma-direction.h>
  #include <linux/kref.h>
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+#include <linux/miscdevice.h>
+#endif
  #include <linux/mm_types.h>
  #include <linux/mutex.h>
  #include <linux/rbtree.h>
  #include <linux/sched.h>
  #include <linux/shrinker.h>
  #include <linux/types.h>
-#include <linux/miscdevice.h>
    #include "../uapi/ion.h"
  @@ -92,12 +95,16 @@ void ion_buffer_destroy(struct ion_buffer *buffer);
  /**
   * struct ion_device - the metadata of the ion device node
   * @dev:              the actual misc device
+ * @devt:              Ion device
   * @buffers:          an rb tree of all the existing buffers
   * @buffer_lock:      lock protecting the tree of buffers
   * @lock:             rwsem protecting the tree of heaps and clients
   */
  struct ion_device {
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
        struct miscdevice dev;
+#endif
+       dev_t devt;
        struct rb_root buffers;
        struct mutex buffer_lock;
        struct rw_semaphore lock;

@@ -153,6 +160,8 @@ struct ion_heap_ops {
   * struct ion_heap - represents a heap in the system
   * @node:             rb node to put the heap on the device's tree of

heaps
   * @dev:              back pointer to the ion_device
+ * @ddev:              device structure
+ * @chrdev:            associated character device
   * @type:             type of heap
   * @ops:              ops struct as above
   * @flags:            flags

@@ -177,6 +186,8 @@ struct ion_heap_ops {
  struct ion_heap {
        struct plist_node node;
        struct ion_device *dev;
+       struct device ddev;
+       struct cdev chrdev;
        enum ion_heap_type type;
        struct ion_heap_ops *ops;
        unsigned long flags;

@@ -213,7 +224,7 @@ bool ion_buffer_fault_user_mappings(struct ion_buffer

*buffer);
   * ion_device_add_heap - adds a heap to the ion device
   * @heap:             the heap to add
   */
-void ion_device_add_heap(struct ion_heap *heap);
+int ion_device_add_heap(struct ion_heap *heap);
    /**
   * some helpers for common operations on buffers using the sg_table

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Mon, Nov 27, 2017 at 11:46:18AM +0100, Benjamin Gaignard wrote:

2017-11-09 22:17 GMT+01:00 Laura Abbott [off-list ref]:

quoted

On 11/06/2017 07:59 AM, Benjamin Gaignard wrote:

quoted

Instead a getting only one common device "/dev/ion" for
all the heaps this patch allow to create one device
entry ("/dev/ionX") per heap.
Getting an entry per heap could allow to set security rules
per heap and global ones for all heaps.

Allocation requests will be only allowed if the mask_id
match with device minor.
Query request could be done on any of the devices.

With this patch, sysfs looks like:

$ ls /sys/devices/
breakpoint ion platform software system virtual

From an Ion perspective, you can have

Acked-by: Laura Abbott <redacted>

Another Ack for the device model stuff would be good but I'll
assume deafening silence means nobody hates it.

Greg, can we get your point of view of this ?

It's 1 day after the merge window has closed, and my todo patch queue
looks like this:
	$ mdfrm -c ~/mail/todo/
	1523 messages in /home/gregkh/mail/todo/

Please give me a chance to catch up...

greg k-h

On Mon, Nov 27, 2017 at 12:43:57PM +0100, Greg Kroah-Hartman wrote:

On Mon, Nov 27, 2017 at 11:46:18AM +0100, Benjamin Gaignard wrote:

quoted

2017-11-09 22:17 GMT+01:00 Laura Abbott [off-list ref]:

quoted

On 11/06/2017 07:59 AM, Benjamin Gaignard wrote:

quoted

Instead a getting only one common device "/dev/ion" for
all the heaps this patch allow to create one device
entry ("/dev/ionX") per heap.
Getting an entry per heap could allow to set security rules
per heap and global ones for all heaps.

Allocation requests will be only allowed if the mask_id
match with device minor.
Query request could be done on any of the devices.

With this patch, sysfs looks like:

$ ls /sys/devices/
breakpoint ion platform software system virtual

From an Ion perspective, you can have

Acked-by: Laura Abbott <redacted>

Another Ack for the device model stuff would be good but I'll
assume deafening silence means nobody hates it.

Greg, can we get your point of view of this ?

It's 1 day after the merge window has closed, and my todo patch queue
looks like this:
	$ mdfrm -c ~/mail/todo/
	1523 messages in /home/gregkh/mail/todo/

Please give me a chance to catch up...

commit model ftw, we have 400+ patches for 4.16 already merged and tested
and all ready, right when -rc1 gets tagged. Makes the merge window the
most relaxed time of all, because all the other maintainers are drowning
and wont pester you.

Just saying, this is an entirely fixable problem :-P

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Mon, Nov 27, 2017 at 05:12:23PM +0100, Daniel Vetter wrote:

commit model ftw, we have 400+ patches for 4.16 already merged and tested
and all ready, right when -rc1 gets tagged. Makes the merge window the
most relaxed time of all, because all the other maintainers are drowning
and wont pester you.

Just saying, this is an entirely fixable problem :-P

Or even just pre-review things and flag them in a mailbox if you want to
wait for -rc1 before applying things.

On Mon, Nov 06, 2017 at 04:59:44PM +0100, Benjamin Gaignard wrote:

Put include in alphabetic order

Why???

That should not matter at all.

I'll take this, but really, ick ick ick ick ick.

On Mon, Nov 06, 2017 at 04:59:45PM +0100, Benjamin Gaignard wrote:

You want to default to Y here, so when you do 'make oldconfig' nothing
breaks, right?

+	help
+	  Choose this option to keep using Ion legacy misc device API
+	  i.e. /dev/ion

You need more text here to describe the trade offs.  Why would I not
want to keep doing this?  What does turning this off get me?  What does
keeping it on keep me from doing?

Why return 0?  Because it is already allocated?

Some comments here as to exactly what you are doing would be nice.

+#endif
+		if (!(arg->allocation.heap_id_mask & mask))

This doesn't allocate anthing, just check to see if it is?

Why 32?  Where did that number come from?

Oh look, a struct device on the stack.  Watch bad things happen :(

This is a dynamic device, make it dynamic, or else you had better know
exactly what you are doing...

+
+static struct bus_type ion_bus_type = {
+	.name = ION_NAME,
+};
+
+int ion_device_add_heap(struct ion_heap *heap)
 {
 	struct dentry *debug_file;
 	struct ion_device *dev = internal_dev;
+	int ret = 0;

Why set this to 0?

 
 	if (!heap->ops->allocate || !heap->ops->free)
 		pr_err("%s: can not add heap with invalid ops struct.\n",
 		       __func__);
 
+	if (heap_id >= ION_DEV_MAX)
+		return -EBUSY;

Busy for an invalid value?

You call device_register for something you are calling a bus???  Are you
_sure_ about this?

What exactly are you creating in sysfs here?  You are throwing around
"raw" devices, which is almost never what you ever want to do.
Especially for some random char driver.

+	if (ret)
+		goto clean_misc;
+
+	ret = bus_register(&ion_bus_type);
+	if (ret)
+		goto clean_device;
+
+	ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, ION_NAME);
+	if (ret)
+		goto clean_device;
+
+	idev->debug_root = debugfs_create_dir(ION_NAME, NULL);
+	if (!idev->debug_root)

Why do you care about this?  (hint, never care about any debugfs
call...)

Why do you need a #ifdef here?

devt?  That's not a "device", it's a dev_t, which means something else,
right?

  * @buffers:		an rb tree of all the existing buffers
  * @buffer_lock:	lock protecting the tree of buffers
  * @lock:		rwsem protecting the tree of heaps and clients
  */
 struct ion_device {
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
 	struct miscdevice dev;
+#endif

Why use #ifdef?

How many different reference counted objects do you now have in this
structure?  And what exactly is the lifetime rules involved in them?

Hint, you never tried removing any of these from the system, otherwise
you would have seen the kernel warnings...

Step back here, what exactly do you want to do?  What do you expect
sysfs to look like?  What do you want /dev/ to look like?

Where is the documentation for the new sysfs files and the new ioctl
call you added?  What did you do to test this out?  Where are the AOSP
patches to use this?  Happen to have a VTS test for it?

This needs a lot more work, if for no other reason than the integration
into the driver model is totally wrong and will blow up into tiny
pieces...

thanks,

greg k-h

2017-11-28 14:20 GMT+01:00 Greg KH [off-list ref]:

On Mon, Nov 06, 2017 at 04:59:44PM +0100, Benjamin Gaignard wrote:

quoted

Put include in alphabetic order

Why???

Mainly because the next patch in the series adds new includes and I have
decide to split clean-up and new feature patches

That should not matter at all.

I'll take this, but really, ick ick ick ick ick.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Tue, Nov 28, 2017 at 02:34:00PM +0100, Benjamin Gaignard wrote:

2017-11-28 14:20 GMT+01:00 Greg KH [off-list ref]:

quoted

On Mon, Nov 06, 2017 at 04:59:44PM +0100, Benjamin Gaignard wrote:

quoted

Put include in alphabetic order

Why???

Mainly because the next patch in the series adds new includes and I have
decide to split clean-up and new feature patches

That's fine, but this really isn't needed for any type of "clean-up" at
all.  But oh well, if you all like it that way, I'm not going to
complain that much :)

greg k-h
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Tue, Nov 28, 2017 at 02:32:17PM +0100, Greg KH wrote:

Where is the documentation for the new sysfs files and the new ioctl

Didn't see any sysfs files in there?

call you added?  What did you do to test this out?  Where are the AOSP
patches to use this?  Happen to have a VTS test for it?

Do we need to convert Android for this to be accepted?  The single
device is being kept around for it and the use case was from non-Android
users wasn't it?

On Tue, Nov 28, 2017 at 04:26:20PM +0000, Mark Brown wrote:

On Tue, Nov 28, 2017 at 02:32:17PM +0100, Greg KH wrote:

quoted

Where is the documentation for the new sysfs files and the new ioctl

Didn't see any sysfs files in there?

New struct devices were created and registered.  Why would that happen
if there was not a need for sysfs files? :)

quoted

call you added?  What did you do to test this out?  Where are the AOSP
patches to use this?  Happen to have a VTS test for it?

Do we need to convert Android for this to be accepted?  The single
device is being kept around for it and the use case was from non-Android
users wasn't it?

So we are just going to add kernel features with no userspace users of
it at all?  Why would we do that?  How was it even tested?

greg k-h
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Tue, Nov 28, 2017 at 06:08:22PM +0100, Greg KH wrote:

On Tue, Nov 28, 2017 at 04:26:20PM +0000, Mark Brown wrote:

quoted

On Tue, Nov 28, 2017 at 02:32:17PM +0100, Greg KH wrote:

quoted

quoted

call you added?  What did you do to test this out?  Where are the AOSP
patches to use this?  Happen to have a VTS test for it?

quoted

Do we need to convert Android for this to be accepted?  The single
device is being kept around for it and the use case was from non-Android
users wasn't it?

So we are just going to add kernel features with no userspace users of
it at all?  Why would we do that?  How was it even tested?

I think it's reasonable to ask for userspace, I'm querying why it needs
to specifically be Android.

On Tue, Nov 28, 2017 at 05:12:23PM +0000, Mark Brown wrote:

On Tue, Nov 28, 2017 at 06:08:22PM +0100, Greg KH wrote:

quoted

On Tue, Nov 28, 2017 at 04:26:20PM +0000, Mark Brown wrote:

quoted

On Tue, Nov 28, 2017 at 02:32:17PM +0100, Greg KH wrote:

quoted

quoted

quoted

call you added?  What did you do to test this out?  Where are the AOSP
patches to use this?  Happen to have a VTS test for it?

quoted

quoted

Do we need to convert Android for this to be accepted?  The single
device is being kept around for it and the use case was from non-Android
users wasn't it?

quoted

So we are just going to add kernel features with no userspace users of
it at all?  Why would we do that?  How was it even tested?

I think it's reasonable to ask for userspace, I'm querying why it needs
to specifically be Android.

Does anyone other than Android use this interface?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Tue, Nov 28, 2017 at 06:28:38PM +0100, Greg KH wrote:

On Tue, Nov 28, 2017 at 05:12:23PM +0000, Mark Brown wrote:

quoted

I think it's reasonable to ask for userspace, I'm querying why it needs
to specifically be Android.

Does anyone other than Android use this interface?

There's plenty of other userspaces that have the same requirements for
allocation for applications like video capture, I believe some of them
have actually moved to ION already and they're certainly where some of
the requirements here are coming from.

On Tue, Nov 28, 2017 at 05:37:53PM +0000, Mark Brown wrote:

On Tue, Nov 28, 2017 at 06:28:38PM +0100, Greg KH wrote:

quoted

On Tue, Nov 28, 2017 at 05:12:23PM +0000, Mark Brown wrote:

quoted

quoted

I think it's reasonable to ask for userspace, I'm querying why it needs
to specifically be Android.

quoted

Does anyone other than Android use this interface?

There's plenty of other userspaces that have the same requirements for
allocation for applications like video capture, I believe some of them
have actually moved to ION already and they're certainly where some of
the requirements here are coming from.

Then the Kconfig option should start to describe some of this :)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

2017-11-28 14:32 GMT+01:00 Greg KH [off-list ref]:

On Mon, Nov 06, 2017 at 04:59:45PM +0100, Benjamin Gaignard wrote:

quoted

Instead a getting only one common device "/dev/ion" for
all the heaps this patch allow to create one device
entry ("/dev/ionX") per heap.
Getting an entry per heap could allow to set security rules
per heap and global ones for all heaps.

Allocation requests will be only allowed if the mask_id
match with device minor.
Query request could be done on any of the devices.

Signed-off-by: Benjamin Gaignard <redacted>
---
 drivers/staging/android/TODO            |  1 -
 drivers/staging/android/ion/Kconfig     |  7 ++++
 drivers/staging/android/ion/ion-ioctl.c | 18 ++++++++--
 drivers/staging/android/ion/ion.c       | 62 +++++++++++++++++++++++++++++----
 drivers/staging/android/ion/ion.h       | 15 ++++++--
 5 files changed, 91 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
index 687e0ea..8a11931 100644
--- a/drivers/staging/android/TODO
+++ b/drivers/staging/android/TODO

@@ -8,7 +8,6 @@ TODO:
 ion/
  - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
    involve putting appropriate bindings in a memory node for Ion to find.
- - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
  - Better test framework (integration with VGEM was suggested)

 Please send patches to Greg Kroah-Hartman <greg@kroah.com> and Cc:

diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
index a517b2d..cb4666e 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig

@@ -10,6 +10,13 @@ menuconfig ION
        If you're not using Android its probably safe to
        say N here.

+config ION_LEGACY_DEVICE_API
+     bool "Keep using Ion legacy misc device API"
+     depends on ION

You want to default to Y here, so when you do 'make oldconfig' nothing
breaks, right?

I will add it.

quoted

+     help
+       Choose this option to keep using Ion legacy misc device API
+       i.e. /dev/ion

You need more text here to describe the trade offs.  Why would I not
want to keep doing this?  What does turning this off get me?  What does
keeping it on keep me from doing?

Does describe it like that sound better ?
"Choose this option to keep using ION legacy misc device API
  i.e. /dev/ion. If this option isn't selected you will only
  have per heap device node (i.e /dev/ionX) and allocating buffer
  from an unique device node won't be possible."

quoted

+
 config ION_SYSTEM_HEAP
      bool "Ion system heap"
      depends on ION

diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index e26b786..bb5c77b 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c

@@ -25,7 +25,8 @@ union ion_ioctl_arg {
      struct ion_heap_query query;
 };

-static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
+static int validate_ioctl_arg(struct file *filp,
+                           unsigned int cmd, union ion_ioctl_arg *arg)
 {
      switch (cmd) {
      case ION_IOC_HEAP_QUERY:

@@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
                  arg->query.reserved2 )
                      return -EINVAL;
              break;
+
+     case ION_IOC_ALLOC:
+     {
+             int mask = 1 << iminor(filp->f_inode);
+
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+             if (imajor(filp->f_inode) == MISC_MAJOR)
+                     return 0;

Why return 0?  Because it is already allocated?

No it is because in legacy mode all mask are valids so to keep legacy behavoir
it will not test if the requested heap  match with the device.

I will add a comment about that.

Some comments here as to exactly what you are doing would be nice.

quoted

+#endif
+             if (!(arg->allocation.heap_id_mask & mask))

This doesn't allocate anthing, just check to see if it is?

No this function only check if ioctl args are correct.

quoted

+                     return -EINVAL;
+             break;
+     }
      default:
              break;
      }

@@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
      if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
              return -EFAULT;

-     ret = validate_ioctl_arg(cmd, &data);
+     ret = validate_ioctl_arg(filp, cmd, &data);
      if (WARN_ON_ONCE(ret))
              return ret;

diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index fda9756..2c2568b 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c

@@ -40,6 +40,9 @@

 #include "ion.h"

+#define ION_DEV_MAX 32

Why 32?  Where did that number come from?

It is the size of heap_id_mask field is struct ion_allocation_data.
I will use FIELD_SIZEOF(struct ion_allocation_data, heap_id_mask) instead.

quoted

+#define ION_NAME "ion"
+
 static struct ion_device *internal_dev;
 static int heap_id;

@@ -535,15 +538,38 @@ static int debug_shrink_get(void *data, u64 *val)
 DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
                      debug_shrink_set, "%llu\n");

-void ion_device_add_heap(struct ion_heap *heap)
+static struct device ion_bus = {
+     .init_name = ION_NAME,
+};

Oh look, a struct device on the stack.  Watch bad things happen :(

This is a dynamic device, make it dynamic, or else you had better know
exactly what you are doing...

The naming is bad here, I will rename it ion_parent because I use it to provide
a parent to ion heap device either they won't go in /sys/bus/ion

quoted

+
+static struct bus_type ion_bus_type = {
+     .name = ION_NAME,
+};
+
+int ion_device_add_heap(struct ion_heap *heap)
 {
      struct dentry *debug_file;
      struct ion_device *dev = internal_dev;
+     int ret = 0;

Why set this to 0?

I agree it is useless

quoted

      if (!heap->ops->allocate || !heap->ops->free)
              pr_err("%s: can not add heap with invalid ops struct.\n",
                     __func__);

+     if (heap_id >= ION_DEV_MAX)
+             return -EBUSY;

Busy for an invalid value?

I will change it to -EINVAL

quoted

+
+     heap->ddev.parent = &ion_bus;
+     heap->ddev.bus = &ion_bus_type;
+     heap->ddev.devt = MKDEV(MAJOR(internal_dev->devt), heap_id);
+     dev_set_name(&heap->ddev, ION_NAME"%d", heap_id);
+     device_initialize(&heap->ddev);
+     cdev_init(&heap->chrdev, &ion_fops);
+     heap->chrdev.owner = THIS_MODULE;
+     ret = cdev_device_add(&heap->chrdev, &heap->ddev);
+     if (ret < 0)
+             return ret;
+
      spin_lock_init(&heap->free_lock);
      heap->free_list_size = 0;

@@ -581,6 +607,8 @@ void ion_device_add_heap(struct ion_heap *heap)

      dev->heap_cnt++;
      up_write(&dev->lock);
+
+     return ret;
 }
 EXPORT_SYMBOL(ion_device_add_heap);

@@ -593,8 +621,9 @@ static int ion_device_create(void)
      if (!idev)
              return -ENOMEM;

+#ifdef CONFIG_ION_LEGACY_DEVICE_API
      idev->dev.minor = MISC_DYNAMIC_MINOR;
-     idev->dev.name = "ion";
+     idev->dev.name = ION_NAME;
      idev->dev.fops = &ion_fops;
      idev->dev.parent = NULL;
      ret = misc_register(&idev->dev);

@@ -603,19 +632,38 @@ static int ion_device_create(void)
              kfree(idev);
              return ret;
      }
+#endif

-     idev->debug_root = debugfs_create_dir("ion", NULL);
-     if (!idev->debug_root) {
+     ret = device_register(&ion_bus);

You call device_register for something you are calling a bus???  Are you
_sure_ about this?

What exactly are you creating in sysfs here?  You are throwing around
"raw" devices, which is almost never what you ever want to do.
Especially for some random char driver.

ion heap devices need a parent to be correctly put in /sys/bus/ion either they
will go directly in /sys/bus directory. You are right name it ion_bus
is confusing
I will rename it ion_parent.

quoted

+     if (ret)
+             goto clean_misc;
+
+     ret = bus_register(&ion_bus_type);
+     if (ret)
+             goto clean_device;
+
+     ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, ION_NAME);
+     if (ret)
+             goto clean_device;
+
+     idev->debug_root = debugfs_create_dir(ION_NAME, NULL);
+     if (!idev->debug_root)

Why do you care about this?  (hint, never care about any debugfs
call...)

I will remove it

quoted

              pr_err("ion: failed to create debugfs root directory.\n");
-             goto debugfs_done;
-     }

-debugfs_done:
      idev->buffers = RB_ROOT;
      mutex_init(&idev->buffer_lock);
      init_rwsem(&idev->lock);
      plist_head_init(&idev->heaps);
      internal_dev = idev;
      return 0;
+
+clean_device:
+     device_unregister(&ion_bus);
+clean_misc:
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+     misc_deregister(&idev->dev);
+#endif
+     kfree(idev);
+     return ret;
 }
 subsys_initcall(ion_device_create);

diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
index f5f9cd6..4869e96 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h

@@ -17,16 +17,19 @@
 #ifndef _ION_H
 #define _ION_H

+#include <linux/cdev.h>
 #include <linux/device.h>
 #include <linux/dma-direction.h>
 #include <linux/kref.h>
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+#include <linux/miscdevice.h>
+#endif

Why do you need a #ifdef here?

I have put all misc related call under CONFIG_ION_LEGACY_DEVICE_API
flag but I will remove it from .h file.

quoted

 #include <linux/mm_types.h>
 #include <linux/mutex.h>
 #include <linux/rbtree.h>
 #include <linux/sched.h>
 #include <linux/shrinker.h>
 #include <linux/types.h>
-#include <linux/miscdevice.h>

 #include "../uapi/ion.h"

@@ -92,12 +95,16 @@ void ion_buffer_destroy(struct ion_buffer *buffer);
 /**
  * struct ion_device - the metadata of the ion device node
  * @dev:             the actual misc device
+ * @devt:            Ion device

devt?  That's not a "device", it's a dev_t, which means something else,
right?

OK

quoted

  * @buffers:         an rb tree of all the existing buffers
  * @buffer_lock:     lock protecting the tree of buffers
  * @lock:            rwsem protecting the tree of heaps and clients
  */
 struct ion_device {
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
      struct miscdevice dev;
+#endif

Why use #ifdef?

quoted

+     dev_t devt;
      struct rb_root buffers;
      struct mutex buffer_lock;
      struct rw_semaphore lock;

@@ -153,6 +160,8 @@ struct ion_heap_ops {
  * struct ion_heap - represents a heap in the system
  * @node:            rb node to put the heap on the device's tree of heaps
  * @dev:             back pointer to the ion_device
+ * @ddev:            device structure

How many different reference counted objects do you now have in this
structure?  And what exactly is the lifetime rules involved in them?

Hint, you never tried removing any of these from the system, otherwise
you would have seen the kernel warnings...

No I have never try because ION doesn't allow to be removed.

Step back here, what exactly do you want to do?  What do you expect
sysfs to look like?  What do you want /dev/ to look like?

Where is the documentation for the new sysfs files and the new ioctl
call you added?  What did you do to test this out?  Where are the AOSP
patches to use this?  Happen to have a VTS test for it?

I do not add ioctl just creating a device node per heap while keeping alive
the legacy mode on the misc device.
Until now all the users of ion have the same access rights on all the heaps
because they must use a common device misc to do the allocation.
Creating on device node per heap will allow to customize the access rights
per heap.

This is one of the item in the TODO list before been able to unstage ION
which is my real need.

I have look but I haven't found any VTS for ion...
I know that Laura had propose some patches in vgem to be able to it test
but I don't have news about this since a while.

This needs a lot more work, if for no other reason than the integration
into the driver model is totally wrong and will blow up into tiny
pieces...

thanks,

greg k-h

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Wed, Nov 29, 2017 at 03:00:56PM +0100, Benjamin Gaignard wrote:

2017-11-28 14:32 GMT+01:00 Greg KH [off-list ref]:

quoted

On Mon, Nov 06, 2017 at 04:59:45PM +0100, Benjamin Gaignard wrote:

quoted

Instead a getting only one common device "/dev/ion" for
all the heaps this patch allow to create one device
entry ("/dev/ionX") per heap.
Getting an entry per heap could allow to set security rules
per heap and global ones for all heaps.

Allocation requests will be only allowed if the mask_id
match with device minor.
Query request could be done on any of the devices.

Signed-off-by: Benjamin Gaignard <redacted>
---
 drivers/staging/android/TODO            |  1 -
 drivers/staging/android/ion/Kconfig     |  7 ++++
 drivers/staging/android/ion/ion-ioctl.c | 18 ++++++++--
 drivers/staging/android/ion/ion.c       | 62 +++++++++++++++++++++++++++++----
 drivers/staging/android/ion/ion.h       | 15 ++++++--
 5 files changed, 91 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
index 687e0ea..8a11931 100644
--- a/drivers/staging/android/TODO
+++ b/drivers/staging/android/TODO

@@ -8,7 +8,6 @@ TODO:
 ion/
  - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
    involve putting appropriate bindings in a memory node for Ion to find.
- - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
  - Better test framework (integration with VGEM was suggested)

 Please send patches to Greg Kroah-Hartman <greg@kroah.com> and Cc:

diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
index a517b2d..cb4666e 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig

@@ -10,6 +10,13 @@ menuconfig ION
        If you're not using Android its probably safe to
        say N here.

+config ION_LEGACY_DEVICE_API
+     bool "Keep using Ion legacy misc device API"
+     depends on ION

You want to default to Y here, so when you do 'make oldconfig' nothing
breaks, right?

I will add it.

quoted

quoted

+     help
+       Choose this option to keep using Ion legacy misc device API
+       i.e. /dev/ion

You need more text here to describe the trade offs.  Why would I not
want to keep doing this?  What does turning this off get me?  What does
keeping it on keep me from doing?

Does describe it like that sound better ?
"Choose this option to keep using ION legacy misc device API
  i.e. /dev/ion. If this option isn't selected you will only
  have per heap device node (i.e /dev/ionX) and allocating buffer
  from an unique device node won't be possible."

I still don't know why I would not select such an option, other than it
would break my working Android system if I did so :)

Please try to explain it a bit better.  For example, I really don't know
why you want to do this at all.

quoted

quoted

-void ion_device_add_heap(struct ion_heap *heap)
+static struct device ion_bus = {
+     .init_name = ION_NAME,
+};

Oh look, a struct device on the stack.  Watch bad things happen :(

This is a dynamic device, make it dynamic, or else you had better know
exactly what you are doing...

The naming is bad here, I will rename it ion_parent because I use it to provide
a parent to ion heap device either they won't go in /sys/bus/ion

Again, never use a static struct device, if you do, it is a _huge_ hint
the code is incorrect.

quoted

quoted

-     idev->debug_root = debugfs_create_dir("ion", NULL);
-     if (!idev->debug_root) {
+     ret = device_register(&ion_bus);

You call device_register for something you are calling a bus???  Are you
_sure_ about this?

What exactly are you creating in sysfs here?  You are throwing around
"raw" devices, which is almost never what you ever want to do.
Especially for some random char driver.

ion heap devices need a parent to be correctly put in /sys/bus/ion either they
will go directly in /sys/bus directory. You are right name it ion_bus
is confusing
I will rename it ion_parent.

Again, what are you trying to show in sysfs?  What type of
representation?  What is the bus?  What device type?  Why in sysfs at
all?

quoted

How many different reference counted objects do you now have in this
structure?  And what exactly is the lifetime rules involved in them?

Hint, you never tried removing any of these from the system, otherwise
you would have seen the kernel warnings...

No I have never try because ION doesn't allow to be removed.

That seems odd, there's no way to free up all resources and remove the
module?

quoted

Step back here, what exactly do you want to do?  What do you expect
sysfs to look like?  What do you want /dev/ to look like?

Where is the documentation for the new sysfs files and the new ioctl
call you added?  What did you do to test this out?  Where are the AOSP
patches to use this?  Happen to have a VTS test for it?

I do not add ioctl just creating a device node per heap while keeping alive
the legacy mode on the misc device.
Until now all the users of ion have the same access rights on all the heaps
because they must use a common device misc to do the allocation.
Creating on device node per heap will allow to customize the access rights
per heap.

This is not explaining your sysfs representation at all.  I have no idea
what you are wanting to do here.

You are confusing struct device with a character device node here I
think.  They are two totally different things, yet are related in some
ways.

This is one of the item in the TODO list before been able to unstage ION
which is my real need.

Why does it matter where in the tree this code is?  Don't go adding new
things to it that are not needed.  Who needs this?  What userspace code
wants this type of multiple ion devices?

thanks,

greg k-h
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

On 12/02/2017 07:53 AM, Greg KH wrote:

quoted

This is one of the item in the TODO list before been able to unstage ION
which is my real need.

Why does it matter where in the tree this code is?  Don't go adding new
things to it that are not needed.  Who needs this?  What userspace code
wants this type of multiple ion devices?

Requirements came in from several places to split /dev/ion -> /dev/ion0
and /dev/ion1 so that security policy (i.e. selinux) could be used to
protect access to certain heaps. I wanted the ABI to be settled before
trying to move out of staging, hence the line in the TODO list about
doing the split.

thanks,

greg k-h

Thanks,
Laura

On Tue, Dec 05, 2017 at 03:01:42PM -0800, Laura Abbott wrote:

On 12/02/2017 07:53 AM, Greg KH wrote:

quoted

quoted

This is one of the item in the TODO list before been able to unstage ION
which is my real need.

Why does it matter where in the tree this code is?  Don't go adding new
things to it that are not needed.  Who needs this?  What userspace code
wants this type of multiple ion devices?

Requirements came in from several places to split /dev/ion -> /dev/ion0
and /dev/ion1 so that security policy (i.e. selinux) could be used to
protect access to certain heaps. I wanted the ABI to be settled before
trying to move out of staging, hence the line in the TODO list about
doing the split.

Ok, but we should have some way of testing it works, right?  :)