| Message ID | 20231220151952.415232-1-benjamin@sipsolutions.net |
|---|---|
| Headers | show |
| Series | Add some more cfg80211 and mac80211 kunit tests | expand |
> > This patchset adds a couple of helpers for kunit as well as tests for > cfg80211 and mac80211 that use them. I can take this through the wireless tree, but then I'd like to have ACKs from kunit folks for the kunit patches: > kunit: add parameter generation macro using description from array > kunit: add a convenience allocation wrapper for SKBs > Thanks, johannes
On 12/21/23 12:39, Johannes Berg wrote: >> >> This patchset adds a couple of helpers for kunit as well as tests for >> cfg80211 and mac80211 that use them. > > I can take this through the wireless tree, but then I'd like to have > ACKs from kunit folks for the kunit patches: > We have run into conflicts in the past with the kunit tree. I take the kunit patches through linux-kselftest tree. I do want to make sure there are no conflicts. I don't mind taking these through my tree. >> kunit: add parameter generation macro using description from array >> kunit: add a convenience allocation wrapper for SKBs >> > Adding David and Brendan to the thread for their review. thanks, -- Shuah
On Thu, 2023-12-21 at 13:06 -0700, Shuah Khan wrote: > On 12/21/23 12:39, Johannes Berg wrote: > > > > > > This patchset adds a couple of helpers for kunit as well as tests for > > > cfg80211 and mac80211 that use them. > > > > I can take this through the wireless tree, but then I'd like to have > > ACKs from kunit folks for the kunit patches: > > > > We have run into conflicts in the past with the kunit tree. I take the > kunit patches through linux-kselftest tree. I do want to make sure there > are no conflicts. I don't mind taking these through my tree. OK, fair enough. If you can still put it into 6.8, then I think you can also take the wireless tests, assuming they pass (I haven't run them in the posted version). I don't think we'll have conflicts there, we don't have much work in wireless that's likely to land for 6.8. johannes
On 12/21/23 13:40, Johannes Berg wrote: > On Thu, 2023-12-21 at 13:06 -0700, Shuah Khan wrote: >> On 12/21/23 12:39, Johannes Berg wrote: >>>> >>>> This patchset adds a couple of helpers for kunit as well as tests for >>>> cfg80211 and mac80211 that use them. >>> >>> I can take this through the wireless tree, but then I'd like to have >>> ACKs from kunit folks for the kunit patches: >>> >> >> We have run into conflicts in the past with the kunit tree. I take the >> kunit patches through linux-kselftest tree. I do want to make sure there >> are no conflicts. I don't mind taking these through my tree. > > OK, fair enough. > > If you can still put it into 6.8, then I think you can also take the > wireless tests, assuming they pass (I haven't run them in the posted > version). I don't think we'll have conflicts there, we don't have much > work in wireless that's likely to land for 6.8. > Sounds good. David, will you be able to look at these patches and let me know if I can apply for Linux 6.8-rc1. thanks, -- Shuah
On Fri, 22 Dec 2023 at 05:47, Shuah Khan <skhan@linuxfoundation.org> wrote: > > On 12/21/23 13:40, Johannes Berg wrote: > > On Thu, 2023-12-21 at 13:06 -0700, Shuah Khan wrote: > >> On 12/21/23 12:39, Johannes Berg wrote: > >>>> > >>>> This patchset adds a couple of helpers for kunit as well as tests for > >>>> cfg80211 and mac80211 that use them. > >>> > >>> I can take this through the wireless tree, but then I'd like to have > >>> ACKs from kunit folks for the kunit patches: > >>> > >> > >> We have run into conflicts in the past with the kunit tree. I take the > >> kunit patches through linux-kselftest tree. I do want to make sure there > >> are no conflicts. I don't mind taking these through my tree. > > > > OK, fair enough. > > > > If you can still put it into 6.8, then I think you can also take the > > wireless tests, assuming they pass (I haven't run them in the posted > > version). I don't think we'll have conflicts there, we don't have much > > work in wireless that's likely to land for 6.8. > > > > Sounds good. > > David, will you be able to look at these patches and let me know if > I can apply for Linux 6.8-rc1. The two initial KUnit patches look fine, modulo a couple of minor docs issues and checkpatch warnings. 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. The remaining patches don't apply on top of the kunit branch as-is. 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'). 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. Cheers, -- David
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? > 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 > 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. > 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. Thanks! johannes
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
On 12/22/23 03:09, Johannes Berg 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? > >> 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 >> 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. > >> 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. > Thank you David for the reviews. johannes, Please take these through wireless - makes it easier for all of us. Acked-by: Shuah Khan <skhan@linuxfoundation.org> thanks, -- Shuah
From: Benjamin Berg <benjamin.berg@intel.com> This patchset adds a couple of helpers for kunit as well as tests for cfg80211 and mac80211 that use them. Benjamin Berg (3): kunit: add parameter generation macro using description from array kunit: add a convenience allocation wrapper for SKBs wifi: cfg80211: tests: add some scanning related tests Johannes Berg (3): wifi: mac80211: add kunit tests for public action handling wifi: mac80211: kunit: generalize public action test wifi: mac80211: kunit: extend MFP tests Documentation/dev-tools/kunit/usage.rst | 12 +- include/kunit/skbuff.h | 56 +++ include/kunit/test.h | 19 + net/mac80211/ieee80211_i.h | 10 + net/mac80211/rx.c | 4 +- net/mac80211/tests/Makefile | 2 +- net/mac80211/tests/mfp.c | 286 +++++++++++ net/wireless/core.h | 13 +- net/wireless/scan.c | 9 +- net/wireless/tests/Makefile | 2 +- net/wireless/tests/scan.c | 625 ++++++++++++++++++++++++ net/wireless/tests/util.c | 56 +++ net/wireless/tests/util.h | 66 +++ 13 files changed, 1145 insertions(+), 15 deletions(-) create mode 100644 include/kunit/skbuff.h create mode 100644 net/mac80211/tests/mfp.c create mode 100644 net/wireless/tests/scan.c create mode 100644 net/wireless/tests/util.c create mode 100644 net/wireless/tests/util.h