by Shuah Khan

On Fri, 22 Dec 2023 at 18:09, Johannes Berg <[email protected]> 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

Attachments:

smime.p7s (3.91 kB)
S/MIME Cryptographic Signature

2023-12-22 15:15:14

by Benjamin Berg

[permalink] [raw]

Subject: Re: [PATCH 1/6] kunit: add parameter generation macro using description from array

On Fri, 2023-12-22 at 18:02 +0800, David Gow wrote:
> On Wed, 20 Dec 2023 at 23:20, <[email protected]> wrote:
> >
> > From: Benjamin Berg <[email protected]>
> >
> > The existing KUNIT_ARRAY_PARAM macro requires a separate function
> > to
> > get the description. However, in a lot of cases the description can
> > just be copied directly from the array. Add a second macro that
> > avoids having to write a static function just for a single strscpy.
> >
> > Signed-off-by: Benjamin Berg <[email protected]>
> > ---
>
> I'm generally pretty happy with this, though note the checkpatch
> warning below.
>
> There was some discussion at plumbers about expanding the
> parameterised test APIs, so we may need to adjust the implementation
> of this down the line, but I don't think that'll happen for a while,
> so don't worry.
>
> With the warnings fixed, this is:

I think the checkpatch warning is a false positive. It seems to confuse
the * as a multiplication due to typeof() looking like a function call
rather than a variable declaration.

Benjamin

> Reviewed-by: David Gow <[email protected]>
>
> I'm okay with this going in via the wireless tree if that's easier;
> certainly there are some conflicts with the later patches in this
> series and the kunit one.
>
> Cheers,
> -- David
>
> > Documentation/dev-tools/kunit/usage.rst | 12 ++++--------
> > include/kunit/test.h                    | 19 +++++++++++++++++++
> > 2 files changed, 23 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/dev-tools/kunit/usage.rst
> > b/Documentation/dev-tools/kunit/usage.rst
> > index c27e1646ecd9..b959e5befcbe 100644
> > --- a/Documentation/dev-tools/kunit/usage.rst
> > +++ b/Documentation/dev-tools/kunit/usage.rst
> > @@ -566,13 +566,9 @@ By reusing the same ``cases`` array from
> > above, we can write the test as a
> >                 },
> >         };
> >
> > -       // Need a helper function to generate a name for each test
> > case.
> > -       static void case_to_desc(const struct sha1_test_case *t,
> > char *desc)
> > -       {
> > -               strcpy(desc, t->str);
> > -       }
> > -       // Creates `sha1_gen_params()` to iterate over `cases`.
> > -       KUNIT_ARRAY_PARAM(sha1, cases, case_to_desc);
> > +       // Creates `sha1_gen_params()` to iterate over `cases`
> > while using
> > +       // the struct member `str` for the case description.
> > +       KUNIT_ARRAY_PARAM_DESC(sha1, cases, str);
> >
> >         // Looks no different from a normal test.
> >         static void sha1_test(struct kunit *test)
> > @@ -588,7 +584,7 @@ By reusing the same ``cases`` array from above,
> > we can write the test as a
> >         }
> >
> >         // Instead of KUNIT_CASE, we use KUNIT_CASE_PARAM and pass
> > in the
> > -       // function declared by KUNIT_ARRAY_PARAM.
> > +       // function declared by KUNIT_ARRAY_PARAM or
> > KUNIT_ARRAY_PARAM_DESC.
> >         static struct kunit_case sha1_test_cases[] = {
> >                 KUNIT_CASE_PARAM(sha1_test, sha1_gen_params),
> >                 {}
> > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > index 20ed9f9275c9..2dfa851e1f88 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -1514,6 +1514,25 @@ do
> > {
> >           \
> >                 return
> > NULL;
> >       \
> >         }
> >
> > +/**
> > + * KUNIT_ARRAY_PARAM_DESC() - Define test parameter generator from
> > an array.
> > + * @name: prefix for the test parameter generator function.
> > + * @array: array of test parameters.
> > + * @desc_member: structure member from array element to use as
> > description
> > + *
> > + * Define function @name_gen_params which uses @array to generate
> > parameters.
> > + */
> > +#define KUNIT_ARRAY_PARAM_DESC(name, array,
> > desc_member)                                       \
> > +       static const void *name##_gen_params(const void *prev, char
> > *desc)                      \
> > +
> > {
> >                      \
> > +               typeof((array)[0]) *__next = prev ?
> > ((typeof(__next)) prev) + 1 : (array);      \
>
> checkpatch is complaining here:
> ERROR: need consistent spacing around '*' (ctx:WxV)
> #71: FILE: include/kunit/test.h:1528:
>
> +               typeof((array)[0]) *__next = prev ? ((typeof(__next))
> prev) + 1 : (array);      \
>
> > +               if (__next - (array) < ARRAY_SIZE((array)))
> > {                                   \
> > +                       strscpy(desc, __next->desc_member,
> > KUNIT_PARAM_DESC_SIZE);              \
> > +                       return
> > __next;                                                          \
> > +
> > }
> >              \
> > +               return
> > NULL;
> >       \
> > +       }
> > +
> > // TODO([email protected]): consider eventually migrating users
> > to explicitly
> > // include resource.h themselves if they need it.
> > #include <kunit/resource.h>
> > --
> > 2.43.0
> >
> > --
> > You received this message because you are subscribed to the Google
> > Groups "KUnit Development" group.
> > To unsubscribe from this group and stop receiving emails from it,
> > send an email to [email protected].
> > To view this discussion on the web visit
> > https://groups.google.com/d/msgid/kunit-dev/20231220151952.415232-2-benjamin%40sipsolutions.net
> > .

2023-12-22 15:45:02

by Shuah Khan

[permalink] [raw]

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

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 <[email protected]>

thanks,
-- Shuah

2023-12-23 05:09:22

by David Gow

[permalink] [raw]

Subject: Re: [PATCH 1/6] kunit: add parameter generation macro using description from array

On Fri, 22 Dec 2023 at 23:09, Benjamin Berg <[email protected]> wrote:
>
> On Fri, 2023-12-22 at 18:02 +0800, David Gow wrote:
> > On Wed, 20 Dec 2023 at 23:20, <[email protected]> wrote:
> > >
> > > From: Benjamin Berg <[email protected]>
> > >
> > > The existing KUNIT_ARRAY_PARAM macro requires a separate function
> > > to
> > > get the description. However, in a lot of cases the description can
> > > just be copied directly from the array. Add a second macro that
> > > avoids having to write a static function just for a single strscpy.
> > >
> > > Signed-off-by: Benjamin Berg <[email protected]>
> > > ---
> >
> > I'm generally pretty happy with this, though note the checkpatch
> > warning below.
> >
> > There was some discussion at plumbers about expanding the
> > parameterised test APIs, so we may need to adjust the implementation
> > of this down the line, but I don't think that'll happen for a while,
> > so don't worry.
> >
> > With the warnings fixed, this is:
>
> I think the checkpatch warning is a false positive. It seems to confuse
> the * as a multiplication due to typeof() looking like a function call
> rather than a variable declaration.
>
> Benjamin
>

Ah, yeah: this appears to be due to checkpatch not handling nested ()
within a typeof().

Reviewed-by: David Gow <[email protected]>

Cheers,
-- David

Attachments:

smime.p7s (3.91 kB)
S/MIME Cryptographic Signature