Re: [PATCH net v3] net: stmmac: protect updates of 64-bit statistics counters

On Wed, Mar 6, 2024 at 11:03 AM Petr Tesařík [off-list ref] wrote:

On Wed, 6 Mar 2024 10:01:53 +0100
Petr Tesařík [off-list ref] wrote:

quoted

On Wed, 6 Mar 2024 09:23:53 +0100
"Linux regression tracking (Thorsten Leemhuis)" [off-list ref] wrote:

quoted

On 28.02.24 12:03, Petr Tesařík wrote:

quoted

On Wed, 28 Feb 2024 07:19:56 +0100
"Linux regression tracking (Thorsten Leemhuis)" [off-list ref] wrote:

quoted

Net maintainers, chiming in here, as it seems handling this regression
stalled.

Indeed, I was too busy with sandbox mode...

Hmm, no reply in the past week to Petr's request for help from someone
with more knowledge about the field. :-/

So I guess this means that this won't be fixed for 6.8? Unfortunate, but
well, that's how it it sometimes.

For the record, I _can_ reproduce lockdep splats on my device, but they
don't make any sense to me. They seem to confirm Jisheng Zhang's
conclusion that lockdep conflates two locks which should have different
lock-classes.

So far I have noticed only one issue: the per-cpu syncp's are not
initialized. I'll recompile and see if that's what confuses lockdep.

That wasn't the issue. FTR the syncp was in fact initialized, because
devm_netdev_alloc_pcpu_stats() is a macro that also takes care of the
initialization of the syncp struct field.

The problem is u64_stats_init().

Commit 9464ca650008 ("net: make u64_stats_init() a function") changed
it to an inline function. But that's wrong. It uses seqcount_init(),
which in turn declares:

        static struct lock_class_key __key;

This assumes that each lock gets its own instance. But if
u64_stats_init() is a function (albeit an inline one), all calls
within the same file end up using the same instance.

Eric, would it be OK to revert the above-mentioned commit?

Oh, nice !

Well, this would not be a revert, let's keep type safety checks if possible.

Thanks.