[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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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

[kernel test robot]

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

Hi Tomer,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on staging/staging-testing]

url:    https://github.com/0day-ci/linux/commits/Tomer-Samara/staging-android-Remove-BUG-BUG_ON-from-ion/20200821-233003
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git bc752d2f345bf55d71b3422a6a24890ea03168dc
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/staging/android/ion/ion_page_pool.c: In function 'ion_page_pool_free':
>> drivers/staging/android/ion/ion_page_pool.c:83:2: error: expected ';' before 'ion_page_pool_add'
      83 |  ion_page_pool_add(pool, page);
         |  ^~~~~~~~~~~~~~~~~
   At top level:
   drivers/staging/android/ion/ion_page_pool.c:28:13: warning: 'ion_page_pool_add' defined but not used [-Wunused-function]
      28 | static void ion_page_pool_add(struct ion_page_pool *pool, struct page *page)
         |             ^~~~~~~~~~~~~~~~~

# https://github.com/0day-ci/linux/commit/855a2d27af07d8bf02bc2f5cd75a7f87d1e94686
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Tomer-Samara/staging-android-Remove-BUG-BUG_ON-from-ion/20200821-233003
git checkout 855a2d27af07d8bf02bc2f5cd75a7f87d1e94686
vim +83 drivers/staging/android/ion/ion_page_pool.c

0214c7f20bf4d5d Rebecca Schultz Zavin 2013-12-13  78  
0214c7f20bf4d5d Rebecca Schultz Zavin 2013-12-13  79  void ion_page_pool_free(struct ion_page_pool *pool, struct page *page)
0214c7f20bf4d5d Rebecca Schultz Zavin 2013-12-13  80  {
855a2d27af07d8b Tomer Samara          2020-08-21  81  	WARN_ON(pool->order != compound_order(page))
bdeb9f1c4276864 Heesub Shin           2014-05-28  82  
a3f75c43594ccfe Yisheng Xie           2018-02-12 @83  	ion_page_pool_add(pool, page);
0214c7f20bf4d5d Rebecca Schultz Zavin 2013-12-13  84  }
0214c7f20bf4d5d Rebecca Schultz Zavin 2013-12-13  85  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 75920 bytes --]

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

[kernel test robot]

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

Hi Tomer,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on staging/staging-testing]

url:    https://github.com/0day-ci/linux/commits/Tomer-Samara/staging-android-Remove-BUG-BUG_ON-from-ion/20200821-233003
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git bc752d2f345bf55d71b3422a6a24890ea03168dc
config: s390-randconfig-r036-20200820 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project b587ca93be114d07ec3bf654add97d7872325281)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:20:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0x0000ff00UL) <<  8) |            \
                     ^
   In file included from drivers/staging/android/ion/ion_page_pool.c:10:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:22:
   In file included from include/linux/writeback.h:14:
   In file included from include/linux/blk-cgroup.h:23:
   In file included from include/linux/blkdev.h:25:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:21:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0x00ff0000UL) >>  8) |            \
                     ^
   In file included from drivers/staging/android/ion/ion_page_pool.c:10:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:22:
   In file included from include/linux/writeback.h:14:
   In file included from include/linux/blk-cgroup.h:23:
   In file included from include/linux/blkdev.h:25:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:22:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0xff000000UL) >> 24)))
                     ^
   In file included from drivers/staging/android/ion/ion_page_pool.c:10:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:22:
   In file included from include/linux/writeback.h:14:
   In file included from include/linux/blk-cgroup.h:23:
   In file included from include/linux/blkdev.h:25:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:120:12: note: expanded from macro '__swab32'
           __fswab32(x))
                     ^
   In file included from drivers/staging/android/ion/ion_page_pool.c:10:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:22:
   In file included from include/linux/writeback.h:14:
   In file included from include/linux/blk-cgroup.h:23:
   In file included from include/linux/blkdev.h:25:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
>> drivers/staging/android/ion/ion_page_pool.c:81:46: error: expected ';' after expression
           WARN_ON(pool->order != compound_order(page))
                                                       ^
                                                       ;
   20 warnings and 1 error generated.

# https://github.com/0day-ci/linux/commit/855a2d27af07d8bf02bc2f5cd75a7f87d1e94686
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Tomer-Samara/staging-android-Remove-BUG-BUG_ON-from-ion/20200821-233003
git checkout 855a2d27af07d8bf02bc2f5cd75a7f87d1e94686
vim +81 drivers/staging/android/ion/ion_page_pool.c

    78	
    79	void ion_page_pool_free(struct ion_page_pool *pool, struct page *page)
    80	{
  > 81		WARN_ON(pool->order != compound_order(page))
    82	
    83		ion_page_pool_add(pool, page);
    84	}
    85	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 19858 bytes --]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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