Re: [REGRESSION] [PATCH net-next v5 2/2] net: stmmac: use per-queue 64 bit statistics where necessary

[Johannes Berg <johannes@sipsolutions.net>]

On Tue, 2023-09-12 at 11:24 +0200, Uwe Kleine-König wrote:
> > 
> > The newly added "struct u64_stats_sync syncp" uses a seqlock
> > internally, which is broken into multiple words on 32bit machines, and
> > needs to be initialized properly. You need to call u64_stats_init on
> > syncp before first usage.
> 
> This is done. The problematic thing is that in stmmac_open() ->
> __stmmac_open() the syncp initialized before is overwritten by
> 
> 	memcpy(&priv->dma_conf, dma_conf, sizeof(*dma_conf));
> 
> Do I need to point out that this is ugly?

I think it also leaks the (lockdep) state since it reinits the syncp
(and a lot of other state) doing this. This is also called when the MTU
changes.

Also, I couldn't convince myself that it's even race-free? Even if it
is, it's not really obvious, IMHO.

So it seems to me that really this needs to be split into data that
actually should be reinitialized, and data that shouldn't, or just not
use memcpy() here but copy only the relevant state?

But anyway, I have no skin in this game - just reviewing this because I
was trying to help out Uwe.

johannes