[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <919b1fa9-3b95-4be1-a966-9f0cfc166756@linaro.org>
Date: Mon, 17 Mar 2025 10:59:06 +0000
From: James Clark <james.clark@...aro.org>
To: Anshuman Khandual <anshuman.khandual@....com>
Cc: linux-kernel@...r.kernel.org, coresight@...ts.linaro.org,
linux-arm-kernel@...ts.infradead.org,
Suzuki K Poulose <suzuki.poulose@....com>, Mike Leach
<mike.leach@...aro.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Leo Yan <leo.yan@....com>
Subject: Re: [PATCH v4] coresight: Add a KUnit test for
coresight_find_default_sink()
On 14/03/2025 12:35 pm, Anshuman Khandual wrote:
> On 3/12/25 16:01, 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.
>
> The test overall looks good but there are some comments below.
>
>>
>> Reviewed-by: Leo Yan <leo.yan@....com>
>> Signed-off-by: James Clark <james.clark@...aro.org>
>> ---
>> Changes in v4:
>> - Rename etm to src now that it's not CORESIGHT_DEV_SUBTYPE_SOURCE_PROC
>> - Remove the now empty src_ops too
>> - Fix a rebase mistake in the Makefile that removed CTCU
>> - Link to v3: https://lore.kernel.org/r/20250312-james-cs-kunit-test-v3-1-dcfb69730161@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 | 1 +
>> drivers/hwtracing/coresight/coresight-core.c | 1 +
>> .../hwtracing/coresight/coresight-kunit-tests.c | 74 ++++++++++++++++++++++
>> 4 files changed, 85 insertions(+)
>>
>> 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.
>
> The description sounds adequate given that more unit tests might be added later.
>
>> +
>> endif
>> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
>> index 8e62c3150aeb..4e6ea5b05b01 100644
>> --- a/drivers/hwtracing/coresight/Makefile
>> +++ b/drivers/hwtracing/coresight/Makefile
>> @@ -53,3 +53,4 @@ 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 fb43ef6a3b1f..47af75ba7a00 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);
>
> This symbol needs exporting given that the kunit test here can also be built
> as a module.
>
>>
>> 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..c8f361767c45
>> --- /dev/null
>> +++ b/drivers/hwtracing/coresight/coresight-kunit-tests.c
>> @@ -0,0 +1,74 @@
>> +// 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);
>
> Guess both these memory allocations gets freed up here automatically
> via kunit_device's device framework when the test suit exits ?
>
Yes, see the change log in the cover letter.
>> + return csdev;
>> +}
>> +
>> +static void test_default_sink(struct kunit *test)
>> +{
>> + /*
>> + * Source -> ETF -> ETR -> CATU
>> + * ^
>> + * | default
>> + */
>
> Should some more random path combinations involving ETR and ETF along with
> other coresight devices also be tested where ETR gets selected as default ?
>
Probably, but I only added what was required to test Steve's original
bug report. We can add more tests for coresight_find_default_sink() if
when it gets changed or refactored in the future. At the moment without
any changes in sight there wouldn't be any benefit.
>> + struct device *dev = kunit_device_register(test, "coresight_kunit");
>> + struct coresight_device *src = coresight_test_device(dev),
>> + *etf = coresight_test_device(dev),
>> + *etr = coresight_test_device(dev),
>> + *catu = coresight_test_device(dev);
>> + struct coresight_connection conn = {};
>> +
>> + src->type = CORESIGHT_DEV_TYPE_SOURCE;
>> + /*
>> + * Don't use CORESIGHT_DEV_SUBTYPE_SOURCE_PROC, that would always return
>> + * a TRBE sink if one is registered.
>
> Probably this should be tested as well i.e when TRBE sink is also available
> and it gets picked up as a default.
>
This is quite simple logic so it's probably even less of a valuable
test. I only added a test for the other sinks because the recursive
search for them is not easy code to follow by eye.
Powered by blists - more mailing lists