[PATCH v4 0/2] staging: android: Remove BUG/BUG_ON from ion

[PATCH v4 0/2] staging: android: Remove BUG/BUG_ON from ion

[Tomer Samara]

Remove BUG/BUG_ON from androind/ion


-v4:
	some changes based on Dan Carpenter review:
	- Remove error check at ion_page_pool_remove (conditions are impossible)
	- Remove error check at opn_page_pool_alloc
	- restore WARN_ON at ion_page_pool_free
	- Remove unnecessary error check at ion_page_pool_shrink
	- Add /* This is impossible. */ comment at order_to_index
	- Remove error handling of order_to_index

-v3:
	remove WARN/WARN_ON as Gerg KH suggests

-v2: 
	add error check to order_to_index callers

Tomer Samara (2):
  staging: android: Remove BUG_ON from ion_page_pool.c
  staging: android: Remove BUG from ion_system_heap.c

 drivers/staging/android/ion/ion_page_pool.c   | 6 +-----
 drivers/staging/android/ion/ion_system_heap.c | 2 +-
 2 files changed, 2 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH v4 1/2] staging: android: Remove BUG_ON from ion_page_pool.c

[Tomer Samara]

BUG_ON() is removed 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 | 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 0198b886d906..a3ed3bfd47ee 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -46,11 +46,9 @@ static struct page *ion_page_pool_remove(struct ion_page_pool *pool, bool high)
 	struct page *page;
 
 	if (high) {
-		BUG_ON(!pool->high_count);
 		page = list_first_entry(&pool->high_items, struct page, lru);
 		pool->high_count--;
 	} else {
-		BUG_ON(!pool->low_count);
 		page = list_first_entry(&pool->low_items, struct page, lru);
 		pool->low_count--;
 	}
@@ -65,8 +63,6 @@ struct page *ion_page_pool_alloc(struct ion_page_pool *pool)
 {
 	struct page *page = NULL;
 
-	BUG_ON(!pool);
-
 	mutex_lock(&pool->mutex);
 	if (pool->high_count)
 		page = ion_page_pool_remove(pool, true);
@@ -82,7 +78,7 @@ 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));
+	WARN_ON(pool->order != compound_order(page))
 
 	ion_page_pool_add(pool, page);
 }
-- 
2.25.1

Re: [PATCH v4 1/2] staging: android: Remove BUG_ON from ion_page_pool.c

[Dan Carpenter]

On Fri, Aug 21, 2020 at 06:27:37PM +0300, Tomer Samara wrote:
> BUG_ON() is removed 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>
> ---

You should put a note here about what changed between v3 vs v4. Like so:
---
v4: Just remove the BUG_ON()s instead of adding new returns.
v3: Hand the new return paths or whatever.

Anyway, looks good.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

[PATCH v4 2/2] staging: android: Remove BUG from ion_system_heap.c

[Tomer Samara]

Remove BUG() from 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
index eac0632ab4e8..00d6154aec34 100644
--- a/drivers/staging/android/ion/ion_system_heap.c
+++ b/drivers/staging/android/ion/ion_system_heap.c
@@ -30,7 +30,7 @@ static int order_to_index(unsigned int order)
 	for (i = 0; i < NUM_ORDERS; i++)
 		if (order == orders[i])
 			return i;
-	BUG();
+	/* This is impossible. */
 	return -1;
 }
 
-- 
2.25.1

Re: [PATCH v4 2/2] staging: android: Remove BUG from ion_system_heap.c

[Randy Dunlap]

On 8/21/20 8:28 AM, Tomer Samara wrote:
> Remove BUG() from 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
> index eac0632ab4e8..00d6154aec34 100644
> --- a/drivers/staging/android/ion/ion_system_heap.c
> +++ b/drivers/staging/android/ion/ion_system_heap.c
> @@ -30,7 +30,7 @@ static int order_to_index(unsigned int order)
>  	for (i = 0; i < NUM_ORDERS; i++)
>  		if (order == orders[i])
>  			return i;
> -	BUG();
> +	/* This is impossible. */
>  	return -1;
>  }

Hi,
Please explain why this is impossible.

If some caller calls order_to_index(5), it will return -1, yes?

-- 
~Randy

Re: [PATCH v4 2/2] staging: android: Remove BUG from ion_system_heap.c

[Tomer Samara]

On Fri, Aug 21, 2020 at 09:25:26AM -0700, Randy Dunlap wrote:
> On 8/21/20 8:28 AM, Tomer Samara wrote:
> > Remove BUG() from 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 | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
> > index eac0632ab4e8..00d6154aec34 100644
> > --- a/drivers/staging/android/ion/ion_system_heap.c
> > +++ b/drivers/staging/android/ion/ion_system_heap.c
> > @@ -30,7 +30,7 @@ static int order_to_index(unsigned int order)
> >  	for (i = 0; i < NUM_ORDERS; i++)
> >  		if (order == orders[i])
> >  			return i;
> > -	BUG();
> > +	/* This is impossible. */
> >  	return -1;
> >  }
> 
> Hi,
> Please explain why this is impossible.
> 
> If some caller calls order_to_index(5), it will return -1, yes?
> 
> -- 
> ~Randy
> 

As Dan Carpenter says here https://lkml.kernel.org/lkml/cover.1597865771.git.tomersamara98@gmail.com/T/#mc790b91029565b1bb0cb87997b39007d9edb6e04.
After looking at callers we see that order_to_index called from 2 functions:
- alloc_buffer_page called from alloc_largest_available which 
  loop over all legit order nubmers
  ( Flow:
   alloc_largest_available-->alloc_buffer_page-->order_to_index
  )

- free_buffer_page takes the order using compound_order, which return 0 or
  the order number for the page, this function has 2 callers too,
  ion_system_heap_allocate (which called it in case of failure at sg_alloc_table,
  thus calling from this flow will no casue error) and ion_system_heap_free
  (which will be called on every sg table in the buffer that allocated good,
  meaning from this flow also error will not be created).
  ( Flows:
   ion_system_heap_free     --> free_buffer_page --> order_to_index
   ion_system_heap_allocate --> free_buffer_page --> order_to_index
  )

Of course if some user will use this function with wrong order number he will be able to get this -1.
So should I remove this comment and resotre the error checks?
Btw, this is the same reason that I dropped the error check at ion_page_pool_shrink, so should I restore here also?

Thanks,
	Tomer Samara

Re: [PATCH v4 2/2] staging: android: Remove BUG from ion_system_heap.c

[Randy Dunlap]

On 8/22/20 2:34 AM, Tomer Samara wrote:
> On Fri, Aug 21, 2020 at 09:25:26AM -0700, Randy Dunlap wrote:
>> On 8/21/20 8:28 AM, Tomer Samara wrote:
>>> Remove BUG() from 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 | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
>>> index eac0632ab4e8..00d6154aec34 100644
>>> --- a/drivers/staging/android/ion/ion_system_heap.c
>>> +++ b/drivers/staging/android/ion/ion_system_heap.c
>>> @@ -30,7 +30,7 @@ static int order_to_index(unsigned int order)
>>>  	for (i = 0; i < NUM_ORDERS; i++)
>>>  		if (order == orders[i])
>>>  			return i;
>>> -	BUG();
>>> +	/* This is impossible. */
>>>  	return -1;
>>>  }
>>
>> Hi,
>> Please explain why this is impossible.
>>
>> If some caller calls order_to_index(5), it will return -1, yes?
>>
>> -- 
>> ~Randy
>>
> 
> As Dan Carpenter says here https://lkml.kernel.org/lkml/cover.1597865771.git.tomersamara98@gmail.com/T/#mc790b91029565b1bb0cb87997b39007d9edb6e04.
> After looking at callers we see that order_to_index called from 2 functions:
> - alloc_buffer_page called from alloc_largest_available which 
>   loop over all legit order nubmers
>   ( Flow:
>    alloc_largest_available-->alloc_buffer_page-->order_to_index
>   )
> 
> - free_buffer_page takes the order using compound_order, which return 0 or
>   the order number for the page, this function has 2 callers too,
>   ion_system_heap_allocate (which called it in case of failure at sg_alloc_table,
>   thus calling from this flow will no casue error) and ion_system_heap_free
>   (which will be called on every sg table in the buffer that allocated good,
>   meaning from this flow also error will not be created).
>   ( Flows:
>    ion_system_heap_free     --> free_buffer_page --> order_to_index
>    ion_system_heap_allocate --> free_buffer_page --> order_to_index
>   )
> 
> Of course if some user will use this function with wrong order number he will be able to get this -1.
> So should I remove this comment and resotre the error checks?

I think so, but that's just an opinion.

> Btw, this is the same reason that I dropped the error check at ion_page_pool_shrink, so should I restore here also?

IMO yes.

Getting rid of BUG()s is a good goal, but usually it's not so easy.

thanks.
-- 
~Randy

Re: [PATCH v4 2/2] staging: android: Remove BUG from ion_system_heap.c

[Dan Carpenter]

On Fri, Aug 21, 2020 at 09:25:26AM -0700, Randy Dunlap wrote:
> On 8/21/20 8:28 AM, Tomer Samara wrote:
> > Remove BUG() from 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 | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
> > index eac0632ab4e8..00d6154aec34 100644
> > --- a/drivers/staging/android/ion/ion_system_heap.c
> > +++ b/drivers/staging/android/ion/ion_system_heap.c
> > @@ -30,7 +30,7 @@ static int order_to_index(unsigned int order)
> >  	for (i = 0; i < NUM_ORDERS; i++)
> >  		if (order == orders[i])
> >  			return i;
> > -	BUG();
> > +	/* This is impossible. */
> >  	return -1;
> >  }
> 
> Hi,
> Please explain why this is impossible.
> 
> If some caller calls order_to_index(5), it will return -1, yes?
> 

I was happy enough with the comment as-is given that I suggested it.
But an alternative comment could be "/* This is impossible.
We always pass valid values to this function. */

regards,
dan carpenter

Re: [PATCH v4 2/2] staging: android: Remove BUG from ion_system_heap.c

[Dan Carpenter]

On Mon, Aug 24, 2020 at 02:24:57PM +0300, Dan Carpenter wrote:
> On Fri, Aug 21, 2020 at 09:25:26AM -0700, Randy Dunlap wrote:
> > On 8/21/20 8:28 AM, Tomer Samara wrote:
> > > Remove BUG() from 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 | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
> > > index eac0632ab4e8..00d6154aec34 100644
> > > --- a/drivers/staging/android/ion/ion_system_heap.c
> > > +++ b/drivers/staging/android/ion/ion_system_heap.c
> > > @@ -30,7 +30,7 @@ static int order_to_index(unsigned int order)
> > >  	for (i = 0; i < NUM_ORDERS; i++)
> > >  		if (order == orders[i])
> > >  			return i;
> > > -	BUG();
> > > +	/* This is impossible. */
> > >  	return -1;
> > >  }
> > 
> > Hi,
> > Please explain why this is impossible.
> > 
> > If some caller calls order_to_index(5), it will return -1, yes?
> > 
> 
> I was happy enough with the comment as-is given that I suggested it.
> But an alternative comment could be "/* This is impossible.
> We always pass valid values to this function. */

Another option is to just change the BUG_ON() to a WARN_ON().  I feel
like that communicates the same thing but makes checkpatch happy.

regards,
dan carpenter

Re: [PATCH v4 2/2] staging: android: Remove BUG from ion_system_heap.c

[Dan Carpenter]

On Mon, Aug 24, 2020 at 02:27:08PM +0300, Dan Carpenter wrote:
> On Mon, Aug 24, 2020 at 02:24:57PM +0300, Dan Carpenter wrote:
> > On Fri, Aug 21, 2020 at 09:25:26AM -0700, Randy Dunlap wrote:
> > > On 8/21/20 8:28 AM, Tomer Samara wrote:
> > > > Remove BUG() from 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 | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
> > > > index eac0632ab4e8..00d6154aec34 100644
> > > > --- a/drivers/staging/android/ion/ion_system_heap.c
> > > > +++ b/drivers/staging/android/ion/ion_system_heap.c
> > > > @@ -30,7 +30,7 @@ static int order_to_index(unsigned int order)
> > > >  	for (i = 0; i < NUM_ORDERS; i++)
> > > >  		if (order == orders[i])
> > > >  			return i;
> > > > -	BUG();
> > > > +	/* This is impossible. */
> > > >  	return -1;
> > > >  }
> > > 
> > > Hi,
> > > Please explain why this is impossible.
> > > 
> > > If some caller calls order_to_index(5), it will return -1, yes?
> > > 
> > 
> > I was happy enough with the comment as-is given that I suggested it.
> > But an alternative comment could be "/* This is impossible.
> > We always pass valid values to this function. */
> 
> Another option is to just change the BUG_ON() to a WARN_ON().  I feel
> like that communicates the same thing but makes checkpatch happy.

Actually earlier Greg pointed out that some systems have panic on warn
so WARN_ON() doesn't work.  Just add the comment.

regards,
dan carpenter

Re: [PATCH v4 0/2] staging: android: Remove BUG/BUG_ON from ion

[Christoph Hellwig]

On Fri, Aug 21, 2020 at 06:27:04PM +0300, Tomer Samara wrote:
> Remove BUG/BUG_ON from androind/ion

Please just remove ion.  It has been rejected and we have developed
proper kernel subsystens to replace it.  Don't waste your time on it.

Re: [PATCH v4 0/2] staging: android: Remove BUG/BUG_ON from ion

[Greg Kroah-Hartman]

On Tue, Aug 25, 2020 at 07:47:29AM +0100, Christoph Hellwig wrote:
> On Fri, Aug 21, 2020 at 06:27:04PM +0300, Tomer Samara wrote:
> > Remove BUG/BUG_ON from androind/ion
> 
> Please just remove ion.  It has been rejected and we have developed
> proper kernel subsystens to replace it.  Don't waste your time on it.

It is going to be removed at the end of this year.  Why we keep it
around until then, I really don't know, but John and Laura have this as
the plan.

thanks,

greg k-h

Re: [PATCH v4 0/2] staging: android: Remove BUG/BUG_ON from ion

[Christoph Hellwig]

On Tue, Aug 25, 2020 at 08:52:29AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Aug 25, 2020 at 07:47:29AM +0100, Christoph Hellwig wrote:
> > On Fri, Aug 21, 2020 at 06:27:04PM +0300, Tomer Samara wrote:
> > > Remove BUG/BUG_ON from androind/ion
> > 
> > Please just remove ion.  It has been rejected and we have developed
> > proper kernel subsystens to replace it.  Don't waste your time on it.
> 
> It is going to be removed at the end of this year.  Why we keep it
> around until then, I really don't know, but John and Laura have this as
> the plan.

It keeps getting in the way of various projects and has no path
going upstream properly.  Seems weird to keep this dead and not all
that great code around.

Re: [PATCH v4 0/2] staging: android: Remove BUG/BUG_ON from ion

[Greg Kroah-Hartman]

On Thu, Aug 27, 2020 at 08:16:54AM +0100, Christoph Hellwig wrote:
> On Tue, Aug 25, 2020 at 08:52:29AM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Aug 25, 2020 at 07:47:29AM +0100, Christoph Hellwig wrote:
> > > On Fri, Aug 21, 2020 at 06:27:04PM +0300, Tomer Samara wrote:
> > > > Remove BUG/BUG_ON from androind/ion
> > > 
> > > Please just remove ion.  It has been rejected and we have developed
> > > proper kernel subsystens to replace it.  Don't waste your time on it.
> > 
> > It is going to be removed at the end of this year.  Why we keep it
> > around until then, I really don't know, but John and Laura have this as
> > the plan.
> 
> It keeps getting in the way of various projects and has no path
> going upstream properly.  Seems weird to keep this dead and not all
> that great code around.

In looking at the mess of ion changes that are currently in the AOSP
kernel tree (where android devices are pulled from), it looks almost
nothing like what we currently have here in the mainline kernel tree.

So if what we have here, today, is not of use to anyone who actually
wants to use this interface, why are we keeping it around?

John, why can't we just drop all of this code from the kernel today, and
then Android will keep their own copy for their next LTS release anyway.
It doesn't look like what we have here, and the merge issues it causes
is a pain (as I know from having to do them...)  So keeping this around
in-tree any longer feels pointless to me, and actively causes me, and
others, more work for no gain.

I'll go make a patch set to just drop this code now...

thanks,

greg k-h