This series convert BUGs/BUG_ONs to WARNs/WARN_ONs
Tomer Samara (4): staging: android: Replace BUG_ON with WARN_ON staging: android: Add error handling to ion_page_pool_shrink staging: android: Convert BUG to WARN staging: android: Add error handling to order_to_index callers
drivers/staging/android/ion/ion_page_pool.c | 14 ++++++++++---- drivers/staging/android/ion/ion_system_heap.c | 16 +++++++++++++--- 2 files changed, 23 insertions(+), 7 deletions(-)
BUG_ON() is replaced with WARN_ON at ion_page_pool.c Fixes the following issue: Avoid crashing the kernel - try using WARN_ON & recovery code ratherthan BUG() or BUG_ON().
Signed-off-by: Tomer Samara tomersamara98@gmail.com --- drivers/staging/android/ion/ion_page_pool.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c index 0198b886d906..c1b9eda35c96 100644 --- a/drivers/staging/android/ion/ion_page_pool.c +++ b/drivers/staging/android/ion/ion_page_pool.c @@ -46,11 +46,13 @@ static struct page *ion_page_pool_remove(struct ion_page_pool *pool, bool high) struct page *page;
if (high) { - BUG_ON(!pool->high_count); + if (WARN_ON(!pool->high_count)) + return NULL; page = list_first_entry(&pool->high_items, struct page, lru); pool->high_count--; } else { - BUG_ON(!pool->low_count); + if (WARN_ON(!pool->low_count)) + return NULL; page = list_first_entry(&pool->low_items, struct page, lru); pool->low_count--; } @@ -65,7 +67,8 @@ struct page *ion_page_pool_alloc(struct ion_page_pool *pool) { struct page *page = NULL;
- BUG_ON(!pool); + if (WARN_ON(!pool)) + return NULL;
mutex_lock(&pool->mutex); if (pool->high_count) @@ -82,7 +85,8 @@ struct page *ion_page_pool_alloc(struct ion_page_pool *pool)
void ion_page_pool_free(struct ion_page_pool *pool, struct page *page) { - BUG_ON(pool->order != compound_order(page)); + if (WARN_ON(pool->order != compound_order(page))) + return;
ion_page_pool_add(pool, page); }
On Sun, Aug 16, 2020 at 10:23:25PM +0300, Tomer Samara wrote:
BUG_ON() is replaced with WARN_ON at ion_page_pool.c
Why?
Fixes the following issue: Avoid crashing the kernel - try using WARN_ON & recovery code ratherthan BUG() or BUG_ON().
Ideally you can get rid of WARN_ON() too, right?
Many systems run in panic-on-warn mode, so this really does not change anything. Try fixing this up properly to not crash at all.
Signed-off-by: Tomer Samara tomersamara98@gmail.com
drivers/staging/android/ion/ion_page_pool.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c index 0198b886d906..c1b9eda35c96 100644 --- a/drivers/staging/android/ion/ion_page_pool.c +++ b/drivers/staging/android/ion/ion_page_pool.c @@ -46,11 +46,13 @@ static struct page *ion_page_pool_remove(struct ion_page_pool *pool, bool high) struct page *page;
if (high) {
BUG_ON(!pool->high_count);
if (WARN_ON(!pool->high_count))
return NULL;
And can you test this that it works properly?
thanks,
greg k-h
On Tue, Aug 18, 2020 at 04:11:06PM +0200, Greg Kroah-Hartman wrote:
On Sun, Aug 16, 2020 at 10:23:25PM +0300, Tomer Samara wrote:
BUG_ON() is replaced with WARN_ON at ion_page_pool.c
Why?
Fixes the following issue: Avoid crashing the kernel - try using WARN_ON & recovery code ratherthan BUG() or BUG_ON().
Ideally you can get rid of WARN_ON() too, right?
Many systems run in panic-on-warn mode, so this really does not change anything. Try fixing this up properly to not crash at all.
You mean by that to just remove the WARN_ON and leave the condition the same?
Signed-off-by: Tomer Samara tomersamara98@gmail.com
drivers/staging/android/ion/ion_page_pool.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c index 0198b886d906..c1b9eda35c96 100644 --- a/drivers/staging/android/ion/ion_page_pool.c +++ b/drivers/staging/android/ion/ion_page_pool.c @@ -46,11 +46,13 @@ static struct page *ion_page_pool_remove(struct ion_page_pool *pool, bool high) struct page *page;
if (high) {
BUG_ON(!pool->high_count);
if (WARN_ON(!pool->high_count))
return NULL;
And can you test this that it works properly?
thanks,
greg k-h
Will do :)
On Tue, Aug 18, 2020 at 05:19:40PM +0300, Tomer Samara wrote:
On Tue, Aug 18, 2020 at 04:11:06PM +0200, Greg Kroah-Hartman wrote:
On Sun, Aug 16, 2020 at 10:23:25PM +0300, Tomer Samara wrote:
BUG_ON() is replaced with WARN_ON at ion_page_pool.c
Why?
Fixes the following issue: Avoid crashing the kernel - try using WARN_ON & recovery code ratherthan BUG() or BUG_ON().
Ideally you can get rid of WARN_ON() too, right?
Many systems run in panic-on-warn mode, so this really does not change anything. Try fixing this up properly to not crash at all.
You mean by that to just remove the WARN_ON and leave the condition the same?
Or fix the problem that could ever cause this check to fire.
thanks,
greg k-h
Add error check to ion_page_pool_shrink after calling ion_page_pool_remove, due to converting BUG_ON to WARN_ON.
Signed-off-by: Tomer Samara tomersamara98@gmail.com --- drivers/staging/android/ion/ion_page_pool.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c index c1b9eda35c96..031550473000 100644 --- a/drivers/staging/android/ion/ion_page_pool.c +++ b/drivers/staging/android/ion/ion_page_pool.c @@ -128,6 +128,8 @@ int ion_page_pool_shrink(struct ion_page_pool *pool, gfp_t gfp_mask, break; } mutex_unlock(&pool->mutex); + if (!page) + break; ion_page_pool_free_pages(pool, page); freed += (1 << pool->order); }
On Sun, Aug 16, 2020 at 10:24:22PM +0300, Tomer Samara wrote:
Add error check to ion_page_pool_shrink after calling ion_page_pool_remove, due to converting BUG_ON to WARN_ON.
Signed-off-by: Tomer Samara tomersamara98@gmail.com
So this fixes a previous patch? That's not good, please merge them together so you do not cause a bug and then fix it up later on.
thanks,
greg k-h
replace BUG() with WARN() at ion_sytem_heap.c, this fix the following checkpatch issue: Avoid crashing the kernel - try using WARN_ON & recovery code ratherthan BUG() or BUG_ON().
Signed-off-by: Tomer Samara tomersamara98@gmail.com --- drivers/staging/android/ion/ion_system_heap.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c index eac0632ab4e8..37065a59ca69 100644 --- a/drivers/staging/android/ion/ion_system_heap.c +++ b/drivers/staging/android/ion/ion_system_heap.c @@ -30,7 +30,8 @@ static int order_to_index(unsigned int order) for (i = 0; i < NUM_ORDERS; i++) if (order == orders[i]) return i; - BUG(); + + WARN(1, "%s: Did not found index to order %d", __FUNCTION__, order); return -1; }
On Sun, Aug 16, 2020 at 10:30:10PM +0300, Tomer Samara wrote:
replace BUG() with WARN() at ion_sytem_heap.c, this fix the following checkpatch issue: Avoid crashing the kernel - try using WARN_ON & recovery code ratherthan BUG() or BUG_ON().
Signed-off-by: Tomer Samara tomersamara98@gmail.com
drivers/staging/android/ion/ion_system_heap.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c index eac0632ab4e8..37065a59ca69 100644 --- a/drivers/staging/android/ion/ion_system_heap.c +++ b/drivers/staging/android/ion/ion_system_heap.c @@ -30,7 +30,8 @@ static int order_to_index(unsigned int order) for (i = 0; i < NUM_ORDERS; i++) if (order == orders[i]) return i;
- BUG();
- WARN(1, "%s: Did not found index to order %d", __FUNCTION__, order);
Same question as before, I think this didn't really change anything :(
Add error check to: - free_buffer_page - alloc_buffer_page after calling order_to_index, due to converting BUG to WARN at order_to_index.
Signed-off-by: Tomer Samara tomersamara98@gmail.com --- drivers/staging/android/ion/ion_system_heap.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c index 37065a59ca69..1e73bfc88884 100644 --- a/drivers/staging/android/ion/ion_system_heap.c +++ b/drivers/staging/android/ion/ion_system_heap.c @@ -49,8 +49,13 @@ static struct page *alloc_buffer_page(struct ion_system_heap *heap, struct ion_buffer *buffer, unsigned long order) { - struct ion_page_pool *pool = heap->pools[order_to_index(order)]; + struct ion_page_pool *pool; + int index = order_to_index(order); + + if (index < 0) + return NULL;
+ pool = heap->pools[index]; return ion_page_pool_alloc(pool); }
@@ -59,6 +64,7 @@ static void free_buffer_page(struct ion_system_heap *heap, { struct ion_page_pool *pool; unsigned int order = compound_order(page); + int index;
/* go to system */ if (buffer->private_flags & ION_PRIV_FLAG_SHRINKER_FREE) { @@ -66,8 +72,11 @@ static void free_buffer_page(struct ion_system_heap *heap, return; }
- pool = heap->pools[order_to_index(order)]; + index = order_to_index(order); + if (index < 0) + return;
+ pool = heap->pools[index]; ion_page_pool_free(pool, page); }
On Sun, Aug 16, 2020 at 10:31:22PM +0300, Tomer Samara wrote:
Add error check to:
- free_buffer_page
- alloc_buffer_page
after calling order_to_index, due to converting BUG to WARN at order_to_index.
You are fixing a bug you caused in a previous patch, not good :)
thanks,
greg k-h
dri-devel@lists.freedesktop.org