Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 2 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] This patch breaks `kunit.py run --alltests` and drops the MCTP
core KUnit suite from all-tests CI runs.
- [Medium] mctp: core test suite disabled on minimal configs due to
hidden Kconfig dependency
--
net: mctp: Make KUnit tests depend on flow support
This patch changes MCTP_TEST so that it depends on MCTP_FLOWS instead of
selecting it. The goal is to stop the tests from silently turning on
SKB_EXTENSIONS. The suite is then available only when something else,
such as the I2C transport, enables flow support.
> diff --git a/net/mctp/Kconfig b/net/mctp/Kconfig
> index d8d3413a37f7e..e64d4f7ef6aaf 100644
> --- a/net/mctp/Kconfig
> +++ b/net/mctp/Kconfig
> @@ -14,7 +14,7 @@ menuconfig MCTP
>
> config MCTP_TEST
> bool "MCTP core tests" if !KUNIT_ALL_TESTS
> - select MCTP_FLOWS
> + depends on MCTP_FLOWS
> depends on MCTP=y && KUNIT=y
> default KUNIT_ALL_TESTS
[Severity: Medium]
Does this make the whole MCTP core suite unreachable on minimal KUnit
configs?
MCTP_FLOWS is a bool with no prompt, so it can only be enabled through
a select. After this change, the only way to satisfy the new dependency
is to pull in a transport that selects it. Today that means I2C,
I2C_SLAVE and MCTP_TRANSPORT_I2C.
A .kunitconfig that asks only for the core pieces, something like:
CONFIG_KUNIT=y
CONFIG_NET=y
CONFIG_MCTP=y
CONFIG_MCTP_TEST=y
now resolves MCTP_TEST to n. KunitConfig.validate_config() would then
reject the run, because CONFIG_MCTP_TEST=y never reaches the generated
.config. In menuconfig the "MCTP core tests" prompt simply disappears,
with no hint that an I2C slave transport is needed to get it back.
The commit message says the suite is available only when flow support
is enabled. That undersells the cost. The gate is not only on the two
flow tests but on every routing and socket case in route-test.c and
sock-test.c. Most of those have nothing to do with flows. The configs
that lose them are exactly the small ones developers are most likely
to use for quick KUnit runs.
The patch also leaves dead code behind. route-test.c still has the
!CONFIG_MCTP_FLOWS stubs, such as mctp_test_packet_flow(), which only
call kunit_skip(test, "Requires CONFIG_MCTP_FLOWS=y"). With MCTP_TEST
now depending on MCTP_FLOWS, those branches can never be compiled.
Which direction do you and Jeremy prefer here? Some options:
- Keep the core suite independent of MCTP_FLOWS and let the existing
skip stubs report the flow cases as skipped.
- Move the flow cases into a separate suite that carries the
dependency.
- Provide a net/mctp/.kunitconfig that enables the I2C transport
bits so the suite is still easy to run.
If the dependency stays as written, should the unreachable kunit_skip()
stubs be removed in the same patch?
[Severity: Medium]
Will this break kunit.py run --alltests?
tools/testing/kunit/configs/all_tests.config still contains:
CONFIG_MCTP=y
CONFIG_MCTP_FLOWS=y
That line came in with commit 109a5331143d. It only took effect because
MCTP_TEST, enabled via KUNIT_ALL_TESTS=y, selected MCTP_FLOWS.
MCTP_FLOWS has no prompt:
config MCTP_FLOWS
bool
depends on MCTP
select SKB_EXTENSIONS
so a direct assignment in a config fragment is ignored unless something
selects the symbol.
After this patch, the only selector left in the tree is
MCTP_TRANSPORT_I2C in drivers/net/mctp/Kconfig:
depends on I2C_SLAVE
select MCTP_FLOWS
all_tests.config enables neither I2C_SLAVE nor MCTP_TRANSPORT_I2C, so
MCTP_FLOWS resolves to n.
KunitConfig.validate_config() in tools/testing/kunit/kunit_kernel.py
would then find CONFIG_MCTP_FLOWS=y missing from the generated .config.
It would report "Not all Kconfig options selected in kunitconfig were in
the generated .config" and return False. build_config() then fails, so
the whole --alltests run would stop at the config step.
Even if validation were skipped, the new "depends on MCTP_FLOWS" would
make MCTP_TEST resolve to n there. That drops the whole MCTP core suite
(sock-test.c and route-test.c) from all-tests coverage.
Given that, is this statement in the commit message accurate for the
in-tree all-tests config?
This keeps the full flow-test coverage: the suite is available only
when flow support is enabled, for example by the I2C transport.
Could all_tests.config be updated in the same patch? For example, it
could add CONFIG_I2C_SLAVE=y and CONFIG_MCTP_TRANSPORT_I2C=y
(CONFIG_I2C=y is already set there). At minimum, the line that can no
longer be satisfied could be removed.
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928205352.30502-1-kmehltretter%40gmail.com
