Re: [PATCH 0/6] Add some more cfg80211 and mac80211 kunit tests

From: David Gow <davidgow@google.com>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: Shuah Khan <skhan@linuxfoundation.org>,
	benjamin@sipsolutions.net,  linux-wireless@vger.kernel.org,
	linux-kselftest@vger.kernel.org,  kunit-dev@googlegroups.com,
	Brendan Higgins <brendanhiggins@google.com>,
	 Benjamin Berg <benjamin.berg@intel.com>
Subject: Re: [PATCH 0/6] Add some more cfg80211 and mac80211 kunit tests
Date: Fri, 22 Dec 2023 22:12:51 +0800	[thread overview]
Message-ID: <CABVgOS=vZzDdr0xxqUgYoZ39i3ADdwKnRyp4hXOSLGwAU0eN_g@mail.gmail.com> (raw)
In-Reply-To: <2a508793563c46116ef8ef274a9fa3b5675cd7b3.camel@sipsolutions.net>

[-- Attachment #1: Type: text/plain, Size: 5885 bytes --]

On Fri, 22 Dec 2023 at 18:09, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> Hi,
>
> Thanks for taking a look!
>
> On Fri, 2023-12-22 at 18:02 +0800, David Gow wrote:
> > The two initial KUnit patches look fine, modulo a couple of minor docs
> > issues and checkpatch warnings.
>
> I can run checkpatch (even if I can't always take it seriously), but do
> you want to comment more specifically wrt. the docs?
>

Sorry, the 'docs' issue was just the initial comment on the
include/linux/skbuff.h file in patch 2, which could have been more
specific to skbuff and resource management.
The actual kerneldoc comments seem fine to me.

> > They apply cleanly, and I doubt
> > there's much chance of there being a merge conflict for 6.8 -- there
> > are no other changes to the parameterised test macros, and the skb
> > stuff is in its own file.
>
> Right.
>
> > The remaining patches don't apply on top of the kunit branch as-is.
>
> Oh, OK. That makes some sense though, we've had a number of changes in
> the stack this cycle before. I somehow thought the tests were likely
> standalone, but apparently not.
>

I managed to get this to apply locally. The only real changes are to
net/mac80211/ieee80211_i.h  so it may be possible to port this across
to the kselftest/kunit branch if you want, but it doesn't apply
cleanly as-is.

Also, there are a couple of cfg80211 failures:
---
KTAP version 1
1..1
   KTAP version 1
   # Subtest: cfg80211-inform-bss
   # module: cfg80211_tests
   1..2
platform regulatory.0: Direct firmware load for regulatory.db failed
with error -2
cfg80211: failed to load regulatory.db
   ok 1 test_inform_bss_ssid_only
       KTAP version 1
       # Subtest: test_inform_bss_ml_sta
   # test_inform_bss_ml_sta: EXPECTATION FAILED at net/wireless/tests/scan.c:592
   Expected ies->len == 6 + 2 + sizeof(rnr) + 2 + 155 +
mle_basic_common_info.var_len + 5, but
       ies->len == 185 (0xb9)
       6 + 2 + sizeof(rnr) + 2 + 155 + mle_basic_common_info.var_len +
5 == 203 (0xcb)
       not ok 1 no_mld_id
   # test_inform_bss_ml_sta: EXPECTATION FAILED at net/wireless/tests/scan.c:588
   Expected ies->len == 6 + 2 + sizeof(rnr) + 2 + 160 + 2 + 165 +
mle_basic_common_info.var_len + 5, but
       ies->len == 357 (0x165)
       6 + 2 + sizeof(rnr) + 2 + 160 + 2 + 165 +
mle_basic_common_info.var_len + 5 == 376 (0x178)
       not ok 2 mld_id_eq_1
   # test_inform_bss_ml_sta: pass:0 fail:2 skip:0 total:2
   not ok 2 test_inform_bss_ml_sta
# cfg80211-inform-bss: pass:1 fail:1 skip:0 total:2
# Totals: pass:1 fail:2 skip:0 total:3
not ok 1 cfg80211-inform-bss
---

If the failures are because of the missing 'regulatory.db' file, would
it make more sense to have that SKIP the tests instead? (And, if you
actually want to check that it loads correctly, have that be its own,
separate test?)

> > I
> > haven't had a chance to review them properly yet; the initial glance I
> > had didn't show any serious issues (though I think checkpatch
> > suggested some things to 'check').
>
> I can check.

Yeah, it mostly looks like really minor style 'suggestions' around
indenting and putting blank lines in, along with a couple of "you're
reusing a value in a macro, double check this" ones.. I'll paste them
below (but warning, they're a bit verbose).

CHECK: Please use a blank line after function/struct/union/enum declarations
#1142: FILE: net/wireless/tests/scan.c:225:
+};
+KUNIT_ARRAY_PARAM_DESC(gen_new_ie, gen_new_ie_cases, desc)

CHECK: Please use a blank line after function/struct/union/enum declarations
#1330: FILE: net/wireless/tests/scan.c:413:
+};
+KUNIT_ARRAY_PARAM_DESC(inform_bss_ml_sta, inform_bss_ml_sta_cases, desc)

CHECK: Alignment should match open parenthesis
#1489: FILE: net/wireless/tests/scan.c:572:
+       KUNIT_EXPECT_EQ(test, link_bss->beacon_interval,
+                             le16_to_cpu(sta_prof.beacon_int));

CHECK: Alignment should match open parenthesis
#1491: FILE: net/wireless/tests/scan.c:574:
+       KUNIT_EXPECT_EQ(test, link_bss->capability,
+                             le16_to_cpu(sta_prof.capabilities));

CHECK: Macro argument reuse '_freq' - possible side-effects?
#1620: FILE: net/wireless/tests/util.h:10:
+#define CHAN2G(_freq)  { \
+       .band = NL80211_BAND_2GHZ, \
+       .center_freq = (_freq), \
+       .hw_value = (_freq), \
+}

CHECK: Macro argument reuse 'test' - possible side-effects?
#1653: FILE: net/wireless/tests/util.h:43:
+#define T_WIPHY(test, ctx) ({                                          \
+               struct wiphy *__wiphy =                                 \
+                       kunit_alloc_resource(test, t_wiphy_init,        \
+                                            t_wiphy_exit,              \
+                                            GFP_KERNEL, &(ctx));       \
+                                                                       \
+               KUNIT_ASSERT_NOT_NULL(test, __wiphy);                   \
+               __wiphy;                                                \
+       })

>
> > So (once those small issues are finished), I'm okay with the first two
> > patches going in via either tree. The remaining ones are probably best
> > done via the wireless tree, as they seem to depend on some existing
> > patches there, so maybe it makes sense to push everything via
> > wireless.
>
> If not through wireless I doubt we'll get it synchronized for 6.8,
> though of course it's also not needed for 6.8 to have the extra unit
> tests :)
>
> I'll let Shuah decide.
>

I think you should be able to rebase on top of the kunit tree if Shuah
prefers that -- it's a reasonably straightforward conflict. But I
think we'd want to make sure that the various issues above are fixed
(and I'd not want the tests to fail out-of-the-box on the kunit.py UML
setup, though having them depend on !UML or 'SKIP' should be fine).

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]