| Message ID | 52E4C862.9050103@huawei.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sun, 2014-01-26 at 16:33 +0800, Wang Weidong wrote: > 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. > Signed-off-by: Wang Weidong <wangweidong1@huawei.com> > --- > drivers/net/ethernet/realtek/8139cp.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c > index 737c1a8..b70e184 100644 > --- a/drivers/net/ethernet/realtek/8139cp.c > +++ b/drivers/net/ethernet/realtek/8139cp.c > @@ -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); > }
On 2014/1/27 7:23, Ben Hutchings wrote: > On Sun, 2014-01-26 at 16:33 +0800, Wang Weidong wrote: >> 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 >> Signed-off-by: Wang Weidong <wangweidong1@huawei.com> >> --- >> drivers/net/ethernet/realtek/8139cp.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c >> index 737c1a8..b70e184 100644 >> --- a/drivers/net/ethernet/realtek/8139cp.c >> +++ b/drivers/net/ethernet/realtek/8139cp.c >> @@ -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); >> } > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2014-01-27 at 09:14 +0800, Wang Weidong wrote: > On 2014/1/27 7:23, Ben Hutchings wrote: > > On Sun, 2014-01-26 at 16:33 +0800, Wang Weidong wrote: > >> 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? [...] 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.
From: Wang Weidong <wangweidong1@huawei.com> On 2014/1/27 19:54, Ben Hutchings wrote: > On Mon, 2014-01-27 at 09:14 +0800, Wang Weidong wrote: >> On 2014/1/27 7:23, Ben Hutchings wrote: >>> On Sun, 2014-01-26 at 16:33 +0800, Wang Weidong wrote: >>>> 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? > [...] > > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c index 737c1a8..b70e184 100644 --- a/drivers/net/ethernet/realtek/8139cp.c +++ b/drivers/net/ethernet/realtek/8139cp.c @@ -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); }
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 <wangweidong1@huawei.com> --- drivers/net/ethernet/realtek/8139cp.c | 1 - 1 file changed, 1 deletion(-)