Skip to contentSkip to conversation

8139cp: remove a won't occurred BUG_ON

PATCH In review Wang Weidong 3 participants 5 messages Jan 26, 2014 – Jan 27, 2014

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Wang Weidong

author
#0 lore raw

PATCH 1/1 8139cp: remove a won't occurred BUG_ON

when variable i go to the BUG_ON the value is equal to the CP_NUM_STATS, so the BUG_ON won't occur, so remove it

Signed-off-by Wang Weidong <[email protected]>
---
 drivers/net/ethernet/realtek/8139cp.c | 1 -
 1 file changed, 1 deletion(-)
drivers/net/ethernet/realtek/8139cp.c +0 -1
index 737c1a8..b70e184 100644
@@ -1585,7 +1585,6 @@ static void cp_get_ethtool_stats (struct net_device *dev,
 	tmp_stats[i++] = le16_to_cpu(nic_stats->tx_abort);
 	tmp_stats[i++] = le16_to_cpu(nic_stats->tx_underrun);
 	tmp_stats[i++] = cp->cp_stats.rx_frags;
-	BUG_ON(i != CP_NUM_STATS);
 
 	dma_free_coherent(&cp->pdev->dev, sizeof(*nic_stats), nic_stats, dma);
 }
Signature · 1 line
1.7.12
#1 lore raw
Quoted 2 lines · Wang Weidong

when variable i go to the BUG_ON the value is equal to the CP_NUM_STATS, so the BUG_ON won't occur, so remove it

We hope that every BUG_ON() does not occur, but that doesn't mean they should be removed. This check is meant to catch mistakes when adding new statistics.

Ben.

Quoted 17 lines
Signed-off-by Wang Weidong <[email protected]>
---
 drivers/net/ethernet/realtek/8139cp.c | 1 -
 1 file changed, 1 deletion(-)
drivers/net/ethernet/realtek/8139cp.c +0 -1
index 737c1a8..b70e184 100644
@@ -1585,7 +1585,6 @@ static void cp_get_ethtool_stats (struct net_device *dev,
 	tmp_stats[i++] = le16_to_cpu(nic_stats->tx_abort);
 	tmp_stats[i++] = le16_to_cpu(nic_stats->tx_underrun);
 	tmp_stats[i++] = cp->cp_stats.rx_frags;
-	BUG_ON(i != CP_NUM_STATS);
 
 	dma_free_coherent(&cp->pdev->dev, sizeof(*nic_stats), nic_stats, dma);
 }
Signature · 2 lines
Ben Hutchings
If the facts do not conform to your theory, they must be disposed of.
author
#2 lore raw
Quoted 10 lines · Ben Hutchings
Quoted 2 lines · Wang Weidong

when variable i go to the BUG_ON the value is equal to the CP_NUM_STATS, so the BUG_ON won't occur, so remove it

We hope that every BUG_ON() does not occur, but that doesn't mean they should be removed. This check is meant to catch mistakes when adding new statistics.

Ben.

Hi, Ben.

Yeah, but I think If someone would add new statistics, he should take into account it instead the BUG_ON helper.

And that, I found some other drivers' get_ethtool_stats no have BUG_ON. Should we add the BUG_ON into them?

Regards,
Wang

Quoted 18 lines
Quoted 17 lines
Signed-off-by Wang Weidong <[email protected]>
---
 drivers/net/ethernet/realtek/8139cp.c | 1 -
 1 file changed, 1 deletion(-)
drivers/net/ethernet/realtek/8139cp.c +0 -1
index 737c1a8..b70e184 100644
@@ -1585,7 +1585,6 @@ static void cp_get_ethtool_stats (struct net_device *dev,
 	tmp_stats[i++] = le16_to_cpu(nic_stats->tx_abort);
 	tmp_stats[i++] = le16_to_cpu(nic_stats->tx_underrun);
 	tmp_stats[i++] = cp->cp_stats.rx_frags;
-	BUG_ON(i != CP_NUM_STATS);
 
 	dma_free_coherent(&cp->pdev->dev, sizeof(*nic_stats), nic_stats, dma);
 }
#3 lore raw
Quoted 11 lines · Wang Weidong
Quoted 10 lines · Ben Hutchings
Quoted 2 lines · Wang Weidong

when variable i go to the BUG_ON the value is equal to the CP_NUM_STATS, so the BUG_ON won't occur, so remove it

We hope that every BUG_ON() does not occur, but that doesn't mean they should be removed. This check is meant to catch mistakes when adding new statistics.

Ben.

Quoted 7 lines

Hi, Ben.

Yeah, but I think If someone would add new statistics, he should take into account it instead the BUG_ON helper.

And that, I found some other drivers' get_ethtool_stats no have BUG_ON. Should we add the BUG_ON into them?

[...]

The important thing is that the get_stats, get_sset_count and get_strings operations are consistent. Depending on how they are implemented, a BUG_ON or BUILD_BUG_ON may be useful to check that. I don't think there's any universal best practice.

Ben.

Signature · 2 lines
Ben Hutchings
If at first you don't succeed, you're doing about average.
#4 lore raw

From: Wang Weidong <[email protected]>

Quoted 19 lines · Ben Hutchings
Quoted 11 lines · Wang Weidong
Quoted 10 lines · Ben Hutchings
Quoted 2 lines · Wang Weidong

when variable i go to the BUG_ON the value is equal to the CP_NUM_STATS, so the BUG_ON won't occur, so remove it

We hope that every BUG_ON() does not occur, but that doesn't mean they should be removed. This check is meant to catch mistakes when adding new statistics.

Ben.

Quoted 7 lines

Hi, Ben.

Yeah, but I think If someone would add new statistics, he should take into account it instead the BUG_ON helper.

And that, I found some other drivers' get_ethtool_stats no have BUG_ON. Should we add the BUG_ON into them?

Quoted 9 lines

[...]

The important thing is that the get_stats, get_sset_count and get_strings operations are consistent. Depending on how they are implemented, a BUG_ON or BUILD_BUG_ON may be useful to check that. I don't think there's any universal best practice.

Ben.

Ok, Got it.
Thanks for your answers.

Regards,
Wang