linux-kernel - Re: [PATCH v5 4/4] iommu/io-pgtable-arm-selftests: Use KUnit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b48193a4-a37b-41ba-b4ba-8b5c67d812bd@arm.com>
Date: Wed, 15 Oct 2025 14:51:15 +0100
From: Robin Murphy <robin.murphy@....com>
To: Mostafa Saleh <smostafa@...gle.com>
Cc: iommu@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, will@...nel.org, joro@...tes.org,
 jgg@...pe.ca, praan@...gle.com, Jason Gunthorpe <jgg@...dia.com>
Subject: Re: [PATCH v5 4/4] iommu/io-pgtable-arm-selftests: Use KUnit

On 2025-10-15 10:53 am, Mostafa Saleh wrote:
[...]
>>> +		KUNIT_FAIL(test, "");						\
>>> +		kunit_err(test, "selftest: test failed for fmt idx %d\n", (i));	\
>>
>> This looks suspect - AFAICS open-coded kunit_err() is intended for test
>> infrastucture errors (like the allocation in the next hunk below), while for
>> an actual test report message it seems it should just be:
>>
>> 	KUNIT_FAIL(test, "selftest: test failed for fmt idx %d\n", (i));
> 
> I see, I used kunit_err, as KUNIT_FAIL adds extra information which was
> noisy to be printed more than once as:
>    # arm_lpae_do_selftests: EXPECTATION FAILED at drivers/iommu/io-pgtable-arm-selftests.c:91
> I will check if there is a better way to do this.

But isn't that exactly what we want? The tests should not fail, but if 
they ever do then inevitably you're going to be asked to debug it from 
nothing more than a dmesg snippet in a forwarded email. Loud and 
detailed is good ;)

If anything we can trim the generic message to just the minimal useful 
information about the format, if KUnit can now provide the rest of the 
attribution for us. Ultimately we should aim to log all the test 
conditions together in one place (and probably only upon a failure, such 
that normal successful runs are quiet), but that can be a job for the 
future follow-up refactoring.

[...]
>>> +	dev = kunit_device_register(test, "io-pgtable-test");
>>> +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, dev);
>>
>> Conversely, this is infrastructure, not an actual test of expected
>> io-pgtable behaviour, so I think just:
>>
>> 	cfg.iommu_dev = kunit_device_register(test, "io-pgtable-test");
>> 	if (IS_ERR(cfg.iommu_dev))
>> 		return;
>>
>> (it doesn't return NULLs either)
>>
> 
> Yes, I was not sure about this one, when checking the code base, every test
> handles kunit_device_register() failure differently, this seemed the
> most strict one so I used it, I will update that in the next version.

Yeah, my impression is that those have likely been copied from the 
lib/kunit/ code where it is actually testing its own API. I've now sent 
a patch for the example in the docs...

Cheers,
Robin.