Re: [PATCH v2] coresight: Add a KUnit test for coresight_find_default_sink()

[Suzuki K Poulose]

On 11/03/2025 16:06, James Clark wrote:
> On 11/03/2025 3:00 pm, Suzuki K Poulose wrote:
> > On 05/03/2025 15:07, James Clark wrote:
> > > Add a test to confirm that default sink selection skips over an ETF
> > > and returns an ETR even if it's further away.
> > >
> > > This also makes it easier to add new unit tests in the future.
> > >
> > > Signed-off-by: James Clark <james.clark@xxxxxxxxxx>
> > > ---
> > > Changes in v2:
> > > - Let devm free everything rather than doing individual kfrees:
> > >    "Like with managed drivers, KUnit-managed fake devices are
> > >    automatically cleaned up when the test finishes, but can be manually
> > >    cleaned up early with kunit_device_unregister()."
> > > - Link to v1: https://lore.kernel.org/r/20250225164639.522741-1- 
> > > james.clark@xxxxxxxxxx
> > > ---
> > >   drivers/hwtracing/coresight/Kconfig                |  9 +++
> > >   drivers/hwtracing/coresight/Makefile               |  3 +-
> > >   drivers/hwtracing/coresight/coresight-core.c       |  1 +
> > >   .../hwtracing/coresight/coresight-kunit-tests.c    | 77 +++++++++++ 
> > > + ++++++++++
> > >   4 files changed, 88 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/ 
> > > coresight/Kconfig
> > > index ecd7086a5b83..f064e3d172b3 100644
> > > --- a/drivers/hwtracing/coresight/Kconfig
> > > +++ b/drivers/hwtracing/coresight/Kconfig
> > > @@ -259,4 +259,13 @@ config CORESIGHT_DUMMY
> > >         To compile this driver as a module, choose M here: the module 
> > > will be
> > >         called coresight-dummy.
> > > +
> > > +config CORESIGHT_KUNIT_TESTS
> > > +      tristate "Enable Coresight unit tests"
> > > +      depends on KUNIT
> > > +      default KUNIT_ALL_TESTS
> > > +      help
> > > +        Enable Coresight unit tests. Only useful for development and 
> > > not
> > > +        intended for production.
> > > +
> > >   endif
> > > diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/ 
> > > hwtracing/ coresight/Makefile
> > > index 8e62c3150aeb..96f0dfedb1bf 100644
> > > --- a/drivers/hwtracing/coresight/Makefile
> > > +++ b/drivers/hwtracing/coresight/Makefile
> > > @@ -51,5 +51,4 @@ coresight-cti-y := coresight-cti-core.o coresight- 
> > > cti-platform.o \
> > >              coresight-cti-sysfs.o
> > >   obj-$(CONFIG_ULTRASOC_SMB) += ultrasoc-smb.o
> > >   obj-$(CONFIG_CORESIGHT_DUMMY) += coresight-dummy.o
> > > -obj-$(CONFIG_CORESIGHT_CTCU) += coresight-ctcu.o
> > > -coresight-ctcu-y := coresight-ctcu-core.o
> > > +obj-$(CONFIG_CORESIGHT_KUNIT_TESTS) += coresight-kunit-tests.o
> > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/ 
> > > hwtracing/coresight/coresight-core.c
> > > index bd0a7edd38c9..b101aa133ceb 100644
> > > --- a/drivers/hwtracing/coresight/coresight-core.c
> > > +++ b/drivers/hwtracing/coresight/coresight-core.c
> > > @@ -959,6 +959,7 @@ coresight_find_default_sink(struct 
> > > coresight_device *csdev)
> > >       }
> > >       return csdev->def_sink;
> > >   }
> > > +EXPORT_SYMBOL_GPL(coresight_find_default_sink);
> > >   static int coresight_remove_sink_ref(struct device *dev, void *data)
> > >   {
> > > diff --git a/drivers/hwtracing/coresight/coresight-kunit-tests.c b/ 
> > > drivers/hwtracing/coresight/coresight-kunit-tests.c
> > > new file mode 100644
> > > index 000000000000..a136af05eaf4
> > > --- /dev/null
> > > +++ b/drivers/hwtracing/coresight/coresight-kunit-tests.c
> > > @@ -0,0 +1,77 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +#include <kunit/test.h>
> > > +#include <kunit/device.h>
> > > +#include <linux/coresight.h>
> > > +
> > > +#include "coresight-priv.h"
> > > +
> > > +static struct coresight_device *coresight_test_device(struct device 
> > > *dev)
> > > +{
> > > +    struct coresight_device *csdev = devm_kcalloc(dev, 1,
> > > +                             sizeof(struct coresight_device),
> > > +                             GFP_KERNEL);
> > > +    csdev->pdata = devm_kcalloc(dev, 1,
> > > +                   sizeof(struct coresight_platform_data),
> > > +                   GFP_KERNEL);
> > > +    return csdev;
> > > +}
> > > +
> > > +static int coresight_test_cpuid(struct coresight_device *csdev)
> > > +{
> > > +    return 0;
> > > +}
> > > +
> > > +static void test_default_sink(struct kunit *test)
> > > +{
> > > +    /*
> > > +     * ETM -> ETF -> ETR -> CATU
> > > +     *                ^
> > > +     *                | default
> > > +     */
> > > +    struct device *dev = kunit_device_register(test, 
> > > "coresight_kunit");
> > > +    struct coresight_device *etm = coresight_test_device(dev),
> > > +                *etf = coresight_test_device(dev),
> > > +                *etr = coresight_test_device(dev),
> > > +                *catu = coresight_test_device(dev);
> > > +    struct coresight_connection conn = {};
> > > +    struct coresight_ops_source src_ops = {.cpu_id = 
> > > coresight_test_cpuid };
> >
> > Do we need the trace_id() ? Why is this required ?
> >
> > Suzuki
>
> Because it tests with per-cpu sources so it checks if any of the TRBE 
> sinks have been assigned first:
>
>   /* look for a default sink if we have not found for this device */
>     if (!csdev->def_sink) {
>        if (coresight_is_percpu_source(csdev))
>       csdev->def_sink = per_cpu(csdev_sink,
>                                     source_ops(csdev)->cpu_id(csdev));
>
> I think this test would probably fail if run somewhere TRBE was already 
> probed. I can make the source subtype to be 
> CORESIGHT_DEV_SUBTYPE_SOURCE_BUS for the test to work around it. Any 
> value other than CORESIGHT_DEV_SUBTYPE_SOURCE_PROC will work. And then I 
> can drop the fake cpu_id() callback.

If the purpose is to detect the ETR, then I guess we could make this a 
non-ETM source

TBH, I don't see any value in the test as such. But it is a good 
starting point for adding further cases. So, lets keep this in.

Suzuki

>