[RFC][PATCH 0/3] ION fixups for staging-next

[RFC][PATCH 0/3] ION fixups for staging-next

[John Stultz]

Since the ION patchset landed in staging-next, there have
been a few build issue reports, and this patchset tries
to address them.

I've marked these as RFC, as I'd really like to get acks
from Colin or someone else on the Android team on these
before merging them.

The first patch is the most trivial.

The second, I not sure if it is better to free the buffer and
continue looking for a smaller order, or if we should just free
the buffer and return NULL.

The last patch is the one I'm least confident of. We could simply
add a depends on RT_MUTEXES to the ION option, but since so few
drivers directly use rt_mutexes, and there's no documentation in
the code that I've noticed as to why the ion_heap uses an rt_mutex,
it probably is best to just convert it to a normal mutex. If this
is wrong, we can instead add the dependency and hopefully some
documentation.

I'd appreciate any thoughts or comments!

thanks
-john


Cc: Colin Cross <ccross@android.com>
Cc: Android Kernel Team <kernel-team@android.com>
Cc: Greg KH <gregkh@linuxfoundation.org>

John Stultz (3):
  staging: ion: Add HAVE_MEMBLOCK config dependency
  staging: ion: Fix possible null pointer dereference
  staging: ion: Avoid using rt_mutexes directly.

 drivers/staging/android/ion/Kconfig           |  1 +
 drivers/staging/android/ion/ion_heap.c        | 20 ++++++++++----------
 drivers/staging/android/ion/ion_system_heap.c |  5 +++++
 3 files changed, 16 insertions(+), 10 deletions(-)

-- 
1.8.3.2

[RFC][PATCH 1/3] staging: ion: Add HAVE_MEMBLOCK config dependency

[John Stultz]

The kbuild test robot reported a build issue w/ ION on m68k:

drivers/staging/android/ion/ion.c: In function 'ion_reserve':
drivers/staging/android/ion/ion.c:1526:4: error: implicit declaration of function 'memblock_alloc_base' [-Werror=implicit-function-declaration]
drivers/staging/android/ion/ion.c:1528:11: error: 'MEMBLOCK_ALLOC_ANYWHERE' undeclared (first use in this function)
drivers/staging/android/ion/ion.c:1528:11: note: each undeclared identifier is reported only once for each function it appears in
drivers/staging/android/ion/ion.c:1537:4: error: implicit declaration of function 'memblock_reserve' [-Werror=implicit-function-declaration]
   cc1: some warnings being treated as errors

This is caused by ION using memblock functionality which m68k doesn't support.

This patch adds a HAVE_MEMBLOCK dependency to the ION config.

Cc: Colin Cross <ccross@android.com>
Cc: Android Kernel Team <kernel-team@android.com>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: kbuild test robot <fengguang.wu@intel.com>
Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/staging/android/ion/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
index b95281e..a9a64ea 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -1,5 +1,6 @@
 menuconfig ION
 	bool "Ion Memory Manager"
+	depends on HAVE_MEMBLOCK
 	select GENERIC_ALLOCATOR
 	select DMA_SHARED_BUFFER
 	---help---
-- 
1.8.3.2

[RFC][PATCH 2/3] staging: ion: Fix possible null pointer dereference

[John Stultz]

The kbuild test robot reported:

drivers/staging/android/ion/ion_system_heap.c:122 alloc_largest_available() error: potential null dereference 'info'.  (kmalloc returns null)

Where the pointer returned from kmalloc goes unchecked for failure.

This patch adds a simple check for a null return, and handles the error.

XXX: Not sure if continue or 'return NULL' is the right thing to do.

Cc: Colin Cross <ccross@android.com>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Android Kernel Team <kernel-team@android.com>
Cc: kbuild test robot <fengguang.wu@intel.com>
Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/staging/android/ion/ion_system_heap.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
index 144b2272..cc2e4da 100644
--- a/drivers/staging/android/ion/ion_system_heap.c
+++ b/drivers/staging/android/ion/ion_system_heap.c
@@ -119,6 +119,11 @@ static struct page_info *alloc_largest_available(struct ion_system_heap *heap,
 			continue;
 
 		info = kmalloc(sizeof(struct page_info), GFP_KERNEL);
+		if(!info) {
+			free_buffer_page(heap, buffer, page, orders[i]);
+			continue;
+		}
+
 		info->page = page;
 		info->order = orders[i];
 		return info;
-- 
1.8.3.2

Re: [RFC][PATCH 2/3] staging: ion: Fix possible null pointer dereference

[Greg KH]

On Mon, Dec 16, 2013 at 01:32:43PM -0800, John Stultz wrote:
> The kbuild test robot reported:
> 
> drivers/staging/android/ion/ion_system_heap.c:122 alloc_largest_available() error: potential null dereference 'info'.  (kmalloc returns null)
> 
> Where the pointer returned from kmalloc goes unchecked for failure.
> 
> This patch adds a simple check for a null return, and handles the error.
> 
> XXX: Not sure if continue or 'return NULL' is the right thing to do.
> 
> Cc: Colin Cross <ccross@android.com>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Cc: Android Kernel Team <kernel-team@android.com>
> Cc: kbuild test robot <fengguang.wu@intel.com>
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/staging/android/ion/ion_system_heap.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
> index 144b2272..cc2e4da 100644
> --- a/drivers/staging/android/ion/ion_system_heap.c
> +++ b/drivers/staging/android/ion/ion_system_heap.c
> @@ -119,6 +119,11 @@ static struct page_info *alloc_largest_available(struct ion_system_heap *heap,
>  			continue;
>  
>  		info = kmalloc(sizeof(struct page_info), GFP_KERNEL);
> +		if(!info) {

Please don't add new coding style issues to staging drivers, the goal is
to clean them up, not make more mess :)

Care to resend this when you have figured out the XXX: line above?

thanks,

greg k-h

Re: [RFC][PATCH 2/3] staging: ion: Fix possible null pointer dereference

[Colin Cross]

On Mon, Dec 16, 2013 at 1:32 PM, John Stultz <john.stultz@linaro.org> wrote:
> The kbuild test robot reported:
>
> drivers/staging/android/ion/ion_system_heap.c:122 alloc_largest_available() error: potential null dereference 'info'.  (kmalloc returns null)
>
> Where the pointer returned from kmalloc goes unchecked for failure.
>
> This patch adds a simple check for a null return, and handles the error.
>
> XXX: Not sure if continue or 'return NULL' is the right thing to do.

Returning NULL is fine, there is no reason the kmalloc is going to
succeed if it retries, and it will just have to allocate more of them
if it starts fulfilling the allocation with smaller order chunks.

Allocating the struct before entering the loop might make error handling easier.

Re: [RFC][PATCH 2/3] staging: ion: Fix possible null pointer dereference

[John Stultz]

On 12/16/2013 04:26 PM, Colin Cross wrote:
> On Mon, Dec 16, 2013 at 1:32 PM, John Stultz <john.stultz@linaro.org> wrote:
>> The kbuild test robot reported:
>>
>> drivers/staging/android/ion/ion_system_heap.c:122 alloc_largest_available() error: potential null dereference 'info'.  (kmalloc returns null)
>>
>> Where the pointer returned from kmalloc goes unchecked for failure.
>>
>> This patch adds a simple check for a null return, and handles the error.
>>
>> XXX: Not sure if continue or 'return NULL' is the right thing to do.
> Returning NULL is fine, there is no reason the kmalloc is going to
> succeed if it retries, and it will just have to allocate more of them
> if it starts fulfilling the allocation with smaller order chunks.
>
> Allocating the struct before entering the loop might make error handling easier.

Ok, I had actually done that in my first pass, but then got worried the
allocation/free might have extra overhead in the case where the pool is
exhausted, so I stuck with the existing pattern. But if that's not a
common state, then I agree, it does make the handling simpler.

I'll rework this and re-submit. Thanks for the feedback!

thanks
-john

[RFC][PATCH 3/3] staging: ion: Avoid using rt_mutexes directly.

[John Stultz]

RT_MUTEXES can be configured out of the kernel, causing compile
problems with ION.

Since there is no documentation as to why directly using rt_mutexes
is necessary (and very few drivers directly use rt_mutexes), simply
convert the ion_heap lock to a normal mutex.

Cc: Colin Cross <ccross@android.com>
Cc: Android Kernel Team <kernel-team@android.com>
Cc: Greg KH <gregkh@linuxfoundation.org>
Reported-by: Jim Davis <jim.epost@gmail.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/staging/android/ion/ion_heap.c | 20 ++++++++++----------
 drivers/staging/android/ion/ion_priv.h |  2 +-
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/android/ion/ion_heap.c b/drivers/staging/android/ion/ion_heap.c
index 9cf5622..88685da 100644
--- a/drivers/staging/android/ion/ion_heap.c
+++ b/drivers/staging/android/ion/ion_heap.c
@@ -160,10 +160,10 @@ int ion_heap_pages_zero(struct page *page, size_t size, pgprot_t pgprot)
 
 void ion_heap_freelist_add(struct ion_heap *heap, struct ion_buffer *buffer)
 {
-	rt_mutex_lock(&heap->lock);
+	mutex_lock(&heap->lock);
 	list_add(&buffer->list, &heap->free_list);
 	heap->free_list_size += buffer->size;
-	rt_mutex_unlock(&heap->lock);
+	mutex_unlock(&heap->lock);
 	wake_up(&heap->waitqueue);
 }
 
@@ -171,9 +171,9 @@ size_t ion_heap_freelist_size(struct ion_heap *heap)
 {
 	size_t size;
 
-	rt_mutex_lock(&heap->lock);
+	mutex_lock(&heap->lock);
 	size = heap->free_list_size;
-	rt_mutex_unlock(&heap->lock);
+	mutex_unlock(&heap->lock);
 
 	return size;
 }
@@ -186,7 +186,7 @@ size_t ion_heap_freelist_drain(struct ion_heap *heap, size_t size)
 	if (ion_heap_freelist_size(heap) == 0)
 		return 0;
 
-	rt_mutex_lock(&heap->lock);
+	mutex_lock(&heap->lock);
 	if (size == 0)
 		size = heap->free_list_size;
 
@@ -198,7 +198,7 @@ size_t ion_heap_freelist_drain(struct ion_heap *heap, size_t size)
 		total_drained += buffer->size;
 		ion_buffer_destroy(buffer);
 	}
-	rt_mutex_unlock(&heap->lock);
+	mutex_unlock(&heap->lock);
 
 	return total_drained;
 }
@@ -213,16 +213,16 @@ static int ion_heap_deferred_free(void *data)
 		wait_event_freezable(heap->waitqueue,
 				     ion_heap_freelist_size(heap) > 0);
 
-		rt_mutex_lock(&heap->lock);
+		mutex_lock(&heap->lock);
 		if (list_empty(&heap->free_list)) {
-			rt_mutex_unlock(&heap->lock);
+			mutex_unlock(&heap->lock);
 			continue;
 		}
 		buffer = list_first_entry(&heap->free_list, struct ion_buffer,
 					  list);
 		list_del(&buffer->list);
 		heap->free_list_size -= buffer->size;
-		rt_mutex_unlock(&heap->lock);
+		mutex_unlock(&heap->lock);
 		ion_buffer_destroy(buffer);
 	}
 
@@ -235,7 +235,7 @@ int ion_heap_init_deferred_free(struct ion_heap *heap)
 
 	INIT_LIST_HEAD(&heap->free_list);
 	heap->free_list_size = 0;
-	rt_mutex_init(&heap->lock);
+	mutex_init(&heap->lock);
 	init_waitqueue_head(&heap->waitqueue);
 	heap->task = kthread_run(ion_heap_deferred_free, heap,
 				 "%s", heap->name);
diff --git a/drivers/staging/android/ion/ion_priv.h b/drivers/staging/android/ion/ion_priv.h
index fc8a4c3..df81ce1 100644
--- a/drivers/staging/android/ion/ion_priv.h
+++ b/drivers/staging/android/ion/ion_priv.h
@@ -159,7 +159,7 @@ struct ion_heap {
 	struct shrinker shrinker;
 	struct list_head free_list;
 	size_t free_list_size;
-	struct rt_mutex lock;
+	struct mutex lock;
 	wait_queue_head_t waitqueue;
 	struct task_struct *task;
 	int (*debug_show)(struct ion_heap *heap, struct seq_file *, void *);
-- 
1.8.3.2

Re: [RFC][PATCH 3/3] staging: ion: Avoid using rt_mutexes directly.

[Colin Cross]

On Mon, Dec 16, 2013 at 1:32 PM, John Stultz <john.stultz@linaro.org> wrote:
> RT_MUTEXES can be configured out of the kernel, causing compile
> problems with ION.
>
> Since there is no documentation as to why directly using rt_mutexes
> is necessary (and very few drivers directly use rt_mutexes), simply
> convert the ion_heap lock to a normal mutex.

rt_mutexes were added with the deferred freeing feature.  Heaps need
to return zeroed memory to userspace, but zeroing the memory on every
allocation was causing performance issues.  We added a SCHED_IDLE
thread to zero memory in the background after freeing, but locking the
heap from the SCHED_IDLE thread might block a high priority allocation
thread for a long time.

The lock is only used to protect the heap's free_list and
free_list_size members, and is not held for any long or sleeping
operations.  Converting to a spinlock should prevent priority
inversion without using the rt_mutex.  I'd also rename it to free_lock
to so it doesn't get used as a general heap lock.

Re: [RFC][PATCH 3/3] staging: ion: Avoid using rt_mutexes directly.

[John Stultz]

On 12/16/2013 04:17 PM, Colin Cross wrote:
> The lock is only used to protect the heap's free_list and
> free_list_size members, and is not held for any long or sleeping
> operations.  Converting to a spinlock should prevent priority
> inversion without using the rt_mutex.  I'd also rename it to free_lock
> to so it doesn't get used as a general heap lock.

Hrm.. So at least a trivial conversion to use spinlocks doesn't quite
work out, as we call ion_buffer_destroy() in ion_heap_freelist_drain()
while holding the lock, and that calls all sorts of not safe stuff.

I'll spend some more time looking at it later tonight, but let me know
if you have an approach for this case in mind.

thanks
-john

Re: [RFC][PATCH 3/3] staging: ion: Avoid using rt_mutexes directly.

[Colin Cross]

On Mon, Dec 16, 2013 at 5:22 PM, John Stultz <john.stultz@linaro.org> wrote:
> On 12/16/2013 04:17 PM, Colin Cross wrote:
>> The lock is only used to protect the heap's free_list and
>> free_list_size members, and is not held for any long or sleeping
>> operations.  Converting to a spinlock should prevent priority
>> inversion without using the rt_mutex.  I'd also rename it to free_lock
>> to so it doesn't get used as a general heap lock.
>
> Hrm.. So at least a trivial conversion to use spinlocks doesn't quite
> work out, as we call ion_buffer_destroy() in ion_heap_freelist_drain()
> while holding the lock, and that calls all sorts of not safe stuff.
>
> I'll spend some more time looking at it later tonight, but let me know
> if you have an approach for this case in mind.

Drop and re-grab the lock around ion_buffer_destroy, it's not
necessary during the destroy, and the list iteration is already using
list_for_each_entry_safe, so it doesn't matter if another caller
modifies the list during the destroy.