Hi all,
When I tried to review the code of ion, I find that there are many place
can be cleanup, so I post this patchset.
I mark it as v2, because I have post them in different patchset before[1][2]
[3][4]. And now, I combine them in one patchset to make it easy to review,
Meanwhile, I add some acks and rebase them to v4.15-rc1.
Thanks.
[1] https://lkml.org/lkml/2018/1/31/711
[2] https://lkml.org/lkml/2018/2/1/240
[3] https://lkml.org/lkml/2018/2/4/320
[4] https://lkml.org/lkml/2018/2/6/949
Yisheng Xie (9):
staging: android: ion: Remove unused declaration
ion_buffer_fault_user_mappings
staging: android: ion: Remove unused include files for
ion_page_pool.c
staging: android: ion: Nuke ion_page_pool_init
staging: android: ion: Avoid NULL point in error path
staging: android: ion: Remove lable debugfs_done
staging: android: ion: Remove dead code in ion_page_pool_free
staging: android: ion: Return void instead of int
staging: android: ion: Cleanup ion_page_pool_alloc_pages
staging: android: ion: Combine cache and uncache pools
drivers/staging/android/ion/ion.c | 20 ++-----
drivers/staging/android/ion/ion.h | 22 +-------
drivers/staging/android/ion/ion_page_pool.c | 33 ++----------
drivers/staging/android/ion/ion_system_heap.c | 76 +++++----------------------
4 files changed, 24 insertions(+), 127 deletions(-)
--
1.7.12.4
ion_page_pool_alloc_pages calls alloc_pages to allocate pages for page
pools. If alloc_pages return NULL, it will return NULL, or it will
return the pages allocate from alloc_pages. So we can just return
alloc_pages without any judgement.
Acked-by: Sumit Semwal <[email protected]>
Signed-off-by: Yisheng Xie <[email protected]>
---
drivers/staging/android/ion/ion_page_pool.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c
index e3a6e32..6d2caf0 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -11,13 +11,9 @@
#include "ion.h"
-static void *ion_page_pool_alloc_pages(struct ion_page_pool *pool)
+static inline struct page *ion_page_pool_alloc_pages(struct ion_page_pool *pool)
{
- struct page *page = alloc_pages(pool->gfp_mask, pool->order);
-
- if (!page)
- return NULL;
- return page;
+ return alloc_pages(pool->gfp_mask, pool->order);
}
static void ion_page_pool_free_pages(struct ion_page_pool *pool,
--
1.7.12.4
Now, nobody care about the return value of ion_page_pool_add, therefore
we can just make it return void.
Acked-by: Laura Abbott <[email protected]>
Signed-off-by: Yisheng Xie <[email protected]>
---
drivers/staging/android/ion/ion_page_pool.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c
index 150626f..e3a6e32 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -26,7 +26,7 @@ static void ion_page_pool_free_pages(struct ion_page_pool *pool,
__free_pages(page, pool->order);
}
-static int ion_page_pool_add(struct ion_page_pool *pool, struct page *page)
+static void ion_page_pool_add(struct ion_page_pool *pool, struct page *page)
{
mutex_lock(&pool->mutex);
if (PageHighMem(page)) {
@@ -37,7 +37,6 @@ static int ion_page_pool_add(struct ion_page_pool *pool, struct page *page)
pool->low_count++;
}
mutex_unlock(&pool->mutex);
- return 0;
}
static struct page *ion_page_pool_remove(struct ion_page_pool *pool, bool high)
--
1.7.12.4
ion_buffer_fault_user_mappings's definition has been removed and not be
used anymore, just remove its useless declaration.
Acked-by: Laura Abbott <[email protected]>
Signed-off-by: Yisheng Xie <[email protected]>
---
drivers/staging/android/ion/ion.h | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
index a238f23..1bc443f 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -193,15 +193,6 @@ struct ion_heap {
bool ion_buffer_cached(struct ion_buffer *buffer);
/**
- * ion_buffer_fault_user_mappings - fault in user mappings of this buffer
- * @buffer: buffer
- *
- * indicates whether userspace mappings of this buffer will be faulted
- * in, this can affect how buffers are allocated from the heap.
- */
-bool ion_buffer_fault_user_mappings(struct ion_buffer *buffer);
-
-/**
* ion_device_add_heap - adds a heap to the ion device
* @heap: the heap to add
*/
--
1.7.12.4
On Mon, Feb 12, 2018 at 06:43:05PM +0800, Yisheng Xie wrote:
> Hi all,
>
> When I tried to review the code of ion, I find that there are many place
> can be cleanup, so I post this patchset.
>
> I mark it as v2, because I have post them in different patchset before[1][2]
> [3][4]. And now, I combine them in one patchset to make it easy to review,
> Meanwhile, I add some acks and rebase them to v4.15-rc1.
>
> Thanks.
>
> [1] https://lkml.org/lkml/2018/1/31/711
> [2] https://lkml.org/lkml/2018/2/1/240
> [3] https://lkml.org/lkml/2018/2/4/320
> [4] https://lkml.org/lkml/2018/2/6/949
>
Could you go back and reply to these threads that you have redone the
patches? Otherwise, what's likely to happen is that Greg will apply
as many as he can get to this series and it won't apply.
regards,
dan carpenter
Hi Dan,
On 2018/2/12 19:17, Dan Carpenter wrote:
> On Mon, Feb 12, 2018 at 06:43:05PM +0800, Yisheng Xie wrote:
>> Hi all,
>>
>> When I tried to review the code of ion, I find that there are many place
>> can be cleanup, so I post this patchset.
>>
>> I mark it as v2, because I have post them in different patchset before[1][2]
>> [3][4]. And now, I combine them in one patchset to make it easy to review,
>> Meanwhile, I add some acks and rebase them to v4.15-rc1.
>>
>> Thanks.
>>
>> [1] https://lkml.org/lkml/2018/1/31/711
>> [2] https://lkml.org/lkml/2018/2/1/240
>> [3] https://lkml.org/lkml/2018/2/4/320
>> [4] https://lkml.org/lkml/2018/2/6/949
>>
>
> Could you go back and reply to these threads that you have redone the
> patches? Otherwise, what's likely to happen is that Greg will apply
> as many as he can get to this series and it won't apply.
Sure! thanks
Yisheng
>
> regards,
> dan carpenter
>
>
>
When failed to create debug_root, we will go on initail other part of
ion, so we can just info this message to user and do not need a lable
to jump.
Acked-by: Laura Abbott <[email protected]>
Signed-off-by: Yisheng Xie <[email protected]>
---
drivers/staging/android/ion/ion.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 4b69372..461b193 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -595,12 +595,9 @@ static int ion_device_create(void)
}
idev->debug_root = debugfs_create_dir("ion", NULL);
- if (!idev->debug_root) {
+ if (!idev->debug_root)
pr_err("ion: failed to create debugfs root directory.\n");
- goto debugfs_done;
- }
-debugfs_done:
idev->buffers = RB_ROOT;
mutex_init(&idev->buffer_lock);
init_rwsem(&idev->lock);
--
1.7.12.4
After rewrite of ion_page_pool, some of its include file is no need
anymore, just remove it.
Signed-off-by: Yisheng Xie <[email protected]>
---
drivers/staging/android/ion/ion_page_pool.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c
index b3017f1..f0847b3 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -5,10 +5,6 @@
* Copyright (C) 2011 Google, Inc.
*/
-#include <linux/debugfs.h>
-#include <linux/dma-mapping.h>
-#include <linux/err.h>
-#include <linux/fs.h>
#include <linux/list.h>
#include <linux/init.h>
#include <linux/slab.h>
--
1.7.12.4
ion_page_pool.c now is used to apply pool APIs for system heap, which do
not need do any initial at device_initcall. Therefore ion_page_pool_init
can be nuked.
Signed-off-by: Yisheng Xie <[email protected]>
---
drivers/staging/android/ion/ion_page_pool.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c
index f0847b3..4452e28 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -6,7 +6,6 @@
*/
#include <linux/list.h>
-#include <linux/init.h>
#include <linux/slab.h>
#include <linux/swap.h>
@@ -158,9 +157,3 @@ void ion_page_pool_destroy(struct ion_page_pool *pool)
{
kfree(pool);
}
-
-static int __init ion_page_pool_init(void)
-{
- return 0;
-}
-device_initcall(ion_page_pool_init);
--
1.7.12.4
If we failed to create debugfs for ion at ion_device_create, the
debug_root of ion_device will be NULL, and then when try to create debug
file for shrinker of heap it will be create on the top of debugfs. If we
also failed to create this the debug file, it call dentry_path to found
the path of debug_root, then a NULL point will occur.
Fix this by avoiding call dentry_path, but show the debug name only when
failed to create debug file for shrinker.
Acked-by: Laura Abbott <[email protected]>
Signed-off-by: Yisheng Xie <[email protected]>
---
drivers/staging/android/ion/ion.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 57e0d80..4b69372 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -564,13 +564,9 @@ void ion_device_add_heap(struct ion_heap *heap)
debug_file = debugfs_create_file(debug_name,
0644, dev->debug_root, heap,
&debug_shrink_fops);
- if (!debug_file) {
- char buf[256], *path;
-
- path = dentry_path(dev->debug_root, buf, 256);
- pr_err("Failed to create heap shrinker debugfs at %s/%s\n",
- path, debug_name);
- }
+ if (!debug_file)
+ pr_err("Failed to create ion heap shrinker debugfs at %s\n",
+ debug_name);
}
dev->heap_cnt++;
--
1.7.12.4
ion_page_pool_add will always return 0, however ion_page_pool_free will
call ion_page_pool_free_pages when ion_page_pool_add's return value is
not 0, so it is a dead code which can be removed.
Acked-by: Laura Abbott <[email protected]>
Signed-off-by: Yisheng Xie <[email protected]>
---
drivers/staging/android/ion/ion_page_pool.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c
index 4452e28..150626f 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -79,13 +79,9 @@ struct page *ion_page_pool_alloc(struct ion_page_pool *pool)
void ion_page_pool_free(struct ion_page_pool *pool, struct page *page)
{
- int ret;
-
BUG_ON(pool->order != compound_order(page));
- ret = ion_page_pool_add(pool, page);
- if (ret)
- ion_page_pool_free_pages(pool, page);
+ ion_page_pool_add(pool, page);
}
static int ion_page_pool_total(struct ion_page_pool *pool, bool high)
--
1.7.12.4
Now we call dma_map in the dma_buf API callbacks and handle explicit
caching by the dma_buf sync API, which make cache and uncache pools
in the same handling flow, which can be combined.
Acked-by: Sumit Semwal <[email protected]>
Signed-off-by: Yisheng Xie <[email protected]>
---
drivers/staging/android/ion/ion.c | 5 --
drivers/staging/android/ion/ion.h | 13 +----
drivers/staging/android/ion/ion_page_pool.c | 5 +-
drivers/staging/android/ion/ion_system_heap.c | 76 +++++----------------------
4 files changed, 16 insertions(+), 83 deletions(-)
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 461b193..c094be2 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -33,11 +33,6 @@
static struct ion_device *internal_dev;
static int heap_id;
-bool ion_buffer_cached(struct ion_buffer *buffer)
-{
- return !!(buffer->flags & ION_FLAG_CACHED);
-}
-
/* this function should only be called while dev->lock is held */
static void ion_buffer_add(struct ion_device *dev,
struct ion_buffer *buffer)
diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
index 1bc443f..ea08978 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -185,14 +185,6 @@ struct ion_heap {
};
/**
- * ion_buffer_cached - this ion buffer is cached
- * @buffer: buffer
- *
- * indicates whether this ion buffer is cached
- */
-bool ion_buffer_cached(struct ion_buffer *buffer);
-
-/**
* ion_device_add_heap - adds a heap to the ion device
* @heap: the heap to add
*/
@@ -302,7 +294,6 @@ size_t ion_heap_freelist_shrink(struct ion_heap *heap,
* @gfp_mask: gfp_mask to use from alloc
* @order: order of pages in the pool
* @list: plist node for list of pools
- * @cached: it's cached pool or not
*
* Allows you to keep a pool of pre allocated pages to use from your heap.
* Keeping a pool of pages that is ready for dma, ie any cached mapping have
@@ -312,7 +303,6 @@ size_t ion_heap_freelist_shrink(struct ion_heap *heap,
struct ion_page_pool {
int high_count;
int low_count;
- bool cached;
struct list_head high_items;
struct list_head low_items;
struct mutex mutex;
@@ -321,8 +311,7 @@ struct ion_page_pool {
struct plist_node list;
};
-struct ion_page_pool *ion_page_pool_create(gfp_t gfp_mask, unsigned int order,
- bool cached);
+struct ion_page_pool *ion_page_pool_create(gfp_t gfp_mask, unsigned int order);
void ion_page_pool_destroy(struct ion_page_pool *pool);
struct page *ion_page_pool_alloc(struct ion_page_pool *pool);
void ion_page_pool_free(struct ion_page_pool *pool, struct page *page);
diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c
index 6d2caf0..db8f614 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -123,8 +123,7 @@ int ion_page_pool_shrink(struct ion_page_pool *pool, gfp_t gfp_mask,
return freed;
}
-struct ion_page_pool *ion_page_pool_create(gfp_t gfp_mask, unsigned int order,
- bool cached)
+struct ion_page_pool *ion_page_pool_create(gfp_t gfp_mask, unsigned int order)
{
struct ion_page_pool *pool = kmalloc(sizeof(*pool), GFP_KERNEL);
@@ -138,8 +137,6 @@ struct ion_page_pool *ion_page_pool_create(gfp_t gfp_mask, unsigned int order,
pool->order = order;
mutex_init(&pool->mutex);
plist_node_init(&pool->list, order);
- if (cached)
- pool->cached = true;
return pool;
}
diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
index bc19cdd..701eb9f 100644
--- a/drivers/staging/android/ion/ion_system_heap.c
+++ b/drivers/staging/android/ion/ion_system_heap.c
@@ -41,31 +41,16 @@ static inline unsigned int order_to_size(int order)
struct ion_system_heap {
struct ion_heap heap;
- struct ion_page_pool *uncached_pools[NUM_ORDERS];
- struct ion_page_pool *cached_pools[NUM_ORDERS];
+ struct ion_page_pool *pools[NUM_ORDERS];
};
-/**
- * The page from page-pool are all zeroed before. We need do cache
- * clean for cached buffer. The uncached buffer are always non-cached
- * since it's allocated. So no need for non-cached pages.
- */
static struct page *alloc_buffer_page(struct ion_system_heap *heap,
struct ion_buffer *buffer,
unsigned long order)
{
- bool cached = ion_buffer_cached(buffer);
- struct ion_page_pool *pool;
- struct page *page;
+ struct ion_page_pool *pool = heap->pools[order_to_index(order)];
- if (!cached)
- pool = heap->uncached_pools[order_to_index(order)];
- else
- pool = heap->cached_pools[order_to_index(order)];
-
- page = ion_page_pool_alloc(pool);
-
- return page;
+ return ion_page_pool_alloc(pool);
}
static void free_buffer_page(struct ion_system_heap *heap,
@@ -73,7 +58,6 @@ static void free_buffer_page(struct ion_system_heap *heap,
{
struct ion_page_pool *pool;
unsigned int order = compound_order(page);
- bool cached = ion_buffer_cached(buffer);
/* go to system */
if (buffer->private_flags & ION_PRIV_FLAG_SHRINKER_FREE) {
@@ -81,10 +65,7 @@ static void free_buffer_page(struct ion_system_heap *heap,
return;
}
- if (!cached)
- pool = heap->uncached_pools[order_to_index(order)];
- else
- pool = heap->cached_pools[order_to_index(order)];
+ pool = heap->pools[order_to_index(order)];
ion_page_pool_free(pool, page);
}
@@ -190,8 +171,7 @@ static void ion_system_heap_free(struct ion_buffer *buffer)
static int ion_system_heap_shrink(struct ion_heap *heap, gfp_t gfp_mask,
int nr_to_scan)
{
- struct ion_page_pool *uncached_pool;
- struct ion_page_pool *cached_pool;
+ struct ion_page_pool *pool;
struct ion_system_heap *sys_heap;
int nr_total = 0;
int i, nr_freed;
@@ -203,26 +183,15 @@ static int ion_system_heap_shrink(struct ion_heap *heap, gfp_t gfp_mask,
only_scan = 1;
for (i = 0; i < NUM_ORDERS; i++) {
- uncached_pool = sys_heap->uncached_pools[i];
- cached_pool = sys_heap->cached_pools[i];
+ pool = sys_heap->pools[i];
if (only_scan) {
- nr_total += ion_page_pool_shrink(uncached_pool,
+ nr_total += ion_page_pool_shrink(pool,
gfp_mask,
nr_to_scan);
- nr_total += ion_page_pool_shrink(cached_pool,
- gfp_mask,
- nr_to_scan);
} else {
- nr_freed = ion_page_pool_shrink(uncached_pool,
- gfp_mask,
- nr_to_scan);
- nr_to_scan -= nr_freed;
- nr_total += nr_freed;
- if (nr_to_scan <= 0)
- break;
- nr_freed = ion_page_pool_shrink(cached_pool,
+ nr_freed = ion_page_pool_shrink(pool,
gfp_mask,
nr_to_scan);
nr_to_scan -= nr_freed;
@@ -253,26 +222,16 @@ static int ion_system_heap_debug_show(struct ion_heap *heap, struct seq_file *s,
struct ion_page_pool *pool;
for (i = 0; i < NUM_ORDERS; i++) {
- pool = sys_heap->uncached_pools[i];
+ pool = sys_heap->pools[i];
- seq_printf(s, "%d order %u highmem pages uncached %lu total\n",
+ seq_printf(s, "%d order %u highmem pages %lu total\n",
pool->high_count, pool->order,
(PAGE_SIZE << pool->order) * pool->high_count);
- seq_printf(s, "%d order %u lowmem pages uncached %lu total\n",
+ seq_printf(s, "%d order %u lowmem pages %lu total\n",
pool->low_count, pool->order,
(PAGE_SIZE << pool->order) * pool->low_count);
}
- for (i = 0; i < NUM_ORDERS; i++) {
- pool = sys_heap->cached_pools[i];
-
- seq_printf(s, "%d order %u highmem pages cached %lu total\n",
- pool->high_count, pool->order,
- (PAGE_SIZE << pool->order) * pool->high_count);
- seq_printf(s, "%d order %u lowmem pages cached %lu total\n",
- pool->low_count, pool->order,
- (PAGE_SIZE << pool->order) * pool->low_count);
- }
return 0;
}
@@ -285,8 +244,7 @@ static void ion_system_heap_destroy_pools(struct ion_page_pool **pools)
ion_page_pool_destroy(pools[i]);
}
-static int ion_system_heap_create_pools(struct ion_page_pool **pools,
- bool cached)
+static int ion_system_heap_create_pools(struct ion_page_pool **pools)
{
int i;
gfp_t gfp_flags = low_order_gfp_flags;
@@ -297,7 +255,7 @@ static int ion_system_heap_create_pools(struct ion_page_pool **pools,
if (orders[i] > 4)
gfp_flags = high_order_gfp_flags;
- pool = ion_page_pool_create(gfp_flags, orders[i], cached);
+ pool = ion_page_pool_create(gfp_flags, orders[i]);
if (!pool)
goto err_create_pool;
pools[i] = pool;
@@ -320,18 +278,12 @@ static struct ion_heap *__ion_system_heap_create(void)
heap->heap.type = ION_HEAP_TYPE_SYSTEM;
heap->heap.flags = ION_HEAP_FLAG_DEFER_FREE;
- if (ion_system_heap_create_pools(heap->uncached_pools, false))
+ if (ion_system_heap_create_pools(heap->pools))
goto free_heap;
- if (ion_system_heap_create_pools(heap->cached_pools, true))
- goto destroy_uncached_pools;
-
heap->heap.debug_show = ion_system_heap_debug_show;
return &heap->heap;
-destroy_uncached_pools:
- ion_system_heap_destroy_pools(heap->uncached_pools);
-
free_heap:
kfree(heap);
return ERR_PTR(-ENOMEM);
--
1.7.12.4
On Mon, Feb 12, 2018 at 06:43:09PM +0800, Yisheng Xie wrote:
> If we failed to create debugfs for ion at ion_device_create, the
> debug_root of ion_device will be NULL, and then when try to create debug
> file for shrinker of heap it will be create on the top of debugfs. If we
> also failed to create this the debug file, it call dentry_path to found
> the path of debug_root, then a NULL point will occur.
>
> Fix this by avoiding call dentry_path, but show the debug name only when
> failed to create debug file for shrinker.
>
> Acked-by: Laura Abbott <[email protected]>
> Signed-off-by: Yisheng Xie <[email protected]>
> ---
> drivers/staging/android/ion/ion.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 57e0d80..4b69372 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -564,13 +564,9 @@ void ion_device_add_heap(struct ion_heap *heap)
> debug_file = debugfs_create_file(debug_name,
> 0644, dev->debug_root, heap,
> &debug_shrink_fops);
> - if (!debug_file) {
> - char buf[256], *path;
> -
> - path = dentry_path(dev->debug_root, buf, 256);
> - pr_err("Failed to create heap shrinker debugfs at %s/%s\n",
> - path, debug_name);
> - }
> + if (!debug_file)
> + pr_err("Failed to create ion heap shrinker debugfs at %s\n",
> + debug_name);
Really we can just remove this, there's no need to check the return
value of this debugfs call at all, it doesn't matter.
thanks,
greg k-h
On Mon, Feb 12, 2018 at 06:43:10PM +0800, Yisheng Xie wrote:
> When failed to create debug_root, we will go on initail other part of
> ion, so we can just info this message to user and do not need a lable
> to jump.
>
> Acked-by: Laura Abbott <[email protected]>
> Signed-off-by: Yisheng Xie <[email protected]>
> ---
> drivers/staging/android/ion/ion.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 4b69372..461b193 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -595,12 +595,9 @@ static int ion_device_create(void)
> }
>
> idev->debug_root = debugfs_create_dir("ion", NULL);
> - if (!idev->debug_root) {
> + if (!idev->debug_root)
> pr_err("ion: failed to create debugfs root directory.\n");
> - goto debugfs_done;
> - }
Again, does not matter to check the return value of any debugfs call.
thanks,
greg k-h
Hi Greg,
Sorry for late responds for I was on vacation.
On 2018/2/17 0:27, Greg KH wrote:
> On Mon, Feb 12, 2018 at 06:43:09PM +0800, Yisheng Xie wrote:
>> If we failed to create debugfs for ion at ion_device_create, the
>> debug_root of ion_device will be NULL, and then when try to create debug
>> file for shrinker of heap it will be create on the top of debugfs. If we
>> also failed to create this the debug file, it call dentry_path to found
>> the path of debug_root, then a NULL point will occur.
>>
>> Fix this by avoiding call dentry_path, but show the debug name only when
>> failed to create debug file for shrinker.
>>
>> Acked-by: Laura Abbott <[email protected]>
>> Signed-off-by: Yisheng Xie <[email protected]>
>> ---
>> drivers/staging/android/ion/ion.c | 10 +++-------
>> 1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
>> index 57e0d80..4b69372 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -564,13 +564,9 @@ void ion_device_add_heap(struct ion_heap *heap)
>> debug_file = debugfs_create_file(debug_name,
>> 0644, dev->debug_root, heap,
>> &debug_shrink_fops);
>> - if (!debug_file) {
>> - char buf[256], *path;
>> -
>> - path = dentry_path(dev->debug_root, buf, 256);
>> - pr_err("Failed to create heap shrinker debugfs at %s/%s\n",
>> - path, debug_name);
>> - }
>> + if (!debug_file)
>> + pr_err("Failed to create ion heap shrinker debugfs at %s\n",
>> + debug_name);
>
> Really we can just remove this, there's no need to check the return
> value of this debugfs call at all, it doesn't matter.
Right, and I found that you have already patched this patch into next, should I send
another version for the two patches your commented, or just send fix patches to fold?
Thanks
Yisheng
>
> thanks,
>
> greg k-h
>
> .
>
On Thu, Feb 22, 2018 at 04:59:27PM +0800, Yisheng Xie wrote:
> Hi Greg,
>
> Sorry for late responds for I was on vacation.
>
> On 2018/2/17 0:27, Greg KH wrote:
> > On Mon, Feb 12, 2018 at 06:43:09PM +0800, Yisheng Xie wrote:
> >> If we failed to create debugfs for ion at ion_device_create, the
> >> debug_root of ion_device will be NULL, and then when try to create debug
> >> file for shrinker of heap it will be create on the top of debugfs. If we
> >> also failed to create this the debug file, it call dentry_path to found
> >> the path of debug_root, then a NULL point will occur.
> >>
> >> Fix this by avoiding call dentry_path, but show the debug name only when
> >> failed to create debug file for shrinker.
> >>
> >> Acked-by: Laura Abbott <[email protected]>
> >> Signed-off-by: Yisheng Xie <[email protected]>
> >> ---
> >> drivers/staging/android/ion/ion.c | 10 +++-------
> >> 1 file changed, 3 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> >> index 57e0d80..4b69372 100644
> >> --- a/drivers/staging/android/ion/ion.c
> >> +++ b/drivers/staging/android/ion/ion.c
> >> @@ -564,13 +564,9 @@ void ion_device_add_heap(struct ion_heap *heap)
> >> debug_file = debugfs_create_file(debug_name,
> >> 0644, dev->debug_root, heap,
> >> &debug_shrink_fops);
> >> - if (!debug_file) {
> >> - char buf[256], *path;
> >> -
> >> - path = dentry_path(dev->debug_root, buf, 256);
> >> - pr_err("Failed to create heap shrinker debugfs at %s/%s\n",
> >> - path, debug_name);
> >> - }
> >> + if (!debug_file)
> >> + pr_err("Failed to create ion heap shrinker debugfs at %s\n",
> >> + debug_name);
> >
> > Really we can just remove this, there's no need to check the return
> > value of this debugfs call at all, it doesn't matter.
>
> Right, and I found that you have already patched this patch into next, should I send
> another version for the two patches your commented, or just send fix patches to fold?
Just send add-on patches, I can't "fold" anything as the commit is
already in the tree.
thanks,
greg k-h