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]
Message-ID: <20230323122925.kqdnomr7i46qnyo4@houat>
Date:   Thu, 23 Mar 2023 13:29:25 +0100
From:   Maxime Ripard <maxime@...no.tech>
To:     Matti Vaittinen <mazziesaccount@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        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

On Thu, Mar 23, 2023 at 02:16:52PM +0200, Matti Vaittinen wrote:
> 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...)

This is the description of what was happening:
https://lore.kernel.org/dri-devel/20221117165311.vovrc7usy4efiytl@houat/

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)