The protocol device drivers under drivers/platform/chrome/ are responsible
to communicate to the ChromeOS EC (Embedded Controller).  They need to pack
the data in a pre-defined format and check if the EC responds accordingly.

The series adds some fundamental unit tests for the protocol.  It calls the
.cmd_xfer() and .pkt_xfer() callbacks (which are the most crucial parts for
the protocol), mocks the rest of the system, and checks if the interactions
are all correct.

The series isn't ready for landing.  It's more like a PoC for the
binary-level function redirection and its use cases.

The 1st patch adds ftrace stub which is originally from [1][2].  There is no
follow-up discussion about the ftrace stub.  As a result, the patch is still
on the mailing list.

The 2nd patch adds Kunit tests for cros_ec_i2c.  It relies on the ftrace stub
for redirecting cros_ec_{un,}register().

The 3rd patch uses static stub instead (if ftrace stub isn't really an option).
However, I'm not a big fan to change the production code (i.e. adding the
prologue in cros_ec_{un,}register()) for testing.

The 4th patch adds Kunit tests for cros_ec_spi.  It relies on the ftrace stub
for redirecting cros_ec_{un,}register() again.

The 5th patch calls .probe() directly instead of forcing the driver probe
needs to be synchronous.  In comparison with the 4th patch, I don't think
this is simpler.  I'd prefer to the way in the 4th patch.

After talked to Masami about the work, he suggested to use Kprobes for
function redirection.  The 6th patch adds kprobes stub.

The 7th patch uses kprobes stub instead for cros_ec_spi.

Questions:
- Are we going to support ftrace stub so that tests can use it?

- If ftrace stub isn't on the plate (e.g. due to too many dependencies), how
  about the kprobes stub?  Is it something we could pursue?

- (minor) I'm unsure if people would prefer 'kprobes stub' vs. 'kprobe stub'.

[1]: https://kunit.dev/mocking.html#binary-level-ftrace-et-al
[2]: https://lore.kernel.org/linux-kselftest/20220318021314.3225240-3-davidgow@google.com/

Daniel Latypov (1):
  kunit: expose ftrace-based API for stubbing out functions during tests

Tzung-Bi Shih (6):
  platform/chrome: kunit: cros_ec_i2c: Add tests with ftrace stub
  platform/chrome: kunit: cros_ec_i2c: Use static stub instead
  platform/chrome: kunit: cros_ec_spi: Add tests with ftrace stub
  platform/chrome: kunit: cros_ec_spi: Call .probe() directly
  kunit: Expose 'kprobes stub' API to redirect functions
  platform/chrome: kunit: cros_ec_spi: Use kprobes stub instead

 drivers/platform/chrome/Kconfig            |  17 +
 drivers/platform/chrome/Makefile           |   2 +
 drivers/platform/chrome/cros_ec.c          |   6 +
 drivers/platform/chrome/cros_ec_i2c_test.c | 479 +++++++++++++++++++++
 drivers/platform/chrome/cros_ec_spi_test.c | 355 +++++++++++++++
 include/kunit/ftrace_stub.h                |  84 ++++
 include/kunit/kprobes_stub.h               |  19 +
 kernel/trace/ftrace.c                      |   3 +
 lib/kunit/Kconfig                          |  18 +
 lib/kunit/Makefile                         |   8 +
 lib/kunit/ftrace_stub.c                    | 139 ++++++
 lib/kunit/kprobes_stub.c                   | 113 +++++
 lib/kunit/kunit-example-test.c             |  27 +-
 lib/kunit/stubs_example.kunitconfig        |  11 +
 14 files changed, 1280 insertions(+), 1 deletion(-)
 create mode 100644 drivers/platform/chrome/cros_ec_i2c_test.c
 create mode 100644 drivers/platform/chrome/cros_ec_spi_test.c
 create mode 100644 include/kunit/ftrace_stub.h
 create mode 100644 include/kunit/kprobes_stub.h
 create mode 100644 lib/kunit/ftrace_stub.c
 create mode 100644 lib/kunit/kprobes_stub.c
 create mode 100644 lib/kunit/stubs_example.kunitconfig

-- 
2.49.0.1101.gccaa498523-goog

On Tue, May 20, 2025 at 1:25 AM 'Tzung-Bi Shih' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> The protocol device drivers under drivers/platform/chrome/ are responsible
> to communicate to the ChromeOS EC (Embedded Controller).  They need to pack
> the data in a pre-defined format and check if the EC responds accordingly.
>
> The series adds some fundamental unit tests for the protocol.  It calls the
> .cmd_xfer() and .pkt_xfer() callbacks (which are the most crucial parts for
> the protocol), mocks the rest of the system, and checks if the interactions
> are all correct.
>
> The series isn't ready for landing.  It's more like a PoC for the
> binary-level function redirection and its use cases.
>
> The 1st patch adds ftrace stub which is originally from [1][2].  There is no
> follow-up discussion about the ftrace stub.  As a result, the patch is still
> on the mailing list.
>
> The 2nd patch adds Kunit tests for cros_ec_i2c.  It relies on the ftrace stub
> for redirecting cros_ec_{un,}register().
>
> The 3rd patch uses static stub instead (if ftrace stub isn't really an option).
> However, I'm not a big fan to change the production code (i.e. adding the
> prologue in cros_ec_{un,}register()) for testing.
>
> The 4th patch adds Kunit tests for cros_ec_spi.  It relies on the ftrace stub
> for redirecting cros_ec_{un,}register() again.
>
> The 5th patch calls .probe() directly instead of forcing the driver probe
> needs to be synchronous.  In comparison with the 4th patch, I don't think
> this is simpler.  I'd prefer to the way in the 4th patch.
>
> After talked to Masami about the work, he suggested to use Kprobes for
> function redirection.  The 6th patch adds kprobes stub.
>
> The 7th patch uses kprobes stub instead for cros_ec_spi.
>
> Questions:
> - Are we going to support ftrace stub so that tests can use it?
>
> - If ftrace stub isn't on the plate (e.g. due to too many dependencies), how
>   about the kprobes stub?  Is it something we could pursue?

Quick comment,
If I recall, the thought process was that we could consider it in the
future if there was enough demand for it.

We have these drawbacks with the current ftrace stubs:
* doesn't compile on all arches
* silently doesn't work on inlined functions <== scariest one to me
* is more complicated and has more dependencies

So it felt like the better move to go with static stubs which has none
of those drawbacks (works on all arches, all functions, and is dead
simple) as opposed to simultaneously introducing two ways to do the
same thing.

You mention you don't like how static stubs requires modifying the
code-under-test.
Since it gets eliminated by the preprocessor unless you're compiling
for KUnit, is the concern more so about how it conceptually feels
wrong to do so?
For the Android GKI kernel, they have (or had) KUnit enabled so there
is potentially concern about real runtime cost there, not sure if you
have something similar in mind.

But stepping back, ftrace_stubs technically require modifying the code
to make sure funcs are marked as `noinline`, which this patch series
does not do.
I've not looked at cros_ec_{un,}register() to check if they're at risk
of inlining, but wanted to call that out, that ftrace stubs
technically don't handle your usage pattern 100% properly.

>
> - (minor) I'm unsure if people would prefer 'kprobes stub' vs. 'kprobe stub'.
>

I'd personally vote for kprobe_stub.

Daniel

On Tue, May 20, 2025 at 09:04:53AM -0700, Daniel Latypov wrote:
> On Tue, May 20, 2025 at 1:25 AM 'Tzung-Bi Shih' via KUnit Development
> <kunit-dev@googlegroups.com> wrote:
> > [...]
> > Questions:
> > - Are we going to support ftrace stub so that tests can use it?
> >
> > - If ftrace stub isn't on the plate (e.g. due to too many dependencies), how
> >   about the kprobes stub?  Is it something we could pursue?
> 
> Quick comment,
> If I recall, the thought process was that we could consider it in the
> future if there was enough demand for it.

Sorry for the late reply. I was doing some more experiments and stuck by
some else.

> We have these drawbacks with the current ftrace stubs:
> * doesn't compile on all arches
> * silently doesn't work on inlined functions <== scariest one to me

I see. I did some experiments and realized that kprobe stubs also share
the same concern. Thus I'm wondering if there is a way that kprobe stub
detects the redirection may fail, at least it can skip the test case
(e.g. register_kprobe() returns -ENOENT when it can't find the symbol
via kprobe_lookup_name()). But it seems no way if the target function
is partially inlined.

> You mention you don't like how static stubs requires modifying the
> code-under-test.
> Since it gets eliminated by the preprocessor unless you're compiling
> for KUnit, is the concern more so about how it conceptually feels
> wrong to do so?
> For the Android GKI kernel, they have (or had) KUnit enabled so there
> is potentially concern about real runtime cost there, not sure if you
> have something similar in mind.

Not exactly. Ideally, I think we shouldn't modify the CUT. I'm wondering
if there is a way to not change the CUT but also break the external
dependencies.

> But stepping back, ftrace_stubs technically require modifying the code
> to make sure funcs are marked as `noinline`, which this patch series
> does not do.
> I've not looked at cros_ec_{un,}register() to check if they're at risk
> of inlining, but wanted to call that out, that ftrace stubs
> technically don't handle your usage pattern 100% properly.

True. They could be partially inlined even though they are exported symbols.

On Mon, Jun 30, 2025 at 3:32 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> On Tue, May 20, 2025 at 09:04:53AM -0700, Daniel Latypov wrote:

(snip)

>
> > We have these drawbacks with the current ftrace stubs:
> > * doesn't compile on all arches
> > * silently doesn't work on inlined functions <== scariest one to me
>
> I see. I did some experiments and realized that kprobe stubs also share
> the same concern. Thus I'm wondering if there is a way that kprobe stub
> detects the redirection may fail, at least it can skip the test case
> (e.g. register_kprobe() returns -ENOENT when it can't find the symbol
> via kprobe_lookup_name()). But it seems no way if the target function
> is partially inlined.

Yeah, any such approach will need to be cautious of inlining, so
they'd have to always be "for power users only: only use this if you
understand the risk"
Skipping when we can detect the user is obviously doing the wrong
thing makes that a *bit* more palatable.

It's not like static stubs can detect at runtime if you've failed to
add the necessary corresponding KUNIT_STATIC_STUB_REDIRECT() either,
for example.

But I still personally lean slightly against adding either kprobe or
ftrace stubs, see below.

>
> > You mention you don't like how static stubs requires modifying the
> > code-under-test.
> > Since it gets eliminated by the preprocessor unless you're compiling
> > for KUnit, is the concern more so about how it conceptually feels
> > wrong to do so?
> > For the Android GKI kernel, they have (or had) KUnit enabled so there
> > is potentially concern about real runtime cost there, not sure if you
> > have something similar in mind.
>
> Not exactly. Ideally, I think we shouldn't modify the CUT. I'm wondering
> if there is a way to not change the CUT but also break the external
> dependencies.
>
> > But stepping back, ftrace_stubs technically require modifying the code
> > to make sure funcs are marked as `noinline`, which this patch series
> > does not do.
...
> They could be partially inlined even though they are exported symbols.

So to summarize, right now we're stuck with having to modify the code.
(Unless someone can come up with something really clever, but not too clever)

To make it concrete, the current approach would look like:

int func(char* arg1, int arg2) {
  KUNIT_STATIC_STUB_REDIRECT(func, arg1, arg2);
  ... // unchanged
}

vs an ftrace/kprobe approach that needs a conditional `noinline`

KUNIT_STUBBABLE int func(char* arg1, int arg2) {
  ... // unchanged
}

The latter is definitely simpler and less burdensome.
But I don't know if it's simpler enough to warrant a second
implementation existing for me personally.

E.g. we already have some people who justifiably say it's too hard to
figure out KUnit, so this is another decision point where a user might
get stuck with "how should I know which one I should use?" and give
up.

Compatibility tangent:
A smaller annoyance is KUNIT_STATIC_STUB_REDIRECT and KUNIT_STUBBABLE
are incompatible (and always will be?)

E.g. imagine a func has KUNIT_STUBBABLE on it, but a person authoring
a new test wants needs to run without kprobe support, so they must add
KUNIT_STATIC_STUB_REDIRECT.
I can imagine an author deciding to make the func have *both* macros on it...
That feels like a worst case outcome from the perspective of "we
shouldn't modify the CUT just for the sake of tests" :\

To be clear, the right approach in this scenario is to 1) swap to
KUNIT_STATIC_STUB_REDIRECT and update the previous tests to use static
stubs, 2) then add your new tests.
But I can imagine that won't always happen, esp. if it crosses
maintainer boundaries.

On Tue, Jul 01, 2025 at 02:22:40PM +0900, Daniel Latypov wrote:
> On Mon, Jun 30, 2025 at 3:32 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> > On Tue, May 20, 2025 at 09:04:53AM -0700, Daniel Latypov wrote:
> > > You mention you don't like how static stubs requires modifying the
> > > code-under-test.
> > > Since it gets eliminated by the preprocessor unless you're compiling
> > > for KUnit, is the concern more so about how it conceptually feels
> > > wrong to do so?
> > > For the Android GKI kernel, they have (or had) KUnit enabled so there
> > > is potentially concern about real runtime cost there, not sure if you
> > > have something similar in mind.
> >
> > Not exactly. Ideally, I think we shouldn't modify the CUT. I'm wondering
> > if there is a way to not change the CUT but also break the external
> > dependencies.
> >
> > > But stepping back, ftrace_stubs technically require modifying the code
> > > to make sure funcs are marked as `noinline`, which this patch series
> > > does not do.
> ...
> > They could be partially inlined even though they are exported symbols.
> 
> So to summarize, right now we're stuck with having to modify the code.
> (Unless someone can come up with something really clever, but not too clever)
> 
> To make it concrete, the current approach would look like:
> 
> int func(char* arg1, int arg2) {
>   KUNIT_STATIC_STUB_REDIRECT(func, arg1, arg2);
>   ... // unchanged
> }
> 
> vs an ftrace/kprobe approach that needs a conditional `noinline`
> 
> KUNIT_STUBBABLE int func(char* arg1, int arg2) {
>   ... // unchanged
> }
> 
> The latter is definitely simpler and less burdensome.
> But I don't know if it's simpler enough to warrant a second
> implementation existing for me personally.

Instead of KUNIT_STUBBABLE macros, I was thinking of:

diff --git a/Makefile b/Makefile
index 35e6e5240c61..40319083f58b 100644
--- a/Makefile
+++ b/Makefile
@@ -979,6 +979,10 @@ ifdef CONFIG_DEBUG_SECTION_MISMATCH
 KBUILD_CFLAGS += -fno-inline-functions-called-once
 endif
 
+ifdef CONFIG_KUNIT_KPROBE_STUBS
+KBUILD_CFLAGS += -fno-inline
+endif
+
 # `rustc`'s `-Zfunction-sections` applies to data too (as of 1.59.0).
 ifdef CONFIG_LD_DEAD_CODE_DATA_ELIMINATION
 KBUILD_CFLAGS_KERNEL += -ffunction-sections -fdata-sections


I don't know what are most people's usages. I always run KUnit tests in qemu
or at least in some real devices that I less care about the performance.
Thus, turning inline off globally in such environments is totally acceptable.