Re: [PATCH net-next] ibmveth: Use WARN_ON with error handling rather than BUG_ON

[Michal Swiatkowski <michal.swiatkowski@linux.intel.com>]

On Thu, Apr 10, 2025 at 01:39:18PM -0500, Dave Marquardt wrote:
> - Replaced BUG_ON calls with WARN_ON calls with error handling,
>   with calls to a new ibmveth_reset routine, which resets the device.
> - Added KUnit tests for ibmveth_remove_buffer_from_pool and
>   ibmveth_rxq_get_buffer under new IBMVETH_KUNIT_TEST config option.
> - Removed unneeded forward declaration of ibmveth_rxq_harvest_buffer.

It will be great if you split this patch into 3 patches according to
your description.

> 
> Signed-off-by: Dave Marquardt <davemarq@linux.ibm.com>
> ---
>  drivers/net/ethernet/ibm/Kconfig   |  13 ++
>  drivers/net/ethernet/ibm/ibmveth.c | 242 ++++++++++++++++++++++++++---
>  drivers/net/ethernet/ibm/ibmveth.h |  65 ++++----
>  3 files changed, 269 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/Kconfig b/drivers/net/ethernet/ibm/Kconfig
> index c0c112d95b89..4f4b23465c47 100644
> --- a/drivers/net/ethernet/ibm/Kconfig
> +++ b/drivers/net/ethernet/ibm/Kconfig
> @@ -27,6 +27,19 @@ config IBMVETH
>  	  To compile this driver as a module, choose M here. The module will
>  	  be called ibmveth.
>  
> +config IBMVETH_KUNIT_TEST
> +	bool "KUnit test for IBM LAN Virtual Ethernet support" if !KUNIT_ALL_TESTS
> +	depends on KUNIT
> +	depends on KUNIT=y && IBMVETH=y
> +	default KUNIT_ALL_TESTS
> +	help
> +	  This builds unit tests for the IBM LAN Virtual Ethernet driver.
> +
> +	  For more information on KUnit and unit tests in general, please refer
> +	  to the KUnit documentation in Documentation/dev-tools/kunit/.
> +
> +	  If unsure, say N.
> +
>  source "drivers/net/ethernet/ibm/emac/Kconfig"
>  
>  config EHEA
> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> index 04192190beba..ea201e5cc8bc 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> @@ -28,6 +28,7 @@
>  #include <linux/ip.h>
>  #include <linux/ipv6.h>
>  #include <linux/slab.h>
> +#include <linux/workqueue.h>
>  #include <asm/hvcall.h>
>  #include <linux/atomic.h>
>  #include <asm/vio.h>
> @@ -39,8 +40,6 @@
>  #include "ibmveth.h"
>  
>  static irqreturn_t ibmveth_interrupt(int irq, void *dev_instance);
> -static void ibmveth_rxq_harvest_buffer(struct ibmveth_adapter *adapter,
> -				       bool reuse);
>  static unsigned long ibmveth_get_desired_dma(struct vio_dev *vdev);
>  
>  static struct kobj_type ktype_veth_pool;
> @@ -231,7 +230,10 @@ static void ibmveth_replenish_buffer_pool(struct ibmveth_adapter *adapter,
>  		index = pool->free_map[free_index];
>  		skb = NULL;
>  
> -		BUG_ON(index == IBM_VETH_INVALID_MAP);
> +		if (WARN_ON(index == IBM_VETH_INVALID_MAP)) {
> +			(void)schedule_work(&adapter->work);

What is the purpose of void casting here (and in other places in this
patch)?

> +			goto failure2;

Maybe increment_buffer_failure, or sth that is telling what happen after
goto.

> +		}
>  
>  		/* are we allocating a new buffer or recycling an old one */
>  		if (pool->skbuff[index])
> @@ -300,6 +302,7 @@ static void ibmveth_replenish_buffer_pool(struct ibmveth_adapter *adapter,
>  		                 DMA_FROM_DEVICE);
>  	dev_kfree_skb_any(pool->skbuff[index]);
>  	pool->skbuff[index] = NULL;
> +failure2:
>  	adapter->replenish_add_buff_failure++;
>  
>  	mb();
> @@ -370,20 +373,36 @@ static void ibmveth_free_buffer_pool(struct ibmveth_adapter *adapter,
>  	}
>  }
>  

[...]