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

[James Clark]

On 12/03/2025 10:14 am, Suzuki K Poulose wrote:
> Hi James
>
>
> On 12/03/2025 10:11, 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.
> >
> > Reviewed-by: Leo Yan <leo.yan@xxxxxxx>
> > Signed-off-by: James Clark <james.clark@xxxxxxxxxx>
> > ---
> > Changes in v3:
> > - Use CORESIGHT_DEV_SUBTYPE_SOURCE_BUS type instead of the default
> >    (CORESIGHT_DEV_SUBTYPE_SOURCE_PROC) so that the test still works even
> >    when TRBE sinks are registered. This also removes the need for the
> >    fake CPU ID callback.
> > - Link to v2: https://lore.kernel.org/r/20250305-james-cs-kunit-test- 
> > v2-1-83ba682b976c@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..87439769207c
> > --- /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 void test_default_sink(struct kunit *test)
> > +{
> > +    /*
> > +     * ETM -> ETF -> ETR -> CATU
> > +     *                ^
> > +     *                | default
>
> minor nit: Using ETM here is going to be confusing, with the SUBTYPE we
> are using. Please could we rename it something else and also rename the 
> variable "etm" ? Say, "source" ?
>
> Rest looks good to me.
>
> Suzuki

Yeah good point, "Source" makes sense.