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