linux-kernel - Re: [PATCH v5 1/8] drivers: kunit: Generic helpers for test device creation

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]

Date:   Thu, 23 Mar 2023 14:16:52 +0200
From:   Matti Vaittinen <mazziesaccount@...il.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Maxime Ripard <maxime@...no.tech>
Cc:     Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Brendan Higgins <brendan.higgins@...ux.dev>,
        David Gow <davidgow@...gle.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        kunit-dev@...glegroups.com, Stephen Boyd <sboyd@...nel.org>,
        Jonathan Cameron <jic23@...nel.org>, linux-iio@...r.kernel.org
Subject: Re: [PATCH v5 1/8] drivers: kunit: Generic helpers for test device
 creation

Hi Maxime, all

On 3/23/23 12:21, Greg Kroah-Hartman wrote:
> On Thu, Mar 23, 2023 at 11:12:16AM +0100, Maxime Ripard wrote:
>> On Wed, Mar 22, 2023 at 07:57:10PM +0100, Greg Kroah-Hartman wrote:
>>>>>> +/**
>>>>>> + * test_kunit_helper_alloc_device - Allocate a mock device for a KUnit test
>>>>>> + * @test: The test context object
>>>>>> + *
>>>>>> + * This allocates a fake struct &device to create a mock for a KUnit
>>>>>> + * test. The device will also be bound to a fake driver. It will thus be
>>>>>> + * able to leverage the usual infrastructure and most notably the
>>>>>> + * device-managed resources just like a "real" device.
>>>>>
>>>>> What specific "usual infrastructure" are you wanting to access here?
>>>>>
>>>>> And again, if you want a fake device, make a virtual one, by just
>>>>> calling device_create().
>>>>>
>>>>> Or are you wanting to do "more" with that device pointer than
>>>>> device_create() can give you?
>>>>
>>>> Personally, I was (am) only interested in devm_ unwinding. I guess the
>>>> device_create(), device_add(), device_remove()... (didn't study this
>>>> sequence in details so sorry if there is errors) could've been sufficient
>>>> for me. I haven't looked how much of the code that there is for 'platform
>>>> devices' should be duplicated to support that sequence for testability
>>>> purposes.
>>>
>>> Any device can access devm_ code, there's no need for it to be a
>>> platform device at all.
>>
>> Sure but the resources are only released if the device is part of a bus,
>> so it can't be a root_device (or bare device) either
> 
> The resources are not cleaned up when the device is freed no matter if
> it's on a bus or not?  If so, then that's a bug that needs to be fixed,
> and tested :)

This is strange. I just ran a test on a beaglebone black using Linux 
6.3.0-rc2 + the IIO patches we se here (but the IIO test patch modified 
to use the root_device_register() and root_device_unregister().

I passed the device pointer from root_device_register() to the 
devm_iio_init_iio_gts()

// snip
         dev = root_device_register(IIO_GTS_TEST_DEV);
         KUNIT_EXPECT_NOT_ERR_OR_NULL(test, dev);
         if (IS_ERR_OR_NULL(dev))
                 return NULL;

         ret = devm_iio_init_iio_gts(dev, TEST_SCALE_1X, 0, g_table, num_g,
                                     i_table, num_i, gts);

  - and saw the tables for available scales allocated:

  if (gts.num_avail_all_scales)
                 pr_info("GTS: table allocation succeeded\n");
         else
                 pr_info("GTS: table allocation failed\n");

         pr_info("gts: num_avail_all_scales %d\n", 
gts.num_avail_all_scales);

(this printed:
[   52.132966]     # Subtest: iio-gain-time-scale
[   52.132982]     1..7
[   52.157455] GTS: table allocation succeeded
[   52.164077] gts: num_avail_all_scales 16

Next I unregister the root-device and check if the unwinding code which 
frees the tables and zeroes the scale count was ran:

         root_device_unregister(dev);
         pr_info("gts: num_avail_all_scales %d\n", 
gts.num_avail_all_scales);

         if (gts.num_avail_all_scales)
                 pr_info("devm unwinding not done\n");
         else
                 pr_info("devm unwinding succeeded\n");

Which printed:
[   52.168101] gts: num_avail_all_scales 0
[   52.171957] devm unwinding succeeded

I can send patch(es) just for testing this on other machines if someone 
want's to see if the lack of devm unwinding is somehow architecture 
specific (which sounds very strange to me) - although using this IIO 
series just for checking the unwinding is a bit of an overkill. I just 
happened to have these tests at my hands / in my tree for testing.

In any case, devm unwinding using root_device_[un]register() seems to 
"work on my machine".

Naxime, what was the environment where you observed lack of unwinding? 
(Huh, I am so afraid of sending this post out - I've experienced too 
many "Oh, boy - how I didn't notice THAT" moments in the past and maybe 
I am again overlooking something...)

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~