* [PATCH] staging: ion: create one device entry per heap
@ 2017-09-18 14:58 Benjamin Gaignard
2017-09-19 9:40 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Gaignard @ 2017-09-18 14:58 UTC (permalink / raw)
To: labbott, sumit.semwal, gregkh, arve, riandrews, broonie
Cc: devel, linux-kernel, Benjamin Gaignard
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 <benjamin.gaignard@linaro.org>
---
drivers/staging/android/ion/ion-ioctl.c | 9 +++++++--
drivers/staging/android/ion/ion.c | 20 ++++++++++++++------
drivers/staging/android/ion/ion.h | 10 +++++++---
3 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index d9f8b14..ff06ce3 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -25,9 +25,11 @@ 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)
{
int ret = 0;
+ int mask = 1 << iminor(filp->f_inode);
switch (cmd) {
case ION_IOC_HEAP_QUERY:
@@ -35,6 +37,9 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
ret |= arg->query.reserved1 != 0;
ret |= arg->query.reserved2 != 0;
break;
+ case ION_IOC_ALLOC:
+ ret = !(arg->allocation.heap_id_mask & mask);
+ break;
default:
break;
}
@@ -70,7 +75,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..5144f1a 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -40,6 +40,8 @@
#include "ion.h"
+#define ION_DEV_MAX 32
+
static struct ion_device *internal_dev;
static int heap_id;
@@ -541,11 +543,21 @@ void ion_device_add_heap(struct ion_heap *heap)
{
struct dentry *debug_file;
struct ion_device *dev = internal_dev;
+ int ret;
if (!heap->ops->allocate || !heap->ops->free)
pr_err("%s: can not add heap with invalid ops struct.\n",
__func__);
+ 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;
+
spin_lock_init(&heap->free_lock);
heap->free_list_size = 0;
@@ -595,13 +607,9 @@ static int ion_device_create(void)
if (!idev)
return -ENOMEM;
- idev->dev.minor = MISC_DYNAMIC_MINOR;
- idev->dev.name = "ion";
- idev->dev.fops = &ion_fops;
- idev->dev.parent = NULL;
- ret = misc_register(&idev->dev);
+ ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, "ion");
if (ret) {
- pr_err("ion: failed to register misc device.\n");
+ pr_err("ion: unable to allocate major\n");
kfree(idev);
return ret;
}
diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
index 621e5f7..ef51ff5 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -17,6 +17,7 @@
#ifndef _ION_H
#define _ION_H
+#include <linux/cdev.h>
#include <linux/device.h>
#include <linux/dma-direction.h>
#include <linux/kref.h>
@@ -26,7 +27,6 @@
#include <linux/sched.h>
#include <linux/shrinker.h>
#include <linux/types.h>
-#include <linux/miscdevice.h>
#include "../uapi/ion.h"
@@ -90,13 +90,13 @@ void ion_buffer_destroy(struct ion_buffer *buffer);
/**
* struct ion_device - the metadata of the ion device node
- * @dev: the actual misc device
+ * @dev: the actual 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 {
- struct miscdevice dev;
+ dev_t devt;
struct rb_root buffers;
struct mutex buffer_lock;
struct rw_semaphore lock;
@@ -152,6 +152,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 +178,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;
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: ion: create one device entry per heap
2017-09-18 14:58 [PATCH] staging: ion: create one device entry per heap Benjamin Gaignard
@ 2017-09-19 9:40 ` Dan Carpenter
2017-09-19 10:07 ` Benjamin Gaignard
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2017-09-19 9:40 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: labbott, sumit.semwal, gregkh, arve, riandrews, broonie, devel,
linux-kernel
On Mon, Sep 18, 2017 at 04:58:46PM +0200, Benjamin Gaignard wrote:
> -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)
> {
> int ret = 0;
> + int mask = 1 << iminor(filp->f_inode);
>
> switch (cmd) {
> case ION_IOC_HEAP_QUERY:
> @@ -35,6 +37,9 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
> ret |= arg->query.reserved1 != 0;
> ret |= arg->query.reserved2 != 0;
> break;
> + case ION_IOC_ALLOC:
> + ret = !(arg->allocation.heap_id_mask & mask);
validate_ioctl_arg() is really convoluted. From reading just the patch
I at first thought we were returning 1 on failure. Just say:
if (!(arg->allocation.heap_id_mask & mask))
return -EINVAL;
return 0;
If you want to fix the surrounding code in a separate patch that would
be good. It would be more clear to say:
if (arg->query.reserved0 != 0 ||
arg->query.reserved1 != 0 ||
arg->query.reserved2 != 0)
return -EINVAL;
> + break;
> default:
> break;
> }
> @@ -70,7 +75,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..5144f1a 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -40,6 +40,8 @@
>
> #include "ion.h"
>
> +#define ION_DEV_MAX 32
> +
> static struct ion_device *internal_dev;
> static int heap_id;
>
> @@ -541,11 +543,21 @@ void ion_device_add_heap(struct ion_heap *heap)
> {
> struct dentry *debug_file;
> struct ion_device *dev = internal_dev;
> + int ret;
>
> if (!heap->ops->allocate || !heap->ops->free)
> pr_err("%s: can not add heap with invalid ops struct.\n",
> __func__);
>
I don't think it can happen in current code but we should proabably have
a check here for:
if (heap_id >= ION_DEV_MAX)
return -EBUSY;
(It's possible I have missed something).
> + 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;
> +
> spin_lock_init(&heap->free_lock);
> heap->free_list_size = 0;
>
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: ion: create one device entry per heap
2017-09-19 9:40 ` Dan Carpenter
@ 2017-09-19 10:07 ` Benjamin Gaignard
2017-09-19 10:15 ` Tomas Winkler
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Gaignard @ 2017-09-19 10:07 UTC (permalink / raw)
To: Dan Carpenter
Cc: Laura Abbott, Sumit Semwal, Greg Kroah-Hartman,
Arve Hjønnevåg, Riley Andrews, Mark Brown, devel,
Linux Kernel Mailing List
2017-09-19 11:40 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>:
> On Mon, Sep 18, 2017 at 04:58:46PM +0200, Benjamin Gaignard wrote:
>> -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)
>> {
>> int ret = 0;
>> + int mask = 1 << iminor(filp->f_inode);
>>
>> switch (cmd) {
>> case ION_IOC_HEAP_QUERY:
>> @@ -35,6 +37,9 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
>> ret |= arg->query.reserved1 != 0;
>> ret |= arg->query.reserved2 != 0;
>> break;
>> + case ION_IOC_ALLOC:
>> + ret = !(arg->allocation.heap_id_mask & mask);
>
>
> validate_ioctl_arg() is really convoluted. From reading just the patch
> I at first thought we were returning 1 on failure. Just say:
>
> if (!(arg->allocation.heap_id_mask & mask))
> return -EINVAL;
> return 0;
>
> If you want to fix the surrounding code in a separate patch that would
> be good. It would be more clear to say:
>
> if (arg->query.reserved0 != 0 ||
> arg->query.reserved1 != 0 ||
> arg->query.reserved2 != 0)
> return -EINVAL;
I agree I will add a fix for that in next version
>
>> + break;
>> default:
>> break;
>> }
>> @@ -70,7 +75,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..5144f1a 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -40,6 +40,8 @@
>>
>> #include "ion.h"
>>
>> +#define ION_DEV_MAX 32
>> +
>> static struct ion_device *internal_dev;
>> static int heap_id;
>>
>> @@ -541,11 +543,21 @@ void ion_device_add_heap(struct ion_heap *heap)
>> {
>> struct dentry *debug_file;
>> struct ion_device *dev = internal_dev;
>> + int ret;
>>
>> if (!heap->ops->allocate || !heap->ops->free)
>> pr_err("%s: can not add heap with invalid ops struct.\n",
>> __func__);
>>
>
> I don't think it can happen in current code but we should proabably have
> a check here for:
>
> if (heap_id >= ION_DEV_MAX)
> return -EBUSY;
>
> (It's possible I have missed something).
>
You are right I will add that
Thanks
>
>> + 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;
>> +
>> spin_lock_init(&heap->free_lock);
>> heap->free_list_size = 0;
>>
>
> regards,
> dan carpenter
>
>
--
Benjamin Gaignard
Graphic Study Group
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: ion: create one device entry per heap
2017-09-19 10:07 ` Benjamin Gaignard
@ 2017-09-19 10:15 ` Tomas Winkler
2017-09-19 10:20 ` Benjamin Gaignard
0 siblings, 1 reply; 6+ messages in thread
From: Tomas Winkler @ 2017-09-19 10:15 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: Dan Carpenter, driverdevel, Greg Kroah-Hartman,
Arve Hjønnevåg, Linux Kernel Mailing List,
Riley Andrews, Mark Brown, Sumit Semwal
On Tue, Sep 19, 2017 at 1:07 PM, Benjamin Gaignard
<benjamin.gaignard@linaro.org> wrote:
> 2017-09-19 11:40 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>:
>> On Mon, Sep 18, 2017 at 04:58:46PM +0200, Benjamin Gaignard wrote:
>>> -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)
>>> {
>>> int ret = 0;
>>> + int mask = 1 << iminor(filp->f_inode);
>>>
>>> switch (cmd) {
>>> case ION_IOC_HEAP_QUERY:
>>> @@ -35,6 +37,9 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
>>> ret |= arg->query.reserved1 != 0;
>>> ret |= arg->query.reserved2 != 0;
>>> break;
>>> + case ION_IOC_ALLOC:
>>> + ret = !(arg->allocation.heap_id_mask & mask);
>>
>>
>> validate_ioctl_arg() is really convoluted. From reading just the patch
>> I at first thought we were returning 1 on failure. Just say:
>>
>> if (!(arg->allocation.heap_id_mask & mask))
>> return -EINVAL;
>> return 0;
>>
>> If you want to fix the surrounding code in a separate patch that would
>> be good. It would be more clear to say:
>>
>> if (arg->query.reserved0 != 0 ||
>> arg->query.reserved1 != 0 ||
>> arg->query.reserved2 != 0)
>> return -EINVAL;
>
> I agree I will add a fix for that in next version
>
>>
>>> + break;
>>> default:
>>> break;
>>> }
>>> @@ -70,7 +75,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..5144f1a 100644
>>> --- a/drivers/staging/android/ion/ion.c
>>> +++ b/drivers/staging/android/ion/ion.c
>>> @@ -40,6 +40,8 @@
>>>
>>> #include "ion.h"
>>>
>>> +#define ION_DEV_MAX 32
>>> +
>>> static struct ion_device *internal_dev;
>>> static int heap_id;
>>>
>>> @@ -541,11 +543,21 @@ void ion_device_add_heap(struct ion_heap *heap)
>>> {
>>> struct dentry *debug_file;
>>> struct ion_device *dev = internal_dev;
>>> + int ret;
>>>
>>> if (!heap->ops->allocate || !heap->ops->free)
>>> pr_err("%s: can not add heap with invalid ops struct.\n",
>>> __func__);
>>>
>>
>> I don't think it can happen in current code but we should proabably have
>> a check here for:
>>
>> if (heap_id >= ION_DEV_MAX)
>> return -EBUSY;
>>
>> (It's possible I have missed something).
>>
>
> You are right I will add that
>
> Thanks
>>
>>> + 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;
>>> +
>>> spin_lock_init(&heap->free_lock);
>>> heap->free_list_size = 0;
What will happen to an application which looks for /dev/ion?
Thanks
Tomas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: ion: create one device entry per heap
2017-09-19 10:15 ` Tomas Winkler
@ 2017-09-19 10:20 ` Benjamin Gaignard
2017-09-19 10:59 ` Greg Kroah-Hartman
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Gaignard @ 2017-09-19 10:20 UTC (permalink / raw)
To: Tomas Winkler
Cc: Dan Carpenter, driverdevel, Greg Kroah-Hartman,
Arve Hjønnevåg, Linux Kernel Mailing List,
Riley Andrews, Mark Brown, Sumit Semwal
2017-09-19 12:15 GMT+02:00 Tomas Winkler <tomasw@gmail.com>:
> On Tue, Sep 19, 2017 at 1:07 PM, Benjamin Gaignard
> <benjamin.gaignard@linaro.org> wrote:
>> 2017-09-19 11:40 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>:
>>> On Mon, Sep 18, 2017 at 04:58:46PM +0200, Benjamin Gaignard wrote:
>>>> -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)
>>>> {
>>>> int ret = 0;
>>>> + int mask = 1 << iminor(filp->f_inode);
>>>>
>>>> switch (cmd) {
>>>> case ION_IOC_HEAP_QUERY:
>>>> @@ -35,6 +37,9 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
>>>> ret |= arg->query.reserved1 != 0;
>>>> ret |= arg->query.reserved2 != 0;
>>>> break;
>>>> + case ION_IOC_ALLOC:
>>>> + ret = !(arg->allocation.heap_id_mask & mask);
>>>
>>>
>>> validate_ioctl_arg() is really convoluted. From reading just the patch
>>> I at first thought we were returning 1 on failure. Just say:
>>>
>>> if (!(arg->allocation.heap_id_mask & mask))
>>> return -EINVAL;
>>> return 0;
>>>
>>> If you want to fix the surrounding code in a separate patch that would
>>> be good. It would be more clear to say:
>>>
>>> if (arg->query.reserved0 != 0 ||
>>> arg->query.reserved1 != 0 ||
>>> arg->query.reserved2 != 0)
>>> return -EINVAL;
>>
>> I agree I will add a fix for that in next version
>>
>>>
>>>> + break;
>>>> default:
>>>> break;
>>>> }
>>>> @@ -70,7 +75,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..5144f1a 100644
>>>> --- a/drivers/staging/android/ion/ion.c
>>>> +++ b/drivers/staging/android/ion/ion.c
>>>> @@ -40,6 +40,8 @@
>>>>
>>>> #include "ion.h"
>>>>
>>>> +#define ION_DEV_MAX 32
>>>> +
>>>> static struct ion_device *internal_dev;
>>>> static int heap_id;
>>>>
>>>> @@ -541,11 +543,21 @@ void ion_device_add_heap(struct ion_heap *heap)
>>>> {
>>>> struct dentry *debug_file;
>>>> struct ion_device *dev = internal_dev;
>>>> + int ret;
>>>>
>>>> if (!heap->ops->allocate || !heap->ops->free)
>>>> pr_err("%s: can not add heap with invalid ops struct.\n",
>>>> __func__);
>>>>
>>>
>>> I don't think it can happen in current code but we should proabably have
>>> a check here for:
>>>
>>> if (heap_id >= ION_DEV_MAX)
>>> return -EBUSY;
>>>
>>> (It's possible I have missed something).
>>>
>>
>> You are right I will add that
>>
>> Thanks
>>>
>>>> + 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;
>>>> +
>>>> spin_lock_init(&heap->free_lock);
>>>> heap->free_list_size = 0;
>
> What will happen to an application which looks for /dev/ion?
/dev/ion will no more exist with this patch.
Since ion ABI have already change a lot I don't think that could
be a problem to change also ion device.
>
> Thanks
> Tomas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: ion: create one device entry per heap
2017-09-19 10:20 ` Benjamin Gaignard
@ 2017-09-19 10:59 ` Greg Kroah-Hartman
0 siblings, 0 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2017-09-19 10:59 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: Tomas Winkler, driverdevel, Riley Andrews,
Linux Kernel Mailing List, Arve Hjønnevåg, Mark Brown,
Sumit Semwal, Dan Carpenter
On Tue, Sep 19, 2017 at 12:20:15PM +0200, Benjamin Gaignard wrote:
> 2017-09-19 12:15 GMT+02:00 Tomas Winkler <tomasw@gmail.com>:
> > On Tue, Sep 19, 2017 at 1:07 PM, Benjamin Gaignard
> > <benjamin.gaignard@linaro.org> wrote:
> >> 2017-09-19 11:40 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>:
> >>> On Mon, Sep 18, 2017 at 04:58:46PM +0200, Benjamin Gaignard wrote:
> >>>> -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)
> >>>> {
> >>>> int ret = 0;
> >>>> + int mask = 1 << iminor(filp->f_inode);
> >>>>
> >>>> switch (cmd) {
> >>>> case ION_IOC_HEAP_QUERY:
> >>>> @@ -35,6 +37,9 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
> >>>> ret |= arg->query.reserved1 != 0;
> >>>> ret |= arg->query.reserved2 != 0;
> >>>> break;
> >>>> + case ION_IOC_ALLOC:
> >>>> + ret = !(arg->allocation.heap_id_mask & mask);
> >>>
> >>>
> >>> validate_ioctl_arg() is really convoluted. From reading just the patch
> >>> I at first thought we were returning 1 on failure. Just say:
> >>>
> >>> if (!(arg->allocation.heap_id_mask & mask))
> >>> return -EINVAL;
> >>> return 0;
> >>>
> >>> If you want to fix the surrounding code in a separate patch that would
> >>> be good. It would be more clear to say:
> >>>
> >>> if (arg->query.reserved0 != 0 ||
> >>> arg->query.reserved1 != 0 ||
> >>> arg->query.reserved2 != 0)
> >>> return -EINVAL;
> >>
> >> I agree I will add a fix for that in next version
> >>
> >>>
> >>>> + break;
> >>>> default:
> >>>> break;
> >>>> }
> >>>> @@ -70,7 +75,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..5144f1a 100644
> >>>> --- a/drivers/staging/android/ion/ion.c
> >>>> +++ b/drivers/staging/android/ion/ion.c
> >>>> @@ -40,6 +40,8 @@
> >>>>
> >>>> #include "ion.h"
> >>>>
> >>>> +#define ION_DEV_MAX 32
> >>>> +
> >>>> static struct ion_device *internal_dev;
> >>>> static int heap_id;
> >>>>
> >>>> @@ -541,11 +543,21 @@ void ion_device_add_heap(struct ion_heap *heap)
> >>>> {
> >>>> struct dentry *debug_file;
> >>>> struct ion_device *dev = internal_dev;
> >>>> + int ret;
> >>>>
> >>>> if (!heap->ops->allocate || !heap->ops->free)
> >>>> pr_err("%s: can not add heap with invalid ops struct.\n",
> >>>> __func__);
> >>>>
> >>>
> >>> I don't think it can happen in current code but we should proabably have
> >>> a check here for:
> >>>
> >>> if (heap_id >= ION_DEV_MAX)
> >>> return -EBUSY;
> >>>
> >>> (It's possible I have missed something).
> >>>
> >>
> >> You are right I will add that
> >>
> >> Thanks
> >>>
> >>>> + 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;
> >>>> +
> >>>> spin_lock_init(&heap->free_lock);
> >>>> heap->free_list_size = 0;
> >
> > What will happen to an application which looks for /dev/ion?
>
> /dev/ion will no more exist with this patch.
> Since ion ABI have already change a lot I don't think that could
> be a problem to change also ion device.
So, what did you just break in userspace? :(
Do you also have userspace patches submitted anywhere to handle this
change?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-19 10:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-18 14:58 [PATCH] staging: ion: create one device entry per heap Benjamin Gaignard
2017-09-19 9:40 ` Dan Carpenter
2017-09-19 10:07 ` Benjamin Gaignard
2017-09-19 10:15 ` Tomas Winkler
2017-09-19 10:20 ` Benjamin Gaignard
2017-09-19 10:59 ` Greg Kroah-Hartman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.