| Message ID | 20230302013822.1808711-1-sboyd@kernel.org |
|---|---|
| Headers | show |
| Series | clk: Add kunit tests for fixed rate and parent data | expand |
On 3/2/23 11:32, Rob Herring wrote: > On Thu, Mar 2, 2023 at 2:14 AM David Gow <davidgow@google.com> wrote: >> >> On Thu, 2 Mar 2023 at 09:38, Stephen Boyd <sboyd@kernel.org> wrote: >>> >>> This patch series adds unit tests for the clk fixed rate basic type and >>> the clk registration functions that use struct clk_parent_data. To get >>> there, we add support for loading a DTB into the UML kernel that's >>> running the unit tests along with probing platform drivers to bind to >>> device nodes specified in DT. >>> >>> With this series, we're able to exercise some of the code in the common >>> clk framework that uses devicetree lookups to find parents and the fixed >>> rate clk code that scans devicetree directly and creates clks. Please >>> review. >>> >> >> Thanks Stephen -- this is really neat! >> >> This works well here, and I love all of the tests for the >> KUnit/device-tree integration as well. >> >> I'm still looking through the details of it (alas, I've mostly lived >> in x86-land, so my device-tree knowledge is, uh, spotty to say the >> least), but apart from possibly renaming some things or similarly >> minor tweaks, I've not got any real suggestions thus far. >> >> I do wonder whether we'll want, on the KUnit side, to have some way of >> supporting KUnit device trees on non-UML architecctures (e.g., if we >> need to test something architecture-specific, or on a big-endian >> platform, etc), but I think that's a question for the future, rather >> than something that affects this series. > > I'll say that's a requirement. We should be able to structure the > tests to not interfere with the running system's DT. The DT unittest > does that. > > As a side topic, Is anyone looking at getting UML to work on arm64? > It's surprising how much x86 stuff there is which is I guess one > reason it hasn't happened. > >> Similarly, I wonder if there's something we could do with device tree >> overlays, in order to make it possible for tests to swap nodes in and >> out for testing. > > Yes, that's how the DT unittest works. But it is pretty much one big > overlay (ignoring the overlay tests). It could probably be more > modular where it is apply overlay, test, remove overlay, repeat. Actually, no, the bulk of the DT unittest devicetree data is _not_ an overlay. It is an FDT that is loaded via of_fdt_unflatten_tree() instead of the overlay load API. Note that the base DT unittest runs with CONFIG_OF_DYNAMIC=n CONFIG_OF_OVERLAY=n so the overlay support code is not even present in the built kernel. One can then enable CONFIG_OF_DYNAMIC to test the dynamic code. One can further enable CONFIG_OF_OVERLAY to test the overlay code (this will in turn select CONFIG_OF_DYNAMIC if not already enabled). I would strongly discourage use of the overlay APIs for kunit tests, unless the point of the kunit test is to test the overlay API. Basic tests should always be performed with devicetree data that has been populated by the normal processing of an FDT during early boot. If one want to test proper overlay infrastructure functionality, then those (essentially) same basic tests could/should be repeated with devicetree data that has been populated by loading an overlay. > > Rob
On 3/2/23 13:27, Stephen Boyd wrote: > Quoting Rob Herring (2023-03-02 09:32:09) >> On Thu, Mar 2, 2023 at 2:14 AM David Gow <davidgow@google.com> wrote: >>> >>> On Thu, 2 Mar 2023 at 09:38, Stephen Boyd <sboyd@kernel.org> wrote: >>>> >>>> This patch series adds unit tests for the clk fixed rate basic type and >>>> the clk registration functions that use struct clk_parent_data. To get >>>> there, we add support for loading a DTB into the UML kernel that's >>>> running the unit tests along with probing platform drivers to bind to >>>> device nodes specified in DT. >>>> >>>> With this series, we're able to exercise some of the code in the common >>>> clk framework that uses devicetree lookups to find parents and the fixed >>>> rate clk code that scans devicetree directly and creates clks. Please >>>> review. >>>> >>> >>> Thanks Stephen -- this is really neat! >>> >>> This works well here, and I love all of the tests for the >>> KUnit/device-tree integration as well. >>> >>> I'm still looking through the details of it (alas, I've mostly lived >>> in x86-land, so my device-tree knowledge is, uh, spotty to say the >>> least), but apart from possibly renaming some things or similarly >>> minor tweaks, I've not got any real suggestions thus far. >>> >>> I do wonder whether we'll want, on the KUnit side, to have some way of >>> supporting KUnit device trees on non-UML architecctures (e.g., if we >>> need to test something architecture-specific, or on a big-endian >>> platform, etc), but I think that's a question for the future, rather >>> than something that affects this series. >> >> I'll say that's a requirement. We should be able to structure the >> tests to not interfere with the running system's DT. The DT unittest >> does that. > > That could be another choice in the unit test choice menu. > CONFIG_OF_KUNIT_NOT_UML that injects some built-in DTB overlay on an > architecture that wants to run tests. > >> >> As a side topic, Is anyone looking at getting UML to work on arm64? >> It's surprising how much x86 stuff there is which is I guess one >> reason it hasn't happened. > > I've no idea but it would be nice indeed. > >> >>> Similarly, I wonder if there's something we could do with device tree >>> overlays, in order to make it possible for tests to swap nodes in and >>> out for testing. >> >> Yes, that's how the DT unittest works. But it is pretty much one big >> overlay (ignoring the overlay tests). It could probably be more >> modular where it is apply overlay, test, remove overlay, repeat. >> > > I didn't want to rely on the overlay code to inject DT nodes. Having > tests written for the fake KUnit machine is simple. It closely matches > how clk code probes the DTB and how nodes are created and populated on > the platform bus as devices. CLK_OF_DECLARE() would need the overlay to > be applied early too, which doesn't happen otherwise as far as I know. > > But perhaps this design is too much of an end-to-end test and not a unit > test? In the spirit of unit testing we shouldn't care about how the node > is added to the live devicetree, just that there is a devicetree at all. > > Supporting overlays to more easily test combinations sounds like a good > idea. Probably some kunit_*() prefixed functions could be used to In an imaginary world where overlay support was completed, then _maybe_. To me, the most important environment to test is where the devictree data is populated in early boot from an FDT. This is the environment that drivers currently exist in. Populating devicetree data via an overlay adds in the functioning of the overlay apply code (and how the rules behind that functioning may differ from devicetree data populated in early boot from an FDT). In an ideal world where overlay support was completed, most or all of the devicetree tests that were performed against the devicetree data populated in early boot from an FDT would be repeated, but against comparable devicetree data populated via an overlay load. The tests with the overlay data may have to be aware of some differences in how an overlay load processes an FDT vs how the early boot processing of an FDT behaves. This extra testing would verify that the overlay environment behaves the same as the non-overlay environment (with some known exceptions due to overlay policies). Overlay support is not complete: https://elinux.org/Device_Tree_Reference#Mainline_Linux_Support https://elinux.org/Frank%27s_Evolving_Overlay_Thoughts -Frank > apply a test managed overlay and automatically remove it when the test > is over would work. The clk registration tests could use this API to > inject an overlay and then manually call the of_platform_populate() > function to create the platform device(s). The overlay could be built in > drivers/clk/ too and then probably some macroish function can find the > blob and apply it. > > Is there some way to delete the platform devices that we populate from > the overlay? I'd like the tests to be hermetic.
On 3/2/23 11:13, Rob Herring wrote: > On Wed, Mar 1, 2023 at 7:38 PM Stephen Boyd <sboyd@kernel.org> wrote: >> >> This patch series adds unit tests for the clk fixed rate basic type and >> the clk registration functions that use struct clk_parent_data. To get >> there, we add support for loading a DTB into the UML kernel that's >> running the unit tests along with probing platform drivers to bind to >> device nodes specified in DT. >> >> With this series, we're able to exercise some of the code in the common >> clk framework that uses devicetree lookups to find parents and the fixed >> rate clk code that scans devicetree directly and creates clks. Please >> review. >> >> I Cced everyone to all the patches so they get the full context. I'm >> hoping I can take the whole pile through the clk tree as they almost all >> depend on each other. In the future I imagine it will be easy to add >> more test nodes to the clk.dtsi file and not need to go across various >> maintainer trees like this series does. >> >> Stephen Boyd (8): >> dt-bindings: Add linux,kunit binding >> of: Enable DTB loading on UML for KUnit tests >> kunit: Add test managed platform_device/driver APIs >> clk: Add test managed clk provider/consumer APIs >> dt-bindings: kunit: Add fixed rate clk consumer test >> clk: Add KUnit tests for clk fixed rate basic type >> dt-bindings: clk: Add KUnit clk_parent_data test >> clk: Add KUnit tests for clks registered with struct clk_parent_data > > Good to see bindings for this. I've been meaning to do something about > the DT unittest ones being undocumented, but I hadn't really decided > whether it was worth writing schemas for them. The compatibles at > least show up with 'make dt_compatible_check'. Perhaps we want to just > define some vendor (not 'linux') that's an exception rather than > requiring schemas (actually, that already works for 'foo'). It's > likely that we want test DTs that fail normal checks and schemas get > in the way of that as we don't have a way to turn off checks. > > We already have GPIO tests in the DT unittests, so why is clocks > different? Or should the GPIO tests be moved out (yes, please!)? > > What happens when/if the DT unittest is converted to kunit? I think My current plan is to update the DT unittest output to be compatible with the kunit output, so test harnesses can use the same framework to process test output, and detect and report results. kunit moved to the KTAP format a while ago. I am working (more slowly than I would like) to get the next version of the KTAP specification agreed to, which has some features that will be needed to move DT unittests to the KTAP output format. Whether it is possible to subsequently move DT unittests into the kunit framework is a different question, which could be addressed as a possible next step of DT unittest transformation (but see my opinion below). > that would look confusing from the naming. My initial thought is > 'kunit' should be dropped from the naming of a lot of this. Note that > the original kunit submission converted the DT unittests. I would > still like to see that happen. Frank disagreed over what's a unit test > or not, then agreed, then didn't... I don't really care. If there's a > framework to use, then we should use it IMO. I don't think I ever agreed that the kunit framework was suitable to implement DT unittest. At a conceptual level, kunit and DT unittest differ architecturally (the following is not what kunit looks like - the procedural flow is hidden away in macros and the source looks more like data declarations). kunit ----- test_1_initialization(); test_1(); test_1_a(); test_1_b(); ... test_1_N(); test_1_cleanup(); ## Each of test_1_*() reports pass / fail / skip ## I'm not sure if this is just one pass / fail / skip, or ## if multiple are supported. ## ## Each of test_1_*() are independent and could be reordered. DT unittest ----------- some_initialization_in_early_boot() of_unittest() a_lot_of_initialization(); subsystem_or_area_1_test(); test_area_initialization(); test_1_a(); ## test_1_a() may or may not impact the devicetree data ## in a manner that is pre-requisite for test_1_b() test_1_b(); ... ## At any point in test_1_a() .. test_1_N() may goto ## out_ERROR_xxx: if a test fails in a way that ## impacts subsequent test dependencies ## ## Possible clean up between or after each test_1_*() ## Possible validation that the devicetreee data is correct ## after test activity test_1_c(); ... test_1_N(); subsystem_or_area_2_test(); ... ## At arbitrary points, full tree or sub-tree validation to ## confirm tree integrity after completing the previous tests ... ## Much of test_1_*() are dependent on previously executed ## test_1_*() and can _not_ be reordered. -Frank > >> >> .../clock/linux,clk-kunit-parent-data.yaml | 47 ++ >> .../kunit/linux,clk-kunit-fixed-rate.yaml | 35 ++ >> .../bindings/kunit/linux,kunit.yaml | 24 + >> arch/um/kernel/dtb.c | 29 +- >> drivers/clk/.kunitconfig | 3 + >> drivers/clk/Kconfig | 7 + >> drivers/clk/Makefile | 6 + >> drivers/clk/clk-fixed-rate_test.c | 296 ++++++++++++ >> drivers/clk/clk-kunit.c | 204 ++++++++ >> drivers/clk/clk-kunit.h | 28 ++ >> drivers/clk/clk_test.c | 456 +++++++++++++++++- >> drivers/of/Kconfig | 26 + >> drivers/of/Makefile | 1 + >> drivers/of/kunit/.kunitconfig | 4 + >> drivers/of/kunit/Makefile | 4 + >> drivers/of/kunit/clk.dtsi | 30 ++ >> drivers/of/kunit/kunit.dtsi | 9 + >> drivers/of/kunit/kunit.dtso | 4 + >> drivers/of/kunit/uml_dtb_test.c | 55 +++ >> include/kunit/platform_driver.h | 15 + >> lib/kunit/Makefile | 6 + >> lib/kunit/platform_driver-test.c | 107 ++++ >> lib/kunit/platform_driver.c | 207 ++++++++ > > Humm, we have DT platform driver unittests too. What's the difference? > > Anyways, that's all just my initial reaction from only halfway looking > at this. :) > > Rob
On 3/2/23 14:18, Rob Herring wrote: > On Thu, Mar 2, 2023 at 1:44 PM Stephen Boyd <sboyd@kernel.org> wrote: >> >> Quoting Rob Herring (2023-03-02 09:13:59) >>> On Wed, Mar 1, 2023 at 7:38 PM Stephen Boyd <sboyd@kernel.org> wrote: >>>> >>>> This patch series adds unit tests for the clk fixed rate basic type and >>>> the clk registration functions that use struct clk_parent_data. To get >>>> there, we add support for loading a DTB into the UML kernel that's >>>> running the unit tests along with probing platform drivers to bind to >>>> device nodes specified in DT. >>>> >>>> With this series, we're able to exercise some of the code in the common >>>> clk framework that uses devicetree lookups to find parents and the fixed >>>> rate clk code that scans devicetree directly and creates clks. Please >>>> review. >>>> >>>> I Cced everyone to all the patches so they get the full context. I'm >>>> hoping I can take the whole pile through the clk tree as they almost all >>>> depend on each other. In the future I imagine it will be easy to add >>>> more test nodes to the clk.dtsi file and not need to go across various >>>> maintainer trees like this series does. >>>> >>>> Stephen Boyd (8): >>>> dt-bindings: Add linux,kunit binding >>>> of: Enable DTB loading on UML for KUnit tests >>>> kunit: Add test managed platform_device/driver APIs >>>> clk: Add test managed clk provider/consumer APIs >>>> dt-bindings: kunit: Add fixed rate clk consumer test >>>> clk: Add KUnit tests for clk fixed rate basic type >>>> dt-bindings: clk: Add KUnit clk_parent_data test >>>> clk: Add KUnit tests for clks registered with struct clk_parent_data >>> >>> Good to see bindings for this. I've been meaning to do something about >>> the DT unittest ones being undocumented, but I hadn't really decided >>> whether it was worth writing schemas for them. The compatibles at >>> least show up with 'make dt_compatible_check'. Perhaps we want to just >>> define some vendor (not 'linux') that's an exception rather than >>> requiring schemas (actually, that already works for 'foo'). >> >> Sure. Maybe "kunit" should be the vendor prefix? Or "dtbunit"? > > We'd want to use the same thing on the DT unittests or anything else > potentially. How about just 'test'? > >>> It's >>> likely that we want test DTs that fail normal checks and schemas get >>> in the way of that as we don't have a way to turn off checks. >> >> Having the schemas is nice to make sure tests that are expecting some >> binding are actually getting that. But supporting broken bindings is >> also important to test any error paths in functions that parse >> properties. Maybe we keep the schema and have it enforce that incorrect >> properties are being set? > > I wasn't suggesting throwing them out. More why I hadn't written any I guess. > >> Do we really need to test incorrect bindings? Doesn't the >> dt_bindings_check catch these problems so we don't have to write DTB >> verifiers in the kernel? > > Fair enough. Using my frequently stated position against me. :) > > I do have a secret plan to implement (debug) type checks into the > of_property_* APIs by extracting the type information from schemas > into C. > > >>> We already have GPIO tests in the DT unittests, so why is clocks >>> different? Or should the GPIO tests be moved out (yes, please!)? >> >> Ah I didn't notice the GPIO tests in there. There are i2c tests too, >> right? All I can say is clks are using kunit, that's the difference ;-) > > Yeah, they should perhaps all move to the subsystems. > >>> What happens when/if the DT unittest is converted to kunit? I think >>> that would look confusing from the naming. My initial thought is >>> 'kunit' should be dropped from the naming of a lot of this. Note that >>> the original kunit submission converted the DT unittests. I would >>> still like to see that happen. Frank disagreed over what's a unit test >>> or not, then agreed, then didn't... I don't really care. If there's a >>> framework to use, then we should use it IMO. >> >> Honestly I don't want to get involved in migrating the existing DT >> unittest code to kunit. I'm aware that it was attempted years ago when >> kunit was introduced. Maybe if the overlay route works well enough I can >> completely sidestep introducing any code in drivers/of/ besides some >> kunit wrappers for this. I'll cross my fingers! > > Yeah, I wasn't expecting you to. I just want to make sure this meshes > with any future conversion to kunit. > > There's also some plans to always populate the DT root node if not > present. That may help here. Or not. There's been a few versions > posted with Frank's in the last week or 2. As noted in that thread, by code inspection (not actual testing) I think that the patch series breaks DT unittest for UML. It should be a trivial change in the next patch version to fix. > > Rob
On 3/2/23 17:57, Stephen Boyd wrote: > Quoting Rob Herring (2023-03-02 12:18:34) >> On Thu, Mar 2, 2023 at 1:44 PM Stephen Boyd <sboyd@kernel.org> wrote: >>> >>> Quoting Rob Herring (2023-03-02 09:13:59) >>>> >>>> Good to see bindings for this. I've been meaning to do something about >>>> the DT unittest ones being undocumented, but I hadn't really decided >>>> whether it was worth writing schemas for them. The compatibles at >>>> least show up with 'make dt_compatible_check'. Perhaps we want to just >>>> define some vendor (not 'linux') that's an exception rather than >>>> requiring schemas (actually, that already works for 'foo'). >>> >>> Sure. Maybe "kunit" should be the vendor prefix? Or "dtbunit"? >> >> We'd want to use the same thing on the DT unittests or anything else >> potentially. How about just 'test'? > > Sounds good. > >> >>>> It's >>>> likely that we want test DTs that fail normal checks and schemas get >>>> in the way of that as we don't have a way to turn off checks. >>> >>> Having the schemas is nice to make sure tests that are expecting some >>> binding are actually getting that. But supporting broken bindings is >>> also important to test any error paths in functions that parse >>> properties. Maybe we keep the schema and have it enforce that incorrect >>> properties are being set? >> >> I wasn't suggesting throwing them out. More why I hadn't written any I guess. >> >>> Do we really need to test incorrect bindings? Doesn't the >>> dt_bindings_check catch these problems so we don't have to write DTB >>> verifiers in the kernel? >> >> Fair enough. Using my frequently stated position against me. :) >> >> I do have a secret plan to implement (debug) type checks into the >> of_property_* APIs by extracting the type information from schemas >> into C. >> > > Ok. I suspect we may want to test error paths though so I don't know Yes, exactly. > what to do here. For now I'll just leave the bindings in place and > change the prefix to "test". > >> >>>> We already have GPIO tests in the DT unittests, so why is clocks >>>> different? Or should the GPIO tests be moved out (yes, please!)? >>> >>> Ah I didn't notice the GPIO tests in there. There are i2c tests too, >>> right? All I can say is clks are using kunit, that's the difference ;-) >> >> Yeah, they should perhaps all move to the subsystems. > > Got it. > >> >>>> What happens when/if the DT unittest is converted to kunit? I think >>>> that would look confusing from the naming. My initial thought is >>>> 'kunit' should be dropped from the naming of a lot of this. Note that >>>> the original kunit submission converted the DT unittests. I would >>>> still like to see that happen. Frank disagreed over what's a unit test >>>> or not, then agreed, then didn't... I don't really care. If there's a >>>> framework to use, then we should use it IMO. >>> >>> Honestly I don't want to get involved in migrating the existing DT >>> unittest code to kunit. I'm aware that it was attempted years ago when >>> kunit was introduced. Maybe if the overlay route works well enough I can >>> completely sidestep introducing any code in drivers/of/ besides some >>> kunit wrappers for this. I'll cross my fingers! >> >> Yeah, I wasn't expecting you to. I just want to make sure this meshes >> with any future conversion to kunit. > > Phew! > >> >> There's also some plans to always populate the DT root node if not >> present. That may help here. Or not. There's been a few versions >> posted with Frank's in the last week or 2. >> > > Ok. I think I have some time to try this overlay approach so let me see > what is needed. Please avoid overlays. See my other replies in this thread for why.
On 3/1/23 19:38, Stephen Boyd wrote: > This patch series adds unit tests for the clk fixed rate basic type and > the clk registration functions that use struct clk_parent_data. To get > there, we add support for loading a DTB into the UML kernel that's > running the unit tests along with probing platform drivers to bind to > device nodes specified in DT. > > With this series, we're able to exercise some of the code in the common > clk framework that uses devicetree lookups to find parents and the fixed > rate clk code that scans devicetree directly and creates clks. Please > review. I would _really_ like to _not_ have devicetree tests in two locations: DT unittests and kunit tests. For my testing, I already build and boot four times on real hardware: 1) no DT unittests 2) CONFIG_OF_UNITTEST 3) CONFIG_OF_UNITTEST CONFIG_OF_DYNAMIC 4) CONFIG_OF_UNITTEST CONFIG_OF_DYNAMIC CONFIG_OF_OVERLAY I really should also be testing the four configurations on UML, but at the moment I am not. I also check for new compile warnings at various warn levels for all four configurations. If I recall correctly, the kunit framework encourages more (many more?) kunit config options to select which test(s) are build for a test run. Someone please correct this paragraph if I am mis-stating. Adding devicetree tests to kunit adds additional build and boot cycles and additional test output streams to verify. Are there any issues with DT unittests that preclude adding clk tests into the DT unittests? -Frank > > I Cced everyone to all the patches so they get the full context. I'm > hoping I can take the whole pile through the clk tree as they almost all > depend on each other. In the future I imagine it will be easy to add > more test nodes to the clk.dtsi file and not need to go across various > maintainer trees like this series does. > > Stephen Boyd (8): > dt-bindings: Add linux,kunit binding > of: Enable DTB loading on UML for KUnit tests > kunit: Add test managed platform_device/driver APIs > clk: Add test managed clk provider/consumer APIs > dt-bindings: kunit: Add fixed rate clk consumer test > clk: Add KUnit tests for clk fixed rate basic type > dt-bindings: clk: Add KUnit clk_parent_data test > clk: Add KUnit tests for clks registered with struct clk_parent_data > > .../clock/linux,clk-kunit-parent-data.yaml | 47 ++ > .../kunit/linux,clk-kunit-fixed-rate.yaml | 35 ++ > .../bindings/kunit/linux,kunit.yaml | 24 + > arch/um/kernel/dtb.c | 29 +- > drivers/clk/.kunitconfig | 3 + > drivers/clk/Kconfig | 7 + > drivers/clk/Makefile | 6 + > drivers/clk/clk-fixed-rate_test.c | 296 ++++++++++++ > drivers/clk/clk-kunit.c | 204 ++++++++ > drivers/clk/clk-kunit.h | 28 ++ > drivers/clk/clk_test.c | 456 +++++++++++++++++- > drivers/of/Kconfig | 26 + > drivers/of/Makefile | 1 + > drivers/of/kunit/.kunitconfig | 4 + > drivers/of/kunit/Makefile | 4 + > drivers/of/kunit/clk.dtsi | 30 ++ > drivers/of/kunit/kunit.dtsi | 9 + > drivers/of/kunit/kunit.dtso | 4 + > drivers/of/kunit/uml_dtb_test.c | 55 +++ > include/kunit/platform_driver.h | 15 + > lib/kunit/Makefile | 6 + > lib/kunit/platform_driver-test.c | 107 ++++ > lib/kunit/platform_driver.c | 207 ++++++++ > 23 files changed, 1599 insertions(+), 4 deletions(-) > create mode 100644 Documentation/devicetree/bindings/clock/linux,clk-kunit-parent-data.yaml > create mode 100644 Documentation/devicetree/bindings/kunit/linux,clk-kunit-fixed-rate.yaml > create mode 100644 Documentation/devicetree/bindings/kunit/linux,kunit.yaml > create mode 100644 drivers/clk/clk-fixed-rate_test.c > create mode 100644 drivers/clk/clk-kunit.c > create mode 100644 drivers/clk/clk-kunit.h > create mode 100644 drivers/of/kunit/.kunitconfig > create mode 100644 drivers/of/kunit/Makefile > create mode 100644 drivers/of/kunit/clk.dtsi > create mode 100644 drivers/of/kunit/kunit.dtsi > create mode 100644 drivers/of/kunit/kunit.dtso > create mode 100644 drivers/of/kunit/uml_dtb_test.c > create mode 100644 include/kunit/platform_driver.h > create mode 100644 lib/kunit/platform_driver-test.c > create mode 100644 lib/kunit/platform_driver.c > > > base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c
On 3/2/23 13:47, Geert Uytterhoeven wrote: > Hi Stephen, > > On Thu, Mar 2, 2023 at 8:28 PM Stephen Boyd <sboyd@kernel.org> wrote: >> Quoting Rob Herring (2023-03-02 09:32:09) >>> On Thu, Mar 2, 2023 at 2:14 AM David Gow <davidgow@google.com> wrote: >>>> On Thu, 2 Mar 2023 at 09:38, Stephen Boyd <sboyd@kernel.org> wrote: >>>>> This patch series adds unit tests for the clk fixed rate basic type and >>>>> the clk registration functions that use struct clk_parent_data. To get >>>>> there, we add support for loading a DTB into the UML kernel that's >>>>> running the unit tests along with probing platform drivers to bind to >>>>> device nodes specified in DT. >>>>> >>>>> With this series, we're able to exercise some of the code in the common >>>>> clk framework that uses devicetree lookups to find parents and the fixed >>>>> rate clk code that scans devicetree directly and creates clks. Please >>>>> review. >>>>> >>>> >>>> Thanks Stephen -- this is really neat! >>>> >>>> This works well here, and I love all of the tests for the >>>> KUnit/device-tree integration as well. >>>> >>>> I'm still looking through the details of it (alas, I've mostly lived >>>> in x86-land, so my device-tree knowledge is, uh, spotty to say the >>>> least), but apart from possibly renaming some things or similarly >>>> minor tweaks, I've not got any real suggestions thus far. >>>> >>>> I do wonder whether we'll want, on the KUnit side, to have some way of >>>> supporting KUnit device trees on non-UML architecctures (e.g., if we >>>> need to test something architecture-specific, or on a big-endian >>>> platform, etc), but I think that's a question for the future, rather >>>> than something that affects this series. >>> >>> I'll say that's a requirement. We should be able to structure the >>> tests to not interfere with the running system's DT. The DT unittest >>> does that. >> >> That could be another choice in the unit test choice menu. >> CONFIG_OF_KUNIT_NOT_UML that injects some built-in DTB overlay on an >> architecture that wants to run tests. > > As long as you use compatible values that don't exist elsewhere, > and don't overwrite anything, you can load your kunit test overlays > on any running system that has DT support. > >>> As a side topic, Is anyone looking at getting UML to work on arm64? >>> It's surprising how much x86 stuff there is which is I guess one >>> reason it hasn't happened. >> >> I've no idea but it would be nice indeed. > > I believe that's non-trivial. At least for arm32 (I didn't have any arm64 > systems last time I asked the experts). > >>>> Similarly, I wonder if there's something we could do with device tree >>>> overlays, in order to make it possible for tests to swap nodes in and >>>> out for testing. >>> >>> Yes, that's how the DT unittest works. But it is pretty much one big >>> overlay (ignoring the overlay tests). It could probably be more >>> modular where it is apply overlay, test, remove overlay, repeat. >> >> I didn't want to rely on the overlay code to inject DT nodes. Having >> tests written for the fake KUnit machine is simple. It closely matches >> how clk code probes the DTB and how nodes are created and populated on >> the platform bus as devices. CLK_OF_DECLARE() would need the overlay to >> be applied early too, which doesn't happen otherwise as far as I know. > > Don't all generic clock drivers also create a platform driver? > At least drivers/clk/clk-fixed-factor.c does. > >> But perhaps this design is too much of an end-to-end test and not a unit >> test? In the spirit of unit testing we shouldn't care about how the node >> is added to the live devicetree, just that there is a devicetree at all. >> >> Supporting overlays to more easily test combinations sounds like a good >> idea. Probably some kunit_*() prefixed functions could be used to >> apply a test managed overlay and automatically remove it when the test >> is over would work. The clk registration tests could use this API to >> inject an overlay and then manually call the of_platform_populate() >> function to create the platform device(s). The overlay could be built in >> drivers/clk/ too and then probably some macroish function can find the >> blob and apply it. > > No need to manually call of_platform_populate() to create the > platform devices. That is taken care of automatically when applying > an overlay. > >> Is there some way to delete the platform devices that we populate from >> the overlay? I'd like the tests to be hermetic. > > Removing the overlay will delete the platform devices. I _think_ that is incorrect. Do you have a pointer to the overlay code that deletes the device? (If I remember correctly, the overlay remove code does not even check whether the device exists and whether a driver is bound to it -- but this is on my todo list to look into.) -Frank > > All of that works if you have your own code to apply a DT overlay. > The recent fw_devlinks patches did cause some regressions, cfr. > https://lore.kernel.org/all/CAMuHMdXEnSD4rRJ-o90x4OprUacN_rJgyo8x6=9F9rZ+-KzjOg@mail.gmail.com > > P.S. Shameless plug: for loading overlays from userspace, there are > my overlay branches, cfr. https://elinux.org/R-Car/DT-Overlays > > Gr{oetje,eeting}s, > > Geert > > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Frank, On Sun, Mar 5, 2023 at 4:33 AM Frank Rowand <frowand.list@gmail.com> wrote: > On 3/2/23 13:47, Geert Uytterhoeven wrote: > > On Thu, Mar 2, 2023 at 8:28 PM Stephen Boyd <sboyd@kernel.org> wrote: > >> Quoting Rob Herring (2023-03-02 09:32:09) > >>> On Thu, Mar 2, 2023 at 2:14 AM David Gow <davidgow@google.com> wrote: > >>>> On Thu, 2 Mar 2023 at 09:38, Stephen Boyd <sboyd@kernel.org> wrote: > >>>>> This patch series adds unit tests for the clk fixed rate basic type and > >>>>> the clk registration functions that use struct clk_parent_data. To get > >>>>> there, we add support for loading a DTB into the UML kernel that's > >>>>> running the unit tests along with probing platform drivers to bind to > >>>>> device nodes specified in DT. > >>>>> > >>>>> With this series, we're able to exercise some of the code in the common > >>>>> clk framework that uses devicetree lookups to find parents and the fixed > >>>>> rate clk code that scans devicetree directly and creates clks. Please > >>>>> review. > >>>>> > >>>> > >>>> Thanks Stephen -- this is really neat! > >>>> > >>>> This works well here, and I love all of the tests for the > >>>> KUnit/device-tree integration as well. > >>>> > >>>> I'm still looking through the details of it (alas, I've mostly lived > >>>> in x86-land, so my device-tree knowledge is, uh, spotty to say the > >>>> least), but apart from possibly renaming some things or similarly > >>>> minor tweaks, I've not got any real suggestions thus far. > >>>> > >>>> I do wonder whether we'll want, on the KUnit side, to have some way of > >>>> supporting KUnit device trees on non-UML architecctures (e.g., if we > >>>> need to test something architecture-specific, or on a big-endian > >>>> platform, etc), but I think that's a question for the future, rather > >>>> than something that affects this series. > >>> > >>> I'll say that's a requirement. We should be able to structure the > >>> tests to not interfere with the running system's DT. The DT unittest > >>> does that. > >> > >> That could be another choice in the unit test choice menu. > >> CONFIG_OF_KUNIT_NOT_UML that injects some built-in DTB overlay on an > >> architecture that wants to run tests. > > > > As long as you use compatible values that don't exist elsewhere, > > and don't overwrite anything, you can load your kunit test overlays > > on any running system that has DT support. > > > >>> As a side topic, Is anyone looking at getting UML to work on arm64? > >>> It's surprising how much x86 stuff there is which is I guess one > >>> reason it hasn't happened. > >> > >> I've no idea but it would be nice indeed. > > > > I believe that's non-trivial. At least for arm32 (I didn't have any arm64 > > systems last time I asked the experts). > > > >>>> Similarly, I wonder if there's something we could do with device tree > >>>> overlays, in order to make it possible for tests to swap nodes in and > >>>> out for testing. > >>> > >>> Yes, that's how the DT unittest works. But it is pretty much one big > >>> overlay (ignoring the overlay tests). It could probably be more > >>> modular where it is apply overlay, test, remove overlay, repeat. > >> > >> I didn't want to rely on the overlay code to inject DT nodes. Having > >> tests written for the fake KUnit machine is simple. It closely matches > >> how clk code probes the DTB and how nodes are created and populated on > >> the platform bus as devices. CLK_OF_DECLARE() would need the overlay to > >> be applied early too, which doesn't happen otherwise as far as I know. > > > > Don't all generic clock drivers also create a platform driver? > > At least drivers/clk/clk-fixed-factor.c does. > > > >> But perhaps this design is too much of an end-to-end test and not a unit > >> test? In the spirit of unit testing we shouldn't care about how the node > >> is added to the live devicetree, just that there is a devicetree at all. > >> > >> Supporting overlays to more easily test combinations sounds like a good > >> idea. Probably some kunit_*() prefixed functions could be used to > >> apply a test managed overlay and automatically remove it when the test > >> is over would work. The clk registration tests could use this API to > >> inject an overlay and then manually call the of_platform_populate() > >> function to create the platform device(s). The overlay could be built in > >> drivers/clk/ too and then probably some macroish function can find the > >> blob and apply it. > > > > No need to manually call of_platform_populate() to create the > > platform devices. That is taken care of automatically when applying > > an overlay. > > > >> Is there some way to delete the platform devices that we populate from > >> the overlay? I'd like the tests to be hermetic. > > > Removing the overlay will delete the platform devices. > > I _think_ that is incorrect. Do you have a pointer to the overlay code that > deletes the device? (If I remember correctly, the overlay remove code does not > even check whether the device exists and whether a driver is bound to it -- but > this is on my todo list to look into.) https://elixir.bootlin.com/linux/latest/source/drivers/of/platform.c#L769 > > All of that works if you have your own code to apply a DT overlay. > > The recent fw_devlinks patches did cause some regressions, cfr. > > https://lore.kernel.org/all/CAMuHMdXEnSD4rRJ-o90x4OprUacN_rJgyo8x6=9F9rZ+-KzjOg@mail.gmail.com Gr{oetje,eeting}s, Geert
On 3/5/23 03:26, Geert Uytterhoeven wrote: > Hi Frank, > > On Sun, Mar 5, 2023 at 4:33 AM Frank Rowand <frowand.list@gmail.com> wrote: >> On 3/2/23 13:47, Geert Uytterhoeven wrote: >>> On Thu, Mar 2, 2023 at 8:28 PM Stephen Boyd <sboyd@kernel.org> wrote: >>>> Quoting Rob Herring (2023-03-02 09:32:09) >>>>> On Thu, Mar 2, 2023 at 2:14 AM David Gow <davidgow@google.com> wrote: >>>>>> On Thu, 2 Mar 2023 at 09:38, Stephen Boyd <sboyd@kernel.org> wrote: >>>>>>> This patch series adds unit tests for the clk fixed rate basic type and >>>>>>> the clk registration functions that use struct clk_parent_data. To get >>>>>>> there, we add support for loading a DTB into the UML kernel that's >>>>>>> running the unit tests along with probing platform drivers to bind to >>>>>>> device nodes specified in DT. >>>>>>> >>>>>>> With this series, we're able to exercise some of the code in the common >>>>>>> clk framework that uses devicetree lookups to find parents and the fixed >>>>>>> rate clk code that scans devicetree directly and creates clks. Please >>>>>>> review. >>>>>>> >>>>>> >>>>>> Thanks Stephen -- this is really neat! >>>>>> >>>>>> This works well here, and I love all of the tests for the >>>>>> KUnit/device-tree integration as well. >>>>>> >>>>>> I'm still looking through the details of it (alas, I've mostly lived >>>>>> in x86-land, so my device-tree knowledge is, uh, spotty to say the >>>>>> least), but apart from possibly renaming some things or similarly >>>>>> minor tweaks, I've not got any real suggestions thus far. >>>>>> >>>>>> I do wonder whether we'll want, on the KUnit side, to have some way of >>>>>> supporting KUnit device trees on non-UML architecctures (e.g., if we >>>>>> need to test something architecture-specific, or on a big-endian >>>>>> platform, etc), but I think that's a question for the future, rather >>>>>> than something that affects this series. >>>>> >>>>> I'll say that's a requirement. We should be able to structure the >>>>> tests to not interfere with the running system's DT. The DT unittest >>>>> does that. >>>> >>>> That could be another choice in the unit test choice menu. >>>> CONFIG_OF_KUNIT_NOT_UML that injects some built-in DTB overlay on an >>>> architecture that wants to run tests. >>> >>> As long as you use compatible values that don't exist elsewhere, >>> and don't overwrite anything, you can load your kunit test overlays >>> on any running system that has DT support. >>> >>>>> As a side topic, Is anyone looking at getting UML to work on arm64? >>>>> It's surprising how much x86 stuff there is which is I guess one >>>>> reason it hasn't happened. >>>> >>>> I've no idea but it would be nice indeed. >>> >>> I believe that's non-trivial. At least for arm32 (I didn't have any arm64 >>> systems last time I asked the experts). >>> >>>>>> Similarly, I wonder if there's something we could do with device tree >>>>>> overlays, in order to make it possible for tests to swap nodes in and >>>>>> out for testing. >>>>> >>>>> Yes, that's how the DT unittest works. But it is pretty much one big >>>>> overlay (ignoring the overlay tests). It could probably be more >>>>> modular where it is apply overlay, test, remove overlay, repeat. >>>> >>>> I didn't want to rely on the overlay code to inject DT nodes. Having >>>> tests written for the fake KUnit machine is simple. It closely matches >>>> how clk code probes the DTB and how nodes are created and populated on >>>> the platform bus as devices. CLK_OF_DECLARE() would need the overlay to >>>> be applied early too, which doesn't happen otherwise as far as I know. >>> >>> Don't all generic clock drivers also create a platform driver? >>> At least drivers/clk/clk-fixed-factor.c does. >>> >>>> But perhaps this design is too much of an end-to-end test and not a unit >>>> test? In the spirit of unit testing we shouldn't care about how the node >>>> is added to the live devicetree, just that there is a devicetree at all. >>>> >>>> Supporting overlays to more easily test combinations sounds like a good >>>> idea. Probably some kunit_*() prefixed functions could be used to >>>> apply a test managed overlay and automatically remove it when the test >>>> is over would work. The clk registration tests could use this API to >>>> inject an overlay and then manually call the of_platform_populate() >>>> function to create the platform device(s). The overlay could be built in >>>> drivers/clk/ too and then probably some macroish function can find the >>>> blob and apply it. >>> >>> No need to manually call of_platform_populate() to create the >>> platform devices. That is taken care of automatically when applying >>> an overlay. >>> >>>> Is there some way to delete the platform devices that we populate from >>>> the overlay? I'd like the tests to be hermetic. >> >>> Removing the overlay will delete the platform devices. >> >> I _think_ that is incorrect. Do you have a pointer to the overlay code that >> deletes the device? (If I remember correctly, the overlay remove code does not >> even check whether the device exists and whether a driver is bound to it -- but >> this is on my todo list to look into.) > > https://elixir.bootlin.com/linux/latest/source/drivers/of/platform.c#L769 Thanks! That is precisely what I failed to remember. -Frank > >>> All of that works if you have your own code to apply a DT overlay. >>> The recent fw_devlinks patches did cause some regressions, cfr. >>> https://lore.kernel.org/all/CAMuHMdXEnSD4rRJ-o90x4OprUacN_rJgyo8x6=9F9rZ+-KzjOg@mail.gmail.com > > Gr{oetje,eeting}s, > > Geert >
On Sat, Mar 4, 2023 at 9:39 AM Frank Rowand <frowand.list@gmail.com> wrote: > > On 3/2/23 17:57, Stephen Boyd wrote: > > Quoting Rob Herring (2023-03-02 12:18:34) > >> On Thu, Mar 2, 2023 at 1:44 PM Stephen Boyd <sboyd@kernel.org> wrote: > >>> > >>> Quoting Rob Herring (2023-03-02 09:13:59) > >>>> > >>>> Good to see bindings for this. I've been meaning to do something about > >>>> the DT unittest ones being undocumented, but I hadn't really decided > >>>> whether it was worth writing schemas for them. The compatibles at > >>>> least show up with 'make dt_compatible_check'. Perhaps we want to just > >>>> define some vendor (not 'linux') that's an exception rather than > >>>> requiring schemas (actually, that already works for 'foo'). > >>> > >>> Sure. Maybe "kunit" should be the vendor prefix? Or "dtbunit"? > >> > >> We'd want to use the same thing on the DT unittests or anything else > >> potentially. How about just 'test'? > > > > Sounds good. > > > >> > >>>> It's > >>>> likely that we want test DTs that fail normal checks and schemas get > >>>> in the way of that as we don't have a way to turn off checks. > >>> > >>> Having the schemas is nice to make sure tests that are expecting some > >>> binding are actually getting that. But supporting broken bindings is > >>> also important to test any error paths in functions that parse > >>> properties. Maybe we keep the schema and have it enforce that incorrect > >>> properties are being set? > >> > >> I wasn't suggesting throwing them out. More why I hadn't written any I guess. > >> > >>> Do we really need to test incorrect bindings? Doesn't the > >>> dt_bindings_check catch these problems so we don't have to write DTB > >>> verifiers in the kernel? > >> > >> Fair enough. Using my frequently stated position against me. :) > >> > >> I do have a secret plan to implement (debug) type checks into the > >> of_property_* APIs by extracting the type information from schemas > >> into C. > >> > > > > Ok. I suspect we may want to test error paths though so I don't know > > Yes, exactly. > > > what to do here. For now I'll just leave the bindings in place and > > change the prefix to "test". > > > >> > >>>> We already have GPIO tests in the DT unittests, so why is clocks > >>>> different? Or should the GPIO tests be moved out (yes, please!)? > >>> > >>> Ah I didn't notice the GPIO tests in there. There are i2c tests too, > >>> right? All I can say is clks are using kunit, that's the difference ;-) > >> > >> Yeah, they should perhaps all move to the subsystems. > > > > Got it. > > > >> > >>>> What happens when/if the DT unittest is converted to kunit? I think > >>>> that would look confusing from the naming. My initial thought is > >>>> 'kunit' should be dropped from the naming of a lot of this. Note that > >>>> the original kunit submission converted the DT unittests. I would > >>>> still like to see that happen. Frank disagreed over what's a unit test > >>>> or not, then agreed, then didn't... I don't really care. If there's a > >>>> framework to use, then we should use it IMO. > >>> > >>> Honestly I don't want to get involved in migrating the existing DT > >>> unittest code to kunit. I'm aware that it was attempted years ago when > >>> kunit was introduced. Maybe if the overlay route works well enough I can > >>> completely sidestep introducing any code in drivers/of/ besides some > >>> kunit wrappers for this. I'll cross my fingers! > >> > >> Yeah, I wasn't expecting you to. I just want to make sure this meshes > >> with any future conversion to kunit. > > > > Phew! > > > >> > >> There's also some plans to always populate the DT root node if not > >> present. That may help here. Or not. There's been a few versions > >> posted with Frank's in the last week or 2. > >> > > > > Ok. I think I have some time to try this overlay approach so let me see > > what is needed. > > Please avoid overlays. See my other replies in this thread for why. If overlays work for the constrained environment of unit tests, then use them. If overlays are not to be used, then remove the support from the kernel. Putting issues in a todo list is not going to get them done. Having users will. Rob
On 3/6/23 06:53, Rob Herring wrote: > On Sat, Mar 4, 2023 at 9:39 AM Frank Rowand <frowand.list@gmail.com> wrote: >> >> On 3/2/23 17:57, Stephen Boyd wrote: >>> Quoting Rob Herring (2023-03-02 12:18:34) >>>> On Thu, Mar 2, 2023 at 1:44 PM Stephen Boyd <sboyd@kernel.org> wrote: >>>>> >>>>> Quoting Rob Herring (2023-03-02 09:13:59) >>>>>> >>>>>> Good to see bindings for this. I've been meaning to do something about >>>>>> the DT unittest ones being undocumented, but I hadn't really decided >>>>>> whether it was worth writing schemas for them. The compatibles at >>>>>> least show up with 'make dt_compatible_check'. Perhaps we want to just >>>>>> define some vendor (not 'linux') that's an exception rather than >>>>>> requiring schemas (actually, that already works for 'foo'). >>>>> >>>>> Sure. Maybe "kunit" should be the vendor prefix? Or "dtbunit"? >>>> >>>> We'd want to use the same thing on the DT unittests or anything else >>>> potentially. How about just 'test'? >>> >>> Sounds good. >>> >>>> >>>>>> It's >>>>>> likely that we want test DTs that fail normal checks and schemas get >>>>>> in the way of that as we don't have a way to turn off checks. >>>>> >>>>> Having the schemas is nice to make sure tests that are expecting some >>>>> binding are actually getting that. But supporting broken bindings is >>>>> also important to test any error paths in functions that parse >>>>> properties. Maybe we keep the schema and have it enforce that incorrect >>>>> properties are being set? >>>> >>>> I wasn't suggesting throwing them out. More why I hadn't written any I guess. >>>> >>>>> Do we really need to test incorrect bindings? Doesn't the >>>>> dt_bindings_check catch these problems so we don't have to write DTB >>>>> verifiers in the kernel? >>>> >>>> Fair enough. Using my frequently stated position against me. :) >>>> >>>> I do have a secret plan to implement (debug) type checks into the >>>> of_property_* APIs by extracting the type information from schemas >>>> into C. >>>> >>> >>> Ok. I suspect we may want to test error paths though so I don't know >> >> Yes, exactly. >> >>> what to do here. For now I'll just leave the bindings in place and >>> change the prefix to "test". >>> >>>> >>>>>> We already have GPIO tests in the DT unittests, so why is clocks >>>>>> different? Or should the GPIO tests be moved out (yes, please!)? >>>>> >>>>> Ah I didn't notice the GPIO tests in there. There are i2c tests too, >>>>> right? All I can say is clks are using kunit, that's the difference ;-) >>>> >>>> Yeah, they should perhaps all move to the subsystems. >>> >>> Got it. >>> >>>> >>>>>> What happens when/if the DT unittest is converted to kunit? I think >>>>>> that would look confusing from the naming. My initial thought is >>>>>> 'kunit' should be dropped from the naming of a lot of this. Note that >>>>>> the original kunit submission converted the DT unittests. I would >>>>>> still like to see that happen. Frank disagreed over what's a unit test >>>>>> or not, then agreed, then didn't... I don't really care. If there's a >>>>>> framework to use, then we should use it IMO. >>>>> >>>>> Honestly I don't want to get involved in migrating the existing DT >>>>> unittest code to kunit. I'm aware that it was attempted years ago when >>>>> kunit was introduced. Maybe if the overlay route works well enough I can >>>>> completely sidestep introducing any code in drivers/of/ besides some >>>>> kunit wrappers for this. I'll cross my fingers! >>>> >>>> Yeah, I wasn't expecting you to. I just want to make sure this meshes >>>> with any future conversion to kunit. >>> >>> Phew! >>> >>>> >>>> There's also some plans to always populate the DT root node if not >>>> present. That may help here. Or not. There's been a few versions >>>> posted with Frank's in the last week or 2. >>>> >>> >>> Ok. I think I have some time to try this overlay approach so let me see >>> what is needed. >> >> Please avoid overlays. See my other replies in this thread for why. > > If overlays work for the constrained environment of unit tests, then > use them. If overlays are not to be used, then remove the support from > the kernel. Putting issues in a todo list is not going to get them > done. Having users will. Overlays are not used to enable OF unittests that are unrelated to overlays (to the best of my memory - I reserve the right to be corrected). Overlay usage in OF unittests is specifically to test overlay features. > > Rob
On Sat, 4 Mar 2023 at 23:50, Frank Rowand <frowand.list@gmail.com> wrote: > > On 3/1/23 19:38, Stephen Boyd wrote: > > This patch series adds unit tests for the clk fixed rate basic type and > > the clk registration functions that use struct clk_parent_data. To get > > there, we add support for loading a DTB into the UML kernel that's > > running the unit tests along with probing platform drivers to bind to > > device nodes specified in DT. > > > > With this series, we're able to exercise some of the code in the common > > clk framework that uses devicetree lookups to find parents and the fixed > > rate clk code that scans devicetree directly and creates clks. Please > > review. > > I would _really_ like to _not_ have devicetree tests in two locations: > DT unittests and kunit tests. > I agree we don't want to split things up needlessly, but I think there is a meaningful distinction between: - Testing the DT infrastructure itself (with DT unittests) - Testing a driver which may have some interaction with DT (via KUnit) So, rather than going for a "devicetree" KUnit suite (unless we wanted to port OF_UNITTEST to KUnit, which as you point out, would involve a fair bit of reworking), I think the goal is for there to be lots of driver test suites, each of which may verify that their specific properties can be loaded from the devicetree correctly. This is also why I prefer the overlay method, if we can get it to work: it makes it clearer that the organisational hierarchy for these tests is [driver]->[devicetree], not [devicetree]->[drvier]. > For my testing, I already build and boot four times on real hardware: > > 1) no DT unittests > 2) CONFIG_OF_UNITTEST > 3) CONFIG_OF_UNITTEST > CONFIG_OF_DYNAMIC > 4) CONFIG_OF_UNITTEST > CONFIG_OF_DYNAMIC > CONFIG_OF_OVERLAY > > I really should also be testing the four configurations on UML, but at > the moment I am not. > > I also check for new compile warnings at various warn levels for all > four configurations. > > If I recall correctly, the kunit framework encourages more (many more?) > kunit config options to select which test(s) are build for a test run. > Someone please correct this paragraph if I am mis-stating. We do tend to suggest that there is a separate kconfig option for each area being tested (usually one per test suite, but if there are several closely related suites, sticking them under a single config option isn't a problem.) That being said: - It's possible (and encouraged) to just test once with all of those tests enabled, rather than needing to test every possible combination of configs enabled/disabled. - (Indeed, this is what we do with .kunitconfig files a lot: they're collections of related configs, so you can quickly run, e.g., all DRM tests) - Because a KUnit test being run is an independent action from it being built-in, it's possible to build the tests once and then just run different subsets anyway, or possibly run them after boot if they're compiled as modules. - This of course, depends on two test configs not conflicting with each other: obviously if there were some tests which relied on OF_OVERLAY=n, and others which require OF_OVERLAY=y, you'd need two builds. The bigger point is that, if the KUnit tests are focused on individual drivers, rather than the devicetree infrastructure itself, then these probably aren't as critical to run on every devicetree change (the DT unittests should hopefully catch anything which affects devicetree as a whole), but only on tests which affect a specific driver (as they're really intended to make sure the drivers are accessing / interacting with the DT properly, not that the DT infrastructure functions). And obviously if this KUnit/devicetree support ends up depending on overlays, that means there's no need to test them with overlays disabled. :-) > > Adding devicetree tests to kunit adds additional build and boot cycles > and additional test output streams to verify. > > Are there any issues with DT unittests that preclude adding clk tests > into the DT unittests? > I think at least part of it is that there are already some clk KUnit tests, so it's easier to have all of the clk tests behave similarly (for the same reasons, alas, as using DT unittests makes it easier to keep all of the DT tests in the same place). Of course, as DT unittests move to KTAP, and possibly in the future are able to make use of more KUnit infrastructure, this should get simpler for everyone. Does that seem sensible? -- David
On 3/10/23 01:48, David Gow wrote: > On Sat, 4 Mar 2023 at 23:50, Frank Rowand <frowand.list@gmail.com> wrote: >> >> On 3/1/23 19:38, Stephen Boyd wrote: >>> This patch series adds unit tests for the clk fixed rate basic type and >>> the clk registration functions that use struct clk_parent_data. To get >>> there, we add support for loading a DTB into the UML kernel that's >>> running the unit tests along with probing platform drivers to bind to >>> device nodes specified in DT. >>> >>> With this series, we're able to exercise some of the code in the common >>> clk framework that uses devicetree lookups to find parents and the fixed >>> rate clk code that scans devicetree directly and creates clks. Please >>> review. >> >> I would _really_ like to _not_ have devicetree tests in two locations: >> DT unittests and kunit tests. >> > This: > I agree we don't want to split things up needlessly, but I think there > is a meaningful distinction between: > - Testing the DT infrastructure itself (with DT unittests) > - Testing a driver which may have some interaction with DT (via KUnit) > > So, rather than going for a "devicetree" KUnit suite (unless we wanted > to port OF_UNITTEST to KUnit, which as you point out, would involve a > fair bit of reworking), I think the goal is for there to be lots of > driver test suites, each of which may verify that their specific > properties can be loaded from the devicetree correctly. > > This is also why I prefer the overlay method, if we can get it to > work: it makes it clearer that the organisational hierarchy for these > tests is [driver]->[devicetree], not [devicetree]->[drvier]. > >> For my testing, I already build and boot four times on real hardware: >> >> 1) no DT unittests >> 2) CONFIG_OF_UNITTEST >> 3) CONFIG_OF_UNITTEST >> CONFIG_OF_DYNAMIC >> 4) CONFIG_OF_UNITTEST >> CONFIG_OF_DYNAMIC >> CONFIG_OF_OVERLAY >> >> I really should also be testing the four configurations on UML, but at >> the moment I am not. >> >> I also check for new compile warnings at various warn levels for all >> four configurations. >> >> If I recall correctly, the kunit framework encourages more (many more?) >> kunit config options to select which test(s) are build for a test run. >> Someone please correct this paragraph if I am mis-stating. > > We do tend to suggest that there is a separate kconfig option for each > area being tested (usually one per test suite, but if there are > several closely related suites, sticking them under a single config > option isn't a problem.) > > That being said: > - It's possible (and encouraged) to just test once with all of those > tests enabled, rather than needing to test every possible combination > of configs enabled/disabled. > - (Indeed, this is what we do with .kunitconfig files a lot: they're > collections of related configs, so you can quickly run, e.g., all DRM > tests) > - Because a KUnit test being run is an independent action from it > being built-in, it's possible to build the tests once and then just > run different subsets anyway, or possibly run them after boot if > they're compiled as modules. > - This of course, depends on two test configs not conflicting with > each other: obviously if there were some tests which relied on > OF_OVERLAY=n, and others which require OF_OVERLAY=y, you'd need two > builds. > And this: > The bigger point is that, if the KUnit tests are focused on individual > drivers, rather than the devicetree infrastructure itself, then these > probably aren't as critical to run on every devicetree change (the DT > unittests should hopefully catch anything which affects devicetree as > a whole), but only on tests which affect a specific driver (as they're > really intended to make sure the drivers are accessing / interacting > with the DT properly, not that the DT infrastructure functions). Those two paragraphs are correct, and my original assumption was wrong. These tests appear to mostly be clock related and only minimally and indirectly test devicetree functionality. In more generic terms, they are driver tests, not devicetree tests. Thus I withdraw my concern of making the devicetree test environment more complicated. > > And obviously if this KUnit/devicetree support ends up depending on > overlays, that means there's no need to test them with overlays > disabled. :-) > >> >> Adding devicetree tests to kunit adds additional build and boot cycles >> and additional test output streams to verify. >> >> Are there any issues with DT unittests that preclude adding clk tests >> into the DT unittests? >> > > I think at least part of it is that there are already some clk KUnit > tests, so it's easier to have all of the clk tests behave similarly > (for the same reasons, alas, as using DT unittests makes it easier to > keep all of the DT tests in the same place). > > Of course, as DT unittests move to KTAP, and possibly in the future > are able to make use of more KUnit infrastructure, this should get > simpler for everyone. I hope to move DT unitests to create KTAP V2 compatible data as a first step. I highly doubt that DT unittests fit the kunit model, but that would be a question that could be considered after DT unittests move to the KTAP V2 data format. > > > Does that seem sensible? Yes, thanks for the extra explanations. > > -- David