Re: [PATCH v10 3/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver

[Jakub Kicinski]

On Wed, 20 Jan 2021 20:30:14 +0100 Lukasz Stelmach wrote:
> > You need to use 64 bit stats, like struct rtnl_link_stats64.
> > On a 32bit system at 100Mbps ulong can wrap in minutes.
> >  
> 
> Let me see. At first glance
> 
> git grep -l ndo_get_stats\\\> drivers/net/ethernet/  | xargs grep -li SPEED_100\\\>
> 
> quite a number of Fast Ethernet drivers use net_device_stats. Let me
> calculate.
> 
> - bytes
>   100Mbps is ~10MiB/s
>   sending 4GiB at 10MiB/s takes 27 minutes
> 
> - packets
>   minimum frame size is 84 bytes (840 bits on the wire) on 100Mbps means
>   119048 pps at this speed it takse 10 hours to transmit 2^32 packets
> 
> Anyway, I switched to rtnl_link_stats64. Tell me, is it OK to just
> memcpy() in .ndo_get_stats64?

Yup, you can just memcpy() your local copy over the one you get as an
argument of ndo_get_stats64

> >> +	struct work_struct	ax_work;  
> >
> > I don't see you ever canceling / flushing this work.
> > You should do that at least on driver remove if not close.  
> 
> Done.
> 
> Does it mean most drivers do it wrong?
> 
>     git grep INIT_WORK drivers/net/ethernet/ | \
>     sed -e 's/\(^[^:]*\):[^>]*->\([^,]*\),.*/\1        \2/' | \
>     while read file var; do \
>         grep -H $var $file;
>     done | grep INIT_WORK\\\|cancel_work

Some may use flush, but I wouldn't be surprised if there were bugs like
this out there.