[PATCH v2] ethernet: hnae: drop adhoc assert() macros

[PATCH v2] ethernet: hnae: drop adhoc assert() macros

[Igor Stoppa]

Replace assert() with a less misleading test_condition() using WARN()
Drop one check which had bitrotted and didn't compile anymore.

Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
Cc: huangdaode <huangdaode@hisilicon.com>
Cc: Yisen Zhuang <yisen.zhuang@huawei.com>
Cc: Salil Mehta <salil.mehta@huawei.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/ethernet/hisilicon/hns/hnae.c     |  7 ++--
 drivers/net/ethernet/hisilicon/hns/hnae.h     | 34 +++++--------------
 .../net/ethernet/hisilicon/hns/hns_ae_adapt.c | 16 ++++-----
 drivers/net/ethernet/hisilicon/hns/hns_enet.c | 12 +++----
 4 files changed, 25 insertions(+), 44 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hnae.c b/drivers/net/ethernet/hisilicon/hns/hnae.c
index a051e582d541..bd64e6092e4b 100644
--- a/drivers/net/ethernet/hisilicon/hns/hnae.c
+++ b/drivers/net/ethernet/hisilicon/hns/hnae.c
@@ -203,11 +203,12 @@ hnae_init_ring(struct hnae_queue *q, struct hnae_ring *ring, int flags)
 	ring->flags = flags;
 	spin_lock_init(&ring->lock);
 	ring->coal_param = q->handle->coal_param;
-	assert(!ring->desc && !ring->desc_cb && !ring->desc_dma_addr);
+	test_condition(!ring->desc && !ring->desc_cb &&
+		       !ring->desc_dma_addr);
 
 	/* not matter for tx or rx ring, the ntc and ntc start from 0 */
-	assert(ring->next_to_use == 0);
-	assert(ring->next_to_clean == 0);
+	test_condition(ring->next_to_use == 0);
+	test_condition(ring->next_to_clean == 0);
 
 	ring->desc_cb = kcalloc(ring->desc_num, sizeof(ring->desc_cb[0]),
 			GFP_KERNEL);
diff --git a/drivers/net/ethernet/hisilicon/hns/hnae.h b/drivers/net/ethernet/hisilicon/hns/hnae.h
index 08a750fb60c4..47dec7590079 100644
--- a/drivers/net/ethernet/hisilicon/hns/hnae.h
+++ b/drivers/net/ethernet/hisilicon/hns/hnae.h
@@ -43,23 +43,9 @@
 #define HNAE_DEFAULT_DEVICE_DESCR "Hisilicon Network Subsystem"
 
 #ifdef DEBUG
-
-#ifndef assert
-#define assert(expr) \
-do { \
-	if (!(expr)) { \
-		pr_err("Assertion failed! %s, %s, %s, line %d\n", \
-			   #expr, __FILE__, __func__, __LINE__); \
-	} \
-} while (0)
-#endif
-
+#define test_condition(expr) WARN_ON_ONCE(!(expr))
 #else
-
-#ifndef assert
-#define assert(expr)
-#endif
-
+#define test_condition(expr)
 #endif
 
 #define AE_VERSION_1 ('6' << 16 | '6' << 8 | '0')
@@ -314,16 +300,16 @@ enum hns_desc_type {
 	DESC_TYPE_PAGE,
 };
 
-#define assert_is_ring_idx(ring, idx) \
-	assert((idx) >= 0 && (idx) < (ring)->desc_num)
+#define is_ring_idx(ring, idx) \
+	test_condition((idx) >= 0 && (idx) < (ring)->desc_num)
 
 /* the distance between [begin, end) in a ring buffer
  * note: there is a unuse slot between the begin and the end
  */
 static inline int ring_dist(struct hnae_ring *ring, int begin, int end)
 {
-	assert_is_ring_idx(ring, begin);
-	assert_is_ring_idx(ring, end);
+	is_ring_idx(ring, begin);
+	is_ring_idx(ring, end);
 
 	return (end - begin + ring->desc_num) % ring->desc_num;
 }
@@ -336,8 +322,8 @@ static inline int ring_space(struct hnae_ring *ring)
 
 static inline int is_ring_empty(struct hnae_ring *ring)
 {
-	assert_is_ring_idx(ring, ring->next_to_use);
-	assert_is_ring_idx(ring, ring->next_to_clean);
+	is_ring_idx(ring, ring->next_to_use);
+	is_ring_idx(ring, ring->next_to_clean);
 
 	return ring->next_to_use == ring->next_to_clean;
 }
@@ -592,10 +578,6 @@ int hnae_reinit_handle(struct hnae_handle *handle);
 #define hnae_queue_xmit(q, buf_num) writel_relaxed(buf_num, \
 	(q)->tx_ring.io_base + RCB_REG_TAIL)
 
-#ifndef assert
-#define assert(cond)
-#endif
-
 static inline int hnae_reserve_buffer_map(struct hnae_ring *ring,
 					  struct hnae_desc_cb *cb)
 {
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c b/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c
index b52029e26d15..6ff391ccdd7a 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c
@@ -264,7 +264,7 @@ static int hns_ae_set_multicast_one(struct hnae_handle *handle, void *addr)
 	struct hns_mac_cb *mac_cb = hns_get_mac_cb(handle);
 	u8 port_num;
 
-	assert(mac_cb);
+	test_condition(mac_cb);
 
 	if (mac_cb->mac_type != HNAE_PORT_SERVICE)
 		return 0;
@@ -494,7 +494,7 @@ static void hns_ae_get_pauseparam(struct hnae_handle *handle,
 
 static int hns_ae_set_autoneg(struct hnae_handle *handle, u8 enable)
 {
-	assert(handle);
+	test_condition(handle);
 
 	return hns_mac_set_autoneg(hns_get_mac_cb(handle), enable);
 }
@@ -511,7 +511,7 @@ static int hns_ae_get_autoneg(struct hnae_handle *handle)
 {
 	u32     auto_neg;
 
-	assert(handle);
+	test_condition(handle);
 
 	hns_mac_get_autoneg(hns_get_mac_cb(handle), &auto_neg);
 
@@ -618,7 +618,7 @@ static void hns_ae_get_coalesce_range(struct hnae_handle *handle,
 {
 	struct dsaf_device *dsaf_dev;
 
-	assert(handle);
+	test_condition(handle);
 
 	dsaf_dev = hns_ae_get_dsaf_dev(handle->dev);
 
@@ -767,7 +767,7 @@ static void hns_ae_get_strings(struct hnae_handle *handle,
 	u8 *p = data;
 	struct	hnae_vf_cb *vf_cb;
 
-	assert(handle);
+	test_condition(handle);
 
 	vf_cb = hns_ae_get_vf_cb(handle);
 	port = vf_cb->port_index;
@@ -795,7 +795,7 @@ static int hns_ae_get_sset_count(struct hnae_handle *handle, int stringset)
 	struct hns_mac_cb *mac_cb;
 	struct dsaf_device *dsaf_dev = hns_ae_get_dsaf_dev(handle->dev);
 
-	assert(handle);
+	test_condition(handle);
 
 	mac_cb = hns_get_mac_cb(handle);
 
@@ -839,7 +839,7 @@ static void hns_ae_update_led_status(struct hnae_handle *handle)
 {
 	struct hns_mac_cb *mac_cb;
 
-	assert(handle);
+	test_condition(handle);
 	mac_cb = hns_get_mac_cb(handle);
 	if (mac_cb->media_type != HNAE_MEDIA_TYPE_FIBER)
 		return;
@@ -852,7 +852,7 @@ static int hns_ae_cpld_set_led_id(struct hnae_handle *handle,
 {
 	struct hns_mac_cb *mac_cb;
 
-	assert(handle);
+	test_condition(handle);
 
 	mac_cb = hns_get_mac_cb(handle);
 
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.c b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
index f56855e63c96..8ba0d44742d1 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
@@ -366,7 +366,7 @@ netdev_tx_t hns_nic_net_xmit_hw(struct net_device *ndev,
 	ndev->stats.tx_packets++;
 
 	wmb(); /* commit all data before submit */
-	assert(skb->queue_mapping < priv->ae_handle->q_num);
+	test_condition(skb->queue_mapping < priv->ae_handle->q_num);
 	hnae_queue_xmit(priv->ae_handle->qs[skb->queue_mapping], buf_num);
 	ring->stats.tx_pkts++;
 	ring->stats.tx_bytes += skb->len;
@@ -937,9 +937,9 @@ static int is_valid_clean_head(struct hnae_ring *ring, int h)
 	if (unlikely(h > ring->desc_num))
 		return 0;
 
-	assert(u > 0 && u < ring->desc_num);
-	assert(c > 0 && c < ring->desc_num);
-	assert(u != c && h != c); /* must be checked before call this func */
+	test_condition(u > 0 && u < ring->desc_num);
+	test_condition(c > 0 && c < ring->desc_num);
+	test_condition(u != c && h != c); /* must be checked before call this func */
 
 	return u > c ? (h > c && h <= u) : (h > c || h <= u);
 }
@@ -1515,8 +1515,6 @@ static netdev_tx_t hns_nic_net_xmit(struct sk_buff *skb,
 {
 	struct hns_nic_priv *priv = netdev_priv(ndev);
 
-	assert(skb->queue_mapping < ndev->ae_handle->q_num);
-
 	return hns_nic_net_xmit_hw(ndev, skb,
 				   &tx_ring_data(priv, skb->queue_mapping));
 }
@@ -2249,7 +2247,7 @@ static int hns_nic_notifier_action(struct notifier_block *nb,
 	struct hns_nic_priv *priv =
 		container_of(nb, struct hns_nic_priv, notifier_block);
 
-	assert(action == HNAE_AE_REGISTER);
+	test_condition(action == HNAE_AE_REGISTER);
 
 	if (!hns_nic_try_get_ae(priv->netdev)) {
 		hnae_unregister_notifier(&priv->notifier_block);
-- 
2.17.1

Re: [PATCH v2] ethernet: hnae: drop adhoc assert() macros

[David Miller]

From: Igor Stoppa <igor.stoppa@gmail.com>
Date: Sat,  8 Sep 2018 18:01:42 +0300

> Replace assert() with a less misleading test_condition() using WARN()
> Drop one check which had bitrotted and didn't compile anymore.
> 
> Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>

I'm still kind of not happy about this.

Make the driver use kernel interfaces like WARN_ON_ONCE()
etc. directly instead of defining alias CPP macros private to the
driver.

If it needs to be conditional upon DEBUG, we have pr_debug() and
the likes as well.

Thank you.

Re: [PATCH v2] ethernet: hnae: drop adhoc assert() macros

[Igor Stoppa]

On 12/09/18 09:07, David Miller wrote:
> From: Igor Stoppa <igor.stoppa@gmail.com>
> Date: Sat,  8 Sep 2018 18:01:42 +0300
> 
>> Replace assert() with a less misleading test_condition() using WARN()
>> Drop one check which had bitrotted and didn't compile anymore.
>>
>> Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
> 
> I'm still kind of not happy about this.
> 
> Make the driver use kernel interfaces like WARN_ON_ONCE()
> etc. directly instead of defining alias CPP macros private to the
> driver.
> 
> If it needs to be conditional upon DEBUG, we have pr_debug() and
> the likes as well.

WARN_ON_ONCE() will display the error message only once, but from its 
implementation it looks like it will evaluate the condition anyways, 
every time, unless the compiler can do some magic and somehow know that 
the condition was true already onece (/include/asm-generic/bug.h:146)

pr_debug() otoh, will print or not print, depending on 
CONFIG_DYNAMIC_DEBUG being set or not (/include/linux/printk.h:399)

but the logic we are trying to clean up here seems a bit different:

if (in_debug_mode && error_condition_is_verified)
	generate_warning()

Which, I guess, is meant to completely remove *any* test outside of 
debug mode.

That is why I used the test_condition() macro, instead of using directly 
WARN_ON_ONCE().  Admittedly, it probably would have been better to 
depend on CONFIG_DYNAMIC_DEBUG.

Neither using WARN_ON_ONCE(), nor pr_debug() seem to replicate the 
initial behavior of the code that is currently in the tree, with assert().

How would you suggest that I keep the same behavior, without the helper 
macro? I am probably missing it, but I do not see some facility that 
would support the double condition I described above.

--
thanks, igor