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 (under configuartion flag) 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.
That enable thsi new flag change Ion ABI because:
- device name change
- allocation must be done on the correct device/heap.
- device major number will not be the same and that could impact init scripts.
Benjamin Gaignard (2):
staging: ion: simplify ioctl args checking function
staging: ion: create one device entry per heap
drivers/staging/android/TODO | 1 -
drivers/staging/android/ion/Kconfig | 8 ++++++++
drivers/staging/android/ion/ion-ioctl.c | 27 +++++++++++++++++++--------
drivers/staging/android/ion/ion.c | 29 +++++++++++++++++++++++++++--
drivers/staging/android/ion/ion.h | 18 +++++++++++++++++-
5 files changed, 71 insertions(+), 12 deletions(-)
--
2.7.4
Make arguments checking more easy to read.
Signed-off-by: Benjamin Gaignard <[email protected]>
---
drivers/staging/android/ion/ion-ioctl.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index d9f8b14..e26b786 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -27,19 +27,18 @@ union ion_ioctl_arg {
static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
{
- int ret = 0;
-
switch (cmd) {
case ION_IOC_HEAP_QUERY:
- ret = arg->query.reserved0 != 0;
- ret |= arg->query.reserved1 != 0;
- ret |= arg->query.reserved2 != 0;
+ if (arg->query.reserved0 ||
+ arg->query.reserved1 ||
+ arg->query.reserved2 )
+ return -EINVAL;
break;
default:
break;
}
- return ret ? -EINVAL : 0;
+ return 0;
}
/* fix up the cases where the ioctl direction bits are incorrect */
--
2.7.4
Instead a getting 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 <[email protected]>
---
version 4:
- add a configuration flag to switch between legacy Ion misc device
and one device per heap version.
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.
drivers/staging/android/TODO | 1 -
drivers/staging/android/ion/Kconfig | 8 ++++++++
drivers/staging/android/ion/ion-ioctl.c | 16 ++++++++++++++--
drivers/staging/android/ion/ion.c | 29 +++++++++++++++++++++++++++--
drivers/staging/android/ion/ion.h | 18 +++++++++++++++++-
5 files changed, 66 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
index 5f14247..d770ffa 100644
--- a/drivers/staging/android/TODO
+++ b/drivers/staging/android/TODO
@@ -9,7 +9,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 <[email protected]> and Cc:
diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
index a517b2d..6bb07f6 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -10,6 +10,14 @@ menuconfig ION
If you're not using Android its probably safe to
say N here.
+config ION_ONE_DEVICE_ENTRY_PER_HEAP
+ bool "Create one Ion device per heap"
+ depends on ION
+ help
+ Choose this option to have one device entry per heap. Each
+ device with named "/dev/ionX" where X is heap ID.
+ Selecting this option remove the legacy Ion misc device.
+
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..76492cc 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,17 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
arg->query.reserved2 )
return -EINVAL;
break;
+
+#ifdef CONFIG_ION_ONE_DEVICE_ENTRY_PER_HEAP
+ case ION_IOC_ALLOC:
+ {
+ int mask = 1 << iminor(filp->f_inode);
+
+ if (!(arg->allocation.heap_id_mask & mask))
+ return -EINVAL;
+ break;
+ }
+#endif
default:
break;
}
@@ -69,7 +81,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 93e2c90..18a20d2 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -40,6 +40,10 @@
#include "ion.h"
+#ifdef CONFIG_ION_ONE_DEVICE_ENTRY_PER_HEAP
+#define ION_DEV_MAX 32
+#endif
+
static struct ion_device *internal_dev;
static int heap_id;
@@ -537,15 +541,30 @@ 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)
+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__);
+#ifdef CONFIG_ION_ONE_DEVICE_ENTRY_PER_HEAP
+ if (heap_id >= ION_DEV_MAX)
+ return -EBUSY;
+
+ heap->ddev.devt = MKDEV(MAJOR(dev->devt), heap_id);
+ dev_set_name(&heap->ddev, "ion%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;
+#endif
+
spin_lock_init(&heap->free_lock);
heap->free_list_size = 0;
@@ -583,6 +602,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);
@@ -595,13 +616,17 @@ static int ion_device_create(void)
if (!idev)
return -ENOMEM;
+#ifdef CONFIG_ION_ONE_DEVICE_ENTRY_PER_HEAP
+ ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, "ion");
+#else
idev->dev.minor = MISC_DYNAMIC_MINOR;
idev->dev.name = "ion";
idev->dev.fops = &ion_fops;
idev->dev.parent = NULL;
ret = misc_register(&idev->dev);
+#endif
if (ret) {
- pr_err("ion: failed to register misc device.\n");
+ pr_err("ion: unable to allocate device\n");
kfree(idev);
return ret;
}
diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
index 621e5f7..61ada2e 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -26,7 +26,12 @@
#include <linux/sched.h>
#include <linux/shrinker.h>
#include <linux/types.h>
+
+#ifdef CONFIG_ION_ONE_DEVICE_ENTRY_PER_HEAP
+#include <linux/cdev.h>
+#else
#include <linux/miscdevice.h>
+#endif
#include "../uapi/ion.h"
@@ -90,13 +95,18 @@ void ion_buffer_destroy(struct ion_buffer *buffer);
/**
* struct ion_device - the metadata of the ion device node
+ * @devt: Ion device if heap have own device entry
* @dev: the actual misc 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_ONE_DEVICE_ENTRY_PER_HEAP
+ dev_t devt;
+#else
struct miscdevice dev;
+#endif
struct rb_root buffers;
struct mutex buffer_lock;
struct rw_semaphore lock;
@@ -152,6 +162,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
@@ -176,6 +188,10 @@ struct ion_heap_ops {
struct ion_heap {
struct plist_node node;
struct ion_device *dev;
+#ifdef CONFIG_ION_ONE_DEVICE_ENTRY_PER_HEAP
+ struct device ddev;
+ struct cdev chrdev;
+#endif
enum ion_heap_type type;
struct ion_heap_ops *ops;
unsigned long flags;
@@ -212,7 +228,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 Tue, Sep 26, 2017 at 02:07:05PM +0200, Benjamin Gaignard wrote:
> version 4:
> - add a configuration flag to switch between legacy Ion misc device
> and one device per heap version.
Should this be a switch or should it just be enabling and disabling the
legacy device with the per heap ones always availalbe? I can't see that
the new devices would do any harm or have trouble interacting with the
per heap ones. Being able to have both enabled would make things easier
for userspaces that are moving to the device per heap interface.
On 09/26/2017 09:17 AM, Mark Brown wrote:
> On Tue, Sep 26, 2017 at 02:07:05PM +0200, Benjamin Gaignard wrote:
>
>> version 4:
>> - add a configuration flag to switch between legacy Ion misc device
>> and one device per heap version.
>
> Should this be a switch or should it just be enabling and disabling the
> legacy device with the per heap ones always availalbe? I can't see that
> the new devices would do any harm or have trouble interacting with the
> per heap ones. Being able to have both enabled would make things easier
> for userspaces that are moving to the device per heap interface.
>
Agreed. We should be enabling the new interface unconditionally. The
old /dev/ion interface should coexist to allow for backwards
compatibility but keep it under a Kconfig to allow it to be turned off
for security or other reasons.
Thanks,
Laura