[PATCHv2 0/4] New Ion ioctls

[PATCHv2 0/4] New Ion ioctls

[Laura Abbott]

Hi,

This is a follow up to my previous series[1] for Ion ioctls. I've changed the
focus slightly based on the feedback. The ID remapping was less useful than I
originally thought and without that addition there isn't much benefit to have
a new alloc ioctl. The ABI check and query interface still seem beneficial.
There was some discussion on where exactly these types of ioctls would be
called. I expect the answer will depend on exactly how it's integrated.

Long term, I'd still like to fix the ABI to not be a checklist of botching
up ioctls but that focus will come later.

Changes from v1:
- Rebased
- Dropped RFC
- Dropped ID remapping and dependent logic
- Changed query logic to only need one ioctl
- Fixed alignment of query ioctl structure

[1] http://www.mail-archive.com/driverdev-devel@linuxdriverproject.org/msg48036.html

Laura Abbott (4):
  staging: android: ion: Drop heap type masks
  staging: android: ion: Pull out ion ioctls to a separate file
  staging: android: ion: Add an ioctl for ABI checking
  staging: android: ion: Add ioctl to query available heaps

 drivers/staging/android/ion/Makefile    |   3 +-
 drivers/staging/android/ion/ion-ioctl.c | 188 ++++++++++++++++++++++++++
 drivers/staging/android/ion/ion.c       | 226 ++++++--------------------------
 drivers/staging/android/ion/ion_priv.h  |  94 +++++++++++++
 drivers/staging/android/uapi/ion.h      |  67 +++++++++-
 5 files changed, 382 insertions(+), 196 deletions(-)
 create mode 100644 drivers/staging/android/ion/ion-ioctl.c

-- 
2.7.4

[PATCHv2 1/4] staging: android: ion: Drop heap type masks

[Laura Abbott]

There is no advantage to having heap types be a mask. The ion client has
long since dropped the mask. Drop the notion of heap type masks as well.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
 drivers/staging/android/uapi/ion.h | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
index 0a8e40f..a9c4e8b 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -44,14 +44,8 @@ enum ion_heap_type {
 			       * must be last so device specific heaps always
 			       * are at the end of this enum
 			       */
-	ION_NUM_HEAPS = 16,
 };
 
-#define ION_HEAP_SYSTEM_MASK		(1 << ION_HEAP_TYPE_SYSTEM)
-#define ION_HEAP_SYSTEM_CONTIG_MASK	(1 << ION_HEAP_TYPE_SYSTEM_CONTIG)
-#define ION_HEAP_CARVEOUT_MASK		(1 << ION_HEAP_TYPE_CARVEOUT)
-#define ION_HEAP_TYPE_DMA_MASK		(1 << ION_HEAP_TYPE_DMA)
-
 #define ION_NUM_HEAP_IDS		(sizeof(unsigned int) * 8)
 
 /**
-- 
2.7.4

Re: [PATCHv2 1/4] staging: android: ion: Drop heap type masks

[Brian Starkey]

Hi Laura,

On Thu, Sep 01, 2016 at 03:40:41PM -0700, Laura Abbott wrote:
>
>There is no advantage to having heap types be a mask. The ion client has
>long since dropped the mask. Drop the notion of heap type masks as well.
>

I know this is the same patch you sent last time, so sorry for not
picking this up then - but I'm curious what "The" ion client is here?

Our ion client(s) certainly still use these masks, and it's still
used as a mask within ion itself - even if the relationship between a
mask and a heap type has been somewhat lost.

Thanks,
Brian

>Signed-off-by: Laura Abbott <labbott@redhat.com>
>---
> drivers/staging/android/uapi/ion.h | 6 ------
> 1 file changed, 6 deletions(-)
>
>diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
>index 0a8e40f..a9c4e8b 100644
>--- a/drivers/staging/android/uapi/ion.h
>+++ b/drivers/staging/android/uapi/ion.h
>@@ -44,14 +44,8 @@ enum ion_heap_type {
> 			       * must be last so device specific heaps always
> 			       * are at the end of this enum
> 			       */
>-	ION_NUM_HEAPS = 16,
> };
>
>-#define ION_HEAP_SYSTEM_MASK		(1 << ION_HEAP_TYPE_SYSTEM)
>-#define ION_HEAP_SYSTEM_CONTIG_MASK	(1 << ION_HEAP_TYPE_SYSTEM_CONTIG)
>-#define ION_HEAP_CARVEOUT_MASK		(1 << ION_HEAP_TYPE_CARVEOUT)
>-#define ION_HEAP_TYPE_DMA_MASK		(1 << ION_HEAP_TYPE_DMA)
>-
> #define ION_NUM_HEAP_IDS		(sizeof(unsigned int) * 8)
>
> /**
>-- 
>2.7.4
>

Re: [PATCHv2 1/4] staging: android: ion: Drop heap type masks

[Laura Abbott]

On 09/02/2016 06:41 AM, Brian Starkey wrote:
> Hi Laura,
>
> On Thu, Sep 01, 2016 at 03:40:41PM -0700, Laura Abbott wrote:
>>
>> There is no advantage to having heap types be a mask. The ion client has
>> long since dropped the mask. Drop the notion of heap type masks as well.
>>
>
> I know this is the same patch you sent last time, so sorry for not
> picking this up then - but I'm curious what "The" ion client is here?
>

ion_client_create used to take a mask to indicate what heap types it
could allocate from. This hasn't been the case since 2bb9f5034ec7
("gpu: ion: Remove heapmask from client"). "The ion client" probably
should have been "struct ion_client"

> Our ion client(s) certainly still use these masks, and it's still
> used as a mask within ion itself - even if the relationship between a
> mask and a heap type has been somewhat lost.

Where is it used in Ion? I don't see it in tree unless I missed something
and I'm not eager to keep this around for out of tree code. What's the
actual use for this?

>
> Thanks,
> Brian
>
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> ---
>> drivers/staging/android/uapi/ion.h | 6 ------
>> 1 file changed, 6 deletions(-)
>>
>> diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
>> index 0a8e40f..a9c4e8b 100644
>> --- a/drivers/staging/android/uapi/ion.h
>> +++ b/drivers/staging/android/uapi/ion.h
>> @@ -44,14 +44,8 @@ enum ion_heap_type {
>>                    * must be last so device specific heaps always
>>                    * are at the end of this enum
>>                    */
>> -    ION_NUM_HEAPS = 16,
>> };
>>
>> -#define ION_HEAP_SYSTEM_MASK        (1 << ION_HEAP_TYPE_SYSTEM)
>> -#define ION_HEAP_SYSTEM_CONTIG_MASK    (1 << ION_HEAP_TYPE_SYSTEM_CONTIG)
>> -#define ION_HEAP_CARVEOUT_MASK        (1 << ION_HEAP_TYPE_CARVEOUT)
>> -#define ION_HEAP_TYPE_DMA_MASK        (1 << ION_HEAP_TYPE_DMA)
>> -
>> #define ION_NUM_HEAP_IDS        (sizeof(unsigned int) * 8)
>>
>> /**
>> --
>> 2.7.4
>>

Re: [PATCHv2 1/4] staging: android: ion: Drop heap type masks

[Brian Starkey]

Hi,

On Fri, Sep 02, 2016 at 12:36:25PM -0700, Laura Abbott wrote:
>On 09/02/2016 06:41 AM, Brian Starkey wrote:
>>Hi Laura,
>>
>>On Thu, Sep 01, 2016 at 03:40:41PM -0700, Laura Abbott wrote:
>>>
>>>There is no advantage to having heap types be a mask. The ion client has
>>>long since dropped the mask. Drop the notion of heap type masks as well.
>>>
>>
>>I know this is the same patch you sent last time, so sorry for not
>>picking this up then - but I'm curious what "The" ion client is here?
>>
>
>ion_client_create used to take a mask to indicate what heap types it
>could allocate from. This hasn't been the case since 2bb9f5034ec7
>("gpu: ion: Remove heapmask from client"). "The ion client" probably
>should have been "struct ion_client"

Ah I see, the in-kernel ion_client. Sorry, I completely forgot that
even existed (because it's totally useless - how is a driver meant to
find the global ion_device?)

>
>>Our ion client(s) certainly still use these masks, and it's still
>>used as a mask within ion itself - even if the relationship between a
>>mask and a heap type has been somewhat lost.
>
>Where is it used in Ion? I don't see it in tree unless I missed something
>and I'm not eager to keep this around for out of tree code. What's the
>actual use for this?

You're certainly right that these heap-ID-to-allocation-mask macros
are unused in the kernel, but I don't really see the reason for
removing them - they are convenient (for now).

Example: I'm using the dummy ion driver, and I want to allocate from
the SYSTEM_CONTIG heap - the ION_HEAP_SYSTEM_CONTIG_MASK gives me the
exact mask I need for that.

It seems your opinion is that heap-IDs are already, and should be,
completely decoupled from their type. That sounds like a good idea to
me, but it's not true (yet) - again check out the dummy driver.

At the moment, heap-IDs are assigned by ion drivers in any way they
see fit. For as long as that stays the case there's always going to
be heap-masks hard-coded in UAPI kernel headers (in-tree or not), so
removing these particular masks seems a bit fruitless.

I'd rather see driver-assigned heap-IDs disappear completely, and have
them assigned by ion core from an idr or something. At that point
these macros really *are* meaningless, and I'd be totally fine with
removing them (and userspace won't be able to depend on hard-coded
allocation masks any more - it will have to use the query ioctl,
which I assume is the whole point?).

IMO it's not the right time to remove these macros, because they still
have meaning and usefulness.

Cheers,
Brian

>
>>
>>Thanks,
>>Brian
>>
>>>Signed-off-by: Laura Abbott <labbott@redhat.com>
>>>---
>>>drivers/staging/android/uapi/ion.h | 6 ------
>>>1 file changed, 6 deletions(-)
>>>
>>>diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
>>>index 0a8e40f..a9c4e8b 100644
>>>--- a/drivers/staging/android/uapi/ion.h
>>>+++ b/drivers/staging/android/uapi/ion.h
>>>@@ -44,14 +44,8 @@ enum ion_heap_type {
>>>                   * must be last so device specific heaps always
>>>                   * are at the end of this enum
>>>                   */
>>>-    ION_NUM_HEAPS = 16,
>>>};
>>>
>>>-#define ION_HEAP_SYSTEM_MASK        (1 << ION_HEAP_TYPE_SYSTEM)
>>>-#define ION_HEAP_SYSTEM_CONTIG_MASK    (1 << ION_HEAP_TYPE_SYSTEM_CONTIG)
>>>-#define ION_HEAP_CARVEOUT_MASK        (1 << ION_HEAP_TYPE_CARVEOUT)
>>>-#define ION_HEAP_TYPE_DMA_MASK        (1 << ION_HEAP_TYPE_DMA)
>>>-
>>>#define ION_NUM_HEAP_IDS        (sizeof(unsigned int) * 8)
>>>
>>>/**
>>>--
>>>2.7.4
>>>
>

Re: [PATCHv2 1/4] staging: android: ion: Drop heap type masks

[Laura Abbott]

On 09/05/2016 04:20 AM, Brian Starkey wrote:
> Hi,
>
> On Fri, Sep 02, 2016 at 12:36:25PM -0700, Laura Abbott wrote:
>> On 09/02/2016 06:41 AM, Brian Starkey wrote:
>>> Hi Laura,
>>>
>>> On Thu, Sep 01, 2016 at 03:40:41PM -0700, Laura Abbott wrote:
>>>>
>>>> There is no advantage to having heap types be a mask. The ion client has
>>>> long since dropped the mask. Drop the notion of heap type masks as well.
>>>>
>>>
>>> I know this is the same patch you sent last time, so sorry for not
>>> picking this up then - but I'm curious what "The" ion client is here?
>>>
>>
>> ion_client_create used to take a mask to indicate what heap types it
>> could allocate from. This hasn't been the case since 2bb9f5034ec7
>> ("gpu: ion: Remove heapmask from client"). "The ion client" probably
>> should have been "struct ion_client"
>
> Ah I see, the in-kernel ion_client. Sorry, I completely forgot that
> even existed (because it's totally useless - how is a driver meant to
> find the global ion_device?)
>
>>
>>> Our ion client(s) certainly still use these masks, and it's still
>>> used as a mask within ion itself - even if the relationship between a
>>> mask and a heap type has been somewhat lost.
>>
>> Where is it used in Ion? I don't see it in tree unless I missed something
>> and I'm not eager to keep this around for out of tree code. What's the
>> actual use for this?
>
> You're certainly right that these heap-ID-to-allocation-mask macros
> are unused in the kernel, but I don't really see the reason for
> removing them - they are convenient (for now).
>
> Example: I'm using the dummy ion driver, and I want to allocate from
> the SYSTEM_CONTIG heap - the ION_HEAP_SYSTEM_CONTIG_MASK gives me the
> exact mask I need for that.
>
> It seems your opinion is that heap-IDs are already, and should be,
> completely decoupled from their type. That sounds like a good idea to
> me, but it's not true (yet) - again check out the dummy driver.
>

Good point, I need to clean up the dummy driver to stop using heap
types as the id ;)

I get that it's convenient but it's a bad practice to conflate the
namespaces.

> At the moment, heap-IDs are assigned by ion drivers in any way they
> see fit. For as long as that stays the case there's always going to
> be heap-masks hard-coded in UAPI kernel headers (in-tree or not), so
> removing these particular masks seems a bit fruitless.
>

It's not fruitless, the concept of type as mask makes no sense. They
are two different name spaces and I've found Ion users have a hard
time keeping them separate and pass in the heap type mask when using
non dummy

> I'd rather see driver-assigned heap-IDs disappear completely, and have
> them assigned by ion core from an idr or something. At that point
> these macros really *are* meaningless, and I'd be totally fine with
> removing them (and userspace won't be able to depend on hard-coded
> allocation masks any more - it will have to use the query ioctl,
> which I assume is the whole point?).
>

Ideally yes we'd be able to get rid of the hard coded device IDs.
I consider the query ioctl a stepping stone to that, depending on
how enthusiastic people are about Ion.

> IMO it's not the right time to remove these macros, because they still
> have meaning and usefulness.
>

I still think they should be deleted to avoid namespace polution.

> Cheers,
> Brian
>
>>
>>>
>>> Thanks,
>>> Brian
>>>
>>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>>>> ---
>>>> drivers/staging/android/uapi/ion.h | 6 ------
>>>> 1 file changed, 6 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
>>>> index 0a8e40f..a9c4e8b 100644
>>>> --- a/drivers/staging/android/uapi/ion.h
>>>> +++ b/drivers/staging/android/uapi/ion.h
>>>> @@ -44,14 +44,8 @@ enum ion_heap_type {
>>>>                   * must be last so device specific heaps always
>>>>                   * are at the end of this enum
>>>>                   */
>>>> -    ION_NUM_HEAPS = 16,
>>>> };
>>>>
>>>> -#define ION_HEAP_SYSTEM_MASK        (1 << ION_HEAP_TYPE_SYSTEM)
>>>> -#define ION_HEAP_SYSTEM_CONTIG_MASK    (1 << ION_HEAP_TYPE_SYSTEM_CONTIG)
>>>> -#define ION_HEAP_CARVEOUT_MASK        (1 << ION_HEAP_TYPE_CARVEOUT)
>>>> -#define ION_HEAP_TYPE_DMA_MASK        (1 << ION_HEAP_TYPE_DMA)
>>>> -
>>>> #define ION_NUM_HEAP_IDS        (sizeof(unsigned int) * 8)
>>>>
>>>> /**
>>>> --
>>>> 2.7.4
>>>>
>>

Re: [PATCHv2 1/4] staging: android: ion: Drop heap type masks

[Brian Starkey]

On Tue, Sep 06, 2016 at 03:16:52PM -0700, Laura Abbott wrote:
>On 09/05/2016 04:20 AM, Brian Starkey wrote:
>>Hi,
>>
>>On Fri, Sep 02, 2016 at 12:36:25PM -0700, Laura Abbott wrote:
>>>On 09/02/2016 06:41 AM, Brian Starkey wrote:
>>>>Hi Laura,
>>>>
>>>>On Thu, Sep 01, 2016 at 03:40:41PM -0700, Laura Abbott wrote:
>>>>>
>>>>>There is no advantage to having heap types be a mask. The ion client has
>>>>>long since dropped the mask. Drop the notion of heap type masks as well.
>>>>>
>>>>
>>>>I know this is the same patch you sent last time, so sorry for not
>>>>picking this up then - but I'm curious what "The" ion client is here?
>>>>
>>>
>>>ion_client_create used to take a mask to indicate what heap types it
>>>could allocate from. This hasn't been the case since 2bb9f5034ec7
>>>("gpu: ion: Remove heapmask from client"). "The ion client" probably
>>>should have been "struct ion_client"
>>
>>Ah I see, the in-kernel ion_client. Sorry, I completely forgot that
>>even existed (because it's totally useless - how is a driver meant to
>>find the global ion_device?)
>>
>>>
>>>>Our ion client(s) certainly still use these masks, and it's still
>>>>used as a mask within ion itself - even if the relationship between a
>>>>mask and a heap type has been somewhat lost.
>>>
>>>Where is it used in Ion? I don't see it in tree unless I missed something
>>>and I'm not eager to keep this around for out of tree code. What's the
>>>actual use for this?
>>
>>You're certainly right that these heap-ID-to-allocation-mask macros
>>are unused in the kernel, but I don't really see the reason for
>>removing them - they are convenient (for now).
>>
>>Example: I'm using the dummy ion driver, and I want to allocate from
>>the SYSTEM_CONTIG heap - the ION_HEAP_SYSTEM_CONTIG_MASK gives me the
>>exact mask I need for that.
>>
>>It seems your opinion is that heap-IDs are already, and should be,
>>completely decoupled from their type. That sounds like a good idea to
>>me, but it's not true (yet) - again check out the dummy driver.
>>
>
>Good point, I need to clean up the dummy driver to stop using heap
>types as the id ;)
>
>I get that it's convenient but it's a bad practice to conflate the
>namespaces.
>
>>At the moment, heap-IDs are assigned by ion drivers in any way they
>>see fit. For as long as that stays the case there's always going to
>>be heap-masks hard-coded in UAPI kernel headers (in-tree or not), so
>>removing these particular masks seems a bit fruitless.
>>
>
>It's not fruitless, the concept of type as mask makes no sense. They
>are two different name spaces and I've found Ion users have a hard
>time keeping them separate and pass in the heap type mask when using
>non dummy

You can add me to that group :-)

>
>>I'd rather see driver-assigned heap-IDs disappear completely, and have
>>them assigned by ion core from an idr or something. At that point
>>these macros really *are* meaningless, and I'd be totally fine with
>>removing them (and userspace won't be able to depend on hard-coded
>>allocation masks any more - it will have to use the query ioctl,
>>which I assume is the whole point?).
>>
>
>Ideally yes we'd be able to get rid of the hard coded device IDs.
>I consider the query ioctl a stepping stone to that, depending on
>how enthusiastic people are about Ion.
>
>>IMO it's not the right time to remove these macros, because they still
>>have meaning and usefulness.
>>
>
>I still think they should be deleted to avoid namespace polution.
>

If you nuke type-as-ID in dummy, and put a clear comment on the
ion_heap_type enum stating that heap-type and heap-ID are strictly
different, then I'm happy.
I think without that there'll still be confusion.

Thanks!
Brian

>>Cheers,
>>Brian
>>
>>>
>>>>
>>>>Thanks,
>>>>Brian
>>>>
>>>>>Signed-off-by: Laura Abbott <labbott@redhat.com>
>>>>>---
>>>>>drivers/staging/android/uapi/ion.h | 6 ------
>>>>>1 file changed, 6 deletions(-)
>>>>>
>>>>>diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
>>>>>index 0a8e40f..a9c4e8b 100644
>>>>>--- a/drivers/staging/android/uapi/ion.h
>>>>>+++ b/drivers/staging/android/uapi/ion.h
>>>>>@@ -44,14 +44,8 @@ enum ion_heap_type {
>>>>>                  * must be last so device specific heaps always
>>>>>                  * are at the end of this enum
>>>>>                  */
>>>>>-    ION_NUM_HEAPS = 16,
>>>>>};
>>>>>
>>>>>-#define ION_HEAP_SYSTEM_MASK        (1 << ION_HEAP_TYPE_SYSTEM)
>>>>>-#define ION_HEAP_SYSTEM_CONTIG_MASK    (1 << ION_HEAP_TYPE_SYSTEM_CONTIG)
>>>>>-#define ION_HEAP_CARVEOUT_MASK        (1 << ION_HEAP_TYPE_CARVEOUT)
>>>>>-#define ION_HEAP_TYPE_DMA_MASK        (1 << ION_HEAP_TYPE_DMA)
>>>>>-
>>>>>#define ION_NUM_HEAP_IDS        (sizeof(unsigned int) * 8)
>>>>>
>>>>>/**
>>>>>--
>>>>>2.7.4
>>>>>
>>>
>

[PATCHv2 2/4] staging: android: ion: Pull out ion ioctls to a separate file

[Laura Abbott]

The number of Ion ioctls may continue to grow along with necessary
validation. Pull it out into a separate file for easier management
and review.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
 drivers/staging/android/ion/Makefile    |   3 +-
 drivers/staging/android/ion/ion-ioctl.c | 144 ++++++++++++++++++++++
 drivers/staging/android/ion/ion.c       | 210 ++------------------------------
 drivers/staging/android/ion/ion_priv.h  |  91 ++++++++++++++
 4 files changed, 244 insertions(+), 204 deletions(-)
 create mode 100644 drivers/staging/android/ion/ion-ioctl.c

diff --git a/drivers/staging/android/ion/Makefile b/drivers/staging/android/ion/Makefile
index 18cc2aa..376c2b2 100644
--- a/drivers/staging/android/ion/Makefile
+++ b/drivers/staging/android/ion/Makefile
@@ -1,4 +1,5 @@
-obj-$(CONFIG_ION) +=	ion.o ion_heap.o ion_page_pool.o ion_system_heap.o \
+obj-$(CONFIG_ION) +=	ion.o ion-ioctl.o ion_heap.o \
+			ion_page_pool.o ion_system_heap.o \
 			ion_carveout_heap.o ion_chunk_heap.o ion_cma_heap.o
 obj-$(CONFIG_ION_TEST) += ion_test.o
 ifdef CONFIG_COMPAT
diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
new file mode 100644
index 0000000..341ba7d
--- /dev/null
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -0,0 +1,144 @@
+/*
+ *
+ * Copyright (C) 2011 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/file.h>
+#include <linux/fs.h>
+#include <linux/uaccess.h>
+
+#include "ion.h"
+#include "ion_priv.h"
+#include "compat_ion.h"
+
+/* fix up the cases where the ioctl direction bits are incorrect */
+static unsigned int ion_ioctl_dir(unsigned int cmd)
+{
+	switch (cmd) {
+	case ION_IOC_SYNC:
+	case ION_IOC_FREE:
+	case ION_IOC_CUSTOM:
+		return _IOC_WRITE;
+	default:
+		return _IOC_DIR(cmd);
+	}
+}
+
+long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	struct ion_client *client = filp->private_data;
+	struct ion_device *dev = client->dev;
+	struct ion_handle *cleanup_handle = NULL;
+	int ret = 0;
+	unsigned int dir;
+
+	union {
+		struct ion_fd_data fd;
+		struct ion_allocation_data allocation;
+		struct ion_handle_data handle;
+		struct ion_custom_data custom;
+	} data;
+
+	dir = ion_ioctl_dir(cmd);
+
+	if (_IOC_SIZE(cmd) > sizeof(data))
+		return -EINVAL;
+
+	if (dir & _IOC_WRITE)
+		if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
+			return -EFAULT;
+
+	switch (cmd) {
+	case ION_IOC_ALLOC:
+	{
+		struct ion_handle *handle;
+
+		handle = ion_alloc(client, data.allocation.len,
+						data.allocation.align,
+						data.allocation.heap_id_mask,
+						data.allocation.flags);
+		if (IS_ERR(handle))
+			return PTR_ERR(handle);
+
+		data.allocation.handle = handle->id;
+
+		cleanup_handle = handle;
+		break;
+	}
+	case ION_IOC_FREE:
+	{
+		struct ion_handle *handle;
+
+		mutex_lock(&client->lock);
+		handle = ion_handle_get_by_id_nolock(client, data.handle.handle);
+		if (IS_ERR(handle)) {
+			mutex_unlock(&client->lock);
+			return PTR_ERR(handle);
+		}
+		ion_free_nolock(client, handle);
+		ion_handle_put_nolock(handle);
+		mutex_unlock(&client->lock);
+		break;
+	}
+	case ION_IOC_SHARE:
+	case ION_IOC_MAP:
+	{
+		struct ion_handle *handle;
+
+		handle = ion_handle_get_by_id(client, data.handle.handle);
+		if (IS_ERR(handle))
+			return PTR_ERR(handle);
+		data.fd.fd = ion_share_dma_buf_fd(client, handle);
+		ion_handle_put(handle);
+		if (data.fd.fd < 0)
+			ret = data.fd.fd;
+		break;
+	}
+	case ION_IOC_IMPORT:
+	{
+		struct ion_handle *handle;
+
+		handle = ion_import_dma_buf_fd(client, data.fd.fd);
+		if (IS_ERR(handle))
+			ret = PTR_ERR(handle);
+		else
+			data.handle.handle = handle->id;
+		break;
+	}
+	case ION_IOC_SYNC:
+	{
+		ret = ion_sync_for_device(client, data.fd.fd);
+		break;
+	}
+	case ION_IOC_CUSTOM:
+	{
+		if (!dev->custom_ioctl)
+			return -ENOTTY;
+		ret = dev->custom_ioctl(client, data.custom.cmd,
+						data.custom.arg);
+		break;
+	}
+	default:
+		return -ENOTTY;
+	}
+
+	if (dir & _IOC_READ) {
+		if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd))) {
+			if (cleanup_handle)
+				ion_free(client, cleanup_handle);
+			return -EFAULT;
+		}
+	}
+	return ret;
+}
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 88dd17e..975b48f 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -41,80 +41,6 @@
 #include "ion_priv.h"
 #include "compat_ion.h"
 
-/**
- * struct ion_device - the metadata of the ion device node
- * @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
- * @heaps:		list of all the heaps in the system
- * @user_clients:	list of all the clients created from userspace
- */
-struct ion_device {
-	struct miscdevice dev;
-	struct rb_root buffers;
-	struct mutex buffer_lock;
-	struct rw_semaphore lock;
-	struct plist_head heaps;
-	long (*custom_ioctl)(struct ion_client *client, unsigned int cmd,
-			     unsigned long arg);
-	struct rb_root clients;
-	struct dentry *debug_root;
-	struct dentry *heaps_debug_root;
-	struct dentry *clients_debug_root;
-};
-
-/**
- * struct ion_client - a process/hw block local address space
- * @node:		node in the tree of all clients
- * @dev:		backpointer to ion device
- * @handles:		an rb tree of all the handles in this client
- * @idr:		an idr space for allocating handle ids
- * @lock:		lock protecting the tree of handles
- * @name:		used for debugging
- * @display_name:	used for debugging (unique version of @name)
- * @display_serial:	used for debugging (to make display_name unique)
- * @task:		used for debugging
- *
- * A client represents a list of buffers this client may access.
- * The mutex stored here is used to protect both handles tree
- * as well as the handles themselves, and should be held while modifying either.
- */
-struct ion_client {
-	struct rb_node node;
-	struct ion_device *dev;
-	struct rb_root handles;
-	struct idr idr;
-	struct mutex lock;
-	const char *name;
-	char *display_name;
-	int display_serial;
-	struct task_struct *task;
-	pid_t pid;
-	struct dentry *debug_root;
-};
-
-/**
- * ion_handle - a client local reference to a buffer
- * @ref:		reference count
- * @client:		back pointer to the client the buffer resides in
- * @buffer:		pointer to the buffer
- * @node:		node in the client's handle rbtree
- * @kmap_cnt:		count of times this client has mapped to kernel
- * @id:			client-unique id allocated by client->idr
- *
- * Modifications to node, map_cnt or mapping should be protected by the
- * lock in the client.  Other fields are never changed after initialization.
- */
-struct ion_handle {
-	struct kref ref;
-	struct ion_client *client;
-	struct ion_buffer *buffer;
-	struct rb_node node;
-	unsigned int kmap_cnt;
-	int id;
-};
-
 bool ion_buffer_fault_user_mappings(struct ion_buffer *buffer)
 {
 	return (buffer->flags & ION_FLAG_CACHED) &&
@@ -381,7 +307,7 @@ static void ion_handle_get(struct ion_handle *handle)
 	kref_get(&handle->ref);
 }
 
-static int ion_handle_put_nolock(struct ion_handle *handle)
+int ion_handle_put_nolock(struct ion_handle *handle)
 {
 	int ret;
 
@@ -390,7 +316,7 @@ static int ion_handle_put_nolock(struct ion_handle *handle)
 	return ret;
 }
 
-static int ion_handle_put(struct ion_handle *handle)
+int ion_handle_put(struct ion_handle *handle)
 {
 	struct ion_client *client = handle->client;
 	int ret;
@@ -420,7 +346,7 @@ static struct ion_handle *ion_handle_lookup(struct ion_client *client,
 	return ERR_PTR(-EINVAL);
 }
 
-static struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client,
+struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client,
 						int id)
 {
 	struct ion_handle *handle;
@@ -432,7 +358,7 @@ static struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client,
 	return handle ? handle : ERR_PTR(-EINVAL);
 }
 
-static struct ion_handle *ion_handle_get_by_id(struct ion_client *client,
+struct ion_handle *ion_handle_get_by_id(struct ion_client *client,
 					       int id)
 {
 	struct ion_handle *handle;
@@ -545,8 +471,8 @@ struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
 }
 EXPORT_SYMBOL(ion_alloc);
 
-static void ion_free_nolock(struct ion_client *client,
-			    struct ion_handle *handle)
+void ion_free_nolock(struct ion_client *client,
+		     struct ion_handle *handle)
 {
 	bool valid_handle;
 
@@ -1224,7 +1150,7 @@ struct ion_handle *ion_import_dma_buf_fd(struct ion_client *client, int fd)
 }
 EXPORT_SYMBOL(ion_import_dma_buf_fd);
 
-static int ion_sync_for_device(struct ion_client *client, int fd)
+int ion_sync_for_device(struct ion_client *client, int fd)
 {
 	struct dma_buf *dmabuf;
 	struct ion_buffer *buffer;
@@ -1248,128 +1174,6 @@ static int ion_sync_for_device(struct ion_client *client, int fd)
 	return 0;
 }
 
-/* fix up the cases where the ioctl direction bits are incorrect */
-static unsigned int ion_ioctl_dir(unsigned int cmd)
-{
-	switch (cmd) {
-	case ION_IOC_SYNC:
-	case ION_IOC_FREE:
-	case ION_IOC_CUSTOM:
-		return _IOC_WRITE;
-	default:
-		return _IOC_DIR(cmd);
-	}
-}
-
-static long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
-{
-	struct ion_client *client = filp->private_data;
-	struct ion_device *dev = client->dev;
-	struct ion_handle *cleanup_handle = NULL;
-	int ret = 0;
-	unsigned int dir;
-
-	union {
-		struct ion_fd_data fd;
-		struct ion_allocation_data allocation;
-		struct ion_handle_data handle;
-		struct ion_custom_data custom;
-	} data;
-
-	dir = ion_ioctl_dir(cmd);
-
-	if (_IOC_SIZE(cmd) > sizeof(data))
-		return -EINVAL;
-
-	if (dir & _IOC_WRITE)
-		if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
-			return -EFAULT;
-
-	switch (cmd) {
-	case ION_IOC_ALLOC:
-	{
-		struct ion_handle *handle;
-
-		handle = ion_alloc(client, data.allocation.len,
-						data.allocation.align,
-						data.allocation.heap_id_mask,
-						data.allocation.flags);
-		if (IS_ERR(handle))
-			return PTR_ERR(handle);
-
-		data.allocation.handle = handle->id;
-
-		cleanup_handle = handle;
-		break;
-	}
-	case ION_IOC_FREE:
-	{
-		struct ion_handle *handle;
-
-		mutex_lock(&client->lock);
-		handle = ion_handle_get_by_id_nolock(client,
-						     data.handle.handle);
-		if (IS_ERR(handle)) {
-			mutex_unlock(&client->lock);
-			return PTR_ERR(handle);
-		}
-		ion_free_nolock(client, handle);
-		ion_handle_put_nolock(handle);
-		mutex_unlock(&client->lock);
-		break;
-	}
-	case ION_IOC_SHARE:
-	case ION_IOC_MAP:
-	{
-		struct ion_handle *handle;
-
-		handle = ion_handle_get_by_id(client, data.handle.handle);
-		if (IS_ERR(handle))
-			return PTR_ERR(handle);
-		data.fd.fd = ion_share_dma_buf_fd(client, handle);
-		ion_handle_put(handle);
-		if (data.fd.fd < 0)
-			ret = data.fd.fd;
-		break;
-	}
-	case ION_IOC_IMPORT:
-	{
-		struct ion_handle *handle;
-
-		handle = ion_import_dma_buf_fd(client, data.fd.fd);
-		if (IS_ERR(handle))
-			ret = PTR_ERR(handle);
-		else
-			data.handle.handle = handle->id;
-		break;
-	}
-	case ION_IOC_SYNC:
-	{
-		ret = ion_sync_for_device(client, data.fd.fd);
-		break;
-	}
-	case ION_IOC_CUSTOM:
-	{
-		if (!dev->custom_ioctl)
-			return -ENOTTY;
-		ret = dev->custom_ioctl(client, data.custom.cmd,
-						data.custom.arg);
-		break;
-	}
-	default:
-		return -ENOTTY;
-	}
-
-	if (dir & _IOC_READ) {
-		if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd))) {
-			if (cleanup_handle)
-				ion_free(client, cleanup_handle);
-			return -EFAULT;
-		}
-	}
-	return ret;
-}
-
 static int ion_release(struct inode *inode, struct file *file)
 {
 	struct ion_client *client = file->private_data;
diff --git a/drivers/staging/android/ion/ion_priv.h b/drivers/staging/android/ion/ion_priv.h
index 5eed5e2..95df6a9 100644
--- a/drivers/staging/android/ion/ion_priv.h
+++ b/drivers/staging/android/ion/ion_priv.h
@@ -26,6 +26,7 @@
 #include <linux/sched.h>
 #include <linux/shrinker.h>
 #include <linux/types.h>
+#include <linux/miscdevice.h>
 
 #include "ion.h"
 
@@ -83,6 +84,80 @@ struct ion_buffer {
 void ion_buffer_destroy(struct ion_buffer *buffer);
 
 /**
+ * struct ion_device - the metadata of the ion device node
+ * @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
+ * @heaps:		list of all the heaps in the system
+ * @user_clients:	list of all the clients created from userspace
+ */
+struct ion_device {
+	struct miscdevice dev;
+	struct rb_root buffers;
+	struct mutex buffer_lock;
+	struct rw_semaphore lock;
+	struct plist_head heaps;
+	long (*custom_ioctl)(struct ion_client *client, unsigned int cmd,
+			     unsigned long arg);
+	struct rb_root clients;
+	struct dentry *debug_root;
+	struct dentry *heaps_debug_root;
+	struct dentry *clients_debug_root;
+};
+
+/**
+ * struct ion_client - a process/hw block local address space
+ * @node:		node in the tree of all clients
+ * @dev:		backpointer to ion device
+ * @handles:		an rb tree of all the handles in this client
+ * @idr:		an idr space for allocating handle ids
+ * @lock:		lock protecting the tree of handles
+ * @name:		used for debugging
+ * @display_name:	used for debugging (unique version of @name)
+ * @display_serial:	used for debugging (to make display_name unique)
+ * @task:		used for debugging
+ *
+ * A client represents a list of buffers this client may access.
+ * The mutex stored here is used to protect both handles tree
+ * as well as the handles themselves, and should be held while modifying either.
+ */
+struct ion_client {
+	struct rb_node node;
+	struct ion_device *dev;
+	struct rb_root handles;
+	struct idr idr;
+	struct mutex lock;
+	const char *name;
+	char *display_name;
+	int display_serial;
+	struct task_struct *task;
+	pid_t pid;
+	struct dentry *debug_root;
+};
+
+/**
+ * ion_handle - a client local reference to a buffer
+ * @ref:		reference count
+ * @client:		back pointer to the client the buffer resides in
+ * @buffer:		pointer to the buffer
+ * @node:		node in the client's handle rbtree
+ * @kmap_cnt:		count of times this client has mapped to kernel
+ * @id:			client-unique id allocated by client->idr
+ *
+ * Modifications to node, map_cnt or mapping should be protected by the
+ * lock in the client.  Other fields are never changed after initialization.
+ */
+struct ion_handle {
+	struct kref ref;
+	struct ion_client *client;
+	struct ion_buffer *buffer;
+	struct rb_node node;
+	unsigned int kmap_cnt;
+	int id;
+};
+
+/**
  * struct ion_heap_ops - ops to operate on a given heap
  * @allocate:		allocate memory
  * @free:		free memory
@@ -378,4 +453,20 @@ int ion_page_pool_shrink(struct ion_page_pool *pool, gfp_t gfp_mask,
 void ion_pages_sync_for_device(struct device *dev, struct page *page,
 		size_t size, enum dma_data_direction dir);
 
+long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
+
+int ion_sync_for_device(struct ion_client *client, int fd);
+
+struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client,
+						int id);
+
+void ion_free_nolock(struct ion_client *client, struct ion_handle *handle);
+
+int ion_handle_put_nolock(struct ion_handle *handle);
+
+struct ion_handle *ion_handle_get_by_id(struct ion_client *client,
+						int id);
+
+int ion_handle_put(struct ion_handle *handle);
+
 #endif /* _ION_PRIV_H */
-- 
2.7.4

Re: [PATCHv2 2/4] staging: android: ion: Pull out ion ioctls to a separate file

[Greg Kroah-Hartman]

On Thu, Sep 01, 2016 at 03:40:42PM -0700, Laura Abbott wrote:
> 
> The number of Ion ioctls may continue to grow along with necessary
> validation. Pull it out into a separate file for easier management
> and review.
> 
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
>  drivers/staging/android/ion/Makefile    |   3 +-
>  drivers/staging/android/ion/ion-ioctl.c | 144 ++++++++++++++++++++++
>  drivers/staging/android/ion/ion.c       | 210 ++------------------------------
>  drivers/staging/android/ion/ion_priv.h  |  91 ++++++++++++++
>  4 files changed, 244 insertions(+), 204 deletions(-)
>  create mode 100644 drivers/staging/android/ion/ion-ioctl.c

This patch doesn't apply cleanly to my tree, are we out of sync somehow?

Can you rebase your outstanding patches against my staging-testing
branch and resend?

thanks,

greg k-h

Re: [PATCHv2 2/4] staging: android: ion: Pull out ion ioctls to a separate file

[Laura Abbott]

On 09/02/2016 05:44 AM, Greg Kroah-Hartman wrote:
> On Thu, Sep 01, 2016 at 03:40:42PM -0700, Laura Abbott wrote:
>>
>> The number of Ion ioctls may continue to grow along with necessary
>> validation. Pull it out into a separate file for easier management
>> and review.
>>
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> ---
>>  drivers/staging/android/ion/Makefile    |   3 +-
>>  drivers/staging/android/ion/ion-ioctl.c | 144 ++++++++++++++++++++++
>>  drivers/staging/android/ion/ion.c       | 210 ++------------------------------
>>  drivers/staging/android/ion/ion_priv.h  |  91 ++++++++++++++
>>  4 files changed, 244 insertions(+), 204 deletions(-)
>>  create mode 100644 drivers/staging/android/ion/ion-ioctl.c
>
> This patch doesn't apply cleanly to my tree, are we out of sync somehow?
>
> Can you rebase your outstanding patches against my staging-testing
> branch and resend?
>
> thanks,
>
> greg k-h
>

Looks like I missed a rebase with one of the cleanup patches. I'll resend.

Thanks,
Laura

[PATCHv2 3/4] staging: android: ion: Add an ioctl for ABI checking

[Laura Abbott]

The current Ion ioctls lack a good way to tell what ioctls are
available. Introduce an ioctl to give an ABI version. This way when the
ABI inevitably gets screwed up userspace will have a way to tell what
version of the screw up is available.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
 drivers/staging/android/ion/ion-ioctl.c | 53 ++++++++++++++++++++++++++-------
 drivers/staging/android/uapi/ion.h      | 22 ++++++++++++++
 2 files changed, 65 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index 341ba7d..53b9520 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -22,6 +22,29 @@
 #include "ion_priv.h"
 #include "compat_ion.h"
 
+union ion_ioctl_arg {
+	struct ion_fd_data fd;
+	struct ion_allocation_data allocation;
+	struct ion_handle_data handle;
+	struct ion_custom_data custom;
+	struct ion_abi_version abi_version;
+};
+
+static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
+{
+	int ret = 0;
+
+	switch (cmd) {
+	case ION_IOC_ABI_VERSION:
+		ret = arg->abi_version.reserved != 0;
+		break;
+	default:
+		break;
+	}
+
+	return ret ? -EINVAL : 0;
+}
+
 /* fix up the cases where the ioctl direction bits are incorrect */
 static unsigned int ion_ioctl_dir(unsigned int cmd)
 {
@@ -42,22 +65,27 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	struct ion_handle *cleanup_handle = NULL;
 	int ret = 0;
 	unsigned int dir;
-
-	union {
-		struct ion_fd_data fd;
-		struct ion_allocation_data allocation;
-		struct ion_handle_data handle;
-		struct ion_custom_data custom;
-	} data;
+	union ion_ioctl_arg data;
 
 	dir = ion_ioctl_dir(cmd);
 
 	if (_IOC_SIZE(cmd) > sizeof(data))
 		return -EINVAL;
 
-	if (dir & _IOC_WRITE)
-		if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
-			return -EFAULT;
+	/*
+	 * The copy_from_user is unconditional here for both read and write
+	 * to do the validate. If there is no write for the ioctl, the
+	 * buffer is cleared
+	 */
+	if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
+		return -EFAULT;
+
+	ret = validate_ioctl_arg(cmd, &data);
+	if (WARN_ON_ONCE(ret))
+		return ret;
+
+	if (!(dir & _IOC_WRITE))
+		memset(&data, 0, sizeof(data));
 
 	switch (cmd) {
 	case ION_IOC_ALLOC:
@@ -129,6 +157,11 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 						data.custom.arg);
 		break;
 	}
+	case ION_IOC_ABI_VERSION:
+	{
+		data.abi_version.abi_version = ION_ABI_VERSION;
+		break;
+	}
 	default:
 		return -ENOTTY;
 	}
diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
index a9c4e8b..7ca4e8b 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -19,6 +19,7 @@
 
 #include <linux/ioctl.h>
 #include <linux/types.h>
+#include <linux/version.h>
 
 typedef int ion_user_handle_t;
 
@@ -128,6 +129,19 @@ struct ion_custom_data {
 	unsigned long arg;
 };
 
+/**
+ * struct ion_abi_version
+ *
+ *  @version - current ABI version
+ */
+
+#define ION_ABI_VERSION                KERNEL_VERSION(0, 1, 0)
+
+struct ion_abi_version {
+	__u32 abi_version;
+	__u32 reserved;
+};
+
 #define ION_IOC_MAGIC		'I'
 
 /**
@@ -194,4 +208,12 @@ struct ion_custom_data {
  */
 #define ION_IOC_CUSTOM		_IOWR(ION_IOC_MAGIC, 6, struct ion_custom_data)
 
+/**
+ * DOC: ION_IOC_ABI_VERSION - return ABI version
+ *
+ * Returns the ABI version of this driver.
+ */
+#define ION_IOC_ABI_VERSION    _IOR(ION_IOC_MAGIC, 8, \
+					struct ion_abi_version)
+
 #endif /* _UAPI_LINUX_ION_H */
-- 
2.7.4

Re: [PATCHv2 3/4] staging: android: ion: Add an ioctl for ABI checking

[Greg Kroah-Hartman]

On Thu, Sep 01, 2016 at 03:40:43PM -0700, Laura Abbott wrote:
> 
> The current Ion ioctls lack a good way to tell what ioctls are
> available. Introduce an ioctl to give an ABI version. This way when the
> ABI inevitably gets screwed up userspace will have a way to tell what
> version of the screw up is available.

This worries me.  Why do we need this?  Shouldn't any "new" abi changes
just add on, and not change existing ioctl structure calls?  Or worst
case, you remove an ioctl and then userspace "knows" that when the call
fails.

And who is the major userspace user of this interface?  Who controls it?
How are we keeping things in sync here?

thanks,

greg k-h

Re: [PATCHv2 3/4] staging: android: ion: Add an ioctl for ABI checking

[Laura Abbott]

On 09/01/2016 11:10 PM, Greg Kroah-Hartman wrote:
> On Thu, Sep 01, 2016 at 03:40:43PM -0700, Laura Abbott wrote:
>>
>> The current Ion ioctls lack a good way to tell what ioctls are
>> available. Introduce an ioctl to give an ABI version. This way when the
>> ABI inevitably gets screwed up userspace will have a way to tell what
>> version of the screw up is available.
>
> This worries me.  Why do we need this?  Shouldn't any "new" abi changes
> just add on, and not change existing ioctl structure calls?  Or worst
> case, you remove an ioctl and then userspace "knows" that when the call
> fails.

This may be more of an "I wish we had this when some poor decisions were
made in the past". There were a couple of instances when the Ion ABI
was broken (adding new fields, new ioctl numbers) that were a nightmare
to deal with and a similar ioctl would have helped a lot. The
botching-ioctls document also made reference to something simliar.

>
> And who is the major userspace user of this interface?  Who controls it?
> How are we keeping things in sync here?
>

I would expect this to not actually be used until we have breakage. The
broken ioctl would then be checked as needed.

Reading all this and thinking some, it sounds like this shouldn't actually
be needed so long as we continue to not break the ioctls. I had a thought
of this possibly making life easier for out of tree users to eventually
convert over but I haven't heard much from actual out of tree users.

I'd like to keep it just to hedge my bets but I also haven't had as much
experience maintaining stable ioctls for the long term. If, from others
experience, this type of ioctl is actually just more prone to breakage
and doesn't help then I don't want to push something that will eventually
break.

> thanks,
>
> greg k-h
>

Thanks,
Laura

Re: [Linaro-mm-sig] [PATCHv2 3/4] staging: android: ion: Add an ioctl for ABI checking

[Arnd Bergmann]

On Thursday, September 1, 2016 3:40:43 PM CEST Laura Abbott wrote:

> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -22,6 +22,29 @@
>  #include "ion_priv.h"
>  #include "compat_ion.h"
>  
> +union ion_ioctl_arg {
> +	struct ion_fd_data fd;
> +	struct ion_allocation_data allocation;
> +	struct ion_handle_data handle;
> +	struct ion_custom_data custom;
> +	struct ion_abi_version abi_version;
> +};

Are you introducing this, or just clarifying the defintion of the
existing interface. For new interfaces, we should not have a union
as an ioctl argument. Instead each ioctl command should have one
specific structure (or better a scalar argument).

> +static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
> +{
> +	int ret = 0;
> +
> +	switch (cmd) {
> +	case ION_IOC_ABI_VERSION:
> +		ret = arg->abi_version.reserved != 0;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret ? -EINVAL : 0;
> +}

I agree with Greg, ioctl interfaces should normally not be versioned,
the usual way is to try a command and see if it fails or not.

> +/**
> + * struct ion_abi_version
> + *
> + *  @version - current ABI version
> + */
> +
> +#define ION_ABI_VERSION                KERNEL_VERSION(0, 1, 0)
> +
> +struct ion_abi_version {
> +	__u32 abi_version;
> +	__u32 reserved;
> +};
> +

This interface doesn't really need a "reserved" field, you could
as well use a __u32 by itself. If you ever need a second field,
just add a new command number.

	Arnd

Re: [Linaro-mm-sig] [PATCHv2 3/4] staging: android: ion: Add an ioctl for ABI checking

[Laura Abbott]

On 09/02/2016 02:02 AM, Arnd Bergmann wrote:
> On Thursday, September 1, 2016 3:40:43 PM CEST Laura Abbott wrote:
>
>> --- a/drivers/staging/android/ion/ion-ioctl.c
>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>> @@ -22,6 +22,29 @@
>>  #include "ion_priv.h"
>>  #include "compat_ion.h"
>>
>> +union ion_ioctl_arg {
>> +	struct ion_fd_data fd;
>> +	struct ion_allocation_data allocation;
>> +	struct ion_handle_data handle;
>> +	struct ion_custom_data custom;
>> +	struct ion_abi_version abi_version;
>> +};
>
> Are you introducing this, or just clarifying the defintion of the
> existing interface. For new interfaces, we should not have a union
> as an ioctl argument. Instead each ioctl command should have one
> specific structure (or better a scalar argument).
>

This was just a structure inside ion_ioctl. I pulled it out for
the validate function. It's not an actual argument to any ioctl from
userspace. ion_ioctl copies using _IOC_SIZE.

>> +static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
>> +{
>> +	int ret = 0;
>> +
>> +	switch (cmd) {
>> +	case ION_IOC_ABI_VERSION:
>> +		ret = arg->abi_version.reserved != 0;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return ret ? -EINVAL : 0;
>> +}
>
> I agree with Greg, ioctl interfaces should normally not be versioned,
> the usual way is to try a command and see if it fails or not.
>

The concern was trying ioctls that wouldn't actually fail or would
have some other unexpected side effect.

My conclusion from the other thread was that assuming we don't botch
up adding new ioctls in the future or make incompatible changes to
these in the future we shouldn't technically need it. I was still
trying to hedge my bets against the future but that might just be
making the problem worse?

>> +/**
>> + * struct ion_abi_version
>> + *
>> + *  @version - current ABI version
>> + */
>> +
>> +#define ION_ABI_VERSION                KERNEL_VERSION(0, 1, 0)
>> +
>> +struct ion_abi_version {
>> +	__u32 abi_version;
>> +	__u32 reserved;
>> +};
>> +
>
> This interface doesn't really need a "reserved" field, you could
> as well use a __u32 by itself. If you ever need a second field,
> just add a new command number.
>

The botching-ioctls.txt document suggested everything should be aligned
to 64-bits. Was I interpreting that too literally?

> 	Arnd
>

Thanks,
Laura

Re: [Linaro-mm-sig] [PATCHv2 3/4] staging: android: ion: Add an ioctl for ABI checking

[Arnd Bergmann]

On Friday, September 2, 2016 1:33:44 PM CEST Laura Abbott wrote:
> On 09/02/2016 02:02 AM, Arnd Bergmann wrote:
> > On Thursday, September 1, 2016 3:40:43 PM CEST Laura Abbott wrote:
> >
> >> --- a/drivers/staging/android/ion/ion-ioctl.c
> >> +++ b/drivers/staging/android/ion/ion-ioctl.c
> >> @@ -22,6 +22,29 @@
> >>  #include "ion_priv.h"
> >>  #include "compat_ion.h"
> >>
> >> +union ion_ioctl_arg {
> >> +	struct ion_fd_data fd;
> >> +	struct ion_allocation_data allocation;
> >> +	struct ion_handle_data handle;
> >> +	struct ion_custom_data custom;
> >> +	struct ion_abi_version abi_version;
> >> +};
> >
> > Are you introducing this, or just clarifying the defintion of the
> > existing interface. For new interfaces, we should not have a union
> > as an ioctl argument. Instead each ioctl command should have one
> > specific structure (or better a scalar argument).
> >
> 
> This was just a structure inside ion_ioctl. I pulled it out for
> the validate function. It's not an actual argument to any ioctl from
> userspace. ion_ioctl copies using _IOC_SIZE.

Ok, got it. This is fine from an interface point of view, just
a bit unusual in the way it's written.

> >> +static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
> >> +{
> >> +	int ret = 0;
> >> +
> >> +	switch (cmd) {
> >> +	case ION_IOC_ABI_VERSION:
> >> +		ret = arg->abi_version.reserved != 0;
> >> +		break;
> >> +	default:
> >> +		break;
> >> +	}
> >> +
> >> +	return ret ? -EINVAL : 0;
> >> +}
> >
> > I agree with Greg, ioctl interfaces should normally not be versioned,
> > the usual way is to try a command and see if it fails or not.
> >
> 
> The concern was trying ioctls that wouldn't actually fail or would
> have some other unexpected side effect.
> 
> My conclusion from the other thread was that assuming we don't botch
> up adding new ioctls in the future or make incompatible changes to
> these in the future we shouldn't technically need it. I was still
> trying to hedge my bets against the future but that might just be
> making the problem worse?

We've had a number of cases where versioned ABIs just didn't work out.

The versions are either used to distinguish incompatible APIs, which
we should avoid to start with, or they are used for backwards-compatible
extensions that you should detect by checking whether an ioctl
succeeds. Relying on the API version number breaks if you get a partial
backport of features from a later version, and it's unclear what a
user space tool should expect when the kernel reports a newer ABI
than it knows.

I think the wireless extensions and KVM are examples of versioned
APIs that turned out to make things more complicated than they
would have been otherwise.

> >> +/**
> >> + * struct ion_abi_version
> >> + *
> >> + *  @version - current ABI version
> >> + */
> >> +
> >> +#define ION_ABI_VERSION                KERNEL_VERSION(0, 1, 0)
> >> +
> >> +struct ion_abi_version {
> >> +	__u32 abi_version;
> >> +	__u32 reserved;
> >> +};
> >> +
> >
> > This interface doesn't really need a "reserved" field, you could
> > as well use a __u32 by itself. If you ever need a second field,
> > just add a new command number.
> >
> 
> The botching-ioctls.txt document suggested everything should be aligned
> to 64-bits. Was I interpreting that too literally?

I didn't even know that file existed ;-)

I'm pretty sure the paragraph refers to the problem of x86 of having
a structure like

	struct ioctl_arg {
		__u64 first;
		__u32 second;
	};

which is 12 bytes long on x86, but 16 bytes long including implied
padding on all 64-bit architectures and most (maybe all) 32-bit ones
other than x86.

If there is no 64-bit member in the struct, there is no need for padding
at the end.

	Arnd

Re: [Linaro-mm-sig] [PATCHv2 3/4] staging: android: ion: Add an ioctl for ABI checking

[Laura Abbott]

On 09/02/2016 02:33 PM, Arnd Bergmann wrote:
> On Friday, September 2, 2016 1:33:44 PM CEST Laura Abbott wrote:
>> On 09/02/2016 02:02 AM, Arnd Bergmann wrote:
>>> On Thursday, September 1, 2016 3:40:43 PM CEST Laura Abbott wrote:
>>>
>>>> --- a/drivers/staging/android/ion/ion-ioctl.c
>>>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>>>> @@ -22,6 +22,29 @@
>>>>  #include "ion_priv.h"
>>>>  #include "compat_ion.h"
>>>>
>>>> +union ion_ioctl_arg {
>>>> +	struct ion_fd_data fd;
>>>> +	struct ion_allocation_data allocation;
>>>> +	struct ion_handle_data handle;
>>>> +	struct ion_custom_data custom;
>>>> +	struct ion_abi_version abi_version;
>>>> +};
>>>
>>> Are you introducing this, or just clarifying the defintion of the
>>> existing interface. For new interfaces, we should not have a union
>>> as an ioctl argument. Instead each ioctl command should have one
>>> specific structure (or better a scalar argument).
>>>
>>
>> This was just a structure inside ion_ioctl. I pulled it out for
>> the validate function. It's not an actual argument to any ioctl from
>> userspace. ion_ioctl copies using _IOC_SIZE.
>
> Ok, got it. This is fine from an interface point of view, just
> a bit unusual in the way it's written.
>
>>>> +static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
>>>> +{
>>>> +	int ret = 0;
>>>> +
>>>> +	switch (cmd) {
>>>> +	case ION_IOC_ABI_VERSION:
>>>> +		ret = arg->abi_version.reserved != 0;
>>>> +		break;
>>>> +	default:
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	return ret ? -EINVAL : 0;
>>>> +}
>>>
>>> I agree with Greg, ioctl interfaces should normally not be versioned,
>>> the usual way is to try a command and see if it fails or not.
>>>
>>
>> The concern was trying ioctls that wouldn't actually fail or would
>> have some other unexpected side effect.
>>
>> My conclusion from the other thread was that assuming we don't botch
>> up adding new ioctls in the future or make incompatible changes to
>> these in the future we shouldn't technically need it. I was still
>> trying to hedge my bets against the future but that might just be
>> making the problem worse?
>
> We've had a number of cases where versioned ABIs just didn't work out.
>
> The versions are either used to distinguish incompatible APIs, which
> we should avoid to start with, or they are used for backwards-compatible
> extensions that you should detect by checking whether an ioctl
> succeeds. Relying on the API version number breaks if you get a partial
> backport of features from a later version, and it's unclear what a
> user space tool should expect when the kernel reports a newer ABI
> than it knows.
>
> I think the wireless extensions and KVM are examples of versioned
> APIs that turned out to make things more complicated than they
> would have been otherwise.
>

Okay it sounds like the answer is to strive to never run into a case
where versioned ioctls are necessary. Shouldn't be too hard, right? ;)

>>>> +/**
>>>> + * struct ion_abi_version
>>>> + *
>>>> + *  @version - current ABI version
>>>> + */
>>>> +
>>>> +#define ION_ABI_VERSION                KERNEL_VERSION(0, 1, 0)
>>>> +
>>>> +struct ion_abi_version {
>>>> +	__u32 abi_version;
>>>> +	__u32 reserved;
>>>> +};
>>>> +
>>>
>>> This interface doesn't really need a "reserved" field, you could
>>> as well use a __u32 by itself. If you ever need a second field,
>>> just add a new command number.
>>>
>>
>> The botching-ioctls.txt document suggested everything should be aligned
>> to 64-bits. Was I interpreting that too literally?
>
> I didn't even know that file existed ;-)
>
> I'm pretty sure the paragraph refers to the problem of x86 of having
> a structure like
>
> 	struct ioctl_arg {
> 		__u64 first;
> 		__u32 second;
> 	};
>
> which is 12 bytes long on x86, but 16 bytes long including implied
> padding on all 64-bit architectures and most (maybe all) 32-bit ones
> other than x86.
>

Right, that's the problem it's trying to avoid.

> If there is no 64-bit member in the struct, there is no need for padding
> at the end.
>

That's what I thought as well. I think I'll submit a patch to the docs
clarifying a few things.


> 	Arnd
>

Thanks,
Laura

[PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

[Laura Abbott]

Ion clients currently lack a good method to determine what
heaps are available and what ids they map to. This leads
to tight coupling between user and kernel space and headaches.
Add a query ioctl to let userspace know the availability of
heaps.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
 drivers/staging/android/ion/ion-ioctl.c | 11 +++++++++
 drivers/staging/android/ion/ion.c       | 44 +++++++++++++++++++++++++++++++++
 drivers/staging/android/ion/ion_priv.h  |  3 +++
 drivers/staging/android/uapi/ion.h      | 39 +++++++++++++++++++++++++++++
 4 files changed, 97 insertions(+)

diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index 53b9520..e76d517 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -28,6 +28,7 @@ union ion_ioctl_arg {
 	struct ion_handle_data handle;
 	struct ion_custom_data custom;
 	struct ion_abi_version abi_version;
+	struct ion_heap_query query;
 };
 
 static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
@@ -38,6 +39,11 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
 	case ION_IOC_ABI_VERSION:
 		ret = arg->abi_version.reserved != 0;
 		break;
+	case ION_IOC_HEAP_QUERY:
+		ret = arg->query.reserved0 != 0;
+		ret |= arg->query.reserved1 != 0;
+		ret |= arg->query.reserved2 != 0;
+		break;
 	default:
 		break;
 	}
@@ -162,6 +168,11 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		data.abi_version.abi_version = ION_ABI_VERSION;
 		break;
 	}
+	case ION_IOC_HEAP_QUERY:
+	{
+		ret = ion_query_heaps(client, &data.query);
+		break;
+	}
 	default:
 		return -ENOTTY;
 	}
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 975b48f..91b765c 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -1174,6 +1174,49 @@ int ion_sync_for_device(struct ion_client *client, int fd)
 	return 0;
 }
 
+int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query)
+{
+	struct ion_device *dev = client->dev;
+	struct ion_heap_data __user *buffer =
+		(struct ion_heap_data __user *)query->heaps;
+	int ret = -EINVAL, cnt = 0, max_cnt;
+	struct ion_heap *heap;
+	struct ion_heap_data hdata;
+
+	memset(&hdata, 0, sizeof(hdata));
+
+	down_read(&dev->lock);
+	if (!buffer) {
+		query->cnt = dev->heap_cnt;
+		ret = 0;
+		goto out;
+	}
+
+	if (query->cnt <= 0)
+		goto out;
+
+	max_cnt = query->cnt;
+
+	plist_for_each_entry(heap, &dev->heaps, node) {
+		strncpy(hdata.name, heap->name, MAX_HEAP_NAME);
+		hdata.name[sizeof(hdata.name) - 1] = '\0';
+		hdata.type = heap->type;
+		hdata.heap_id = heap->id;
+
+		ret = copy_to_user(&buffer[cnt],
+				   &hdata, sizeof(hdata));
+
+		cnt++;
+		if (cnt >= max_cnt)
+			break;
+	}
+
+	query->cnt = cnt;
+out:
+	up_read(&dev->lock);
+	return ret;
+}
+
 static int ion_release(struct inode *inode, struct file *file)
 {
 	struct ion_client *client = file->private_data;
@@ -1391,6 +1434,7 @@ void ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
 		}
 	}
 
+	dev->heap_cnt++;
 	up_write(&dev->lock);
 }
 EXPORT_SYMBOL(ion_device_add_heap);
diff --git a/drivers/staging/android/ion/ion_priv.h b/drivers/staging/android/ion/ion_priv.h
index 95df6a9..0d0c0aa 100644
--- a/drivers/staging/android/ion/ion_priv.h
+++ b/drivers/staging/android/ion/ion_priv.h
@@ -104,6 +104,7 @@ struct ion_device {
 	struct dentry *debug_root;
 	struct dentry *heaps_debug_root;
 	struct dentry *clients_debug_root;
+	int heap_cnt;
 };
 
 /**
@@ -469,4 +470,6 @@ struct ion_handle *ion_handle_get_by_id(struct ion_client *client,
 
 int ion_handle_put(struct ion_handle *handle);
 
+int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query);
+
 #endif /* _ION_PRIV_H */
diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
index 7ca4e8b..112f257 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -137,11 +137,41 @@ struct ion_custom_data {
 
 #define ION_ABI_VERSION                KERNEL_VERSION(0, 1, 0)
 
+#define MAX_HEAP_NAME			32
+
 struct ion_abi_version {
 	__u32 abi_version;
 	__u32 reserved;
 };
 
+/**
+ * struct ion_heap_data - data about a heap
+ * @name - first 32 characters of the heap name
+ * @type - heap type
+ * @heap_id - heap id for the heap
+ */
+struct ion_heap_data {
+	char name[MAX_HEAP_NAME];
+	__u32 type;
+	__u32 heap_id;
+	__u32 reserved0;
+	__u32 reserved1;
+	__u32 reserved2;
+};
+
+/**
+ * struct ion_heap_query - collection of data about all heaps
+ * @cnt - total number of heaps to be copied
+ * @heaps - buffer to copy heap data
+ */
+struct ion_heap_query {
+	__u32 cnt; /* Total number of heaps to be copied */
+	__u32 reserved0; /* align to 64bits */
+	__u64 heaps; /* buffer to be populated */
+	__u32 reserved1;
+	__u32 reserved2;
+};
+
 #define ION_IOC_MAGIC		'I'
 
 /**
@@ -216,4 +246,13 @@ struct ion_abi_version {
 #define ION_IOC_ABI_VERSION    _IOR(ION_IOC_MAGIC, 8, \
 					struct ion_abi_version)
 
+/**
+ * DOC: ION_IOC_HEAP_QUERY - information about available heaps
+ *
+ * Takes an ion_heap_query structure and populates information about
+ * available Ion heaps.
+ */
+#define ION_IOC_HEAP_QUERY     _IOWR(ION_IOC_MAGIC, 12, \
+					struct ion_heap_query)
+
 #endif /* _UAPI_LINUX_ION_H */
-- 
2.7.4

Re: [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2228 bytes --]

Hi Laura,

[auto build test WARNING on next-20160825]
[cannot apply to staging/staging-testing v4.8-rc4 v4.8-rc3 v4.8-rc2 v4.8-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Laura-Abbott/staging-android-ion-Drop-heap-type-masks/20160902-071022
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=mips 

All warnings (new ones prefixed by >>):

   drivers/staging/android/ion/ion.c: In function 'ion_query_heaps':
>> drivers/staging/android/ion/ion.c:1181:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
      (struct ion_heap_data __user *)query->heaps;
      ^

vim +1181 drivers/staging/android/ion/ion.c

  1165			       __func__);
  1166			dma_buf_put(dmabuf);
  1167			return -EINVAL;
  1168		}
  1169		buffer = dmabuf->priv;
  1170	
  1171		dma_sync_sg_for_device(NULL, buffer->sg_table->sgl,
  1172				       buffer->sg_table->nents, DMA_BIDIRECTIONAL);
  1173		dma_buf_put(dmabuf);
  1174		return 0;
  1175	}
  1176	
  1177	int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query)
  1178	{
  1179		struct ion_device *dev = client->dev;
  1180		struct ion_heap_data __user *buffer =
> 1181			(struct ion_heap_data __user *)query->heaps;
  1182		int ret = -EINVAL, cnt = 0, max_cnt;
  1183		struct ion_heap *heap;
  1184		struct ion_heap_data hdata;
  1185	
  1186		memset(&hdata, 0, sizeof(hdata));
  1187	
  1188		down_read(&dev->lock);
  1189		if (!buffer) {

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 43266 bytes --]

Re: [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

[Laura Abbott]

On 09/01/2016 04:44 PM, kbuild test robot wrote:
> Hi Laura,
>
> [auto build test WARNING on next-20160825]
> [cannot apply to staging/staging-testing v4.8-rc4 v4.8-rc3 v4.8-rc2 v4.8-rc4]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> [Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
> [Check https://git-scm.com/docs/git-format-patch for more information]
>
> url:    https://github.com/0day-ci/linux/commits/Laura-Abbott/staging-android-ion-Drop-heap-type-masks/20160902-071022
> config: mips-allyesconfig (attached as .config)
> compiler: mips-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
> reproduce:
>         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=mips
>
> All warnings (new ones prefixed by >>):
>
>    drivers/staging/android/ion/ion.c: In function 'ion_query_heaps':
>>> drivers/staging/android/ion/ion.c:1181:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>       (struct ion_heap_data __user *)query->heaps;
>       ^
>

botching-up-ioctls.txt says:

  * Pointers are __u64, cast from/to a uintprt_t on the userspace side and
    from/to a void __user * in the kernel. Try really hard not to delay this
    conversion or worse, fiddle the raw __u64 through your code since that
    diminishes the checking tools like sparse can provide.

This doesn't seem like it will work on 32-bit systems since you'll end up
with the warning above. Is there a better option or did I misunderstand
how that is supposed to work?

Thanks,
Laura

Re: [Linaro-mm-sig] [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

[Arnd Bergmann]

On Friday, September 2, 2016 2:27:21 PM CEST Laura Abbott wrote:
> >
> > All warnings (new ones prefixed by >>):
> >
> >    drivers/staging/android/ion/ion.c: In function 'ion_query_heaps':
> >>> drivers/staging/android/ion/ion.c:1181:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> >       (struct ion_heap_data __user *)query->heaps;
> >       ^
> >
> 
> botching-up-ioctls.txt says:
> 
>   * Pointers are __u64, cast from/to a uintprt_t on the userspace side and
>     from/to a void __user * in the kernel. Try really hard not to delay this
>     conversion or worse, fiddle the raw __u64 through your code since that
>     diminishes the checking tools like sparse can provide.
> 
> This doesn't seem like it will work on 32-bit systems since you'll end up
> with the warning above. Is there a better option or did I misunderstand
> how that is supposed to work?
> 

I don't know a better way that two cast, doing
	
	(struct ion_heap_data __user *)(uintptr_t)query->heaps;

It may help to put this into an inline function though.

	Arnd

Re: [Linaro-mm-sig] [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

[Laura Abbott]

On 09/02/2016 02:37 PM, Arnd Bergmann wrote:
> On Friday, September 2, 2016 2:27:21 PM CEST Laura Abbott wrote:
>>>
>>> All warnings (new ones prefixed by >>):
>>>
>>>    drivers/staging/android/ion/ion.c: In function 'ion_query_heaps':
>>>>> drivers/staging/android/ion/ion.c:1181:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>>>       (struct ion_heap_data __user *)query->heaps;
>>>       ^
>>>
>>
>> botching-up-ioctls.txt says:
>>
>>   * Pointers are __u64, cast from/to a uintprt_t on the userspace side and
>>     from/to a void __user * in the kernel. Try really hard not to delay this
>>     conversion or worse, fiddle the raw __u64 through your code since that
>>     diminishes the checking tools like sparse can provide.
>>
>> This doesn't seem like it will work on 32-bit systems since you'll end up
>> with the warning above. Is there a better option or did I misunderstand
>> how that is supposed to work?
>>
>
> I don't know a better way that two cast, doing
> 	
> 	(struct ion_heap_data __user *)(uintptr_t)query->heaps;
>
> It may help to put this into an inline function though.
>
> 	Arnd
>

There already is one it turns out in kernel.h:

#define u64_to_user_ptr(x) (            \
{                                       \
         typecheck(u64, x);              \
         (void __user *)(uintptr_t)x;    \
}                                       \
)

At least this way I don't have to look at the double casts.

Thanks,
Laura

Re: [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

[Greg Kroah-Hartman]

On Thu, Sep 01, 2016 at 03:40:44PM -0700, Laura Abbott wrote:
> 
> Ion clients currently lack a good method to determine what
> heaps are available and what ids they map to. This leads
> to tight coupling between user and kernel space and headaches.
> Add a query ioctl to let userspace know the availability of
> heaps.
> 
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
>  drivers/staging/android/ion/ion-ioctl.c | 11 +++++++++
>  drivers/staging/android/ion/ion.c       | 44 +++++++++++++++++++++++++++++++++
>  drivers/staging/android/ion/ion_priv.h  |  3 +++
>  drivers/staging/android/uapi/ion.h      | 39 +++++++++++++++++++++++++++++
>  4 files changed, 97 insertions(+)
> 
> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
> index 53b9520..e76d517 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -28,6 +28,7 @@ union ion_ioctl_arg {
>  	struct ion_handle_data handle;
>  	struct ion_custom_data custom;
>  	struct ion_abi_version abi_version;
> +	struct ion_heap_query query;
>  };
>  
>  static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
> @@ -38,6 +39,11 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
>  	case ION_IOC_ABI_VERSION:
>  		ret = arg->abi_version.reserved != 0;
>  		break;
> +	case ION_IOC_HEAP_QUERY:
> +		ret = arg->query.reserved0 != 0;
> +		ret |= arg->query.reserved1 != 0;
> +		ret |= arg->query.reserved2 != 0;
> +		break;
>  	default:
>  		break;
>  	}
> @@ -162,6 +168,11 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		data.abi_version.abi_version = ION_ABI_VERSION;
>  		break;
>  	}
> +	case ION_IOC_HEAP_QUERY:
> +	{
> +		ret = ion_query_heaps(client, &data.query);
> +		break;
> +	}

Minor nit, the { } aren't needed here.  Yeah, I know the other cases
have them, but they aren't all needed there either, no need to keep
copying bad code style :)



>  	default:
>  		return -ENOTTY;
>  	}
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 975b48f..91b765c 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -1174,6 +1174,49 @@ int ion_sync_for_device(struct ion_client *client, int fd)
>  	return 0;
>  }
>  
> +int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query)
> +{
> +	struct ion_device *dev = client->dev;
> +	struct ion_heap_data __user *buffer =
> +		(struct ion_heap_data __user *)query->heaps;

Shouldn't query be marked as __user instead of having this cast?

> +	int ret = -EINVAL, cnt = 0, max_cnt;
> +	struct ion_heap *heap;
> +	struct ion_heap_data hdata;
> +
> +	memset(&hdata, 0, sizeof(hdata));
> +
> +	down_read(&dev->lock);
> +	if (!buffer) {
> +		query->cnt = dev->heap_cnt;

Wait, query is __user?

I think the mixing of user/kernel pointers here isn't quite right, or I
just really can't figure it out...

And kbuild didn't seem to like this patch either :(

But your first 2 patches are great, I'll queue them up later today.

thanks,

greg k-h

Re: [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

[Laura Abbott]

On 09/01/2016 11:14 PM, Greg Kroah-Hartman wrote:
> On Thu, Sep 01, 2016 at 03:40:44PM -0700, Laura Abbott wrote:
>>
>> Ion clients currently lack a good method to determine what
>> heaps are available and what ids they map to. This leads
>> to tight coupling between user and kernel space and headaches.
>> Add a query ioctl to let userspace know the availability of
>> heaps.
>>
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> ---
>>  drivers/staging/android/ion/ion-ioctl.c | 11 +++++++++
>>  drivers/staging/android/ion/ion.c       | 44 +++++++++++++++++++++++++++++++++
>>  drivers/staging/android/ion/ion_priv.h  |  3 +++
>>  drivers/staging/android/uapi/ion.h      | 39 +++++++++++++++++++++++++++++
>>  4 files changed, 97 insertions(+)
>>
>> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
>> index 53b9520..e76d517 100644
>> --- a/drivers/staging/android/ion/ion-ioctl.c
>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>> @@ -28,6 +28,7 @@ union ion_ioctl_arg {
>>  	struct ion_handle_data handle;
>>  	struct ion_custom_data custom;
>>  	struct ion_abi_version abi_version;
>> +	struct ion_heap_query query;
>>  };
>>
>>  static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
>> @@ -38,6 +39,11 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
>>  	case ION_IOC_ABI_VERSION:
>>  		ret = arg->abi_version.reserved != 0;
>>  		break;
>> +	case ION_IOC_HEAP_QUERY:
>> +		ret = arg->query.reserved0 != 0;
>> +		ret |= arg->query.reserved1 != 0;
>> +		ret |= arg->query.reserved2 != 0;
>> +		break;
>>  	default:
>>  		break;
>>  	}
>> @@ -162,6 +168,11 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>  		data.abi_version.abi_version = ION_ABI_VERSION;
>>  		break;
>>  	}
>> +	case ION_IOC_HEAP_QUERY:
>> +	{
>> +		ret = ion_query_heaps(client, &data.query);
>> +		break;
>> +	}
>
> Minor nit, the { } aren't needed here.  Yeah, I know the other cases
> have them, but they aren't all needed there either, no need to keep
> copying bad code style :)
>

Huh, might deserve a checkpatch addition then. Never heard that one before.

>
>
>>  	default:
>>  		return -ENOTTY;
>>  	}
>> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
>> index 975b48f..91b765c 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -1174,6 +1174,49 @@ int ion_sync_for_device(struct ion_client *client, int fd)
>>  	return 0;
>>  }
>>
>> +int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query)
>> +{
>> +	struct ion_device *dev = client->dev;
>> +	struct ion_heap_data __user *buffer =
>> +		(struct ion_heap_data __user *)query->heaps;
>
> Shouldn't query be marked as __user instead of having this cast?
>

No, the query structure itself is copied into the kernel in ion_ioctl.
The sub field query->heaps is a user pointer which is marked as _u64
for compatability ala botching-ioctls.txt hence the cast.

>> +	int ret = -EINVAL, cnt = 0, max_cnt;
>> +	struct ion_heap *heap;
>> +	struct ion_heap_data hdata;
>> +
>> +	memset(&hdata, 0, sizeof(hdata));
>> +
>> +	down_read(&dev->lock);
>> +	if (!buffer) {
>> +		query->cnt = dev->heap_cnt;
>
> Wait, query is __user?
>
> I think the mixing of user/kernel pointers here isn't quite right, or I
> just really can't figure it out...
>
> And kbuild didn't seem to like this patch either :(
>
> But your first 2 patches are great, I'll queue them up later today.
>
> thanks,
>
> greg k-h
>

Re: [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

[Greg Kroah-Hartman]

On Fri, Sep 02, 2016 at 01:41:48PM -0700, Laura Abbott wrote:
> On 09/01/2016 11:14 PM, Greg Kroah-Hartman wrote:
> > On Thu, Sep 01, 2016 at 03:40:44PM -0700, Laura Abbott wrote:
> > > 
> > > Ion clients currently lack a good method to determine what
> > > heaps are available and what ids they map to. This leads
> > > to tight coupling between user and kernel space and headaches.
> > > Add a query ioctl to let userspace know the availability of
> > > heaps.
> > > 
> > > Signed-off-by: Laura Abbott <labbott@redhat.com>
> > > ---
> > >  drivers/staging/android/ion/ion-ioctl.c | 11 +++++++++
> > >  drivers/staging/android/ion/ion.c       | 44 +++++++++++++++++++++++++++++++++
> > >  drivers/staging/android/ion/ion_priv.h  |  3 +++
> > >  drivers/staging/android/uapi/ion.h      | 39 +++++++++++++++++++++++++++++
> > >  4 files changed, 97 insertions(+)
> > > 
> > > diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
> > > index 53b9520..e76d517 100644
> > > --- a/drivers/staging/android/ion/ion-ioctl.c
> > > +++ b/drivers/staging/android/ion/ion-ioctl.c
> > > @@ -28,6 +28,7 @@ union ion_ioctl_arg {
> > >  	struct ion_handle_data handle;
> > >  	struct ion_custom_data custom;
> > >  	struct ion_abi_version abi_version;
> > > +	struct ion_heap_query query;
> > >  };
> > > 
> > >  static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
> > > @@ -38,6 +39,11 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
> > >  	case ION_IOC_ABI_VERSION:
> > >  		ret = arg->abi_version.reserved != 0;
> > >  		break;
> > > +	case ION_IOC_HEAP_QUERY:
> > > +		ret = arg->query.reserved0 != 0;
> > > +		ret |= arg->query.reserved1 != 0;
> > > +		ret |= arg->query.reserved2 != 0;
> > > +		break;
> > >  	default:
> > >  		break;
> > >  	}
> > > @@ -162,6 +168,11 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> > >  		data.abi_version.abi_version = ION_ABI_VERSION;
> > >  		break;
> > >  	}
> > > +	case ION_IOC_HEAP_QUERY:
> > > +	{
> > > +		ret = ion_query_heaps(client, &data.query);
> > > +		break;
> > > +	}
> > 
> > Minor nit, the { } aren't needed here.  Yeah, I know the other cases
> > have them, but they aren't all needed there either, no need to keep
> > copying bad code style :)
> > 
> 
> Huh, might deserve a checkpatch addition then. Never heard that one before.

It's not a checkpatch issue, just a "is this { } even needed" type of an
issue :)

> > >  	default:
> > >  		return -ENOTTY;
> > >  	}
> > > diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> > > index 975b48f..91b765c 100644
> > > --- a/drivers/staging/android/ion/ion.c
> > > +++ b/drivers/staging/android/ion/ion.c
> > > @@ -1174,6 +1174,49 @@ int ion_sync_for_device(struct ion_client *client, int fd)
> > >  	return 0;
> > >  }
> > > 
> > > +int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query)
> > > +{
> > > +	struct ion_device *dev = client->dev;
> > > +	struct ion_heap_data __user *buffer =
> > > +		(struct ion_heap_data __user *)query->heaps;
> > 
> > Shouldn't query be marked as __user instead of having this cast?
> > 
> 
> No, the query structure itself is copied into the kernel in ion_ioctl.
> The sub field query->heaps is a user pointer which is marked as _u64
> for compatability ala botching-ioctls.txt hence the cast.

Ah, ok.  Messy :)

thanks,

greg k-h