drivers/net/ethernet/natsemi/ns83820.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
`netif_rx()` already increments `rx_dropped` core stat when it fails.
The driver was also updating `ndev->stats.rx_dropped` in the same path.
Since both are reported together via `ip -s -s` command, this resulted
in drops being counted twice in user-visible stats.
Keep the driver update on `skb_put()` failure, but skip it after
`netif_rx()` errors.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Yeounsu Moon <yyyynoom@gmail.com>
---
drivers/net/ethernet/natsemi/ns83820.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/natsemi/ns83820.c b/drivers/net/ethernet/natsemi/ns83820.c
index 56d5464222d9..cdbf82affa7b 100644
--- a/drivers/net/ethernet/natsemi/ns83820.c
+++ b/drivers/net/ethernet/natsemi/ns83820.c
@@ -820,7 +820,7 @@ static void rx_irq(struct net_device *ndev)
struct ns83820 *dev = PRIV(ndev);
struct rx_info *info = &dev->rx_info;
unsigned next_rx;
- int rx_rc, len;
+ int len;
u32 cmdsts;
__le32 *desc;
unsigned long flags;
@@ -881,8 +881,10 @@ static void rx_irq(struct net_device *ndev)
if (likely(CMDSTS_OK & cmdsts)) {
#endif
skb_put(skb, len);
- if (unlikely(!skb))
+ if (unlikely(!skb)) {
+ ndev->stats.rx_dropped++;
goto netdev_mangle_me_harder_failed;
+ }
if (cmdsts & CMDSTS_DEST_MULTI)
ndev->stats.multicast++;
ndev->stats.rx_packets++;
@@ -901,15 +903,12 @@ static void rx_irq(struct net_device *ndev)
__vlan_hwaccel_put_tag(skb, htons(ETH_P_IPV6), tag);
}
#endif
- rx_rc = netif_rx(skb);
- if (NET_RX_DROP == rx_rc) {
-netdev_mangle_me_harder_failed:
- ndev->stats.rx_dropped++;
- }
+ netif_rx(skb);
} else {
dev_kfree_skb_irq(skb);
}
+netdev_mangle_me_harder_failed:
nr++;
next_rx = info->next_rx;
desc = info->descs + (DESC_SIZE * next_rx);
--
2.51.0On Wed, Sep 10, 2025 at 10:35 PM Yeounsu Moon <yyyynoom@gmail.com> wrote:
>
> `netif_rx()` already increments `rx_dropped` core stat when it fails.
> The driver was also updating `ndev->stats.rx_dropped` in the same path.
> Since both are reported together via `ip -s -s` command, this resulted
> in drops being counted twice in user-visible stats.
>
> Keep the driver update on `skb_put()` failure, but skip it after
> `netif_rx()` errors.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
I do not think this Fixes: is correct.
I think core networking got this accounting in netif_rx() in 2010
commit caf586e5f23c (" net: add a core netdev->rx_dropped counter")
> Signed-off-by: Yeounsu Moon <yyyynoom@gmail.com>
> ---
> drivers/net/ethernet/natsemi/ns83820.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/natsemi/ns83820.c b/drivers/net/ethernet/natsemi/ns83820.c
> index 56d5464222d9..cdbf82affa7b 100644
> --- a/drivers/net/ethernet/natsemi/ns83820.c
> +++ b/drivers/net/ethernet/natsemi/ns83820.c
> @@ -820,7 +820,7 @@ static void rx_irq(struct net_device *ndev)
> struct ns83820 *dev = PRIV(ndev);
> struct rx_info *info = &dev->rx_info;
> unsigned next_rx;
> - int rx_rc, len;
> + int len;
> u32 cmdsts;
> __le32 *desc;
> unsigned long flags;
> @@ -881,8 +881,10 @@ static void rx_irq(struct net_device *ndev)
> if (likely(CMDSTS_OK & cmdsts)) {
> #endif
> skb_put(skb, len);
> - if (unlikely(!skb))
I doubt this driver is used.
Notice that this test about skb being NULL or not happens after
skb_put(skb, len)
which would have crashed anyway if skb was NULL.
> + if (unlikely(!skb)) {
> + ndev->stats.rx_dropped++;
> goto netdev_mangle_me_harder_failed;
> + }
> if (cmdsts & CMDSTS_DEST_MULTI)
> ndev->stats.multicast++;
> ndev->stats.rx_packets++;
> @@ -901,15 +903,12 @@ static void rx_irq(struct net_device *ndev)
> __vlan_hwaccel_put_tag(skb, htons(ETH_P_IPV6), tag);
> }
> #endif
> - rx_rc = netif_rx(skb);
> - if (NET_RX_DROP == rx_rc) {
> -netdev_mangle_me_harder_failed:
> - ndev->stats.rx_dropped++;
> - }
> + netif_rx(skb);
> } else {
> dev_kfree_skb_irq(skb);
> }
>
> +netdev_mangle_me_harder_failed:
> nr++;
> next_rx = info->next_rx;
> desc = info->descs + (DESC_SIZE * next_rx);
> --
> 2.51.0
>
On Fri Sep 12, 2025 at 11:19 PM KST, Eric Dumazet wrote:
>
> I do not think this Fixes: is correct.
>
> I think core networking got this accounting in netif_rx() in 2010
>
> commit caf586e5f23c (" net: add a core netdev->rx_dropped counter")
>
I hadn't considered that the Fixes: tag can refer to code outside of the
changes being made. Thank you for pointing this out. I also noticed your
earlier work from 2010.
I'll update the Fixes: tag as you suggested.
>> Signed-off-by: Yeounsu Moon <yyyynoom@gmail.com>
>> ---
>> drivers/net/ethernet/natsemi/ns83820.c | 13 ++++++-------
>> 1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/natsemi/ns83820.c b/drivers/net/ethernet/natsemi/ns83820.c
>> index 56d5464222d9..cdbf82affa7b 100644
>> --- a/drivers/net/ethernet/natsemi/ns83820.c
>> +++ b/drivers/net/ethernet/natsemi/ns83820.c
>> @@ -820,7 +820,7 @@ static void rx_irq(struct net_device *ndev)
>> struct ns83820 *dev = PRIV(ndev);
>> struct rx_info *info = &dev->rx_info;
>> unsigned next_rx;
>> - int rx_rc, len;
>> + int len;
>> u32 cmdsts;
>> __le32 *desc;
>> unsigned long flags;
>> @@ -881,8 +881,10 @@ static void rx_irq(struct net_device *ndev)
>> if (likely(CMDSTS_OK & cmdsts)) {
>> #endif
>> skb_put(skb, len);
>> - if (unlikely(!skb))
>
> I doubt this driver is used.
>
I also honestly doubt that this driver is still in use.
I came across it while analyzing the `netif_rx()` and `rx_dropped` code
paths, and I noticed that there are quite a few unmanaged drivers using
this kind of code. So I started to fix that.
But If patches like this only burden busy maintainers and reviewers,
I'll stop sending them. That said, I do think leaving unmanaged drivers
as they are is also problematic.
As a newcomer sending patches to netdev, I realized that there are quite
a few such drivers. I don't necessarily believe they all must be actively
maintained, but it feels like some action is needed.
> Notice that this test about skb being NULL or not happens after
> skb_put(skb, len)
> which would have crashed anyway if skb was NULL.
>
I think I wrote the commit message incorrectly.
The main point was not about `skb_put()`, but rather about the `if`
statement that checks `skb`.
That said, after your comment I realized that `skb_put()` itself also
looks problematic.
Thank you for the detailed review!
Yeounsu Moon
On Thu, Sep 11, 2025 at 02:33:06PM +0900, Yeounsu Moon wrote:
> `netif_rx()` already increments `rx_dropped` core stat when it fails.
> The driver was also updating `ndev->stats.rx_dropped` in the same path.
> Since both are reported together via `ip -s -s` command, this resulted
> in drops being counted twice in user-visible stats.
>
> Keep the driver update on `skb_put()` failure, but skip it after
> `netif_rx()` errors.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Yeounsu Moon <yyyynoom@gmail.com>
Thanks for your patch,
Thinking out loud: Adding use of ndev->stats to drivers that don't already
do so is discouraged. But here, an existing use is being fixed. And I agree
it is a fix. So this looks good to me.
Reviewed-by: Simon Horman <horms@kernel.org>
On Fri Sep 12, 2025 at 10:21 PM KST, Simon Horman wrote: > > Thanks for your patch, > Thank you very much for your review, Simon :) > Thinking out loud: Adding use of ndev->stats to drivers that don't already > do so is discouraged. But here, an existing use is being fixed. And I agree > it is a fix. So this looks good to me. > > Reviewed-by: Simon Horman <horms@kernel.org> I also noticed that many drivers are still using `ndev->stats`. If I work on related changes in the future, I will make sure to use `struct rtnl_link_stats64` instaed and post patches accordingly.
© 2016 - 2025 Red Hat, Inc.