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

[James Clark <james.clark@linaro.org>]

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@arm.com>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>> 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@linaro.org
>>
>> 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@linaro.org
>> ---
>>   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.