Re: [PATCH net] net: natsemi: fix `rx_dropped` double accounting on `netif_rx()` failure

["Yeounsu Moon" <yyyynoom@gmail.com>]

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