build error: f7655d42fcee drm/edid: Add CTA-861-G modes with VIC >= 193

[Frank Rowand]

I am getting a kernel build error at version 5.6-rc1:

drivers/gpu/drm/drm_edid.c: In function 'cea_mode_alternate_timings':
drivers/gpu/drm/drm_edid.c:3275:2: error: call to '__compiletime_assert_3282' 
declared with attribute error: BUILD_BUG_ON failed: cea_mode_for_vic(8)->vtotal 
!= 262 || cea_mode_for_vic(9)->vtotal != 262 || cea_mode_for_vic(12)->vtotal != 
262 || cea_mode_for_vic(13)->vtotal != 262 || cea_mode_for_vic(23)->vtotal != 
312 || cea_mode_for_vic(24)->vtotal != 312 || cea_mode_for_vic(27)->vtotal != 
312 || cea_mode_for_vic(28)->vtotal != 312
make[4]: *** [drivers/gpu/drm/drm_edid.o] Error 1


Kernel configuration:
  make qcom_defconfig

  (arch/arm/configs/qcom_defconfig)


Compiler is arm-eabi-gcc 4.6.x


The build error goes away if I revert:

   f7655d42fcee drm/edid: Add CTA-861-G modes with VIC >= 193


The build error also goes away if I comment out the two lines added to 
cea_mode_for_vic():

if (vic >= 193 && vic < 193 + ARRAY_SIZE(edid_cea_modes_193))
return _cea_modes_193[vic - 193];


-Frank
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: build error: f7655d42fcee drm/edid: Add CTA-861-G modes with VIC >= 193

[Frank Rowand]

On 2/11/20 12:48 PM, Frank Rowand wrote:
> I am getting a kernel build error at version 5.6-rc1:
> 
> drivers/gpu/drm/drm_edid.c: In function 'cea_mode_alternate_timings':
> drivers/gpu/drm/drm_edid.c:3275:2: error: call to '__compiletime_assert_3282' 
> declared with attribute error: BUILD_BUG_ON failed: 
> cea_mode_for_vic(8)->vtotal != 262 || cea_mode_for_vic(9)->vtotal != 262 || 
> cea_mode_for_vic(12)->vtotal != 262 || cea_mode_for_vic(13)->vtotal != 262 || 
> cea_mode_for_vic(23)->vtotal != 312 || cea_mode_for_vic(24)->vtotal != 312 || 
> cea_mode_for_vic(27)->vtotal != 312 || cea_mode_for_vic(28)->vtotal != 312
> make[4]: *** [drivers/gpu/drm/drm_edid.o] Error 1
> 
> 
> Kernel configuration:
>   make qcom_defconfig
> 
>   (arch/arm/configs/qcom_defconfig)
> 
> 
> Compiler is arm-eabi-gcc 4.6.x
> 
> 
> The build error goes away if I revert:
> 
>f7655d42fcee drm/edid: Add CTA-861-G modes with VIC >= 193

I did not actually revert f7655d42fcee from 5.6-rc1.  I checked out the commit
before f7655d42fcee and was able to build the kernel.  Then I checked out
f7655d42fcee and the build failed in the same manner as at 5.6-rc1.

Simply reverting f7655d42fcee at 5.6-rc1 does not result in a buildable
kernel due to other related changes.

-Frank

> 
> 
> The build error also goes away if I comment out the two lines added to 
> cea_mode_for_vic():
> 
>   if (vic >= 193 && vic < 193 + ARRAY_SIZE(edid_cea_modes_193))
>   return _cea_modes_193[vic - 193];
> 
> 
> -Frank
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v3 18/19] of: unittest: split out a couple of test cases from unittest

[Frank Rowand]

On 9/20/19 9:57 AM, Rob Herring wrote:
> Following up from LPC discussions...
> 
> On Thu, Mar 21, 2019 at 8:30 PM Brendan Higgins
>  wrote:
>>
>> On Thu, Mar 21, 2019 at 5:22 PM Frank Rowand  wrote:
>>>
>>> On 2/27/19 7:52 PM, Brendan Higgins wrote:
>>>> On Wed, Feb 20, 2019 at 12:45 PM Frank Rowand  
>>>> wrote:
>>>>>
>>>>> On 2/18/19 2:25 PM, Frank Rowand wrote:
>>>>>> On 2/15/19 2:56 AM, Brendan Higgins wrote:
>>>>>>> On Thu, Feb 14, 2019 at 6:05 PM Frank Rowand  
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> On 2/14/19 4:56 PM, Brendan Higgins wrote:
>>>>>>>>> On Thu, Feb 14, 2019 at 3:57 PM Frank Rowand  
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> On 12/5/18 3:54 PM, Brendan Higgins wrote:
>>>>>>>>>>> On Tue, Dec 4, 2018 at 2:58 AM Frank Rowand 
>>>>>>>>>>>  wrote:
>> < snip >
>>>>>
>>>>> In the base version, the order of execution of the test code requires
>>>>> bouncing back and forth between the test functions and the coding of
>>>>> of_test_find_node_by_name_cases[].
>>>>
>>>> You shouldn't need to bounce back and forth because the order in which
>>>> the tests run shouldn't matter.
>>>
>>> If one can't guarantee total independence of all of the tests, with no
>>> side effects, then yes.  But that is not my world.  To make that
>>> guarantee, I would need to be able to run just a single test in an
>>> entire test run.
>>>
>>> I actually want to make side effects possible.  Whether from other
>>> tests or from live kernel code that is accessing the live devicetree.
>>> Any extra stress makes me happier.
>>>
>>> I forget the exact term that has been tossed around, but to me the
>>> devicetree unittests are more like system validation, release tests,
>>> acceptance tests, and stress tests.  Not unit tests in the philosophy
>>> of KUnit.
>>
>> Ah, I understand. I thought that they were actually trying to be unit
>> tests; that pretty much voids this discussion then. Integration tests
>> and end to end tests are valuable as long as that is actually what you
>> are trying to do.
> 
> There's a mixture. There's a whole bunch of tests that are basically
> just testing various DT APIs and use a static DT. Those are all unit
> tests IMO.
> 
> Then there's all the overlay tests Frank has added. I guess some of
> those are not unittests in the strictest sense. Regardless, if we're
> reporting test results, we should align our reporting with what will
> become the rest of the kernel.

The last time I talked to you at lpc, I was still resisting moving the
DT unittests to the kunit framework.  But I think I am on board now.

Brendan agreed to accept a kunit patch from me (when I write it) to enable
the DT unittests to report # of tests run and # of tests passed/failed,
as is currently the case.

Brendan also agreed that null initialization and clean up would be ok
for the DT unittests.  But that he does not want that model to be
frequently used.  (You mention this idea later in the email I am
replying to.)


> 
>>> I do see the value of pure unit tests, and there are rare times that
>>> my devicetree use case might be better served by that approach.  But
>>> if so, it is very easy for me to add a simple pure test when debugging.
>>> My general use case does not map onto this model.
>>
>> Why do you think it is rare that you would actually want unit tests?
> 
> I don't. We should have a unittest (or multiple) for every single DT
> API call and that should be a requirement to add any new APIs.
> 
>> I mean, if you don't get much code churn, then maybe it's not going to
>> provide you a ton of value to immediately go and write a bunch of unit
>> tests right now, but I can't think of a single time where it's hurt.
>> Unit tests, from my experience, are usually the easiest tests to
>> maintain, and the most helpful when I am developing.
>>
>> Maybe I need to understand your use case better.
>>
>>>
>>>
>>>>>
>>>>> In the frank version the order of execution of the test code is obvious.
>>>>
>>>> So I know we were arguing before over whether order *does* matter in
>>>> some of the other test cases (none in the example that you or I
>>>> posted), but wouldn't it be b

Re: [PATCH v5 00/18] kunit: introduce KUnit, the Linux kernel unit testing framework

[Frank Rowand]

Hi Brendan,

I am only responding to this because you asked me to in the v4 thread.

Thank you for evaluating my comments in the v4 thread and asking me to
comment on v5

On 6/17/19 1:25 AM, Brendan Higgins wrote:
> ## TL;DR
> 
> A not so quick follow-up to Stephen's suggestions on PATCH v4. Nothing
> that really changes any functionality or usage with the minor exception
> of a couple public functions that Stephen asked me to rename.
> Nevertheless, a good deal of clean up and fixes. See changes below.
> 
> As for our current status, right now we got Reviewed-bys on all patches
> except:
> 
> - [PATCH v5 08/18] objtool: add kunit_try_catch_throw to the noreturn
>   list
> 
> However, it would probably be good to get reviews/acks from the
> subsystem maintainers on:
> 
> - [PATCH v5 06/18] kbuild: enable building KUnit
> - [PATCH v5 08/18] objtool: add kunit_try_catch_throw to the noreturn
>   list
> - [PATCH v5 15/18] Documentation: kunit: add documentation for KUnit
> - [PATCH v5 17/18] kernel/sysctl-test: Add null pointer test for
>   sysctl.c:proc_dointvec()
> - [PATCH v5 18/18] MAINTAINERS: add proc sysctl KUnit test to PROC
>   SYSCTL section
> 
> Other than that, I think we should be good to go.
> 
> One last thing, I updated the background to include my thoughts on KUnit
> vs. in kernel testing with kselftest in the background sections as
> suggested by Frank in the discussion on PATCH v2.
> 
> ## Background
> 
> This patch set proposes KUnit, a lightweight unit testing and mocking
> framework for the Linux kernel.
> 
> Unlike Autotest and kselftest, KUnit is a true unit testing framework;
> it does not require installing the kernel on a test machine or in a VM
> (however, KUnit still allows you to run tests on test machines or in VMs
> if you want[1]) and does not require tests to be written in userspace
> running on a host kernel. Additionally, KUnit is fast: From invocation
> to completion KUnit can run several dozen tests in under a second.
> Currently, the entire KUnit test suite for KUnit runs in under a second
> from the initial invocation (build time excluded).
> 
> KUnit is heavily inspired by JUnit, Python's unittest.mock, and
> Googletest/Googlemock for C++. KUnit provides facilities for defining
> unit test cases, grouping related test cases into test suites, providing
> common infrastructure for running tests, mocking, spying, and much more.
> 

I looked only at this section, as was specifically requested:

> ### But wait! Doesn't kselftest support in kernel testing?!
> 
> In a previous version of this patchset Frank pointed out that kselftest
> already supports writing a test that resides in the kernel using the
> test module feature[2]. LWN did a really great summary on this
> discussion here[3].
> 
> Kselftest has a feature that allows a test module to be loaded into a
> kernel using the kselftest framework; this does allow someone to write
> tests against kernel code not directly exposed to userland; however, it
> does not provide much of a framework around how to structure the tests.
> The kselftest test module feature just provides a header which has a
> standardized way of reporting test failures, 


> and then provides
> infrastructure to load and run the tests using the kselftest test
> harness.

The in-kernel tests can also be invoked at boot time if they are
configured (Kconfig) as in-kernel instead of as modules.  I did not
check how many of the tests have tri-state configuration to allow
this, but the few that I looked at did.

> 
> The kselftest test module does not seem to be opinionated at all in
> regards to how tests are structured, how they check for failures, how
> tests are organized. Even in the method it provides for reporting
> failures is pretty simple; it doesn't have any more advanced failure
> reporting or logging features. Given what's there, I think it is fair to
> say that it is not actually a framework, but a feature that makes it
> possible for someone to do some checks in kernel space.

I would call that description a little dismissive.  The set of in-kernel
tests that I looked like followed a common pattern and reported results
in a uniform manner.

> 
> Furthermore, kselftest test module has very few users. I checked for all
> the tests that use it using the following grep command:
> 
> grep -Hrn -e 'kselftest_module\.h'
> 
> and only got three results: lib/test_strscpy.c, lib/test_printf.c, and
> lib/test_bitmap.c.

You missed many tests.  I listed much more than that in the v4 thread, and
someone else also listed more in the v4 thread.


> 
> So despite kselftest test module's existence, there really is no feature
> overlap between kselftest and KUnit, save one: that you can use either
> to write an in-kernel test, but this is a very small feature in
> comparison to everything that KUnit allows you to do. KUnit is a full
> x-unit style unit testing framework, whereas kselftest looks a lot more
> like an end-to-end/functional testing framework, 

Re: [PATCH v2 00/17] kunit: introduce KUnit, the Linux kernel unit testing framework

[Frank Rowand]

On 5/14/19 1:38 AM, Brendan Higgins wrote:
> On Fri, May 10, 2019 at 03:13:40PM -0700, Frank Rowand wrote:
>> On 5/10/19 9:17 AM, Logan Gunthorpe wrote:
>>>
>>>
>>> On 2019-05-09 11:18 p.m., Frank Rowand wrote:
>>>
>>>> YES, kselftest has in-kernel tests.  (Excuse the shouting...)
>>>
>>> Cool. From my cursory look, in my opinion, these would be greatly
>>> improved by converting them to the framework Brendan is proposing for Kunit.
>>>
>>>>> If they do exists, it seems like it would make sense to
>>>>> convert those to kunit and have Kunit tests run-able in a VM or
>>>>> baremetal instance.
>>>>
>>>> They already run in a VM.
>>>>
>>>> They already run on bare metal.
>>>>
>>>> They already run in UML.
>>>
>>> Simply being able to run in UML is not the only thing here. Kunit
>>> provides the infrastructure to quickly build, run and report results for
>>> all the tests from userspace without needing to worry about the details
>>> of building and running a UML kernel, then parsing dmesg to figure out
>>> what tests were run or not.
>>
>> Yes.  But that is not the only environment that KUnit must support to be
>> of use to me for devicetree unittests (this is not new, Brendan is quite
>> aware of my needs and is not ignoring them).
>>
>>
>>>> This is not to say that KUnit does not make sense.  But I'm still trying
>>>> to get a better description of the KUnit features (and there are
>>>> some).
>>>
>>> So read the patches, or the documentation[1] or the LWN article[2]. It's
>>> pretty well described in a lot of places -- that's one of the big
>>> advantages of it. In contrast, few people seems to have any concept of
>>> what kselftests are or where they are or how to run them (I was
>>> surprised to find the in-kernel tests in the lib tree).
>>>
>>> Logan
>>>
>>> [1] https://google.github.io/kunit-docs/third_party/kernel/docs/
>>> [2] https://lwn.net/Articles/780985/
>>
>> I have been following the RFC versions.  I have installed the RFC patches
>> and run them to the extent that they worked (devicetree unittests were
>> a guinea pig for test conversion in the RFC series, but the converted
>> tests did not work).  I read portions of the code while trying to
>> understand the unittests conversion.  I made review comments based on
>> the portion of the code that I did read.  I have read the documentation
>> (very nice btw, as I have said before, but should be expanded).
>>
>> My comment is that the description to submit the patch series should
>> be fuller -- KUnit potentially has a lot of nice attributes, and I
>> still think I have only scratched the surface.  The average reviewer
>> may have even less in-depth knowledge than I do.  And as I have
>> commented before, I keep diving into areas that I had no previous
>> experience with (such as kselftest) to be able to properly judge this
>> patch series.
> 
> Thanks for the praise! That means a lot coming from you!
> 
> I really cannot disagree that I could use more documentation. You can
> pretty much always use more documentation. Nevertheless, is there a
> particular part of the documentation that you think it lacking?

I wasn't talking about the documentation that is part of KUnit.  I was
targeting patch 0.


> It sounds like there was a pretty long discussion here about, a number
> of different things.
> 
> Do you want a better description of what unit testing is and how KUnit
> helps make it possible?
> 
> Do you want more of an explanation distinguishing KUnit from kselftest?
> How so?

The high level issue is to provide reviewers with enough context to be
able to evaluate the patch series.  That is probably not very obvious
at this point in the thread.  At this point I was responding to Logan's
response to me that I should be reading Documentation to get a better
description of KUnit features.  I _think_ that Logan thought that I
did not understand KUnit features and was trying to be helpful by
pointing out where I could get more information.  If so, he was missing
my intended point had been that patch 0 should provide more information
to justify adding this feature.

One thing that has become very apparent in the discussion of this patch
series is that some people do not understand that kselftest includes
in-kernel tests, not just userspace tests.  As such, KUnit is an
additional implementation of "the same feature".  (One can debate
exactly which in-kernel test featu

Re: [PATCH v2 00/17] kunit: introduce KUnit, the Linux kernel unit testing framework

[Frank Rowand]

On 5/11/19 10:33 AM, Theodore Ts'o wrote:
> On Fri, May 10, 2019 at 02:12:40PM -0700, Frank Rowand wrote:
>> However, the reply is incorrect.  Kselftest in-kernel tests (which
>> is the context here) can be configured as built in instead of as
>> a module, and built in a UML kernel.  The UML kernel can boot,
>> running the in-kernel tests before UML attempts to invoke the
>> init process.
> 
> Um, Citation needed?

The paragraph that you quoted tells you exactly how to run a kselftest
in-kernel test in a UML kernel.  Just to what that paragraph says.


> 
> I don't see any evidence for this in the kselftest documentation, nor
> do I see any evidence of this in the kselftest Makefiles.
> 
> There exists test modules in the kernel that run before the init
> scripts run --- but that's not strictly speaking part of kselftests,
> and do not have any kind of infrastructure.  As noted, the
> kselftests_harness header file fundamentally assumes that you are
> running test code in userspace.

You are ignoring the kselftest in-kernel tests.

We are talking in circles.  I'm done with this thread.

-Frank

> 
>   - Ted
> .
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 00/17] kunit: introduce KUnit, the Linux kernel unit testing framework

[Frank Rowand]

Hi Ted,

I'll try answering this again.

The first time I was a little flippant in part of my answer because I
thought your comments somewhat flippant.  This time I'll provide a
more complete answer.


On 5/8/19 7:13 PM, Frank Rowand wrote:
> On 5/8/19 6:58 PM, Theodore Ts'o wrote:
>> On Wed, May 08, 2019 at 05:43:35PM -0700, Frank Rowand wrote:

*  begin context from Greg KH that you snipped  *

On 5/7/19 1:01 AM, Greg KH wrote:

<< note that I have snipped my original question above this point >>

>
> kselftest provides no in-kernel framework for testing kernel code
> specifically.  That should be what kunit provides, an "easy" way to
> write in-kernel tests for things.

*  end   context from Greg KH that you snipped  *

>>> kselftest provides a mechanism for in-kernel tests via modules.  For
>>> example, see:
>>>
>>>   tools/testing/selftests/vm/run_vmtests invokes:
>>> tools/testing/selftests/vm/test_vmalloc.sh
>>>   loads module:
>>> test_vmalloc
>>> (which is built from lib/test_vmalloc.c if CONFIG_TEST_VMALLOC)
>>
>> The majority of the kselftests are implemented as userspace programs.

My flippant answer:

> Non-argument.

This time:

My reply to Greg was pointing out that in-kernel tests do exist in
kselftest.

Your comment that the majority of kselftests are implemented as userspace
programs has no bearing on whether kselftest support in-kernel tests.
It does not counter the fact the kselftest supports in-kernel tests.


>> You *can* run in-kernel test using modules; but there is no framework
>> for the in-kernel code found in the test modules, which means each of
>> the in-kernel code has to create their own in-kernel test
>> infrastructure.

The kselftest in-kernel tests follow a common pattern.  As such, there
is a framework.

This next two paragraphs you ignored entirely in your reply:

> Why create an entire new subsystem (KUnit) when you can add a header
> file (and .c code as appropriate) that outputs the proper TAP formatted
> results from kselftest kernel test modules?
> 
> There are already a multitude of in kernel test modules used by
> kselftest.  It would be good if they all used a common TAP compliant
> mechanism to report results.


 
>> That's much like saying you can use vice grips to turn a nut or
>> bolt-head.  You *can*, but it might be that using a monkey wrench
>> would be a much better tool that is much easier.
>>
>> What would you say to a wood worker objecting that a toolbox should
>> contain a monkey wrench because he already knows how to use vise
>> grips, and his tiny brain shouldn't be forced to learn how to use a
>> wrench when he knows how to use a vise grip, which is a perfectly good
>> tool?
>>
>> If you want to use vice grips as a hammer, screwdriver, monkey wrench,
>> etc.  there's nothing stopping you from doing that.  But it's not fair
>> to object to other people who might want to use better tools.
>>
>> The reality is that we have a lot of testing tools.  It's not just
>> kselftests.  There is xfstests for file system code, blktests for
>> block layer tests, etc.   We use the right tool for the right job.

My flippant answer:

> More specious arguments.

This time:

I took your answer as a straw man, and had no desire to spend time
countering a straw man.

I am not proposing using a vice grips (to use your analogy).  I
am saying that maybe the monkey wrench already exists.

My point of this whole thread has been to try to get the submitter
to provide a better, more complete explanation of how and why KUnit 
is a better tool.

I have not yet objected to the number (and differences among) the
many sub-system tests.  I am questioning whether there is a need
for another _test framework_ for in-kernel testing.  If there is
something important that KUnit provides that does not exist in
existing frameworks then the next question would be to ask how
to implement that important thing (improve the existing
framework, replace the existing framework, or have two
frameworks).  I still think it is premature to ask this
question until we first know the answer to what unique features
KUnit adds (and apparently until we know what the existing
framework provides).

-Frank

> 
> -Frank
> 
>>
>>  - Ted
>>
> 
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 00/17] kunit: introduce KUnit, the Linux kernel unit testing framework

[Frank Rowand]

On 5/9/19 4:40 PM, Logan Gunthorpe wrote:
> 
> 
> On 2019-05-09 5:30 p.m., Theodore Ts'o wrote:
>> On Thu, May 09, 2019 at 04:20:05PM -0600, Logan Gunthorpe wrote:
>>>
>>> The second item, arguably, does have significant overlap with kselftest.
>>> Whether you are running short tests in a light weight UML environment or
>>> higher level tests in an heavier VM the two could be using the same
>>> framework for writing or defining in-kernel tests. It *may* also be valuable
>>> for some people to be able to run all the UML tests in the heavy VM
>>> environment along side other higher level tests.
>>>
>>> Looking at the selftests tree in the repo, we already have similar items to
>>> what Kunit is adding as I described in point (2) above. kselftest_harness.h
>>> contains macros like EXPECT_* and ASSERT_* with very similar intentions to
>>> the new KUNIT_EXECPT_* and KUNIT_ASSERT_* macros.
>>>
>>> However, the number of users of this harness appears to be quite small. Most
>>> of the code in the selftests tree seems to be a random mismash of scripts
>>> and userspace code so it's not hard to see it as something completely
>>> different from the new Kunit:
>>>
>>> $ git grep --files-with-matches kselftest_harness.h *
>>
>> To the extent that we can unify how tests are written, I agree that
>> this would be a good thing.  However, you should note that
>> kselftest_harness.h is currently assums that it will be included in
>> userspace programs.  This is most obviously seen if you look closely
>> at the functions defined in the header files which makes calls to
>> fork(), abort() and fprintf().
> 
> Ah, yes. I obviously did not dig deep enough. Using kunit for
> in-kernel tests and kselftest_harness for userspace tests seems like
> a sensible line to draw to me. Trying to unify kernel and userspace
> here sounds like it could be difficult so it's probably not worth
> forcing the issue unless someone wants to do some really fancy work
> to get it done.
> 
> Based on some of the other commenters, I was under the impression
> that kselftests had in-kernel tests but I'm not sure where or if they
> exist.

YES, kselftest has in-kernel tests.  (Excuse the shouting...)

Here is a likely list of them in the kernel source tree:

$ grep module_init lib/test_*.c
lib/test_bitfield.c:module_init(test_bitfields)
lib/test_bitmap.c:module_init(test_bitmap_init);
lib/test_bpf.c:module_init(test_bpf_init);
lib/test_debug_virtual.c:module_init(test_debug_virtual_init);
lib/test_firmware.c:module_init(test_firmware_init);
lib/test_hash.c:module_init(test_hash_init);/* Does everything */
lib/test_hexdump.c:module_init(test_hexdump_init);
lib/test_ida.c:module_init(ida_checks);
lib/test_kasan.c:module_init(kmalloc_tests_init);
lib/test_list_sort.c:module_init(list_sort_test);
lib/test_memcat_p.c:module_init(test_memcat_p_init);
lib/test_module.c:static int __init test_module_init(void)
lib/test_module.c:module_init(test_module_init);
lib/test_objagg.c:module_init(test_objagg_init);
lib/test_overflow.c:static int __init test_module_init(void)
lib/test_overflow.c:module_init(test_module_init);
lib/test_parman.c:module_init(test_parman_init);
lib/test_printf.c:module_init(test_printf_init);
lib/test_rhashtable.c:module_init(test_rht_init);
lib/test_siphash.c:module_init(siphash_test_init);
lib/test_sort.c:module_init(test_sort_init);
lib/test_stackinit.c:module_init(test_stackinit_init);
lib/test_static_key_base.c:module_init(test_static_key_base_init);
lib/test_static_keys.c:module_init(test_static_key_init);
lib/test_string.c:module_init(string_selftest_init);
lib/test_ubsan.c:module_init(test_ubsan_init);
lib/test_user_copy.c:module_init(test_user_copy_init);
lib/test_uuid.c:module_init(test_uuid_init);
lib/test_vmalloc.c:module_init(vmalloc_test_init)
lib/test_xarray.c:module_init(xarray_checks);


> If they do exists, it seems like it would make sense to
> convert those to kunit and have Kunit tests run-able in a VM or
> baremetal instance.

They already run in a VM.

They already run on bare metal.

They already run in UML.

This is not to say that KUnit does not make sense.  But I'm still trying
to get a better description of the KUnit features (and there are
some).

-Frank
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 00/17] kunit: introduce KUnit, the Linux kernel unit testing framework

[Frank Rowand]

On 5/8/19 6:44 PM, Theodore Ts'o wrote:
> On Wed, May 08, 2019 at 05:58:49PM -0700, Frank Rowand wrote:
>>
>> If KUnit is added to the kernel, and a subsystem that I am submitting
>> code for has chosen to use KUnit instead of kselftest, then yes, I do
>> *have* to use KUnit if my submission needs to contain a test for the
>> code unless I want to convince the maintainer that somehow my case
>> is special and I prefer to use kselftest instead of KUnittest.
> 
> That's going to be between you and the maintainer.  Today, if you want
> to submit a substantive change to xfs or ext4, you're going to be
> asked to create test for that new feature using xfstests.  It doesn't
> matter that xfstests isn't in the kernel --- if that's what is
> required by the maintainer.

Yes, that is exactly what I was saying.

Please do not cut the pertinent parts of context that I am replying to.


>>> supposed to be a simple way to run a large number of small tests that
>>> for specific small components in a system.
>>
>> kselftest also supports running a subset of tests.  That subset of tests
>> can also be a large number of small tests.  There is nothing inherent
>> in KUnit vs kselftest in this regard, as far as I am aware.


> The big difference is that kselftests are driven by a C program that
> runs in userspace.  Take a look at 
> tools/testing/selftests/filesystem/dnotify_test.c
> it has a main(int argc, char *argv) function.
> 
> In contrast, KUnit are fragments of C code which run in the kernel;
> not in userspace.  This allows us to test internal functions inside
> complex file system (such as the block allocator in ext4) directly.
> This makes it *fundamentally* different from kselftest.

No, totally incorrect.  kselftests also supports in kernel modules as
I mention in another reply to this patch.

This is talking past each other a little bit, because your next reply
is a reply to my email about modules.

-Frank

> 
> Cheers,
> 
>   - Ted
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 00/17] kunit: introduce KUnit, the Linux kernel unit testing framework

[Frank Rowand]

On 5/8/19 6:58 PM, Theodore Ts'o wrote:
> On Wed, May 08, 2019 at 05:43:35PM -0700, Frank Rowand wrote:
>> kselftest provides a mechanism for in-kernel tests via modules.  For
>> example, see:
>>
>>   tools/testing/selftests/vm/run_vmtests invokes:
>> tools/testing/selftests/vm/test_vmalloc.sh
>>   loads module:
>> test_vmalloc
>> (which is built from lib/test_vmalloc.c if CONFIG_TEST_VMALLOC)
> 
> The majority of the kselftests are implemented as userspace programs.

Non-argument.


> You *can* run in-kernel test using modules; but there is no framework
> for the in-kernel code found in the test modules, which means each of
> the in-kernel code has to create their own in-kernel test
> infrastructure.

Why create an entire new subsystem (KUnit) when you can add a header
file (and .c code as appropriate) that outputs the proper TAP formatted
results from kselftest kernel test modules?

There are already a multitude of in kernel test modules used by
kselftest.  It would be good if they all used a common TAP compliant
mechanism to report results.

 
> That's much like saying you can use vice grips to turn a nut or
> bolt-head.  You *can*, but it might be that using a monkey wrench
> would be a much better tool that is much easier.
> 
> What would you say to a wood worker objecting that a toolbox should
> contain a monkey wrench because he already knows how to use vise
> grips, and his tiny brain shouldn't be forced to learn how to use a
> wrench when he knows how to use a vise grip, which is a perfectly good
> tool?
> 
> If you want to use vice grips as a hammer, screwdriver, monkey wrench,
> etc.  there's nothing stopping you from doing that.  But it's not fair
> to object to other people who might want to use better tools.
> 
> The reality is that we have a lot of testing tools.  It's not just
> kselftests.  There is xfstests for file system code, blktests for
> block layer tests, etc.   We use the right tool for the right job.

More specious arguments.

-Frank

> 
>   - Ted
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 00/17] kunit: introduce KUnit, the Linux kernel unit testing framework

[Frank Rowand]

On 5/7/19 1:01 AM, Greg KH wrote:
> On Mon, May 06, 2019 at 08:14:12PM -0700, Frank Rowand wrote:
>> On 5/1/19 4:01 PM, Brendan Higgins wrote:
>>> ## TLDR
>>>
>>> I rebased the last patchset on 5.1-rc7 in hopes that we can get this in
>>> 5.2.
>>>
>>> Shuah, I think you, Greg KH, and myself talked off thread, and we agreed
>>> we would merge through your tree when the time came? Am I remembering
>>> correctly?
>>>
>>> ## Background
>>>
>>> This patch set proposes KUnit, a lightweight unit testing and mocking
>>> framework for the Linux kernel.
>>>
>>> Unlike Autotest and kselftest, KUnit is a true unit testing framework;
>>> it does not require installing the kernel on a test machine or in a VM
>>> and does not require tests to be written in userspace running on a host
>>> kernel. Additionally, KUnit is fast: From invocation to completion KUnit
>>> can run several dozen tests in under a second. Currently, the entire
>>> KUnit test suite for KUnit runs in under a second from the initial
>>> invocation (build time excluded).
>>>
>>> KUnit is heavily inspired by JUnit, Python's unittest.mock, and
>>> Googletest/Googlemock for C++. KUnit provides facilities for defining
>>> unit test cases, grouping related test cases into test suites, providing
>>> common infrastructure for running tests, mocking, spying, and much more.
>>
>> As a result of the emails replying to this patch thread, I am now
>> starting to look at kselftest.  My level of understanding is based
>> on some slide presentations, an LWN article, 
>> https://kselftest.wiki.kernel.org/
>> and a _tiny_ bit of looking at kselftest code.
>>
>> tl;dr; I don't really understand kselftest yet.
>>
>>
>> (1) why KUnit exists
>>
>>> ## What's so special about unit testing?
>>>
>>> A unit test is supposed to test a single unit of code in isolation,
>>> hence the name. There should be no dependencies outside the control of
>>> the test; this means no external dependencies, which makes tests orders
>>> of magnitudes faster. Likewise, since there are no external dependencies,
>>> there are no hoops to jump through to run the tests. Additionally, this
>>> makes unit tests deterministic: a failing unit test always indicates a
>>> problem. Finally, because unit tests necessarily have finer granularity,
>>> they are able to test all code paths easily solving the classic problem
>>> of difficulty in exercising error handling code.
>>
>> (2) KUnit is not meant to replace kselftest
>>
>>> ## Is KUnit trying to replace other testing frameworks for the kernel?
>>>
>>> No. Most existing tests for the Linux kernel are end-to-end tests, which
>>> have their place. A well tested system has lots of unit tests, a
>>> reasonable number of integration tests, and some end-to-end tests. KUnit
>>> is just trying to address the unit test space which is currently not
>>> being addressed.
>>
>> My understanding is that the intent of KUnit is to avoid booting a kernel on
>> real hardware or in a virtual machine.  That seems to be a matter of 
>> semantics
>> to me because isn't invoking a UML Linux just running the Linux kernel in
>> a different form of virtualization?
>>
>> So I do not understand why KUnit is an improvement over kselftest.
>>
>> It seems to me that KUnit is just another piece of infrastructure that I
>> am going to have to be familiar with as a kernel developer.  More overhead,
>> more information to stuff into my tiny little brain.
>>
>> I would guess that some developers will focus on just one of the two test
>> environments (and some will focus on both), splitting the development
>> resources instead of pooling them on a common infrastructure.
>>
>> What am I missing?
> 
> kselftest provides no in-kernel framework for testing kernel code
> specifically.  That should be what kunit provides, an "easy" way to
> write in-kernel tests for things.

kselftest provides a mechanism for in-kernel tests via modules.  For
example, see:

  tools/testing/selftests/vm/run_vmtests invokes:
tools/testing/selftests/vm/test_vmalloc.sh
  loads module:
test_vmalloc
(which is built from lib/test_vmalloc.c if CONFIG_TEST_VMALLOC)

A very quick and dirty search (likely to miss some tests) finds modules:

  test_bitmap
  test_bpf
  test_firmware
  test_printf
  test_static_key_base
  test_static_keys
  test_user_copy
  test_vmalloc

-Frank

> 
> Brendan, did I get it right?
> 
> thanks,
> 
> greg k-h
> .
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 00/17] kunit: introduce KUnit, the Linux kernel unit testing framework

[Frank Rowand]

Hi Ted,

On 5/7/19 10:22 AM, Theodore Ts'o wrote:
> On Tue, May 07, 2019 at 10:01:19AM +0200, Greg KH wrote:
Not very helpful to cut the text here, plus not explicitly indicating that
text was cut (yes, I know the ">>>" will be a clue for the careful reader),
losing the set up for my question.


>>> My understanding is that the intent of KUnit is to avoid booting a kernel on
>>> real hardware or in a virtual machine.  That seems to be a matter of 
>>> semantics
>>> to me because isn't invoking a UML Linux just running the Linux kernel in
>>> a different form of virtualization?
>>>
>>> So I do not understand why KUnit is an improvement over kselftest.
>>>
>>> It seems to me that KUnit is just another piece of infrastructure that I
>>> am going to have to be familiar with as a kernel developer.  More overhead,
>>> more information to stuff into my tiny little brain.
>>>
>>> I would guess that some developers will focus on just one of the two test
>>> environments (and some will focus on both), splitting the development
>>> resources instead of pooling them on a common infrastructure.
>>>
>>> What am I missing?
>>
>> kselftest provides no in-kernel framework for testing kernel code
>> specifically.  That should be what kunit provides, an "easy" way to
>> write in-kernel tests for things.
>>
>> Brendan, did I get it right?
> 
> Yes, that's basically right.  You don't *have* to use KUnit.  It's

If KUnit is added to the kernel, and a subsystem that I am submitting
code for has chosen to use KUnit instead of kselftest, then yes, I do
*have* to use KUnit if my submission needs to contain a test for the
code unless I want to convince the maintainer that somehow my case
is special and I prefer to use kselftest instead of KUnittest.


> supposed to be a simple way to run a large number of small tests that
> for specific small components in a system.

kselftest also supports running a subset of tests.  That subset of tests
can also be a large number of small tests.  There is nothing inherent
in KUnit vs kselftest in this regard, as far as I am aware.


> For example, I currently use xfstests using KVM and GCE to test all of
> ext4.  These tests require using multiple 5 GB and 20GB virtual disks,
> and it works by mounting ext4 file systems and exercising ext4 through
> the system call interfaces, using userspace tools such as fsstress,
> fsx, fio, etc.  It requires time overhead to start the VM, create and
> allocate virtual disks, etc.  For example, to run a single 3 seconds
> xfstest (generic/001), it requires full 10 seconds to run it via
> kvm-xfstests.
> 


> KUnit is something else; it's specifically intended to allow you to
> create lightweight tests quickly and easily, and by reducing the
> effort needed to write and run unit tests, hopefully we'll have a lot
> more of them and thus improve kernel quality.

The same is true of kselftest.  You can create lightweight tests in
kselftest.


> As an example, I have a volunteer working on developing KUinit tests
> for ext4.  We're going to start by testing the ext4 extent status
> tree.  The source code is at fs/ext4/extent_status.c; it's
> approximately 1800 LOC.  The Kunit tests for the extent status tree
> will exercise all of the corner cases for the various extent status
> tree functions --- e.g., ext4_es_insert_delayed_block(),
> ext4_es_remove_extent(), ext4_es_cache_extent(), etc.  And it will do
> this in isolation without our needing to create a test file system or
> using a test block device.
> 

> Next we'll test the ext4 block allocator, again in isolation.  To test
> the block allocator we will have to write "mock functions" which
> simulate reading allocation bitmaps from disk.  Again, this will allow
> the test writer to explicitly construct corner cases and validate that
> the block allocator works as expected without having to reverese
> engineer file system data structures which will force a particular
> code path to be executed.

This would be a difference, but mock functions do not exist in KUnit.
The KUnit test will call the real kernel function in the UML kernel.

I think Brendan has indicated a desire to have mock functions in the
future.

Brendan, do I understand that correctly?

-Frank

> So this is why it's largely irrelevant to me that KUinit uses UML.  In
> fact, it's a feature.  We're not testing device drivers, or the
> scheduler, or anything else architecture-specific.  UML is not about
> virtualization.  What it's about in this context is allowing us to
> start running test code as quickly as possible.  Booting KVM takes
> about 3-4 seconds, and this includes initializing virtio_scsi and
> other device drivers.  If by using UML we can hold the amount of
> unnecessary kernel subsystem initialization down to the absolute
> minimum, and if it means that we can communicating to the test
> framework via a userspace "printf" from UML/KUnit code, as opposed to
> via a virtual serial port to KVM's virtual console, it all makes for
> 

Re: [PATCH v2 00/17] kunit: introduce KUnit, the Linux kernel unit testing framework

[Frank Rowand]

On 5/7/19 8:23 AM, shuah wrote:
> On 5/7/19 2:01 AM, Greg KH wrote:
>> On Mon, May 06, 2019 at 08:14:12PM -0700, Frank Rowand wrote:
>>> On 5/1/19 4:01 PM, Brendan Higgins wrote:
>>>> ## TLDR
>>>>
>>>> I rebased the last patchset on 5.1-rc7 in hopes that we can get this in
>>>> 5.2.
>>>>
>>>> Shuah, I think you, Greg KH, and myself talked off thread, and we agreed
>>>> we would merge through your tree when the time came? Am I remembering
>>>> correctly?
>>>>
>>>> ## Background
>>>>
>>>> This patch set proposes KUnit, a lightweight unit testing and mocking
>>>> framework for the Linux kernel.
>>>>
>>>> Unlike Autotest and kselftest, KUnit is a true unit testing framework;
>>>> it does not require installing the kernel on a test machine or in a VM
>>>> and does not require tests to be written in userspace running on a host
>>>> kernel. Additionally, KUnit is fast: From invocation to completion KUnit
>>>> can run several dozen tests in under a second. Currently, the entire
>>>> KUnit test suite for KUnit runs in under a second from the initial
>>>> invocation (build time excluded).
>>>>
>>>> KUnit is heavily inspired by JUnit, Python's unittest.mock, and
>>>> Googletest/Googlemock for C++. KUnit provides facilities for defining
>>>> unit test cases, grouping related test cases into test suites, providing
>>>> common infrastructure for running tests, mocking, spying, and much more.
>>>
>>> As a result of the emails replying to this patch thread, I am now
>>> starting to look at kselftest.  My level of understanding is based
>>> on some slide presentations, an LWN article, 
>>> https://kselftest.wiki.kernel.org/
>>> and a _tiny_ bit of looking at kselftest code.
>>>
>>> tl;dr; I don't really understand kselftest yet.
>>>
>>>
>>> (1) why KUnit exists
>>>
>>>> ## What's so special about unit testing?
>>>>
>>>> A unit test is supposed to test a single unit of code in isolation,
>>>> hence the name. There should be no dependencies outside the control of
>>>> the test; this means no external dependencies, which makes tests orders
>>>> of magnitudes faster. Likewise, since there are no external dependencies,
>>>> there are no hoops to jump through to run the tests. Additionally, this
>>>> makes unit tests deterministic: a failing unit test always indicates a
>>>> problem. Finally, because unit tests necessarily have finer granularity,
>>>> they are able to test all code paths easily solving the classic problem
>>>> of difficulty in exercising error handling code.
>>>
>>> (2) KUnit is not meant to replace kselftest
>>>
>>>> ## Is KUnit trying to replace other testing frameworks for the kernel?
>>>>
>>>> No. Most existing tests for the Linux kernel are end-to-end tests, which
>>>> have their place. A well tested system has lots of unit tests, a
>>>> reasonable number of integration tests, and some end-to-end tests. KUnit
>>>> is just trying to address the unit test space which is currently not
>>>> being addressed.
>>>
>>> My understanding is that the intent of KUnit is to avoid booting a kernel on
>>> real hardware or in a virtual machine.  That seems to be a matter of 
>>> semantics
>>> to me because isn't invoking a UML Linux just running the Linux kernel in
>>> a different form of virtualization?
>>>
>>> So I do not understand why KUnit is an improvement over kselftest.
> 
> They are in two different categories. Kselftest falls into black box
> regression test suite which is a collection of user-space tests with a
> few kernel test modules back-ending the tests in some cases.
> 
> Kselftest can be used by both kernel developers and users and provides
> a good way to regression test releases in test rings.
> 
> KUnit is a white box category and is a better fit as unit test framework
> for development and provides a in-kernel testing. I wouldn't view them
> one replacing the other. They just provide coverage for different areas
> of testing.

I don't see what about kselftest or KUnit is inherently black box or
white box.  I would expect both frameworks to be used for either type
of testing.


> I wouldn't view KUnit as something that would be easily run in test rings for 
> example.

I don't see why not.

-Frank

> 
> Brendan

Re: [PATCH v2 00/17] kunit: introduce KUnit, the Linux kernel unit testing framework

[Frank Rowand]

On 5/1/19 4:01 PM, Brendan Higgins wrote:
> ## TLDR
> 
> I rebased the last patchset on 5.1-rc7 in hopes that we can get this in
> 5.2.
> 
> Shuah, I think you, Greg KH, and myself talked off thread, and we agreed
> we would merge through your tree when the time came? Am I remembering
> correctly?
> 
> ## Background
> 
> This patch set proposes KUnit, a lightweight unit testing and mocking
> framework for the Linux kernel.
> 
> Unlike Autotest and kselftest, KUnit is a true unit testing framework;
> it does not require installing the kernel on a test machine or in a VM
> and does not require tests to be written in userspace running on a host
> kernel. Additionally, KUnit is fast: From invocation to completion KUnit
> can run several dozen tests in under a second. Currently, the entire
> KUnit test suite for KUnit runs in under a second from the initial
> invocation (build time excluded).
> 
> KUnit is heavily inspired by JUnit, Python's unittest.mock, and
> Googletest/Googlemock for C++. KUnit provides facilities for defining
> unit test cases, grouping related test cases into test suites, providing
> common infrastructure for running tests, mocking, spying, and much more.

As a result of the emails replying to this patch thread, I am now
starting to look at kselftest.  My level of understanding is based
on some slide presentations, an LWN article, https://kselftest.wiki.kernel.org/
and a _tiny_ bit of looking at kselftest code.

tl;dr; I don't really understand kselftest yet.


(1) why KUnit exists

> ## What's so special about unit testing?
> 
> A unit test is supposed to test a single unit of code in isolation,
> hence the name. There should be no dependencies outside the control of
> the test; this means no external dependencies, which makes tests orders
> of magnitudes faster. Likewise, since there are no external dependencies,
> there are no hoops to jump through to run the tests. Additionally, this
> makes unit tests deterministic: a failing unit test always indicates a
> problem. Finally, because unit tests necessarily have finer granularity,
> they are able to test all code paths easily solving the classic problem
> of difficulty in exercising error handling code.

(2) KUnit is not meant to replace kselftest

> ## Is KUnit trying to replace other testing frameworks for the kernel?
> 
> No. Most existing tests for the Linux kernel are end-to-end tests, which
> have their place. A well tested system has lots of unit tests, a
> reasonable number of integration tests, and some end-to-end tests. KUnit
> is just trying to address the unit test space which is currently not
> being addressed.

My understanding is that the intent of KUnit is to avoid booting a kernel on
real hardware or in a virtual machine.  That seems to be a matter of semantics
to me because isn't invoking a UML Linux just running the Linux kernel in
a different form of virtualization?

So I do not understand why KUnit is an improvement over kselftest.

It seems to me that KUnit is just another piece of infrastructure that I
am going to have to be familiar with as a kernel developer.  More overhead,
more information to stuff into my tiny little brain.

I would guess that some developers will focus on just one of the two test
environments (and some will focus on both), splitting the development
resources instead of pooling them on a common infrastructure.

What am I missing?

-Frank


> 
> ## More information on KUnit
> 
> There is a bunch of documentation near the end of this patch set that
> describes how to use KUnit and best practices for writing unit tests.
> For convenience I am hosting the compiled docs here:
> https://google.github.io/kunit-docs/third_party/kernel/docs/
> Additionally for convenience, I have applied these patches to a branch:
> https://kunit.googlesource.com/linux/+/kunit/rfc/v5.1-rc7/v1
> The repo may be cloned with:
> git clone https://kunit.googlesource.com/linux
> This patchset is on the kunit/rfc/v5.1-rc7/v1 branch.
> 
> ## Changes Since Last Version
> 
> None. I just rebased the last patchset on v5.1-rc7.
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 12/17] kunit: tool: add Python wrappers for running KUnit tests

[Frank Rowand]

On 5/3/19 4:14 PM, Brendan Higgins wrote:
>> On 5/2/19 10:36 PM, Brendan Higgins wrote:
>>> On Thu, May 2, 2019 at 6:45 PM Frank Rowand  wrote:
>>>>
>>>> On 5/2/19 4:45 PM, Brendan Higgins wrote:
>>>>> On Thu, May 2, 2019 at 2:16 PM Frank Rowand  
>>>>> wrote:
>>>>>>
>>>>>> On 5/2/19 11:07 AM, Brendan Higgins wrote:
>>>>>>> On Thu, May 2, 2019 at 4:02 AM Greg KH  
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> On Wed, May 01, 2019 at 04:01:21PM -0700, Brendan Higgins wrote:
>>>>>>>>> From: Felix Guo 
>>>>>>>>>
>>>>>>>>> The ultimate goal is to create minimal isolated test binaries; in the
>>>>>>>>> meantime we are using UML to provide the infrastructure to run tests, 
>>>>>>>>> so
>>>>>>>>> define an abstract way to configure and run tests that allow us to
>>>>>>>>> change the context in which tests are built without affecting the 
>>>>>>>>> user.
>>>>>>>>> This also makes pretty and dynamic error reporting, and a lot of other
>>>>>>>>> nice features easier.
>>>>>>>>>
>>>>>>>>> kunit_config.py:
>>>>>>>>>   - parse .config and Kconfig files.
>>>>>>>>>
>>>>>>>>> kunit_kernel.py: provides helper functions to:
>>>>>>>>>   - configure the kernel using kunitconfig.
>>>>>>>>>   - build the kernel with the appropriate configuration.
>>>>>>>>>   - provide function to invoke the kernel and stream the output back.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Felix Guo 
>>>>>>>>> Signed-off-by: Brendan Higgins 
>>>>>>>>
>>>>>>>> Ah, here's probably my answer to my previous logging format question,
>>>>>>>> right?  What's the chance that these wrappers output stuff in a 
>>>>>>>> standard
>>>>>>>> format that test-framework-tools can already parse?  :)
>>>>>
>>>>> To be clear, the test-framework-tools format we are talking about is
>>>>> TAP13[1], correct?
>>>>
>>>> I'm not sure what the test community prefers for a format.  I'll let them
>>>> jump in and debate that question.
>>>>
>>>>
>>>>>
>>>>> My understanding is that is what kselftest is being converted to use.
>>>>>
>>>>>>>
>>>>>>> It should be pretty easy to do. I had some patches that pack up the
>>>>>>> results into a serialized format for a presubmit service; it should be
>>>>>>> pretty straightforward to take the same logic and just change the
>>>>>>> output format.
>>>>>>
>>>>>> When examining and trying out the previous versions of the patch I found
>>>>>> the wrappers useful to provide information about how to control and use
>>>>>> the tests, but I had no interest in using the scripts as they do not
>>>>>> fit in with my personal environment and workflow.
>>>>>>
>>>>>> In the previous versions of the patch, these helper scripts are optional,
>>>>>> which is good for my use case.  If the helper scripts are required to
>>>>>
>>>>> They are still optional.
>>>>>
>>>>>> get the data into the proper format then the scripts are not quite so
>>>>>> optional, they become the expected environment.  I think the proper
>>>>>> format should exist without the helper scripts.
>>>>>
>>>>> That's a good point. A couple things,
>>>>>
>>>>> First off, supporting TAP13, either in the kernel or the wrapper
>>>>> script is not hard, but I don't think that is the real issue that you
>>>>> raise.
>>>>>
>>>>> If your only concern is that you will always be able to have human
>>>>> readable KUnit results printed to the kernel log, that is a guarantee
>>>>> I feel comfortable making. Beyond that, I think it is going to take a
>>>>> long while before I would feel comfortable guarantee

Re: [PATCH v2 12/17] kunit: tool: add Python wrappers for running KUnit tests

[Frank Rowand]

On 5/2/19 10:36 PM, Brendan Higgins wrote:
> On Thu, May 2, 2019 at 6:45 PM Frank Rowand  wrote:
>>
>> On 5/2/19 4:45 PM, Brendan Higgins wrote:
>>> On Thu, May 2, 2019 at 2:16 PM Frank Rowand  wrote:
>>>>
>>>> On 5/2/19 11:07 AM, Brendan Higgins wrote:
>>>>> On Thu, May 2, 2019 at 4:02 AM Greg KH  wrote:
>>>>>>
>>>>>> On Wed, May 01, 2019 at 04:01:21PM -0700, Brendan Higgins wrote:
>>>>>>> From: Felix Guo 
>>>>>>>
>>>>>>> The ultimate goal is to create minimal isolated test binaries; in the
>>>>>>> meantime we are using UML to provide the infrastructure to run tests, so
>>>>>>> define an abstract way to configure and run tests that allow us to
>>>>>>> change the context in which tests are built without affecting the user.
>>>>>>> This also makes pretty and dynamic error reporting, and a lot of other
>>>>>>> nice features easier.
>>>>>>>
>>>>>>> kunit_config.py:
>>>>>>>   - parse .config and Kconfig files.
>>>>>>>
>>>>>>> kunit_kernel.py: provides helper functions to:
>>>>>>>   - configure the kernel using kunitconfig.
>>>>>>>   - build the kernel with the appropriate configuration.
>>>>>>>   - provide function to invoke the kernel and stream the output back.
>>>>>>>
>>>>>>> Signed-off-by: Felix Guo 
>>>>>>> Signed-off-by: Brendan Higgins 
>>>>>>
>>>>>> Ah, here's probably my answer to my previous logging format question,
>>>>>> right?  What's the chance that these wrappers output stuff in a standard
>>>>>> format that test-framework-tools can already parse?  :)
>>>
>>> To be clear, the test-framework-tools format we are talking about is
>>> TAP13[1], correct?
>>
>> I'm not sure what the test community prefers for a format.  I'll let them
>> jump in and debate that question.
>>
>>
>>>
>>> My understanding is that is what kselftest is being converted to use.
>>>
>>>>>
>>>>> It should be pretty easy to do. I had some patches that pack up the
>>>>> results into a serialized format for a presubmit service; it should be
>>>>> pretty straightforward to take the same logic and just change the
>>>>> output format.
>>>>
>>>> When examining and trying out the previous versions of the patch I found
>>>> the wrappers useful to provide information about how to control and use
>>>> the tests, but I had no interest in using the scripts as they do not
>>>> fit in with my personal environment and workflow.
>>>>
>>>> In the previous versions of the patch, these helper scripts are optional,
>>>> which is good for my use case.  If the helper scripts are required to
>>>
>>> They are still optional.
>>>
>>>> get the data into the proper format then the scripts are not quite so
>>>> optional, they become the expected environment.  I think the proper
>>>> format should exist without the helper scripts.
>>>
>>> That's a good point. A couple things,
>>>
>>> First off, supporting TAP13, either in the kernel or the wrapper
>>> script is not hard, but I don't think that is the real issue that you
>>> raise.
>>>
>>> If your only concern is that you will always be able to have human
>>> readable KUnit results printed to the kernel log, that is a guarantee
>>> I feel comfortable making. Beyond that, I think it is going to take a
>>> long while before I would feel comfortable guaranteeing anything about
>>> how will KUnit work, what kind of data it will want to expose, and how
>>> it will be organized. I think the wrapper script provides a nice
>>> facade that I can maintain, can mediate between the implementation
>>> details and the user, and can mediate between the implementation
>>> details and other pieces of software that might want to consume
>>> results.
>>>
>>> [1] https://testanything.org/tap-version-13-specification.html
>>
>> My concern is based on a focus on my little part of the world
>> (which in _previous_ versions of the patch series was the devicetree
>> unittest.c tests being converted to use the kunit infrastructure).
>> If I step back 

Re: [PATCH v2 12/17] kunit: tool: add Python wrappers for running KUnit tests

[Frank Rowand]

On 5/2/19 4:45 PM, Brendan Higgins wrote:
> On Thu, May 2, 2019 at 2:16 PM Frank Rowand  wrote:
>>
>> On 5/2/19 11:07 AM, Brendan Higgins wrote:
>>> On Thu, May 2, 2019 at 4:02 AM Greg KH  wrote:
>>>>
>>>> On Wed, May 01, 2019 at 04:01:21PM -0700, Brendan Higgins wrote:
>>>>> From: Felix Guo 
>>>>>
>>>>> The ultimate goal is to create minimal isolated test binaries; in the
>>>>> meantime we are using UML to provide the infrastructure to run tests, so
>>>>> define an abstract way to configure and run tests that allow us to
>>>>> change the context in which tests are built without affecting the user.
>>>>> This also makes pretty and dynamic error reporting, and a lot of other
>>>>> nice features easier.
>>>>>
>>>>> kunit_config.py:
>>>>>   - parse .config and Kconfig files.
>>>>>
>>>>> kunit_kernel.py: provides helper functions to:
>>>>>   - configure the kernel using kunitconfig.
>>>>>   - build the kernel with the appropriate configuration.
>>>>>   - provide function to invoke the kernel and stream the output back.
>>>>>
>>>>> Signed-off-by: Felix Guo 
>>>>> Signed-off-by: Brendan Higgins 
>>>>
>>>> Ah, here's probably my answer to my previous logging format question,
>>>> right?  What's the chance that these wrappers output stuff in a standard
>>>> format that test-framework-tools can already parse?  :)
> 
> To be clear, the test-framework-tools format we are talking about is
> TAP13[1], correct?

I'm not sure what the test community prefers for a format.  I'll let them
jump in and debate that question.


> 
> My understanding is that is what kselftest is being converted to use.
> 
>>>
>>> It should be pretty easy to do. I had some patches that pack up the
>>> results into a serialized format for a presubmit service; it should be
>>> pretty straightforward to take the same logic and just change the
>>> output format.
>>
>> When examining and trying out the previous versions of the patch I found
>> the wrappers useful to provide information about how to control and use
>> the tests, but I had no interest in using the scripts as they do not
>> fit in with my personal environment and workflow.
>>
>> In the previous versions of the patch, these helper scripts are optional,
>> which is good for my use case.  If the helper scripts are required to
> 
> They are still optional.
> 
>> get the data into the proper format then the scripts are not quite so
>> optional, they become the expected environment.  I think the proper
>> format should exist without the helper scripts.
> 
> That's a good point. A couple things,
> 
> First off, supporting TAP13, either in the kernel or the wrapper
> script is not hard, but I don't think that is the real issue that you
> raise.
> 
> If your only concern is that you will always be able to have human
> readable KUnit results printed to the kernel log, that is a guarantee
> I feel comfortable making. Beyond that, I think it is going to take a
> long while before I would feel comfortable guaranteeing anything about
> how will KUnit work, what kind of data it will want to expose, and how
> it will be organized. I think the wrapper script provides a nice
> facade that I can maintain, can mediate between the implementation
> details and the user, and can mediate between the implementation
> details and other pieces of software that might want to consume
> results.
> 
> [1] https://testanything.org/tap-version-13-specification.html

My concern is based on a focus on my little part of the world
(which in _previous_ versions of the patch series was the devicetree
unittest.c tests being converted to use the kunit infrastructure).
If I step back and think of the entire kernel globally I may end
up with a different conclusion - but I'm going to remain myopic
for this email.

I want the test results to be usable by me and my fellow
developers.  I prefer that the test results be easily accessible
(current printk() implementation means that kunit messages are
just as accessible as the current unittest.c printk() output).
If the printk() output needs to be filtered through a script
to generate the actual test results then that is sub-optimal
to me.  It is one more step added to my workflow.  And
potentially with an embedded target a major pain to get a
data file (the kernel log file) transferred from a target
to my development host.

I want a reported test failure to be easy to trace back to the
point in the source where the fail

Re: [PATCH v2 04/17] kunit: test: add kunit_stream a std::stream like logger

[Frank Rowand]

On 5/2/19 1:25 PM, Brendan Higgins wrote:
> On Thu, May 2, 2019 at 4:00 AM Greg KH  wrote:
>>
>> On Wed, May 01, 2019 at 04:01:13PM -0700, Brendan Higgins wrote:
>>> A lot of the expectation and assertion infrastructure prints out fairly
>>> complicated test failure messages, so add a C++ style log library for
>>> for logging test results.
>>
>> Ideally we would always use a standard logging format, like the
>> kselftest tests all are aiming to do.  That way the output can be easily
>> parsed by tools to see if the tests succeed/fail easily.
>>
>> Any chance of having this logging framework enforcing that format as
>> well?
> 
> I agree with your comment on the later patch that we should handle
> this at the wrapper script layer (KUnit tool).

This discussion is a little confusing, because it is spread across two
patches.

I do not agree that this should be handled in the wrapper script, as
noted in my reply to patch 12, so not repeating it here.

-Frank
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 12/17] kunit: tool: add Python wrappers for running KUnit tests

[Frank Rowand]

On 5/2/19 11:07 AM, Brendan Higgins wrote:
> On Thu, May 2, 2019 at 4:02 AM Greg KH  wrote:
>>
>> On Wed, May 01, 2019 at 04:01:21PM -0700, Brendan Higgins wrote:
>>> From: Felix Guo 
>>>
>>> The ultimate goal is to create minimal isolated test binaries; in the
>>> meantime we are using UML to provide the infrastructure to run tests, so
>>> define an abstract way to configure and run tests that allow us to
>>> change the context in which tests are built without affecting the user.
>>> This also makes pretty and dynamic error reporting, and a lot of other
>>> nice features easier.
>>>
>>> kunit_config.py:
>>>   - parse .config and Kconfig files.
>>>
>>> kunit_kernel.py: provides helper functions to:
>>>   - configure the kernel using kunitconfig.
>>>   - build the kernel with the appropriate configuration.
>>>   - provide function to invoke the kernel and stream the output back.
>>>
>>> Signed-off-by: Felix Guo 
>>> Signed-off-by: Brendan Higgins 
>>
>> Ah, here's probably my answer to my previous logging format question,
>> right?  What's the chance that these wrappers output stuff in a standard
>> format that test-framework-tools can already parse?  :)
> 
> It should be pretty easy to do. I had some patches that pack up the
> results into a serialized format for a presubmit service; it should be
> pretty straightforward to take the same logic and just change the
> output format.

When examining and trying out the previous versions of the patch I found
the wrappers useful to provide information about how to control and use
the tests, but I had no interest in using the scripts as they do not
fit in with my personal environment and workflow.

In the previous versions of the patch, these helper scripts are optional,
which is good for my use case.  If the helper scripts are required to
get the data into the proper format then the scripts are not quite so
optional, they become the expected environment.  I think the proper
format should exist without the helper scripts.

-Frank
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v4 16/17] of: unittest: split out a couple of test cases from unittest

[Frank Rowand]

On 2/14/19 1:37 PM, Brendan Higgins wrote:
> Split out a couple of test cases that these features in base.c from the
> unittest.c monolith. The intention is that we will eventually split out
> all test cases and group them together based on what portion of device
> tree they test.

I still object to this patch.  I do not want this code scattered into
additional files.

-Frank


> 
> Signed-off-by: Brendan Higgins 
> ---
>  drivers/of/Makefile  |   2 +-
>  drivers/of/base-test.c   | 214 
>  drivers/of/test-common.c | 175 
>  drivers/of/test-common.h |  16 ++
>  drivers/of/unittest.c| 345 +--
>  5 files changed, 407 insertions(+), 345 deletions(-)
>  create mode 100644 drivers/of/base-test.c
>  create mode 100644 drivers/of/test-common.c
>  create mode 100644 drivers/of/test-common.h
> 
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index 663a4af0cccd5..4a4bd527d586c 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -8,7 +8,7 @@ obj-$(CONFIG_OF_PROMTREE) += pdt.o
>  obj-$(CONFIG_OF_ADDRESS)  += address.o
>  obj-$(CONFIG_OF_IRQ)+= irq.o
>  obj-$(CONFIG_OF_NET) += of_net.o
> -obj-$(CONFIG_OF_UNITTEST) += unittest.o
> +obj-$(CONFIG_OF_UNITTEST) += unittest.o base-test.o test-common.o
>  obj-$(CONFIG_OF_MDIO)+= of_mdio.o
>  obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
>  obj-$(CONFIG_OF_RESOLVE)  += resolver.o
> diff --git a/drivers/of/base-test.c b/drivers/of/base-test.c
> new file mode 100644
> index 0..3d3f4f1b74800
> --- /dev/null
> +++ b/drivers/of/base-test.c
> @@ -0,0 +1,214 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Unit tests for functions defined in base.c.
> + */
> +#include 
> +
> +#include 
> +
> +#include "test-common.h"
> +
> +static void of_unittest_find_node_by_name(struct kunit *test)
> +{
> + struct device_node *np;
> + const char *options, *name;
> +
> + np = of_find_node_by_path("/testcase-data");
> + name = kasprintf(GFP_KERNEL, "%pOF", np);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
> + KUNIT_EXPECT_STREQ_MSG(test, "/testcase-data", name,
> +"find /testcase-data failed\n");
> + of_node_put(np);
> + kfree(name);
> +
> + /* Test if trailing '/' works */
> + KUNIT_EXPECT_EQ_MSG(test, of_find_node_by_path("/testcase-data/"), NULL,
> + "trailing '/' on /testcase-data/ should fail\n");
> +
> + np = of_find_node_by_path("/testcase-data/phandle-tests/consumer-a");
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
> + name = kasprintf(GFP_KERNEL, "%pOF", np);
> + KUNIT_EXPECT_STREQ_MSG(
> + test, "/testcase-data/phandle-tests/consumer-a", name,
> + "find /testcase-data/phandle-tests/consumer-a failed\n");
> + of_node_put(np);
> + kfree(name);
> +
> + np = of_find_node_by_path("testcase-alias");
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
> + name = kasprintf(GFP_KERNEL, "%pOF", np);
> + KUNIT_EXPECT_STREQ_MSG(test, "/testcase-data", name,
> +"find testcase-alias failed\n");
> + of_node_put(np);
> + kfree(name);
> +
> + /* Test if trailing '/' works on aliases */
> + KUNIT_EXPECT_EQ_MSG(test, of_find_node_by_path("testcase-alias/"), NULL,
> + "trailing '/' on testcase-alias/ should fail\n");
> +
> + np = of_find_node_by_path("testcase-alias/phandle-tests/consumer-a");
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
> + name = kasprintf(GFP_KERNEL, "%pOF", np);
> + KUNIT_EXPECT_STREQ_MSG(
> + test, "/testcase-data/phandle-tests/consumer-a", name,
> + "find testcase-alias/phandle-tests/consumer-a failed\n");
> + of_node_put(np);
> + kfree(name);
> +
> + KUNIT_EXPECT_EQ_MSG(
> + test,
> + np = of_find_node_by_path("/testcase-data/missing-path"), NULL,
> + "non-existent path returned node %pOF\n", np);
> + of_node_put(np);
> +
> + KUNIT_EXPECT_EQ_MSG(
> + test, np = of_find_node_by_path("missing-alias"), NULL,
> + "non-existent alias returned node %pOF\n", np);
> + of_node_put(np);
> +
> + KUNIT_EXPECT_EQ_MSG(
> + test,
> + np = of_find_node_by_path("testcase-alias/missing-path"), NULL,
> + "non-existent alias with relative path returned node %pOF\n",
> + np);
> + of_node_put(np);
> +
> + np = of_find_node_opts_by_path("/testcase-data:testoption", );
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
> + KUNIT_EXPECT_STREQ_MSG(test, "testoption", options,
> +"option path test failed\n");
> + of_node_put(np);
> +
> + np = of_find_node_opts_by_path("/testcase-data:test/option", );
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
> + KUNIT_EXPECT_STREQ_MSG(test, "test/option", options,
> +

Re: [RFC v3 18/19] of: unittest: split out a couple of test cases from unittest

[Frank Rowand]

On 3/21/19 5:22 PM, Frank Rowand wrote:
> On 2/27/19 7:52 PM, Brendan Higgins wrote:

< snip >

>> Now I know that, hermeticity especially, but other features as well
>> (test suite summary, error on unused test case function, etc) are not
>> actually in KUnit as it is under consideration here. Maybe it would be
>> best to save these last two patches (18/19, and 19/19) until I have
>> these other features checked in and reconsider them then?
> 
> Thanks for leaving 18/19 and 19/19 off in v4.

Oops, they got into v4 but as 16/17 and 17/17, I think.  But it sounds
like you are planning to leave them out of v5.

> 
> -Frank
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v4 00/17] kunit: introduce KUnit, the Linux kernel unit testing framework

[Frank Rowand]

On 3/21/19 4:33 PM, Brendan Higgins wrote:
> On Thu, Mar 21, 2019 at 3:27 PM Logan Gunthorpe  wrote:
>>
>>
>>
>> On 2019-03-21 4:07 p.m., Brendan Higgins wrote:
>>> A couple of points, as for needing CONFIG_PCI; my plan to deal with
>>> that type of thing has been that we would add support for a KUnit/UML
>>> version that is just for KUnit. It would mock out the necessary bits
>>> to provide a fake hardware implementation for anything that might
>>> depend on it. I wrote a prototype for mocking/faking MMIO that I
>>> presented to the list here[1]; it is not part of the current patchset
>>> because we decided it would be best to focus on getting an MVP in, but
>>> I plan on bringing it back up at some point. Anyway, what do you
>>> generally think of this approach?
>>
>> Yes, I was wondering if that might be possible. I think that's a great
>> approach but it will unfortunately take a lot of work before larger
>> swaths of the kernel are testable in Kunit with UML. Having more common
>> mocked infrastructure will be great by-product of it though.
> 
> Yeah, it's unfortunate that the best way to do something often takes
> so much longer.
> 
>>
>>> Awesome, I looked at the code you posted and it doesn't look like you
>>> have had too many troubles. One thing that stood out to me, why did
>>> you need to put it in the kunit/ dir?
>>
>> Yeah, writing the code was super easy. Only after, did I realized I
>> couldn't get it to easily build.
> 
> Yeah, we really need to fix that; unfortunately, broadly addressing
> that problem is really hard and will most likely take a long time.
> 
>>
>> Putting it in the kunit directory was necessary because nothing in the
>> NTB tree builds unless CONFIG_NTB is set (see drivers/Makefile) and
>> CONFIG_NTB depends on CONFIG_PCI. I didn't experiment to see how hard it
>> would be to set CONFIG_NTB without CONFIG_PCI; I assumed it would be tricky.
>>
>>> I am looking forward to see what you think!
>>
>> Generally, I'm impressed and want to see this work in upstream as soon
>> as possible so I can start to make use of it!
> 
> Great to hear! I was trying to get the next revision out this week,
> but addressing some of the comments is taking a little longer than
> expected. I should have something together fairly soon though
> (hopefully next week). Good news is that next revision will be
> non-RFC; most of the feedback has settled down and I think we are
> ready to start figuring out how to merge it. Fingers crossed :-)
> 
> Cheers

I'll be out of the office next week and will not be able to review.
Please hold off on any devicetree related files until after I review.

Thanks,

Frank

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v4 08/17] kunit: test: add support for test abort

[Frank Rowand]

On 2/27/19 11:42 PM, Brendan Higgins wrote:
> On Tue, Feb 19, 2019 at 10:44 PM Frank Rowand  wrote:
>>
>> On 2/19/19 7:39 PM, Brendan Higgins wrote:
>>> On Mon, Feb 18, 2019 at 11:52 AM Frank Rowand  
>>> wrote:
>>>>
>>>> On 2/14/19 1:37 PM, Brendan Higgins wrote:
>>>>> Add support for aborting/bailing out of test cases. Needed for
>>>>> implementing assertions.
>>>>>
>>>>> Signed-off-by: Brendan Higgins 
>>>>> ---
>>>>> Changes Since Last Version
>>>>>  - This patch is new introducing a new cross-architecture way to abort
>>>>>out of a test case (needed for KUNIT_ASSERT_*, see next patch for
>>>>>details).
>>>>>  - On a side note, this is not a complete replacement for the UML abort
>>>>>mechanism, but covers the majority of necessary functionality. UML
>>>>>architecture specific featurs have been dropped from the initial
>>>>>patchset.
>>>>> ---
>>>>>  include/kunit/test.h |  24 +
>>>>>  kunit/Makefile   |   3 +-
>>>>>  kunit/test-test.c| 127 ++
>>>>>  kunit/test.c | 208 +--
>>>>>  4 files changed, 353 insertions(+), 9 deletions(-)
>>>>>  create mode 100644 kunit/test-test.c
>>>>
>>>> < snip >
>>>>
>>>>> diff --git a/kunit/test.c b/kunit/test.c
>>>>> index d18c50d5ed671..6e5244642ab07 100644
>>>>> --- a/kunit/test.c
>>>>> +++ b/kunit/test.c
>>>>> @@ -6,9 +6,9 @@
>>>>>   * Author: Brendan Higgins 
>>>>>   */
>>>>>
>>>>> -#include 
>>>>>  #include 
>>>>> -#include 
>>>>> +#include 
>>>>> +#include 
>>>>>  #include 
>>>>>
>>>>>  static bool kunit_get_success(struct kunit *test)
>>>>> @@ -32,6 +32,27 @@ static void kunit_set_success(struct kunit *test, bool 
>>>>> success)
>>>>>   spin_unlock_irqrestore(>lock, flags);
>>>>>  }
>>>>>
>>>>> +static bool kunit_get_death_test(struct kunit *test)
>>>>> +{
>>>>> + unsigned long flags;
>>>>> + bool death_test;
>>>>> +
>>>>> + spin_lock_irqsave(>lock, flags);
>>>>> + death_test = test->death_test;
>>>>> + spin_unlock_irqrestore(>lock, flags);
>>>>> +
>>>>> + return death_test;
>>>>> +}
>>>>> +
>>>>> +static void kunit_set_death_test(struct kunit *test, bool death_test)
>>>>> +{
>>>>> + unsigned long flags;
>>>>> +
>>>>> + spin_lock_irqsave(>lock, flags);
>>>>> + test->death_test = death_test;
>>>>> + spin_unlock_irqrestore(>lock, flags);
>>>>> +}
>>>>> +
>>>>>  static int kunit_vprintk_emit(const struct kunit *test,
>>>>> int level,
>>>>> const char *fmt,
>>>>> @@ -70,13 +91,29 @@ static void kunit_fail(struct kunit *test, struct 
>>>>> kunit_stream *stream)
>>>>>   stream->commit(stream);
>>>>>  }
>>>>>
>>>>> +static void __noreturn kunit_abort(struct kunit *test)
>>>>> +{
>>>>> + kunit_set_death_test(test, true);
>>>>> +
>>>>> + test->try_catch.throw(>try_catch);
>>>>> +
>>>>> + /*
>>>>> +  * Throw could not abort from test.
>>>>> +  */
>>>>> + kunit_err(test, "Throw could not abort from test!");
>>>>> + show_stack(NULL, NULL);
>>>>> + BUG();
>>>>
>>>> kunit_abort() is what will be call as the result of an assert failure.
>>>
>>> Yep. Does that need clarified somewhere.
>>>>
>>>> BUG(), which is a panic, which is crashing the system is not acceptable
>>>> in the Linux kernel.  You will just annoy Linus if you submit this.
>>>
>>> Sorry, I thought this was an acceptable use case since, a) this should
>>> never be compiled in a production kernel, b) we are in a pretty bad,
>>

Re: [RFC v3 18/19] of: unittest: split out a couple of test cases from unittest

[Frank Rowand]

On 2/27/19 7:52 PM, Brendan Higgins wrote:
> On Wed, Feb 20, 2019 at 12:45 PM Frank Rowand  wrote:
>>
>> On 2/18/19 2:25 PM, Frank Rowand wrote:
>>> On 2/15/19 2:56 AM, Brendan Higgins wrote:
>>>> On Thu, Feb 14, 2019 at 6:05 PM Frank Rowand  
>>>> wrote:
>>>>>
>>>>> On 2/14/19 4:56 PM, Brendan Higgins wrote:
>>>>>> On Thu, Feb 14, 2019 at 3:57 PM Frank Rowand  
>>>>>> wrote:
>>>>>>>
>>>>>>> On 12/5/18 3:54 PM, Brendan Higgins wrote:
>>>>>>>> On Tue, Dec 4, 2018 at 2:58 AM Frank Rowand  
>>>>>>>> wrote:
>>>>>>>>>
>>
>> < snip >
>>
>>>
>>> It makes it harder for me to read the source of the tests and
>>> understand the order they will execute.  It also makes it harder
>>> for me to read through the actual tests (in this example the
>>> tests that are currently grouped in of_unittest_find_node_by_name())
>>> because of all the extra function headers injected into the
>>> existing single function to break it apart into many smaller
>>> functions.
>>
>> < snip >
>>
>>>>>> This is not something I feel particularly strongly about, it is just
>>>>>> pretty atypical from my experience to have so many unrelated test
>>>>>> cases in a single file.
>>>>>>
>>>>>> Maybe you would prefer that I break up the test cases first, and then
>>>>>> we split up the file as appropriate?
>>>>>
>>>>> I prefer that the test cases not be broken up arbitrarily.  There _may_
>>
>> I expect that I created confusion by putting this in a reply to patch 18/19.
>> It is actually a comment about patch 19/19.  Sorry about that.
>>
> 
> No worries.
> 
>>
>>>>
>>>> I wasn't trying to break them up arbitrarily. I thought I was doing it
>>>> according to a pattern (breaking up the file, that is), but maybe I
>>>> just hadn't looked at enough examples.
>>>
>>> This goes back to the kunit model of putting each test into a separate
>>> function that can be a KUNIT_CASE().  That is a model that I do not agree
>>> with for devicetree.
>>
>> So now that I am actually talking about patch 19/19, let me give a concrete
>> example.  I will cut and paste (after my comments), the beginning portion
>> of base-test.c, after applying patch 19/19 (the "base version".  Then I
>> will cut and paste my alternative version which does not break the tests
>> down into individual functions (the "frank version").
> 
> Awesome, thanks for putting the comparison together!
> 
>>
>> I will also reply to this email with the base version and the frank version
>> as attachments, which will make it easier to save as separate versions
>> for easier viewing.  I'm not sure if an email with attachments will make
>> it through the list servers, but I am cautiously optimistic.
>>
>> I am using v4 of the patch series because I never got v3 to cleanly apply
>> and it is not a constructive use of my time to do so since I have v4 applied.
>>
>> One of the points I was trying to make is that readability suffers from the
>> approach taken by patches 18/19 and 19/19.
> 
> I understood that point.
> 
>>
>> The base version contains the extra text of a function header for each
>> unit test.  This is visual noise and makes the file larger.  It is also
>> one more possible location of an error (although not likely).
> 
> I don't see how it is much more visual noise than a comment.
> Admittedly, a space versus an underscore might be nice, but I think it
> is also more likely that a function name is more likely to be kept up
> to date than a comment even if they are both informational. It also
> forces the user to actually name all the tests. Even then, I am not
> married to doing it this exact way. The thing I really care about is
> isolating the code in each test case so that it can be executed
> separately.
> 
> A side thought, when I was proofreading this, it occurred to me that
> you might not like the function name over comment partly because you
> think about them differently. You aren't used to seeing a function
> used to frame things or communicate information in this way. Is this

No.  It is more visual clutter and it is more functional clutter that
potentially has to be validated.


> true? Admittedly, I have gotten used to a lot of unit test frameworks
> that break up test ca

Re: [RFC v4 17/17] of: unittest: split up some super large test cases

[Frank Rowand]

On 2/14/19 1:37 PM, Brendan Higgins wrote:
> Split up the super large test cases of_unittest_find_node_by_name and
> of_unittest_dynamic into properly sized and defined test cases.

I also still object to this patch.

-Frank


> 
> Signed-off-by: Brendan Higgins 
> ---
>  drivers/of/base-test.c | 297 ++---
>  1 file changed, 249 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/of/base-test.c b/drivers/of/base-test.c
> index 3d3f4f1b74800..7b44c967ed2fd 100644
> --- a/drivers/of/base-test.c
> +++ b/drivers/of/base-test.c
> @@ -8,10 +8,10 @@
>  
>  #include "test-common.h"
>  
> -static void of_unittest_find_node_by_name(struct kunit *test)
> +static void of_test_find_node_by_name_basic(struct kunit *test)
>  {
>   struct device_node *np;
> - const char *options, *name;
> + const char *name;
>  
>   np = of_find_node_by_path("/testcase-data");
>   name = kasprintf(GFP_KERNEL, "%pOF", np);
> @@ -20,11 +20,21 @@ static void of_unittest_find_node_by_name(struct kunit 
> *test)
>  "find /testcase-data failed\n");
>   of_node_put(np);
>   kfree(name);
> +}
>  
> +static void of_test_find_node_by_name_trailing_slash(struct kunit *test)
> +{
>   /* Test if trailing '/' works */
>   KUNIT_EXPECT_EQ_MSG(test, of_find_node_by_path("/testcase-data/"), NULL,
>   "trailing '/' on /testcase-data/ should fail\n");
>  
> +}
> +
> +static void of_test_find_node_by_name_multiple_components(struct kunit *test)
> +{
> + struct device_node *np;
> + const char *name;
> +
>   np = of_find_node_by_path("/testcase-data/phandle-tests/consumer-a");
>   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
>   name = kasprintf(GFP_KERNEL, "%pOF", np);
> @@ -33,6 +43,12 @@ static void of_unittest_find_node_by_name(struct kunit 
> *test)
>   "find /testcase-data/phandle-tests/consumer-a failed\n");
>   of_node_put(np);
>   kfree(name);
> +}
> +
> +static void of_test_find_node_by_name_with_alias(struct kunit *test)
> +{
> + struct device_node *np;
> + const char *name;
>  
>   np = of_find_node_by_path("testcase-alias");
>   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
> @@ -41,10 +57,23 @@ static void of_unittest_find_node_by_name(struct kunit 
> *test)
>  "find testcase-alias failed\n");
>   of_node_put(np);
>   kfree(name);
> +}
>  
> +static void of_test_find_node_by_name_with_alias_and_slash(struct kunit 
> *test)
> +{
>   /* Test if trailing '/' works on aliases */
>   KUNIT_EXPECT_EQ_MSG(test, of_find_node_by_path("testcase-alias/"), NULL,
> - "trailing '/' on testcase-alias/ should fail\n");
> +"trailing '/' on testcase-alias/ should fail\n");
> +}
> +
> +/*
> + * TODO(brendanhigg...@google.com): This looks like a duplicate of
> + * of_test_find_node_by_name_multiple_components
> + */
> +static void of_test_find_node_by_name_multiple_components_2(struct kunit 
> *test)
> +{
> + struct device_node *np;
> + const char *name;
>  
>   np = of_find_node_by_path("testcase-alias/phandle-tests/consumer-a");
>   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
> @@ -54,17 +83,33 @@ static void of_unittest_find_node_by_name(struct kunit 
> *test)
>   "find testcase-alias/phandle-tests/consumer-a failed\n");
>   of_node_put(np);
>   kfree(name);
> +}
> +
> +static void of_test_find_node_by_name_missing_path(struct kunit *test)
> +{
> + struct device_node *np;
>  
>   KUNIT_EXPECT_EQ_MSG(
>   test,
>   np = of_find_node_by_path("/testcase-data/missing-path"), NULL,
>   "non-existent path returned node %pOF\n", np);
>   of_node_put(np);
> +}
> +
> +static void of_test_find_node_by_name_missing_alias(struct kunit *test)
> +{
> + struct device_node *np;
>  
>   KUNIT_EXPECT_EQ_MSG(
>   test, np = of_find_node_by_path("missing-alias"), NULL,
>   "non-existent alias returned node %pOF\n", np);
>   of_node_put(np);
> +}
> +
> +static void of_test_find_node_by_name_missing_alias_with_relative_path(
> + struct kunit *test)
> +{
> + struct device_node *np;
>  
>   KUNIT_EXPECT_EQ_MSG(
>   test,
> @@ -72,12 +117,24 @@ static void of_unittest_find_node_by_name(struct kunit 
> *test)
>   "non-existent alias with relative path returned node %pOF\n",
>   np);
>   of_node_put(np);
> +}
> +
> +static void of_test_find_node_by_name_with_option(struct kunit *test)
> +{
> + struct device_node *np;
> + const char *options;
>  
>   np = of_find_node_opts_by_path("/testcase-data:testoption", );
>   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
>   KUNIT_EXPECT_STREQ_MSG(test, "testoption", options,
>  "option path test failed\n");
>   of_node_put(np);
> +}
> +
> +static void 

Re: [RFC v4 00/17] kunit: introduce KUnit, the Linux kernel unit testing framework

[Frank Rowand]

On 3/4/19 3:01 PM, Brendan Higgins wrote:
> On Thu, Feb 14, 2019 at 1:38 PM Brendan Higgins
>  wrote:
>>
>> This patch set proposes KUnit, a lightweight unit testing and mocking
>> framework for the Linux kernel.
>>
> 
> 
> 
>> ## More information on KUnit
>>
>> There is a bunch of documentation near the end of this patch set that
>> describes how to use KUnit and best practices for writing unit tests.
>> For convenience I am hosting the compiled docs here:
>> https://google.github.io/kunit-docs/third_party/kernel/docs/
>> Additionally for convenience, I have applied these patches to a branch:
>> https://kunit.googlesource.com/linux/+/kunit/rfc/5.0-rc5/v4
>> The repo may be cloned with:
>> git clone https://kunit.googlesource.com/linux
>> This patchset is on the kunit/rfc/5.0-rc5/v4 branch.
>>
>> ## Changes Since Last Version
>>
>>  - Got KUnit working on (hypothetically) all architectures (tested on
>>x86), as per Rob's (and other's) request
>>  - Punting all KUnit features/patches depending on UML for now.
>>  - Broke out UML specific support into arch/um/* as per "[RFC v3 01/19]
>>kunit: test: add KUnit test runner core", as requested by Luis.
>>  - Added support to kunit_tool to allow it to build kernels in external
>>directories, as suggested by Kieran.
>>  - Added a UML defconfig, and a config fragment for KUnit as suggested
>>by Kieran and Luis.
>>  - Cleaned up, and reformatted a bunch of stuff.
>>
>> --
>> 2.21.0.rc0.258.g878e2cd30e-goog
>>
> 
> Someone suggested I should send the next revision out as "PATCH"
> instead of "RFC" since there seems to be general consensus about
> everything at a high level, with a couple exceptions.
> 
> At this time I am planning on sending the next revision out as "[PATCH
> v1 00/NN] kunit: introduce KUnit, the Linux kernel unit testing
> framework". Initially I wasn't sure if the next revision should be
> "[PATCH v1 ...]" or "[PATCH v5 ...]". Please let me know if you have a
> strong objection to the former.
> 
> In the next revision, I will be dropping the last two of three patches
> for the DT unit tests as there doesn't seem to be enough features
> currently available to justify the heavy refactoring I did; however, I

Thank you.


> will still include the patch that just converts everything over to
> KUnit without restructuring the test cases:
> https://lkml.org/lkml/2019/2/14/1133

The link doesn't work for me (don't worry about that), so I'm assuming
this is:

   [RFC v4 15/17] of: unittest: migrate tests to run on KUnit

The conversation on that patch ended after:

   >> After adding patch 15, there are a lot of "unittest internal error" 
messages.
   > 
   > Yeah, I meant to ask you about that. I thought it was due to a change
   > you made, but after further examination, just now, I found it was my
   > fault. Sorry for not mentioning that anywhere. I will fix it in v5.

It is not worth my time to look at patch 15 when it is that broken.  So I
have not done any review of it.

So no, I think you are still in the RFC stage unless you drop patch 15.

> 
> I should have the next revision out in a week or so.
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v3 00/19] kunit: introduce KUnit, the Linux kernel unit testing framework

[Frank Rowand]

On 12/5/18 3:10 PM, Brendan Higgins wrote:
> On Tue, Dec 4, 2018 at 5:49 AM Rob Herring  wrote:
>>
>> On Tue, Dec 4, 2018 at 5:40 AM Frank Rowand  wrote:
>>>
>>> Hi Brendan, Rob,
>>>
>>> Pulling a comment from way back in the v1 patch thread:
>>>
>>> On 10/17/18 3:22 PM, Brendan Higgins wrote:
>>>> On Wed, Oct 17, 2018 at 10:49 AM  wrote:
>>>
>>> < snip >
>>>
>>>> The test and the code under test are linked together in the same
>>>> binary and are compiled under Kbuild. Right now I am linking
>>>> everything into a UML kernel, but I would ultimately like to make
>>>> tests compile into completely independent test binaries. So each test
>>>> file would get compiled into its own test binary and would link
>>>> against only the code needed to run the test, but we are a bit of a
>>>> ways off from that.
>>>
>>> I have never used UML, so you should expect naive questions from me,
>>> exhibiting my lack of understanding.
>>>
>>> Does this mean that I have to build a UML architecture kernel to run
>>> the KUnit tests?
>>
>> In this version of the patch series, yes.
>>
>>> *** Rob, if the answer is yes, then it seems like for my workflow,
>>> which is to build for real ARM hardware, my work is doubled (or
>>> worse), because for every patch/commit that I apply, I not only have
>>> to build the ARM kernel and boot on the real hardware to test, I also
>>> have to build the UML kernel and boot in UML.  If that is correct
>>> then I see this as a major problem for me.
>>
>> I've already raised this issue elsewhere in the series. Restricting
>> the DT tests to UML is a non-starter.
> 

> I have already stated my position elsewhere on the matter, but in
> summary: Ensuring most tests can run without external dependencies
> (hardware, VM, etc) has a lot of benefits and should be supported in
> nearly all cases, but such tests should also work when compiled to run
> on real hardware/VM; the tooling might not be as good in the latter
> case, but I understand that there are good reasons to support it
> nonetheless.

And my needs are the exact opposite.  My tests must run on real hardware,
in the context of the real operating system subsystems and drivers
potentially causing issues.

It is useful if the tests can also run without that dependency.

-Frank


> 
> So I am going to try to add basic support for running tests on other
> architectures in the next version or two.

< snip >
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v3 18/19] of: unittest: split out a couple of test cases from unittest

[Frank Rowand]

On 3/21/19 6:30 PM, Brendan Higgins wrote:
> On Thu, Mar 21, 2019 at 5:22 PM Frank Rowand  wrote:
>>
>> On 2/27/19 7:52 PM, Brendan Higgins wrote:

< snip >  but thanks for the comments in the snipped section.


>>
>> Thanks for leaving 18/19 and 19/19 off in v4.
> 
> Sure, no problem. It was pretty clear that it was a waste of both of
> our times to continue discussing those at this juncture. :-)
> 
> Do you still want me to try to convert the DT not-exactly-unittest to
> KUnit? I would kind of prefer (I don't feel *super* strongly about the
> matter) we don't call it that since I was intending for it to be the
> flagship initial example, but I certainly don't mind trying to clean
> this patch up to get it up to snuff. It's really just a question of
> whether it is worth it to you.

In the long term, if KUnit is adopted by the kernel, then I think it
probably makes sense for devicetree unittest to convert from using
our own unittest() function to report an individual test pass/fail
to instead use something like KUNIT_EXPECT_*() to provide more
consistent test messages to test frameworks.  That is assuming
KUNIT_EXPECT_*() provides comparable functionality.  I still have
not looked into that question since the converted tests (patch 15/17
in v4) still does not execute without throwing internal errors.

If that conversion occurred, I would also avoid the ASSERTs.

> 
> < snip >
> 
> Cheers!
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v3 18/19] of: unittest: split out a couple of test cases from unittest

[Frank Rowand]

On 2/18/19 2:25 PM, Frank Rowand wrote:
> On 2/15/19 2:56 AM, Brendan Higgins wrote:
>> On Thu, Feb 14, 2019 at 6:05 PM Frank Rowand  wrote:
>>>
>>> On 2/14/19 4:56 PM, Brendan Higgins wrote:
>>>> On Thu, Feb 14, 2019 at 3:57 PM Frank Rowand  
>>>> wrote:
>>>>>
>>>>> On 12/5/18 3:54 PM, Brendan Higgins wrote:
>>>>>> On Tue, Dec 4, 2018 at 2:58 AM Frank Rowand  
>>>>>> wrote:
>>>>>>>

< snip >

>
> It makes it harder for me to read the source of the tests and
> understand the order they will execute.  It also makes it harder
> for me to read through the actual tests (in this example the
> tests that are currently grouped in of_unittest_find_node_by_name())
> because of all the extra function headers injected into the
> existing single function to break it apart into many smaller
> functions.

< snip >

>>>> This is not something I feel particularly strongly about, it is just
>>>> pretty atypical from my experience to have so many unrelated test
>>>> cases in a single file.
>>>>
>>>> Maybe you would prefer that I break up the test cases first, and then
>>>> we split up the file as appropriate?
>>>
>>> I prefer that the test cases not be broken up arbitrarily.  There _may_

I expect that I created confusion by putting this in a reply to patch 18/19.
It is actually a comment about patch 19/19.  Sorry about that.


>>
>> I wasn't trying to break them up arbitrarily. I thought I was doing it
>> according to a pattern (breaking up the file, that is), but maybe I
>> just hadn't looked at enough examples.
> 
> This goes back to the kunit model of putting each test into a separate
> function that can be a KUNIT_CASE().  That is a model that I do not agree
> with for devicetree.

So now that I am actually talking about patch 19/19, let me give a concrete
example.  I will cut and paste (after my comments), the beginning portion
of base-test.c, after applying patch 19/19 (the "base version".  Then I
will cut and paste my alternative version which does not break the tests
down into individual functions (the "frank version").

I will also reply to this email with the base version and the frank version
as attachments, which will make it easier to save as separate versions
for easier viewing.  I'm not sure if an email with attachments will make
it through the list servers, but I am cautiously optimistic.

I am using v4 of the patch series because I never got v3 to cleanly apply
and it is not a constructive use of my time to do so since I have v4 applied.

One of the points I was trying to make is that readability suffers from the
approach taken by patches 18/19 and 19/19.

The base version contains the extra text of a function header for each
unit test.  This is visual noise and makes the file larger.  It is also
one more possible location of an error (although not likely).

The frank version has converted each of the new function headers into
a comment, using the function name with '_' converted to ' '.  The
comments are more readable than the function headers.  Note that I added
an extra blank line before each comment, which violates the kernel
coding standards, but I feel this makes the code more readable.

The base version needs to declare each of the individual test functions
in of_test_find_node_by_name_cases[]. It is possible that a test function
could be left out of of_test_find_node_by_name_cases[], in error.  This
will result in a compile warning (I think warning instead of error, but
I have not verified that) so the error might be caught or it might be
overlooked.

In the base version, the order of execution of the test code requires
bouncing back and forth between the test functions and the coding of
of_test_find_node_by_name_cases[].

In the frank version the order of execution of the test code is obvious.

It is possible that a test function could be left out of
of_test_find_node_by_name_cases[], in error.  This will result in a compile
warning (I think warning instead of error, but I have not verified that)
so it might be caught or it might be overlooked.

The base version is 265 lines.  The frank version is 208 lines, 57 lines
less.  Less is better.


## ==  base version  

// SPDX-License-Identifier: GPL-2.0
/*
 * Unit tests for functions defined in base.c.
 */
#include 

#include 

#include "test-common.h"

static void of_test_find_node_by_name_basic(struct kunit *test)
{
struct device_node *np;
const char *name;

np = of_find_node_by_path("/testcase-data");
name = kasprintf(GFP_KERNEL, "%pOF", np);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
KUNIT_EXPEC

Re: [RFC v3 18/19] of: unittest: split out a couple of test cases from unittest

[Frank Rowand]

On 2/20/19 12:44 PM, Frank Rowand wrote:
> On 2/18/19 2:25 PM, Frank Rowand wrote:
>> On 2/15/19 2:56 AM, Brendan Higgins wrote:
>>> On Thu, Feb 14, 2019 at 6:05 PM Frank Rowand  wrote:
>>>>
>>>> On 2/14/19 4:56 PM, Brendan Higgins wrote:
>>>>> On Thu, Feb 14, 2019 at 3:57 PM Frank Rowand  
>>>>> wrote:
>>>>>>
>>>>>> On 12/5/18 3:54 PM, Brendan Higgins wrote:
>>>>>>> On Tue, Dec 4, 2018 at 2:58 AM Frank Rowand  
>>>>>>> wrote:
>>>>>>>>
> 
> < snip >
> 
>>
>> It makes it harder for me to read the source of the tests and
>> understand the order they will execute.  It also makes it harder
>> for me to read through the actual tests (in this example the
>> tests that are currently grouped in of_unittest_find_node_by_name())
>> because of all the extra function headers injected into the
>> existing single function to break it apart into many smaller
>> functions.
> 
> < snip >
> 
>>>>> This is not something I feel particularly strongly about, it is just
>>>>> pretty atypical from my experience to have so many unrelated test
>>>>> cases in a single file.
>>>>>
>>>>> Maybe you would prefer that I break up the test cases first, and then
>>>>> we split up the file as appropriate?
>>>>
>>>> I prefer that the test cases not be broken up arbitrarily.  There _may_
> 
> I expect that I created confusion by putting this in a reply to patch 18/19.
> It is actually a comment about patch 19/19.  Sorry about that.
> 
> 
>>>
>>> I wasn't trying to break them up arbitrarily. I thought I was doing it
>>> according to a pattern (breaking up the file, that is), but maybe I
>>> just hadn't looked at enough examples.
>>
>> This goes back to the kunit model of putting each test into a separate
>> function that can be a KUNIT_CASE().  That is a model that I do not agree
>> with for devicetree.
> 
> So now that I am actually talking about patch 19/19, let me give a concrete
> example.  I will cut and paste (after my comments), the beginning portion
> of base-test.c, after applying patch 19/19 (the "base version".  Then I
> will cut and paste my alternative version which does not break the tests
> down into individual functions (the "frank version").
> 
> I will also reply to this email with the base version and the frank version
> as attachments, which will make it easier to save as separate versions
> for easier viewing.  I'm not sure if an email with attachments will make
> it through the list servers, but I am cautiously optimistic.

base_version and frank_version attached.

-Frank


> 
> I am using v4 of the patch series because I never got v3 to cleanly apply
> and it is not a constructive use of my time to do so since I have v4 applied.
> 
> One of the points I was trying to make is that readability suffers from the
> approach taken by patches 18/19 and 19/19.
> 
> The base version contains the extra text of a function header for each
> unit test.  This is visual noise and makes the file larger.  It is also
> one more possible location of an error (although not likely).
> 
> The frank version has converted each of the new function headers into
> a comment, using the function name with '_' converted to ' '.  The
> comments are more readable than the function headers.  Note that I added
> an extra blank line before each comment, which violates the kernel
> coding standards, but I feel this makes the code more readable.
> 
> The base version needs to declare each of the individual test functions
> in of_test_find_node_by_name_cases[]. It is possible that a test function
> could be left out of of_test_find_node_by_name_cases[], in error.  This
> will result in a compile warning (I think warning instead of error, but
> I have not verified that) so the error might be caught or it might be
> overlooked.
> 
> In the base version, the order of execution of the test code requires
> bouncing back and forth between the test functions and the coding of
> of_test_find_node_by_name_cases[].
> 
> In the frank version the order of execution of the test code is obvious.
> 
> It is possible that a test function could be left out of
> of_test_find_node_by_name_cases[], in error.  This will result in a compile
> warning (I think warning instead of error, but I have not verified that)
> so it might be caught or it might be overlooked.
> 
> The base version is 265 lines.  The frank version is 208 lines, 57 lines
> less.  Less is better.
> 
> 
> ##

Re: [RFC v4 08/17] kunit: test: add support for test abort

[Frank Rowand]

On 2/19/19 7:39 PM, Brendan Higgins wrote:
> On Mon, Feb 18, 2019 at 11:52 AM Frank Rowand  wrote:
>>
>> On 2/14/19 1:37 PM, Brendan Higgins wrote:
>>> Add support for aborting/bailing out of test cases. Needed for
>>> implementing assertions.
>>>
>>> Signed-off-by: Brendan Higgins 
>>> ---
>>> Changes Since Last Version
>>>  - This patch is new introducing a new cross-architecture way to abort
>>>out of a test case (needed for KUNIT_ASSERT_*, see next patch for
>>>details).
>>>  - On a side note, this is not a complete replacement for the UML abort
>>>mechanism, but covers the majority of necessary functionality. UML
>>>architecture specific featurs have been dropped from the initial
>>>patchset.
>>> ---
>>>  include/kunit/test.h |  24 +
>>>  kunit/Makefile   |   3 +-
>>>  kunit/test-test.c| 127 ++
>>>  kunit/test.c | 208 +--
>>>  4 files changed, 353 insertions(+), 9 deletions(-)
>>>  create mode 100644 kunit/test-test.c
>>
>> < snip >
>>
>>> diff --git a/kunit/test.c b/kunit/test.c
>>> index d18c50d5ed671..6e5244642ab07 100644
>>> --- a/kunit/test.c
>>> +++ b/kunit/test.c
>>> @@ -6,9 +6,9 @@
>>>   * Author: Brendan Higgins 
>>>   */
>>>
>>> -#include 
>>>  #include 
>>> -#include 
>>> +#include 
>>> +#include 
>>>  #include 
>>>
>>>  static bool kunit_get_success(struct kunit *test)
>>> @@ -32,6 +32,27 @@ static void kunit_set_success(struct kunit *test, bool 
>>> success)
>>>   spin_unlock_irqrestore(>lock, flags);
>>>  }
>>>
>>> +static bool kunit_get_death_test(struct kunit *test)
>>> +{
>>> + unsigned long flags;
>>> + bool death_test;
>>> +
>>> + spin_lock_irqsave(>lock, flags);
>>> + death_test = test->death_test;
>>> + spin_unlock_irqrestore(>lock, flags);
>>> +
>>> + return death_test;
>>> +}
>>> +
>>> +static void kunit_set_death_test(struct kunit *test, bool death_test)
>>> +{
>>> + unsigned long flags;
>>> +
>>> + spin_lock_irqsave(>lock, flags);
>>> + test->death_test = death_test;
>>> + spin_unlock_irqrestore(>lock, flags);
>>> +}
>>> +
>>>  static int kunit_vprintk_emit(const struct kunit *test,
>>> int level,
>>> const char *fmt,
>>> @@ -70,13 +91,29 @@ static void kunit_fail(struct kunit *test, struct 
>>> kunit_stream *stream)
>>>   stream->commit(stream);
>>>  }
>>>
>>> +static void __noreturn kunit_abort(struct kunit *test)
>>> +{
>>> + kunit_set_death_test(test, true);
>>> +
>>> + test->try_catch.throw(>try_catch);
>>> +
>>> + /*
>>> +  * Throw could not abort from test.
>>> +  */
>>> + kunit_err(test, "Throw could not abort from test!");
>>> + show_stack(NULL, NULL);
>>> + BUG();
>>
>> kunit_abort() is what will be call as the result of an assert failure.
> 
> Yep. Does that need clarified somewhere.
>>
>> BUG(), which is a panic, which is crashing the system is not acceptable
>> in the Linux kernel.  You will just annoy Linus if you submit this.
> 
> Sorry, I thought this was an acceptable use case since, a) this should
> never be compiled in a production kernel, b) we are in a pretty bad,
> unpredictable state if we get here and keep going. I think you might
> have said elsewhere that you think "a" is not valid? In any case, I
> can replace this with a WARN, would that be acceptable?

A WARN may or may not make sense, depending on the context.  It may
be sufficient to simply report a test failure (as in the old version
of case (2) below.

Answers to "a)" and "b)":

a) it might be in a production kernel

a') it is not acceptable in my development kernel either

b) No.  You don't crash a developer's kernel either unless it is
required to avoid data corruption.

b') And you can not do replacements like:

(1) in of_unittest_check_tree_linkage()

-  old  -

if (!of_root)
return;

-  new  -

KUNIT_ASSERT_NOT_ERR_OR_NULL(test, of_root);

(2) in of_unittest_property_string()

-  old  -

/* of_property_read_string_index() tests */
rc = of_property_read_string_index(np, "string-property", 0, strings);
unittest(rc == 0 && !strcmp(strings[0], "foobar"), 
"of_property_read_string_index() failure; rc=%i\n", rc);

-  new  -

/* of_property_read_string_index() tests */
rc = of_property_read_string_index(np, "string-property", 0, strings);
KUNIT_ASSERT_EQ(test, rc, 0);
KUNIT_EXPECT_STREQ(test, strings[0], "foobar");


If a test fails, that is no reason to abort testing.  The remainder of the unit
tests can still run.  There may be cascading failures, but that is ok.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v4 00/17] kunit: introduce KUnit, the Linux kernel unit testing framework

[Frank Rowand]

On 2/19/19 10:34 PM, Brendan Higgins wrote:
> On Mon, Feb 18, 2019 at 12:02 PM Frank Rowand  wrote:
> 
>> I have not read through the patches in any detail.  I have read some of
>> the code to try to understand the patches to the devicetree unit tests.
>> So that may limit how valid my comments below are.
> 
> No problem.
> 
>>
>> I found the code difficult to read in places where it should have been
>> much simpler to read.  Structuring the code in a pseudo object oriented
>> style meant that everywhere in a code path that I encountered a dynamic
>> function call, I had to go find where that dynamic function call was
>> initialized (and being the cautious person that I am, verify that
>> no where else was the value of that dynamic function call).  With
>> primitive vi and tags, that search would have instead just been a
>> simple key press (or at worst a few keys) if hard coded function
>> calls were done instead of dynamic function calls.  In the code paths
>> that I looked at, I did not see any case of a dynamic function being
>> anything other than the value it was originally initialized as.
>> There may be such cases, I did not read the entire patch set.  There
>> may also be cases envisioned in the architects mind of how this
>> flexibility may be of future value.  Dunno.
> 
> Yeah, a lot of it is intended to make architecture specific
> implementations and some other future work easier. Some of it is also
> for testing purposes. Admittedly some is for neither reason, but given
> the heavy usage elsewhere, I figured there was no harm since it was
> all private internal usage anyway.
> 

Increasing the cost for me (and all the other potential code readers)
to read the code is harm.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v3 17/19] of: unittest: migrate tests to run on KUnit

[Frank Rowand]

On 2/12/19 5:44 PM, Brendan Higgins wrote:
> On Wed, Nov 28, 2018 at 12:56 PM Rob Herring  wrote:
>>
>> On Wed, Nov 28, 2018 at 1:38 PM Brendan Higgins
>>  wrote:
>>>
>>> Migrate tests without any cleanup, or modifying test logic in anyway to
>>> run under KUnit using the KUnit expectation and assertion API.
>>
>> Nice! You beat me to it. This is probably going to conflict with what
>> is in the DT tree for 4.21. Also, please Cc the DT list for
>> drivers/of/ changes.
>>
>> Looks good to me, but a few mostly formatting comments below.
> 
> I just realized that we never talked about your other comments, and I
> still have some questions. (Sorry, it was the last thing I looked at
> while getting v4 ready.) No worries if you don't get to it before I
> send v4 out, I just didn't want you to think I was ignoring you.
> 
>>
>>>
>>> Signed-off-by: Brendan Higgins 
>>> ---
>>>  drivers/of/Kconfig|1 +
>>>  drivers/of/unittest.c | 1405 ++---
>>>  2 files changed, 752 insertions(+), 654 deletions(-)
>>>
> 
>>> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
>>> index 41b49716ac75f..a5ef44730ffdb 100644
>>> --- a/drivers/of/unittest.c
>>> +++ b/drivers/of/unittest.c
> 
>>> -
>>> -static void __init of_unittest_find_node_by_name(void)
>>> +static void of_unittest_find_node_by_name(struct kunit *test)
>>
>> Why do we have to drop __init everywhere? The tests run later?
> 
>>From the standpoint of a unit test __init doesn't really make any
> sense, right? I know that right now we are running as part of a
> kernel, but the goal should be that a unit test is not part of a
> kernel and we just include what we need.
> 
> Even so, that's the future. For now, I did not put the KUnit
> infrastructure in the .init section because I didn't think it belonged
> there. In practice, KUnit only knows how to run during the init phase
> of the kernel, but I don't think it should be restricted there. You
> should be able to run tests whenever you want because you should be
> able to test anything right? I figured any restriction on that is
> misleading and will potentially get in the way at worst, and
> unnecessary at best especially since people shouldn't build a
> production kernel with all kinds of unit tests inside.
> 
>>
>>>  {
>>> struct device_node *np;
>>> const char *options, *name;
>>>
> 
>>>
>>>
>>> -   np = of_find_node_by_path("/testcase-data/missing-path");
>>> -   unittest(!np, "non-existent path returned node %pOF\n", np);
>>> +   KUNIT_EXPECT_EQ_MSG(test,
>>> +   
>>> of_find_node_by_path("/testcase-data/missing-path"),
>>> +   NULL,
>>> +   "non-existent path returned node %pOF\n", np);
>>
>> 1 tab indent would help with less vertical code (in general, not this
>> one so much).
> 
> Will do.
> 
>>
>>> of_node_put(np);
>>>
>>> -   np = of_find_node_by_path("missing-alias");
>>> -   unittest(!np, "non-existent alias returned node %pOF\n", np);
>>> +   KUNIT_EXPECT_EQ_MSG(test, of_find_node_by_path("missing-alias"), 
>>> NULL,
>>> +   "non-existent alias returned node %pOF\n", np);
>>> of_node_put(np);
>>>
>>> -   np = of_find_node_by_path("testcase-alias/missing-path");
>>> -   unittest(!np, "non-existent alias with relative path returned node 
>>> %pOF\n", np);
>>> +   KUNIT_EXPECT_EQ_MSG(test,
>>> +   
>>> of_find_node_by_path("testcase-alias/missing-path"),
>>> +   NULL,
>>> +   "non-existent alias with relative path returned 
>>> node %pOF\n",
>>> +   np);
>>> of_node_put(np);
>>>
> 
>>>
>>> -static void __init of_unittest_property_string(void)
>>> +static void of_unittest_property_string(struct kunit *test)
>>>  {
>>> const char *strings[4];
>>> struct device_node *np;
>>> int rc;
>>>
>>> np = 
>>> of_find_node_by_path("/testcase-data/phandle-tests/consumer-a");
>>> -   if (!np) {
>>> -   pr_err("No testcase data in device tree\n");
>>> -   return;
>>> -   }
>>> -
>>> -   rc = of_property_match_string(np, "phandle-list-names", "first");
>>> -   unittest(rc == 0, "first expected:0 got:%i\n", rc);
>>> -   rc = of_property_match_string(np, "phandle-list-names", "second");
>>> -   unittest(rc == 1, "second expected:1 got:%i\n", rc);
>>> -   rc = of_property_match_string(np, "phandle-list-names", "third");
>>> -   unittest(rc == 2, "third expected:2 got:%i\n", rc);
>>> -   rc = of_property_match_string(np, "phandle-list-names", "fourth");
>>> -   unittest(rc == -ENODATA, "unmatched string; rc=%i\n", rc);
>>> -   rc = of_property_match_string(np, "missing-property", "blah");
>>> -   unittest(rc == -EINVAL, "missing property; rc=%i\n", rc);
>>> -   rc = of_property_match_string(np, "empty-property", 

Re: [RFC v4 00/17] kunit: introduce KUnit, the Linux kernel unit testing framework

[Frank Rowand]

On 2/14/19 1:37 PM, Brendan Higgins wrote:
> This patch set proposes KUnit, a lightweight unit testing and mocking
> framework for the Linux kernel.
> 
> Unlike Autotest and kselftest, KUnit is a true unit testing framework;
> it does not require installing the kernel on a test machine or in a VM
> and does not require tests to be written in userspace running on a host
> kernel. Additionally, KUnit is fast: From invocation to completion KUnit
> can run several dozen tests in under a second. Currently, the entire
> KUnit test suite for KUnit runs in under a second from the initial
> invocation (build time excluded).
> 
> KUnit is heavily inspired by JUnit, Python's unittest.mock, and
> Googletest/Googlemock for C++. KUnit provides facilities for defining
> unit test cases, grouping related test cases into test suites, providing
> common infrastructure for running tests, mocking, spying, and much more.
> 
> ## What's so special about unit testing?
> 
> A unit test is supposed to test a single unit of code in isolation,
> hence the name. There should be no dependencies outside the control of
> the test; this means no external dependencies, which makes tests orders
> of magnitudes faster. Likewise, since there are no external dependencies,
> there are no hoops to jump through to run the tests. Additionally, this
> makes unit tests deterministic: a failing unit test always indicates a
> problem. Finally, because unit tests necessarily have finer granularity,
> they are able to test all code paths easily solving the classic problem
> of difficulty in exercising error handling code.
> 
> ## Is KUnit trying to replace other testing frameworks for the kernel?
> 
> No. Most existing tests for the Linux kernel are end-to-end tests, which
> have their place. A well tested system has lots of unit tests, a
> reasonable number of integration tests, and some end-to-end tests. KUnit
> is just trying to address the unit test space which is currently not
> being addressed.
> 
> ## More information on KUnit
> 
> There is a bunch of documentation near the end of this patch set that
> describes how to use KUnit and best practices for writing unit tests.
> For convenience I am hosting the compiled docs here:
> https://google.github.io/kunit-docs/third_party/kernel/docs/
> Additionally for convenience, I have applied these patches to a branch:
> https://kunit.googlesource.com/linux/+/kunit/rfc/5.0-rc5/v4
> The repo may be cloned with:
> git clone https://kunit.googlesource.com/linux
> This patchset is on the kunit/rfc/5.0-rc5/v4 branch.
> 
> ## Changes Since Last Version
> 
>  - Got KUnit working on (hypothetically) all architectures (tested on
>x86), as per Rob's (and other's) request
>  - Punting all KUnit features/patches depending on UML for now.
>  - Broke out UML specific support into arch/um/* as per "[RFC v3 01/19]
>kunit: test: add KUnit test runner core", as requested by Luis.
>  - Added support to kunit_tool to allow it to build kernels in external
>directories, as suggested by Kieran.
>  - Added a UML defconfig, and a config fragment for KUnit as suggested
>by Kieran and Luis.
>  - Cleaned up, and reformatted a bunch of stuff.
> 

I have not read through the patches in any detail.  I have read some of
the code to try to understand the patches to the devicetree unit tests.
So that may limit how valid my comments below are.

I found the code difficult to read in places where it should have been
much simpler to read.  Structuring the code in a pseudo object oriented
style meant that everywhere in a code path that I encountered a dynamic
function call, I had to go find where that dynamic function call was
initialized (and being the cautious person that I am, verify that
no where else was the value of that dynamic function call).  With
primitive vi and tags, that search would have instead just been a
simple key press (or at worst a few keys) if hard coded function
calls were done instead of dynamic function calls.  In the code paths
that I looked at, I did not see any case of a dynamic function being
anything other than the value it was originally initialized as.
There may be such cases, I did not read the entire patch set.  There
may also be cases envisioned in the architects mind of how this
flexibility may be of future value.  Dunno.

-Frank
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v3 18/19] of: unittest: split out a couple of test cases from unittest

[Frank Rowand]

On 2/15/19 2:56 AM, Brendan Higgins wrote:
> On Thu, Feb 14, 2019 at 6:05 PM Frank Rowand  wrote:
>>
>> On 2/14/19 4:56 PM, Brendan Higgins wrote:
>>> On Thu, Feb 14, 2019 at 3:57 PM Frank Rowand  wrote:
>>>>
>>>> On 12/5/18 3:54 PM, Brendan Higgins wrote:
>>>>> On Tue, Dec 4, 2018 at 2:58 AM Frank Rowand  
>>>>> wrote:
>>>>>>
>>>>>> Hi Brendan,
>>>>>>
>>>>>> On 11/28/18 11:36 AM, Brendan Higgins wrote:
>>>>>>> Split out a couple of test cases that these features in base.c from the
>>>>>>> unittest.c monolith. The intention is that we will eventually split out
>>>>>>> all test cases and group them together based on what portion of device
>>>>>>> tree they test.
>>>>>>
>>>>>> Why does splitting this file apart improve the implementation?
>>>>>
>>>>> This is in preparation for patch 19/19 and other hypothetical future
>>>>> patches where test cases are split up and grouped together by what
>>>>> portion of DT they test (for example the parsing tests and the
>>>>> platform/device tests would probably go separate files as well). This
>>>>> patch by itself does not do anything useful, but I figured it made
>>>>> patch 19/19 (and, if you like what I am doing, subsequent patches)
>>>>> easier to review.
>>>>
>>>> I do not see any value in splitting the devicetree tests into
>>>> multiple files.
>>>>
>>>> Please help me understand what the benefits of such a split are.
>>
>> Note that my following comments are specific to the current devicetree
>> unittests, and may not apply to the general case of unit tests in other
>> subsystems.
>>
> Note taken.
>>
>>> Sorry, I thought it made sense in context of what I am doing in the
>>> following patch. All I am trying to do is to provide an effective way
>>> of grouping test cases. To be clear, the idea, assuming you agree, is
>>
>> Looking at _just_ the first few fragments of the following patch, the
>> change is to break down a moderate size function of related tests,
>> of_unittest_find_node_by_name(), into a lot of extremely small functions.
> 
> Hmm...I wouldn't call that a moderate function. By my standards those
> functions are pretty large. In any case, I want to limit the
> discussion to specifically what a test case should look like, and the
> general consensus outside of the kernel is that unit test cases should
> be very very small. The reason is that each test case is supposed to> test 
> one specific property; it should be obvious what that property
> is; and it should be obvious what is needed to exercise that property.

That is a valid model and philosophy of unit test design.

It is not a model that the devicetree unit tests can be shoe horned
into.  Sort of...  In a sense, the existing devicetree unit tests
already to that, if you consider each unittest() (and sometime a few
lines of code that creates the result that unittest() checks) to be a separate
unit test.  But the kunit model does not consider the sort of
equivalent KUNIT_EXPECT_EQ(), etc, to be a unit test, the unit test
in kunit would be KUNIT_CASE().  Although it is a little confusing to
me that the initialization and clean up on exit occur one level
higher than KUNIT_CASE(), in struct kunit_module.  I think the
confusion is just a matter of slight conflict in the documentation
(btw, the documents where very helpful for me to understand the
overall concepts and model).


>> Then to find the execution order of the many small functions requires
>> finding the array of_test_find_node_by_name_cases[].  Then I have to
> 
> Execution order shouldn't matter. Each test case should be totally
> hermetic. Obviously in this case we depend on the preceeding test case
> to clean up properly, but that is something I am working on.

But the order _does_ matter for the devicetree unit tests.

That is one of the problems.  The devicetree unit tests are not small,
independent tests.  Some of the tests change state in a way that
following tests depend upon.

The design documents also mention that each unit test should have
a pre-test initialization, and a post-test cleanup to remove the
results of the initialization.

The devicetree unit tests have a large, intrusive initialization.
Once again, not a good fit for this model.

The devicetree unit tests also have an undocumented (and not at all
obvious) need to leave state changed in some cases after the test
completes.  There are cases where the way that I fully validate
the suc

Re: [RFC v4 08/17] kunit: test: add support for test abort

[Frank Rowand]

On 2/14/19 1:37 PM, Brendan Higgins wrote:
> Add support for aborting/bailing out of test cases. Needed for
> implementing assertions.
> 
> Signed-off-by: Brendan Higgins 
> ---
> Changes Since Last Version
>  - This patch is new introducing a new cross-architecture way to abort
>out of a test case (needed for KUNIT_ASSERT_*, see next patch for
>details).
>  - On a side note, this is not a complete replacement for the UML abort
>mechanism, but covers the majority of necessary functionality. UML
>architecture specific featurs have been dropped from the initial
>patchset.
> ---
>  include/kunit/test.h |  24 +
>  kunit/Makefile   |   3 +-
>  kunit/test-test.c| 127 ++
>  kunit/test.c | 208 +--
>  4 files changed, 353 insertions(+), 9 deletions(-)
>  create mode 100644 kunit/test-test.c

< snip >

> diff --git a/kunit/test.c b/kunit/test.c
> index d18c50d5ed671..6e5244642ab07 100644
> --- a/kunit/test.c
> +++ b/kunit/test.c
> @@ -6,9 +6,9 @@
>   * Author: Brendan Higgins 
>   */
>  
> -#include 
>  #include 
> -#include 
> +#include 
> +#include 
>  #include 
>  
>  static bool kunit_get_success(struct kunit *test)
> @@ -32,6 +32,27 @@ static void kunit_set_success(struct kunit *test, bool 
> success)
>   spin_unlock_irqrestore(>lock, flags);
>  }
>  
> +static bool kunit_get_death_test(struct kunit *test)
> +{
> + unsigned long flags;
> + bool death_test;
> +
> + spin_lock_irqsave(>lock, flags);
> + death_test = test->death_test;
> + spin_unlock_irqrestore(>lock, flags);
> +
> + return death_test;
> +}
> +
> +static void kunit_set_death_test(struct kunit *test, bool death_test)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(>lock, flags);
> + test->death_test = death_test;
> + spin_unlock_irqrestore(>lock, flags);
> +}
> +
>  static int kunit_vprintk_emit(const struct kunit *test,
> int level,
> const char *fmt,
> @@ -70,13 +91,29 @@ static void kunit_fail(struct kunit *test, struct 
> kunit_stream *stream)
>   stream->commit(stream);
>  }
>  
> +static void __noreturn kunit_abort(struct kunit *test)
> +{
> + kunit_set_death_test(test, true);
> +
> + test->try_catch.throw(>try_catch);
> +
> + /*
> +  * Throw could not abort from test.
> +  */
> + kunit_err(test, "Throw could not abort from test!");
> + show_stack(NULL, NULL);
> + BUG();

kunit_abort() is what will be call as the result of an assert failure.

BUG(), which is a panic, which is crashing the system is not acceptable
in the Linux kernel.  You will just annoy Linus if you submit this.

-Frank

< snip >
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v3 18/19] of: unittest: split out a couple of test cases from unittest

[Frank Rowand via dri-devel]

On 2/14/19 4:56 PM, Brendan Higgins wrote:
> On Thu, Feb 14, 2019 at 3:57 PM Frank Rowand  wrote:
>>
>> On 12/5/18 3:54 PM, Brendan Higgins wrote:
>>> On Tue, Dec 4, 2018 at 2:58 AM Frank Rowand  wrote:
>>>>
>>>> Hi Brendan,
>>>>
>>>> On 11/28/18 11:36 AM, Brendan Higgins wrote:
>>>>> Split out a couple of test cases that these features in base.c from the
>>>>> unittest.c monolith. The intention is that we will eventually split out
>>>>> all test cases and group them together based on what portion of device
>>>>> tree they test.
>>>>
>>>> Why does splitting this file apart improve the implementation?
>>>
>>> This is in preparation for patch 19/19 and other hypothetical future
>>> patches where test cases are split up and grouped together by what
>>> portion of DT they test (for example the parsing tests and the
>>> platform/device tests would probably go separate files as well). This
>>> patch by itself does not do anything useful, but I figured it made
>>> patch 19/19 (and, if you like what I am doing, subsequent patches)
>>> easier to review.
>>
>> I do not see any value in splitting the devicetree tests into
>> multiple files.
>>
>> Please help me understand what the benefits of such a split are.

Note that my following comments are specific to the current devicetree
unittests, and may not apply to the general case of unit tests in other
subsystems.


> Sorry, I thought it made sense in context of what I am doing in the
> following patch. All I am trying to do is to provide an effective way
> of grouping test cases. To be clear, the idea, assuming you agree, is

Looking at _just_ the first few fragments of the following patch, the
change is to break down a moderate size function of related tests,
of_unittest_find_node_by_name(), into a lot of extremely small functions.
Then to find the execution order of the many small functions requires
finding the array of_test_find_node_by_name_cases[].  Then I have to
chase off into the kunit test runner core, where I find that the set
of tests in of_test_find_node_by_name_cases[] is processed by a
late_initcall().  So now the order of the various test groupings,
declared via module_test(), are subject to the fragile orderings
of initcalls.

There are ordering dependencies within the devicetree unittests.

I do not like breaking the test cases down into such small atoms.

I do not see any value __for devicetree unittests__ of having
such small atoms.

It makes it harder for me to read the source of the tests and
understand the order they will execute.  It also makes it harder
for me to read through the actual tests (in this example the
tests that are currently grouped in of_unittest_find_node_by_name())
because of all the extra function headers injected into the
existing single function to break it apart into many smaller
functions.

Breaking the tests into separate chunks, each chunk invoked
independently as the result of module_test() of each chunk,
loses the summary result for the devicetree unittests of
how many tests are run and how many passed.  This is the
only statistic that I need to determine whether the
unittests have detected a new fault caused by a specific
patch or commit.  I don't need to look at any individual
test result unless the overall result reports a failure.


> that we would follow up with several other patches like this one and
> the subsequent patch, one which would pull out a couple test
> functions, as I have done here, and another that splits those
> functions up into a bunch of proper test cases.
> 
> I thought that having that many unrelated test cases in a single file
> would just be a pain to sort through deal with, review, whatever.

Having all the test cases in a single file makes it easier for me to
read, understand, modify, and maintain the tests.


> This is not something I feel particularly strongly about, it is just
> pretty atypical from my experience to have so many unrelated test
> cases in a single file.
> 
> Maybe you would prefer that I break up the test cases first, and then
> we split up the file as appropriate?

I prefer that the test cases not be broken up arbitrarily.  There _may_
be cases where the devicetree unittests are currently not well grouped
and may benefit from change, but if so that should be handled independently
of any transformation into a KUnit framework.


> I just assumed that we would agree it would be way too much stuff for
> a single file, so I went ahead and broke it up first, because I
> thought it would make it easier to review in that order rather than
> the other way around.



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v3 18/19] of: unittest: split out a couple of test cases from unittest

[Frank Rowand via dri-devel]

On 12/5/18 3:54 PM, Brendan Higgins wrote:
> On Tue, Dec 4, 2018 at 2:58 AM Frank Rowand  wrote:
>>
>> Hi Brendan,
>>
>> On 11/28/18 11:36 AM, Brendan Higgins wrote:
>>> Split out a couple of test cases that these features in base.c from the
>>> unittest.c monolith. The intention is that we will eventually split out
>>> all test cases and group them together based on what portion of device
>>> tree they test.
>>
>> Why does splitting this file apart improve the implementation?
> 
> This is in preparation for patch 19/19 and other hypothetical future
> patches where test cases are split up and grouped together by what
> portion of DT they test (for example the parsing tests and the
> platform/device tests would probably go separate files as well). This
> patch by itself does not do anything useful, but I figured it made
> patch 19/19 (and, if you like what I am doing, subsequent patches)
> easier to review.

I do not see any value in splitting the devicetree tests into
multiple files.

Please help me understand what the benefits of such a split are.

Thanks,

Frank
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v4 15/17] of: unittest: migrate tests to run on KUnit

[Frank Rowand via dri-devel]

On 2/14/19 1:37 PM, Brendan Higgins wrote:
> Migrate tests without any cleanup, or modifying test logic in anyway to
> run under KUnit using the KUnit expectation and assertion API.
> 
> Signed-off-by: Brendan Higgins 
> ---
>  drivers/of/Kconfig|1 +
>  drivers/of/unittest.c | 1310 +
>  2 files changed, 671 insertions(+), 640 deletions(-)
> 
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index ad3fcad4d75b8..f309399deac20 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -15,6 +15,7 @@ if OF
>  config OF_UNITTEST
>   bool "Device Tree runtime unit tests"
>   depends on !SPARC
> + depends on KUNIT
>   select IRQ_DOMAIN
>   select OF_EARLY_FLATTREE
>   select OF_RESOLVE
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c

These comments are from applying the patches to 5.0-rc3.

The final hunk of this patch fails to apply because it depends upon

   [PATCH v1 0/1] of: unittest: unflatten device tree on UML when testing.

If I apply that patch then I can apply patches 15 through 17.

If I apply patches 1 through 14 and boot the uml kernel then the devicetree
unittest result is:

  ### dt-test ### FAIL of_unittest_overlay_high_level():2372 overlay_base_root 
not initialized
  ### dt-test ### end of unittest - 219 passed, 1 failed

This is as expected from your previous reports, and is fixed after
applying

   [PATCH v1 0/1] of: unittest: unflatten device tree on UML when testing.

with the devicetree unittest result of:

   ### dt-test ### end of unittest - 224 passed, 0 failed

After adding patch 15, there are a lot of "unittest internal error" messages.

-Frank


> index effa4e2b9d992..96de69ccb3e63 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -26,186 +26,189 @@
>  
>  #include 
>  
> +#include ### dt-test ### end of unittest - 224 passed, 0 failed
> +
>  #include "of_private.h"
>  
> -static struct unittest_results {
> - int passed;
> - int failed;
> -} unittest_results;
> -
> -#define unittest(result, fmt, ...) ({ \
> - bool failed = !(result); \
> - if (failed) { \
> - unittest_results.failed++; \
> - pr_err("FAIL %s():%i " fmt, __func__, __LINE__, ##__VA_ARGS__); 
> \
> - } else { \
> - unittest_results.passed++; \
> - pr_debug("pass %s():%i\n", __func__, __LINE__); \
> - } \
> - failed; \
> -})
> -
> -static void __init of_unittest_find_node_by_name(void)
> +static void of_unittest_find_node_by_name(struct kunit *test)
>  {
>   struct device_node *np;
>   const char *options, *name;
>  
>   np = of_find_node_by_path("/testcase-data");
>   name = kasprintf(GFP_KERNEL, "%pOF", np);
> - unittest(np && !strcmp("/testcase-data", name),
> - "find /testcase-data failed\n");
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
> + KUNIT_EXPECT_STREQ_MSG(test, "/testcase-data", name,
> +"find /testcase-data failed\n");
>   of_node_put(np);
>   kfree(name);
>  
>   /* Test if trailing '/' works */
> - np = of_find_node_by_path("/testcase-data/");
> - unittest(!np, "trailing '/' on /testcase-data/ should fail\n");
> + KUNIT_EXPECT_EQ_MSG(test, of_find_node_by_path("/testcase-data/"), NULL,
> + "trailing '/' on /testcase-data/ should fail\n");
>  
>   np = of_find_node_by_path("/testcase-data/phandle-tests/consumer-a");
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
>   name = kasprintf(GFP_KERNEL, "%pOF", np);
> - unittest(np && !strcmp("/testcase-data/phandle-tests/consumer-a", name),
> + KUNIT_EXPECT_STREQ_MSG(
> + test, "/testcase-data/phandle-tests/consumer-a", name,
>   "find /testcase-data/phandle-tests/consumer-a failed\n");
>   of_node_put(np);
>   kfree(name);
>  
>   np = of_find_node_by_path("testcase-alias");
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
>   name = kasprintf(GFP_KERNEL, "%pOF", np);
> - unittest(np && !strcmp("/testcase-data", name),
> - "find testcase-alias failed\n");
> + KUNIT_EXPECT_STREQ_MSG(test, "/testcase-data", name,
> +"find testcase-alias failed\n");
>   of_node_put(np);
>   kfree(name);
>  
>   /* Test if trailing '/' works on aliases */
> - np = of_find_node_by_path("testcase-alias/");
> - unittest(!np, "trailing '/' on testcase-alias/ should fail\n");
> + KUNIT_EXPECT_EQ_MSG(test, of_find_node_by_path("testcase-alias/"), NULL,
> + "trailing '/' on testcase-alias/ should fail\n");
>  
>   np = of_find_node_by_path("testcase-alias/phandle-tests/consumer-a");
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
>   name = kasprintf(GFP_KERNEL, "%pOF", np);
> - unittest(np && !strcmp("/testcase-data/phandle-tests/consumer-a", name),
> + KUNIT_EXPECT_STREQ_MSG(
> + test, 

Re: [RFC v3 00/19] kunit: introduce KUnit, the Linux kernel unit testing framework

[Frank Rowand]

Hi Brendan, Rob,

On 11/28/18 11:36 AM, Brendan Higgins wrote:
> This patch set proposes KUnit, a lightweight unit testing and mocking
> framework for the Linux kernel.
> 
> Unlike Autotest and kselftest, KUnit is a true unit testing framework;
> it does not require installing the kernel on a test machine or in a VM
> and does not require tests to be written in userspace running on a host
> kernel. Additionally, KUnit is fast: From invocation to completion KUnit
> can run several dozen tests in under a second. Currently, the entire
> KUnit test suite for KUnit runs in under a second from the initial
> invocation (build time excluded).
> 
> KUnit is heavily inspired by JUnit, Python's unittest.mock, and
> Googletest/Googlemock for C++. KUnit provides facilities for defining
> unit test cases, grouping related test cases into test suites, providing
> common infrastructure for running tests, mocking, spying, and much more.
> 
> ## What's so special about unit testing?
> 
> A unit test is supposed to test a single unit of code in isolation,
> hence the name. There should be no dependencies outside the control of
> the test; this means no external dependencies, which makes tests orders
> of magnitudes faster. Likewise, since there are no external dependencies,
> there are no hoops to jump through to run the tests. Additionally, this
> makes unit tests deterministic: a failing unit test always indicates a
> problem. Finally, because unit tests necessarily have finer granularity,
> they are able to test all code paths easily solving the classic problem
> of difficulty in exercising error handling code.
> 
> ## Is KUnit trying to replace other testing frameworks for the kernel?
> 
> No. Most existing tests for the Linux kernel are end-to-end tests, which
> have their place. A well tested system has lots of unit tests, a
> reasonable number of integration tests, and some end-to-end tests. KUnit
> is just trying to address the unit test space which is currently not
> being addressed.
> 
> ## More information on KUnit
> 
> There is a bunch of documentation near the end of this patch set that
> describes how to use KUnit and best practices for writing unit tests.
> For convenience I am hosting the compiled docs here:
> https://google.github.io/kunit-docs/third_party/kernel/docs/
> Additionally for convenience, I have applied these patches to a branch:
> https://kunit.googlesource.com/linux/+/kunit/rfc/4.19/v3
> The repo may be cloned with:
> git clone https://kunit.googlesource.com/linux
> This patchset is on the kunit/rfc/4.19/v3 branch.
> 
> ## Changes Since Last Version
> 
>  - Changed namespace prefix from `test_*` to `kunit_*` as requested by
>Shuah.


>  - Started converting/cleaning up the device tree unittest to use KUnit.
>  - Started adding KUnit expectations with custom messages.
> 

Sorry I missed your reply to me in the v1 patch thread.  I've been
traveling a lot the last few weeks.  I'm starting to read messages
that occurred late in the v1 patch thread and the v2 patch thread,
so I'm just coming up to speed on this.

My comments below are motivated by adding the devicetree unittest to
this version of the patch series.

Pulling a comment from way back in the v1 patch thread:

On 10/17/18 3:22 PM, Brendan Higgins wrote:
> On Wed, Oct 17, 2018 at 10:49 AM  wrote:

< snip >

> The test and the code under test are linked together in the same
> binary and are compiled under Kbuild. Right now I am linking
> everything into a UML kernel, but I would ultimately like to make
> tests compile into completely independent test binaries. So each test
> file would get compiled into its own test binary and would link
> against only the code needed to run the test, but we are a bit of a
> ways off from that.

I have never used UML, so you should expect naive questions from me,
exhibiting my lack of understanding.

Does this mean that I have to build a UML architecture kernel to run
the KUnit tests?

*** Rob, if the answer is yes, then it seems like for my workflow,
which is to build for real ARM hardware, my work is doubled (or
worse), because for every patch/commit that I apply, I not only have
to build the ARM kernel and boot on the real hardware to test, I also
have to build the UML kernel and boot in UML.  If that is correct
then I see this as a major problem for me.

Brenden, in the above quote you said that in the future you would
like to make the "tests compile into completely independent test
binaries".  I am assuming those are intended to run as standalone
user space programs instead of inside UML.  Is that correct?  If
so, how will KUnit tests be able to test code that uses locking
mechanisms that require instructions that are not available to
user space execution?  (I _think_ that such instructions may be
present, depending on which locking mechanism, but I might be
mistaken.)

Another possible concern that I have for removing the devicetree
unit tests from my normal kernel build process is that I think

Re: [RFC v3 17/19] of: unittest: migrate tests to run on KUnit

[Frank Rowand]

On 11/28/18 11:36 AM, Brendan Higgins wrote:
> Migrate tests without any cleanup, or modifying test logic in anyway to
> run under KUnit using the KUnit expectation and assertion API.
> 
> Signed-off-by: Brendan Higgins 
> ---
>  drivers/of/Kconfig|1 +
>  drivers/of/unittest.c | 1405 ++---
>  2 files changed, 752 insertions(+), 654 deletions(-)

< snip >

I am travelling and will not have an opportunity to properly review this
patch, patch 18, or patch 19 until next week.

-Frank

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v3 00/19] kunit: introduce KUnit, the Linux kernel unit testing framework

[Frank Rowand]

On 11/28/18 11:36 AM, Brendan Higgins wrote:
> This patch set proposes KUnit, a lightweight unit testing and mocking
> framework for the Linux kernel.
> 
> Unlike Autotest and kselftest, KUnit is a true unit testing framework;
> it does not require installing the kernel on a test machine or in a VM
> and does not require tests to be written in userspace running on a host
> kernel. Additionally, KUnit is fast: From invocation to completion KUnit
> can run several dozen tests in under a second. Currently, the entire
> KUnit test suite for KUnit runs in under a second from the initial
> invocation (build time excluded).
> 
> KUnit is heavily inspired by JUnit, Python's unittest.mock, and
> Googletest/Googlemock for C++. KUnit provides facilities for defining
> unit test cases, grouping related test cases into test suites, providing
> common infrastructure for running tests, mocking, spying, and much more.
> 
> ## What's so special about unit testing?
> 


> A unit test is supposed to test a single unit of code in isolation,
> hence the name. There should be no dependencies outside the control of
> the test; this means no external dependencies, which makes tests orders
> of magnitudes faster. Likewise, since there are no external dependencies,
> there are no hoops to jump through to run the tests. Additionally, this

This question might be a misunderstanding of the intent of some of the
terminology in the above paragraph, so this is mostly a request for
clarification.

With my pre-conception of what unit tests are, I read "test a single unit
of code" to mean a relatively narrow piece of a subsystem.  So if I
understand correctly, taking examples from patch 17 "of: unittest:
migrate tests to run on KUnit", each function call like
KUNIT_ASSERT_NOT_ERR_OR_NULL(), KUNIT_EXPECT_STREQ_MSG(), and
KUNIT_EXPECT_EQ_MSG() are each a separate unit test, and thus the
paragraph says that each of these function calls should have no
dependencies outside the test.  Do I understand that correctly?

< snip >

-Frank

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v3 18/19] of: unittest: split out a couple of test cases from unittest

[Frank Rowand]

Hi Brendan,

On 11/28/18 11:36 AM, Brendan Higgins wrote:
> Split out a couple of test cases that these features in base.c from the
> unittest.c monolith. The intention is that we will eventually split out
> all test cases and group them together based on what portion of device
> tree they test.

Why does splitting this file apart improve the implementation?


> 
> Signed-off-by: Brendan Higgins 
> ---
>  drivers/of/Makefile  |   2 +-
>  drivers/of/base-test.c   | 214 ++
>  drivers/of/test-common.c | 149 ++
>  drivers/of/test-common.h |  16 ++
>  drivers/of/unittest.c| 316 +--
>  5 files changed, 381 insertions(+), 316 deletions(-)
>  create mode 100644 drivers/of/base-test.c
>  create mode 100644 drivers/of/test-common.c
>  create mode 100644 drivers/of/test-common.h
>
< snip >

-Frank
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v2 00/14] kunit: introduce KUnit, the Linux kernel unit testing framework

[Frank Rowand]

On 11/6/18 5:17 PM, Brendan Higgins wrote:
> On Fri, Nov 2, 2018 at 11:23 AM Shuah Khan  wrote:
>>
>> Hi Brendan,
> 
>> Framework looks good. I think it would be helpful to include a real test
> 
> Great to hear!
> 
>> in the patch series to get a feel for how effective it is.
> 
> Alright, will do. Rob suggested converting
> https://elixir.bootlin.com/linux/v4.19/source/drivers/of/unittest.c to
> KUnit, so that might offer a good comparison.

drivers/of/unittest.c might be a bit bigger and more complex test than
you want to start with.

-Frank

< snip >
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] [RFC] drm: rcar-du: keep temporary dtb files around during build

[Frank Rowand]

Hi Geert,

On 03/22/18 07:26, Geert Uytterhoeven wrote:
> Hi Frank,
> 
> On Fri, Mar 16, 2018 at 2:39 AM,   wrote:
>> On Thursday, March 15, 2018 8:37 AM, Arnd Bergmann [mailto:a...@arndb.de]  
>> wrote:
>>>
>>> The *.dtb and *.dtb.S files get removed by 'make' during the build
>>> process,
>>> and later seem to be missed during the 'modpost' stage:
>>>
>>> rm drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dtb
>>> drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dtb
>>> drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dtb.S
>>> drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dtb.S
>>> drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dtb.S
>>> drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dtb
>>> drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dtb
>>> drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dtb
>>> drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dtb.S
>>> drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dtb.S
>>> WARNING: could not open
>>> drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dtb.S: No such file or
>>> directory
>>>
>>> As a workaround, this adds all those files to the 'extra-y' target list,
>>> but that's really ugly. Any ideas for a better fix?
>>
>> Does this work for you (untested, but the way it is done in
>> drivers/of/unittest-data/Makefile):
>>
>> .PRECIOUS: \
>> $(obj)/%.dtb.S \
>> $(obj)/%.dtb
> 
> Shouldn't that just be moved to scripts/Makefile.lib, just above the rule
> to make dtb.S, like is done for other precious objects?
> 
> Gr{oetje,eeting}s,
> 
> Geert
> 

Thank you for adding Yamada-san (later in this thread).

I acked his patch series that does what you suggest.

-Frank
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Overlay sugar syntax (was: Re: [PATCH v6 3/4] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes)

[Frank Rowand]

On 03/06/18 18:30, David Gibson wrote:
> On Tue, Mar 06, 2018 at 01:40:20PM -0800, Frank Rowand wrote:
>> On 03/06/18 11:51, Frank Rowand wrote:
>>> On 03/06/18 04:30, Geert Uytterhoeven wrote:

< snip >

>>> And the patched dtc works for a dts file that I was trying to convert
>>> to sugar dts syntax
>>
>> < snip >
>>
>> I noticed that a space in "&{/}" is an error.  I wanted to check whether
>> that was deliberate, or that the patch wasn't fully complete yet.
> 
> That's essentially deliberate - it's not really related to this patch
> at all.  The patch just re-uses the existing syntax for a "path
> reference".  The whole thing is treated as a single token, hence, no
> spaces.
> 
> It might be possible to change that, but it could introduce some
> complications when the path reference syntax is used in other places.
> So I'm disinclined to change it, unless there's a very strong reason
> to.
> 

< snip >

No, please do not change.  I just wanted to make sure I understood the
intended syntax.

BTW, thanks for all the time you've been putting into this recent stuff.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Overlay sugar syntax (was: Re: [PATCH v6 3/4] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes)

[Frank Rowand]

On 03/06/18 11:51, Frank Rowand wrote:
> On 03/06/18 04:30, Geert Uytterhoeven wrote:
>> Hi David,
>>
>> On Tue, Mar 6, 2018 at 4:54 AM, David Gibson
>> <da...@gibson.dropbear.id.au> wrote:
>>> On Fri, Feb 23, 2018 at 09:05:24AM +0100, Geert Uytterhoeven wrote:
>>>> On Fri, Feb 23, 2018 at 3:38 AM, Frank Rowand <frowand.l...@gmail.com> 
>>>> wrote:
>>>>> I was hoping to be able to convert the .dts files to use sugar syntax
>>>>> instead of hand coding the fragment nodes, but for this specific set
>>>>> of files I failed, since the labels that would have been required do
>>>>> not already exist in the base .dts files that that overlays would be
>>>>> applied against.
>>>>
>>>> Indeed, hence the fixup overlays use "target-path".
>>>>
>>>> BTW, is there any specific reason there is no sugar syntax support in dtc
>>>> for absolute target paths? I guess to prevent adding stuff to a random
>>>> existing node, and to encourage people to use a "connector" API defined in
>>>> term of labels?
>>>
>>> Only because it hasn't been implemented.  Using &{/whatever} should
>>> IMO generate a target-path and the fact it doesn't is a bug.
>>>
>>>> I'm also in the process of converting my collection of DT overlays to sugar
>>>> syntax, and lack of support for "target-path" is the sole thing that holds
>>>> me back from completing this. So for now I use a mix of sugar and
>>>> traditional overlay syntax.
>>>>
>>>> In particular, I need "target-path" for two things:
>>>>   1. To refer to the root node, for adding devices that should live at
>>>>  (a board subnode of) the root node, like:
>>>>- devices connected to GPIO controllers provided by other base or
>>>>  overlay devices (e.g. LEDs, displays, buttons, ...),
>>>>- clock providers for other overlays devices (e.g. fixed-clock).
>>
>>>> The former is the real blocker for me.
>>
>>> Below is draft patch adding target-path support.  The pretty minimal
>>> test examples do include a case using &{/}
>>>
>>> From 8f1b35f88395adea01ce1100c5faa27dacbc8410 Mon Sep 17 00:00:00 2001
>>> From: David Gibson <da...@gibson.dropbear.id.au>
>>> Date: Tue, 6 Mar 2018 13:27:53 +1100
>>> Subject: [PATCH] Correct overlay syntactic sugar for generating target-path
>>>  fragments
>>>
>>> We've recently added "syntactic sugar" support to generate runtime dtb
>>> overlays using similar syntax to the compile time overlays we've had for
>>> a while.  This worked with the  { ... } syntax, adjusting an existing
>>> labelled node, but would fail with the &{/path} { ... } syntax attempting
>>> to adjust an existing node referenced by its path.
>>>
>>> The previous code would always try to use the "target" property in the
>>> output overlay, which needs to be fixed up, and __fixups__ can only encode
>>> symbols, not paths, so the result could never work properly.
>>>
>>> This adds support for the &{/path} syntax for overlays, translating it into
>>> the "target-path" encoding in the output.  It also changes existing
>>> behaviour a little because we now unconditionally one fragment for each
>>> overlay section in the source.  Previously we would only create a fragment
>>> if we couldn't locally resolve the node referenced.  We need this for
>>> path references, because the path is supposed to be referencing something
>>> in the (not yet known) base tree, rather than the overlay tree we are
>>> working with now.  In particular one useful case for path based overlays
>>> is using &{/} - but the constructed overlay tree will always have a root
>>> node, meaning that without the change that would attempt to resolve the
>>> fragment locally, which is not what we want.
>>>
>>> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
>>
>> Thank you, seems to work fine on dtc.git.
> 
> And the patched dtc works for a dts file that I was trying to convert
> to sugar dts syntax

< snip >

I noticed that a space in "&{/}" is an error.  I wanted to check whether
that was deliberate, or that the patch wasn't fully complete yet.
cat path_sugar_v1.dts 

$ nl -ba path_sugar_v1.dts
 1  
 2  /dts-v1/;
 3  /plugin/;
 4  &{/} {
 

Re: Overlay sugar syntax (was: Re: [PATCH v6 3/4] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes)

[Frank Rowand]

On 03/06/18 04:30, Geert Uytterhoeven wrote:
> Hi David,
> 
> On Tue, Mar 6, 2018 at 4:54 AM, David Gibson
> <da...@gibson.dropbear.id.au> wrote:
>> On Fri, Feb 23, 2018 at 09:05:24AM +0100, Geert Uytterhoeven wrote:
>>> On Fri, Feb 23, 2018 at 3:38 AM, Frank Rowand <frowand.l...@gmail.com> 
>>> wrote:
>>>> I was hoping to be able to convert the .dts files to use sugar syntax
>>>> instead of hand coding the fragment nodes, but for this specific set
>>>> of files I failed, since the labels that would have been required do
>>>> not already exist in the base .dts files that that overlays would be
>>>> applied against.
>>>
>>> Indeed, hence the fixup overlays use "target-path".
>>>
>>> BTW, is there any specific reason there is no sugar syntax support in dtc
>>> for absolute target paths? I guess to prevent adding stuff to a random
>>> existing node, and to encourage people to use a "connector" API defined in
>>> term of labels?
>>
>> Only because it hasn't been implemented.  Using &{/whatever} should
>> IMO generate a target-path and the fact it doesn't is a bug.
>>
>>> I'm also in the process of converting my collection of DT overlays to sugar
>>> syntax, and lack of support for "target-path" is the sole thing that holds
>>> me back from completing this. So for now I use a mix of sugar and
>>> traditional overlay syntax.
>>>
>>> In particular, I need "target-path" for two things:
>>>   1. To refer to the root node, for adding devices that should live at
>>>  (a board subnode of) the root node, like:
>>>- devices connected to GPIO controllers provided by other base or
>>>  overlay devices (e.g. LEDs, displays, buttons, ...),
>>>- clock providers for other overlays devices (e.g. fixed-clock).
> 
>>> The former is the real blocker for me.
> 
>> Below is draft patch adding target-path support.  The pretty minimal
>> test examples do include a case using &{/}
>>
>> From 8f1b35f88395adea01ce1100c5faa27dacbc8410 Mon Sep 17 00:00:00 2001
>> From: David Gibson <da...@gibson.dropbear.id.au>
>> Date: Tue, 6 Mar 2018 13:27:53 +1100
>> Subject: [PATCH] Correct overlay syntactic sugar for generating target-path
>>  fragments
>>
>> We've recently added "syntactic sugar" support to generate runtime dtb
>> overlays using similar syntax to the compile time overlays we've had for
>> a while.  This worked with the  { ... } syntax, adjusting an existing
>> labelled node, but would fail with the &{/path} { ... } syntax attempting
>> to adjust an existing node referenced by its path.
>>
>> The previous code would always try to use the "target" property in the
>> output overlay, which needs to be fixed up, and __fixups__ can only encode
>> symbols, not paths, so the result could never work properly.
>>
>> This adds support for the &{/path} syntax for overlays, translating it into
>> the "target-path" encoding in the output.  It also changes existing
>> behaviour a little because we now unconditionally one fragment for each
>> overlay section in the source.  Previously we would only create a fragment
>> if we couldn't locally resolve the node referenced.  We need this for
>> path references, because the path is supposed to be referencing something
>> in the (not yet known) base tree, rather than the overlay tree we are
>> working with now.  In particular one useful case for path based overlays
>> is using &{/} - but the constructed overlay tree will always have a root
>> node, meaning that without the change that would attempt to resolve the
>> fragment locally, which is not what we want.
>>
>> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
> 
> Thank you, seems to work fine on dtc.git.

And the patched dtc works for a dts file that I was trying to convert
to sugar dts syntax in the thread that Geert was responding to when he
created this thread.

(Laurent -- no need to worry about this for your current series.
Converting your dts files will be an easy task to do in a future
kernel version -- remind me if I forget to send a patch.)

-Frank

> 
> Note that while the dtc part applies on the in-kernel copy of dtc, it
> doesn't work there: "&{/}" behaves the same as "/" (i.e. no overlay
> fragment is generated), but "&{/foo}" does create the overlay fragment.
> Merging in Rob's for-next branch to upgrade Linux' copy of dtc fixes that.
> 
> Thanks a lot!
> Converting my remaining overlay fragments to sugar syntax...
> 
> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@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
> .
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 3/4] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes

[Frank Rowand]

On 02/22/18 14:10, Frank Rowand wrote:
> Hi Laurent, Rob,
> 
> Thanks for the prompt spin to address my concerns.  There are some small
> technical issues.
> 
> I did not read the v3 patch until today.  v3 through v6 are still using the
> old overlay apply method which uses an expanded device tree as input.
> 
> Rob, I don't see my overlay patches in you for-next branch, and I have
> not seen an "Applied" message from you.  What is the status of the
> overlay patches?
> 
> Comments in the patch below.
> 
> 
> On 02/22/18 05:13, Laurent Pinchart wrote:
>> The internal LVDS encoders now have their own DT bindings. Before
>> switching the driver infrastructure to those new bindings, implement
>> backward-compatibility through live DT patching.
>>
>> Patching is disabled and will be enabled along with support for the new
>> DT bindings in the DU driver.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>
>> ---
>> Changes since v5:
>>
>> - Use a private copy of rcar_du_of_changeset_add_property()
>>
>> Changes since v3:
>>
>> - Use the OF changeset API
>> - Use of_graph_get_endpoint_by_regs()
>> - Replace hardcoded constants by sizeof()
>>
>> Changes since v2:
>>
>> - Update the SPDX headers to use C-style comments in header files
>> - Removed the manually created __local_fixups__ node
>> - Perform manual fixups on live DT instead of overlay
>>
>> Changes since v1:
>>
>> - Select OF_FLATTREE
>> - Compile LVDS DT bindings patch code when DRM_RCAR_LVDS is selected
>> - Update the SPDX headers to use GPL-2.0 instead of GPL-2.0-only
>> - Turn __dtb_rcar_du_of_lvds_(begin|end) from u8 to char
>> - Pass void begin and end pointers to rcar_du_of_get_overlay()
>> - Use of_get_parent() instead of accessing the parent pointer directly
>> - Find the LVDS endpoints nodes based on the LVDS node instead of the
>>   root of the overlay
>> - Update to the -lvds compatible string format
>> ---
>>  drivers/gpu/drm/rcar-du/Kconfig|   2 +
>>  drivers/gpu/drm/rcar-du/Makefile   |   7 +-
>>  drivers/gpu/drm/rcar-du/rcar_du_of.c   | 342 
>> +
>>  drivers/gpu/drm/rcar-du/rcar_du_of.h   |  20 ++
>>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts|  79 +
>>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts|  53 
>>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts|  53 
>>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts|  53 
>>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts|  53 
>>  9 files changed, 661 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.c
>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.h
>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts
>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts
>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts
>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts
>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts
>>
>> diff --git a/drivers/gpu/drm/rcar-du/Kconfig 
>> b/drivers/gpu/drm/rcar-du/Kconfig
>> index 5d0b4b7119af..3f83352a7313 100644
>> --- a/drivers/gpu/drm/rcar-du/Kconfig
>> +++ b/drivers/gpu/drm/rcar-du/Kconfig
>> @@ -22,6 +22,8 @@ config DRM_RCAR_LVDS
>>  bool "R-Car DU LVDS Encoder Support"
>>  depends on DRM_RCAR_DU
>>  select DRM_PANEL
>> +select OF_FLATTREE
>> +select OF_OVERLAY
>>  help
>>Enable support for the R-Car Display Unit embedded LVDS encoders.
>>  
>> diff --git a/drivers/gpu/drm/rcar-du/Makefile 
>> b/drivers/gpu/drm/rcar-du/Makefile
>> index 0cf5c11030e8..86b337b4be5d 100644
>> --- a/drivers/gpu/drm/rcar-du/Makefile
>> +++ b/drivers/gpu/drm/rcar-du/Makefile
>> @@ -8,7 +8,12 @@ rcar-du-drm-y := rcar_du_crtc.o \
>>   rcar_du_plane.o
>>  
>>  rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS) += rcar_du_lvdsenc.o
>> -
>> +rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS) += rcar_du_of.o \
>> +   rcar_du_of_lvds_r8a7790.dtb.o \
>> +   rcar_du_of_lvds_r8a7791.dtb.o \
>> +   rcar_du_of_lvds_r8a7793.dtb.o \
>> +   rcar_du_of_lvds_r8a7795.dtb.o \
>> +   rcar_du_of_lvds_r8a7796.dtb.o
>&

Re: [PATCH v6 3/4] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes

[Frank Rowand]

On 02/23/18 11:56, Laurent Pinchart wrote:
> Hi Frank,
> 
> On Friday, 23 February 2018 21:43:17 EET Frank Rowand wrote:
>> On 02/23/18 01:00, Laurent Pinchart wrote:
>>> On Friday, 23 February 2018 04:38:06 EET Frank Rowand wrote:
>>>> On 02/22/18 14:10, Frank Rowand wrote:
>>>>> Hi Laurent, Rob,
>>>>>
>>>>> Thanks for the prompt spin to address my concerns.  There are some small
>>>>> technical issues.
>>>>>
>>>>> I did not read the v3 patch until today.  v3 through v6 are still using
>>>>> the old overlay apply method which uses an expanded device tree as
>>>>> input.
>>>>>
>>>>> Rob, I don't see my overlay patches in you for-next branch, and I have
>>>>> not seen an "Applied" message from you.  What is the status of the
>>>>> overlay patches?
>>>>>
>>>>> Comments in the patch below.
>>>>>
>>>>> On 02/22/18 05:13, Laurent Pinchart wrote:
>>>>>> The internal LVDS encoders now have their own DT bindings. Before
>>>>>> switching the driver infrastructure to those new bindings, implement
>>>>>> backward-compatibility through live DT patching.
>>>>>>
>>>>>> Patching is disabled and will be enabled along with support for the new
>>>>>> DT bindings in the DU driver.
>>>>>>
>>>>>> Signed-off-by: Laurent Pinchart
>>>>>> <laurent.pinchart+rene...@ideasonboard.com>
>>>>>> ---
>>>>>> Changes since v5:
>>>>>>
>>>>>> - Use a private copy of rcar_du_of_changeset_add_property()
>>>>>>
>>>>>> Changes since v3:
>>>>>>
>>>>>> - Use the OF changeset API
>>>>>> - Use of_graph_get_endpoint_by_regs()
>>>>>> - Replace hardcoded constants by sizeof()
>>>>>>
>>>>>> Changes since v2:
>>>>>>
>>>>>> - Update the SPDX headers to use C-style comments in header files
>>>>>> - Removed the manually created __local_fixups__ node
>>>>>> - Perform manual fixups on live DT instead of overlay
>>>>>>
>>>>>> Changes since v1:
>>>>>>
>>>>>> - Select OF_FLATTREE
>>>>>> - Compile LVDS DT bindings patch code when DRM_RCAR_LVDS is selected
>>>>>> - Update the SPDX headers to use GPL-2.0 instead of GPL-2.0-only
>>>>>> - Turn __dtb_rcar_du_of_lvds_(begin|end) from u8 to char
>>>>>> - Pass void begin and end pointers to rcar_du_of_get_overlay()
>>>>>> - Use of_get_parent() instead of accessing the parent pointer directly
>>>>>> - Find the LVDS endpoints nodes based on the LVDS node instead of the
>>>>>>   root of the overlay
>>>>>> - Update to the -lvds compatible string format
>>>>>> ---
>>>>>>
>>>>>>  drivers/gpu/drm/rcar-du/Kconfig|   2 +
>>>>>>  drivers/gpu/drm/rcar-du/Makefile   |   7 +-
>>>>>>  drivers/gpu/drm/rcar-du/rcar_du_of.c   | 342 +
>>>>>>  drivers/gpu/drm/rcar-du/rcar_du_of.h   |  20 ++
>>>>>>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts|  79 +
>>>>>>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts|  53 
>>>>>>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts|  53 
>>>>>>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts|  53 
>>>>>>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts|  53 
>>>>>>  9 files changed, 661 insertions(+), 1 deletion(-)
>>>>>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.c
>>>>>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.h
>>>>>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts
>>>>>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts
>>>>>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts
>>>>>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts
>>>>>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/rcar-du/Kconfig
>>>>>> b/drivers/gpu/drm/rcar-du/Kconfig index 5d0b4b7119af..3f83352a7313
>>>>>> 100644
>>>>>> --- a/drivers/gpu/drm/rcar-du/Kconfig
>>>>>> +++ b/drivers/gpu/drm/rcar-du/Kconfig
>>>>
>>>> < snip >
>>>>
>>>>> becomes:
>>>>>   ret = of_overlay_fdt_apply(dtb->begin, _id);
>>>>>
>>>>> If my overlay patches do not exist, there are other small errors
>>>>> in the code block above.  I'll ignore them for the moment.
> 
> If you have time to not ignore them I'd appreciate your comments :-) I'd like 
> to get this patch series merged in v4.17, and for that I want to be prepared 
> to submit it on top of your overlay patches if they make it to mainline, or 
> without them if they don't.

< snip >

OK, I will.  I did the research to verify my memory, so I just need to type
it in.  I'll reply to an email several layers back in this thread that
I did not snip the code out of, so I can comment in line in the code.

-Frank
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 00/16] R-Car DU: Convert LVDS code to bridge driver

[Frank Rowand]

On 02/23/18 01:25, Laurent Pinchart wrote:
> Hi Frank,
> 
> On Friday, 23 February 2018 05:20:43 EET Frank Rowand wrote:
>> On 02/22/18 02:25, Laurent Pinchart wrote:
>>> On Thursday, 22 February 2018 08:07:14 EET Frank Rowand wrote:
>>>> On 02/20/18 15:10, Laurent Pinchart wrote:
>>>>> Hello,
>>>>>
>>>>> This patch series addresses a design mistake that dates back from the
>>>>> initial DU support. Support for the LVDS encoders, which are IP cores
>>>>> separate from the DU, was bundled in the DU driver. Worse, both the DU
>>>>> and LVDS were described through a single DT node.
>>>>>
>>>>> To fix the, patches 01/16 and 02/16 define new DT bindings for the LVDS
>>>>> encoders, and deprecate their description inside the DU bindings. To
>>>>> retain backward compatibility with existing DT, patches 03/16 to 08/16
>>>>> then patch the device tree at runtime to convert the legacy bindings to
>>>>> the new ones.
>>>>>
>>>>> With the DT side addressed, patch 09/16 converts the LVDS support code
>>>>> to a separate bridge driver. Patches 11/16 to 16/16 then update all the
>>>>> device tree sources to the new DU and LVDS encoders bindings.
>>>>>
>>>>> I decided to go for live DT patching in patch 08/16 because implementing
>>>>> support for both the legacy and new bindings in the driver would have
>>>>> been very intrusive, and prevented further cleanups. This version relies
>>>>> more heavily on overlays to avoid touching the internals of the OF core
>>>>> compared to v2, even if manual fixes to the device tree are still
>>>>> needed.
>>>>>
>>>>> Compared to v3, this series uses the OF changeset API to update
>>>>> properties instead of accessing the internals of the property structure.
>>>>> This removes the local implementation of functions to look up nodes by
>>>>> path and update properties. In order to do this, I pulled in Pantelis'
>>>>> patch series titled "[PATCH v2 0/5] of: dynamic: Changesets helpers &
>>>>> fixes" at Rob's request, and rebased it while taking two small review
>>>>> comments into account.
>>>>
>>>> Wait a minute!  Why are you putting a patch set to modify core devicetree
>>>> in the middle of a driver series.  Please pull it out to a separate
>>>> series.
>>>
>>> Because Rob asked for the driver-local implementation of the property add
>>> function to be replaced by Pantelis' series. I want to get the LVDS
>>> changes in v4.17 and asked Rob whether I could then take the OF changeset
>>> patches merged through the DRM tree, and he didn't object. If that causes
>>> an issue I'll switch back to the driver-local implementation to get the
>>> driver changes merged, split the OF changeset series out, and then move
>>> to the OF changeset API once merged. Would you prefer that ?
>>
>> You have already created a new version of the R-Car patches without the
>> set of patches that I was objecting to here.  So this is somewhat just
>> an academic comment.
>>
>> As I mentioned in the v6 thread, I am coming back here to clean up loose
>> ends, and explain why I had the reaction I had.  Basically, this is a
>> process issue to me.
>>
>> (1) The patch set from Pantelis is "hidden" in the driver patch series.
>> When viewing collapsed threads (which is my normal mode to avoid getting
>> overwhelmed by the vast volume of email I scan), the Pantelis patch set
>> is totally invisible.  If the R-Car driver patch series had not had me
>> on the to: list, there is a very good chance I would not have noticed
>> it.  Or noticed in a more delayed time frame.  And the same applies to
>> anyone else who might be subscribed to the devicetree mail list.  If
>> the Pantelis patch series was split out into a separate patch set then
>> it would be more visible on the list.
>>
>> (2) There is no good way to indicate in the email subject lines for
>> the Pantelis patches that they are version 3 of the series, since
>> they are also version 4 of the R-Car patch series.  If one reads the
>> patch 0/0 header carefully, and/or the other Pantelis patch comment
>> headers carefully, and then does a little detective work, it is
>> possible to find version 2 of the Pantelis patch series, and thus
>> be able to track the full history.  If

Re: [PATCH v6 3/4] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes

[Frank Rowand]

On 02/23/18 01:00, Laurent Pinchart wrote:
> Hi Frank,
> 
> On Friday, 23 February 2018 04:38:06 EET Frank Rowand wrote:
>> On 02/22/18 14:10, Frank Rowand wrote:
>>> Hi Laurent, Rob,
>>>
>>> Thanks for the prompt spin to address my concerns.  There are some small
>>> technical issues.
>>>
>>> I did not read the v3 patch until today.  v3 through v6 are still using
>>> the old overlay apply method which uses an expanded device tree as input.
>>>
>>> Rob, I don't see my overlay patches in you for-next branch, and I have
>>> not seen an "Applied" message from you.  What is the status of the
>>> overlay patches?
>>>
>>> Comments in the patch below.
>>>
>>> On 02/22/18 05:13, Laurent Pinchart wrote:
>>>> The internal LVDS encoders now have their own DT bindings. Before
>>>> switching the driver infrastructure to those new bindings, implement
>>>> backward-compatibility through live DT patching.
>>>>
>>>> Patching is disabled and will be enabled along with support for the new
>>>> DT bindings in the DU driver.
>>>>
>>>> Signed-off-by: Laurent Pinchart
>>>> <laurent.pinchart+rene...@ideasonboard.com>
>>>> ---
>>>> Changes since v5:
>>>>
>>>> - Use a private copy of rcar_du_of_changeset_add_property()
>>>>
>>>> Changes since v3:
>>>>
>>>> - Use the OF changeset API
>>>> - Use of_graph_get_endpoint_by_regs()
>>>> - Replace hardcoded constants by sizeof()
>>>>
>>>> Changes since v2:
>>>>
>>>> - Update the SPDX headers to use C-style comments in header files
>>>> - Removed the manually created __local_fixups__ node
>>>> - Perform manual fixups on live DT instead of overlay
>>>>
>>>> Changes since v1:
>>>>
>>>> - Select OF_FLATTREE
>>>> - Compile LVDS DT bindings patch code when DRM_RCAR_LVDS is selected
>>>> - Update the SPDX headers to use GPL-2.0 instead of GPL-2.0-only
>>>> - Turn __dtb_rcar_du_of_lvds_(begin|end) from u8 to char
>>>> - Pass void begin and end pointers to rcar_du_of_get_overlay()
>>>> - Use of_get_parent() instead of accessing the parent pointer directly
>>>> - Find the LVDS endpoints nodes based on the LVDS node instead of the
>>>>   root of the overlay
>>>> - Update to the -lvds compatible string format
>>>> ---
>>>>
>>>>  drivers/gpu/drm/rcar-du/Kconfig|   2 +
>>>>  drivers/gpu/drm/rcar-du/Makefile   |   7 +-
>>>>  drivers/gpu/drm/rcar-du/rcar_du_of.c   | 342 +++
>>>>  drivers/gpu/drm/rcar-du/rcar_du_of.h   |  20 ++
>>>>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts|  79 +
>>>>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts|  53 
>>>>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts|  53 
>>>>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts|  53 
>>>>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts|  53 
>>>>  9 files changed, 661 insertions(+), 1 deletion(-)
>>>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.c
>>>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.h
>>>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts
>>>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts
>>>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts
>>>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts
>>>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts
>>>>
>>>> diff --git a/drivers/gpu/drm/rcar-du/Kconfig
>>>> b/drivers/gpu/drm/rcar-du/Kconfig index 5d0b4b7119af..3f83352a7313
>>>> 100644
>>>> --- a/drivers/gpu/drm/rcar-du/Kconfig
>>>> +++ b/drivers/gpu/drm/rcar-du/Kconfig
>>
>> < snip >
>>
>>> becomes:
>>> ret = of_overlay_fdt_apply(dtb->begin, _id);
>>>
>>> If my overlay patches do not exist, there are other small errors
>>> in the code block above.  I'll ignore them for the moment.
>>>
>>> A quick scan of the rest of the code looks OK.  I'll read through it
>>> more carefully, but wanted to get this reply out without further
>>> delay.
>>
>> < snip >
>>
>> I was hoping to be able to convert the .dts files to use sugar syntax
>> instead of hand coding the fragment nodes, but for this specific set
>> of files I failed, since the labels that would have been required do
>> not already exist in the base .dts files that that overlays would be
>> applied against.
> 
> And even if they existed in the base .dts in the kernel sources, there's no 
> guarantee that the .dtb on the systems we want to patch would contain symbol, 
> so that wouldn't have been an option anyway, would it ?

Correct.  And even more troublesome is that some of the fragments are
targeted at node "/", which dtc does not even allow a label on (at the
moment).


> 
>> Oh well.  It was an interesting exercise anyway, trying to be crafty.
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 0/4] R-Car DU: Convert LVDS code to bridge driver

[Frank Rowand]

On 02/22/18 05:13, Laurent Pinchart wrote:
> Hello,
> 
> This patch series addresses a design mistake that dates back from the initial
> DU support. Support for the LVDS encoders, which are IP cores separate from
> the DU, was bundled in the DU driver. Worse, both the DU and LVDS were
> described through a single DT node.
> 
> To fix the, patches 1/4 and 2/4 define new DT bindings for the LVDS
> encoders, and deprecate their description inside the DU bindings. To retain
> backward compatibility with existing DT, patch 3/4 then patch the device tree
> at runtime to convert the legacy bindings to the new ones.
> 
> With the DT side addressed, patch 4/4 converts the LVDS support code to a
> separate bridge driver.
> 
> I decided to go for live DT patching in patch 3/4 because implementing
> support for both the legacy and new bindings in the driver would have been
> very intrusive, and prevented further cleanups. This version relies more
> heavily on overlays to avoid touching the internals of the OF core compared to
> v2, even if manual fixes to the device tree are still needed.
> 
> As all the patches but the last one have been acked, I plan to send a pull
> request by the end of the week if no new issue is discovered.
> 
> Compared to v5, I've dropped the OF changeset halpers series as Frank raised
> concerns about hidding it in the middle of a driver patch series. I've thus
> copied the implementation of of_changeset_add_property_copy() in the driver to
> avoid blocking this series. The function arguments are identical, so when the
> OF changeset helpers will land it will be very easy to drop the private copy
> and use the of_changeset_add_property_copy() helper.

Thank you Laurent.

My issues with that are procedural, and I'll reply later about this in the
v4 patch thread, where I raised the issue.  (For the peanut gallery, I
replied in thread v4 _after_ Laurent sent v5, so Laurent did not ignore
me in v5.)

My technical comments are more relevent than my process comments, in terms
of helping Laurent get his driver submitted, so I will delay the process
comments.

My technical comments will be in reply to patch 3/4.

-Frank
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 3/4] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes

[Frank Rowand]

Hi Laurent, Rob,

Thanks for the prompt spin to address my concerns.  There are some small
technical issues.

I did not read the v3 patch until today.  v3 through v6 are still using the
old overlay apply method which uses an expanded device tree as input.

Rob, I don't see my overlay patches in you for-next branch, and I have
not seen an "Applied" message from you.  What is the status of the
overlay patches?

Comments in the patch below.


On 02/22/18 05:13, Laurent Pinchart wrote:
> The internal LVDS encoders now have their own DT bindings. Before
> switching the driver infrastructure to those new bindings, implement
> backward-compatibility through live DT patching.
> 
> Patching is disabled and will be enabled along with support for the new
> DT bindings in the DU driver.
> 
> Signed-off-by: Laurent Pinchart 
> ---
> Changes since v5:
> 
> - Use a private copy of rcar_du_of_changeset_add_property()
> 
> Changes since v3:
> 
> - Use the OF changeset API
> - Use of_graph_get_endpoint_by_regs()
> - Replace hardcoded constants by sizeof()
> 
> Changes since v2:
> 
> - Update the SPDX headers to use C-style comments in header files
> - Removed the manually created __local_fixups__ node
> - Perform manual fixups on live DT instead of overlay
> 
> Changes since v1:
> 
> - Select OF_FLATTREE
> - Compile LVDS DT bindings patch code when DRM_RCAR_LVDS is selected
> - Update the SPDX headers to use GPL-2.0 instead of GPL-2.0-only
> - Turn __dtb_rcar_du_of_lvds_(begin|end) from u8 to char
> - Pass void begin and end pointers to rcar_du_of_get_overlay()
> - Use of_get_parent() instead of accessing the parent pointer directly
> - Find the LVDS endpoints nodes based on the LVDS node instead of the
>   root of the overlay
> - Update to the -lvds compatible string format
> ---
>  drivers/gpu/drm/rcar-du/Kconfig|   2 +
>  drivers/gpu/drm/rcar-du/Makefile   |   7 +-
>  drivers/gpu/drm/rcar-du/rcar_du_of.c   | 342 
> +
>  drivers/gpu/drm/rcar-du/rcar_du_of.h   |  20 ++
>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts|  79 +
>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts|  53 
>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts|  53 
>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts|  53 
>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts|  53 
>  9 files changed, 661 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.c
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.h
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts
> 
> diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
> index 5d0b4b7119af..3f83352a7313 100644
> --- a/drivers/gpu/drm/rcar-du/Kconfig
> +++ b/drivers/gpu/drm/rcar-du/Kconfig
> @@ -22,6 +22,8 @@ config DRM_RCAR_LVDS
>   bool "R-Car DU LVDS Encoder Support"
>   depends on DRM_RCAR_DU
>   select DRM_PANEL
> + select OF_FLATTREE
> + select OF_OVERLAY
>   help
> Enable support for the R-Car Display Unit embedded LVDS encoders.
>  
> diff --git a/drivers/gpu/drm/rcar-du/Makefile 
> b/drivers/gpu/drm/rcar-du/Makefile
> index 0cf5c11030e8..86b337b4be5d 100644
> --- a/drivers/gpu/drm/rcar-du/Makefile
> +++ b/drivers/gpu/drm/rcar-du/Makefile
> @@ -8,7 +8,12 @@ rcar-du-drm-y := rcar_du_crtc.o \
>rcar_du_plane.o
>  
>  rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS)  += rcar_du_lvdsenc.o
> -
> +rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS)  += rcar_du_of.o \
> +rcar_du_of_lvds_r8a7790.dtb.o \
> +rcar_du_of_lvds_r8a7791.dtb.o \
> +rcar_du_of_lvds_r8a7793.dtb.o \
> +rcar_du_of_lvds_r8a7795.dtb.o \
> +rcar_du_of_lvds_r8a7796.dtb.o
>  rcar-du-drm-$(CONFIG_DRM_RCAR_VSP)   += rcar_du_vsp.o
>  
>  obj-$(CONFIG_DRM_RCAR_DU)+= rcar-du-drm.o
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_of.c 
> b/drivers/gpu/drm/rcar-du/rcar_du_of.c
> new file mode 100644
> index ..ac442ddfed16
> --- /dev/null
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_of.c
> @@ -0,0 +1,342 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * rcar_du_of.c - Legacy DT bindings compatibility
> + *
> + * Copyright (C) 2018 Laurent Pinchart 
> + *
> + * Based on work from Jyri Sarha 
> + * Copyright (C) 2015 Texas Instruments
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> 

Re: [PATCH v4 00/16] R-Car DU: Convert LVDS code to bridge driver

[Frank Rowand]

Hi Laurent,

On 02/22/18 02:25, Laurent Pinchart wrote:
> Hi Frank,
> 
> On Thursday, 22 February 2018 08:07:14 EET Frank Rowand wrote:
>> On 02/20/18 15:10, Laurent Pinchart wrote:
>>> Hello,
>>>
>>> This patch series addresses a design mistake that dates back from the
>>> initial DU support. Support for the LVDS encoders, which are IP cores
>>> separate from the DU, was bundled in the DU driver. Worse, both the DU
>>> and LVDS were described through a single DT node.
>>>
>>> To fix the, patches 01/16 and 02/16 define new DT bindings for the LVDS
>>> encoders, and deprecate their description inside the DU bindings. To
>>> retain backward compatibility with existing DT, patches 03/16 to 08/16
>>> then patch the device tree at runtime to convert the legacy bindings to
>>> the new ones.
>>>
>>> With the DT side addressed, patch 09/16 converts the LVDS support code to
>>> a separate bridge driver. Patches 11/16 to 16/16 then update all the
>>> device tree sources to the new DU and LVDS encoders bindings.
>>>
>>> I decided to go for live DT patching in patch 08/16 because implementing
>>> support for both the legacy and new bindings in the driver would have been
>>> very intrusive, and prevented further cleanups. This version relies more
>>> heavily on overlays to avoid touching the internals of the OF core
>>> compared to v2, even if manual fixes to the device tree are still needed.
>>>
>>> Compared to v3, this series uses the OF changeset API to update properties
>>> instead of accessing the internals of the property structure. This removes
>>> the local implementation of functions to look up nodes by path and update
>>> properties. In order to do this, I pulled in Pantelis' patch series
>>> titled "[PATCH v2 0/5] of: dynamic: Changesets helpers & fixes" at Rob's
>>> request, and rebased it while taking two small review comments into
>>> account.
>>
>> Wait a minute!  Why are you putting a patch set to modify core devicetree
>> in the middle of a driver series.  Please pull it out to a separate series.
> 
> Because Rob asked for the driver-local implementation of the property add 
> function to be replaced by Pantelis' series. I want to get the LVDS changes 
> in 
> v4.17 and asked Rob whether I could then take the OF changeset patches merged 
> through the DRM tree, and he didn't object. If that causes an issue I'll 
> switch back to the driver-local implementation to get the driver changes 
> merged, split the OF changeset series out, and then move to the OF changeset 
> API once merged. Would you prefer that ?

You have already created a new version of the R-Car patches without the
set of patches that I was objecting to here.  So this is somewhat just
an academic comment.

As I mentioned in the v6 thread, I am coming back here to clean up loose
ends, and explain why I had the reaction I had.  Basically, this is a
process issue to me.

(1) The patch set from Pantelis is "hidden" in the driver patch series.
When viewing collapsed threads (which is my normal mode to avoid getting
overwhelmed by the vast volume of email I scan), the Pantelis patch set
is totally invisible.  If the R-Car driver patch series had not had me
on the to: list, there is a very good chance I would not have noticed
it.  Or noticed in a more delayed time frame.  And the same applies to
anyone else who might be subscribed to the devicetree mail list.  If
the Pantelis patch series was split out into a separate patch set then
it would be more visible on the list.

(2) There is no good way to indicate in the email subject lines for
the Pantelis patches that they are version 3 of the series, since
they are also version 4 of the R-Car patch series.  If one reads the
patch 0/0 header carefully, and/or the other Pantelis patch comment
headers carefully, and then does a little detective work, it is
possible to find version 2 of the Pantelis patch series, and thus
be able to track the full history.  If just glancing at each
individual patch email subject, scanning the patch comment, and
spending more time reading the body of the patch, it would be
very easy to overlook the existence of the previous versions on
the mail list.

(2b) Small quibble:  if the Pantelis patches were in a separate series,
with v3 in the subject header, then you would have normally put
a "changes since v2" section in the patch comment header, which would
have been more visible and less cryptic than what you wrote, which was:

   ...

   Signed-off-by: Pantelis Antoniou <pantelis.anton...@konsulko.com>
   [Fixed memory leak in __of_changeset_add_update_property_copy()]
   Signed-off-by: Laurent Pinchart <

Re: [PATCH v6 3/4] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes

[Frank Rowand]

On 02/22/18 14:10, Frank Rowand wrote:
> Hi Laurent, Rob,
> 
> Thanks for the prompt spin to address my concerns.  There are some small
> technical issues.
> 
> I did not read the v3 patch until today.  v3 through v6 are still using the
> old overlay apply method which uses an expanded device tree as input.
> 
> Rob, I don't see my overlay patches in you for-next branch, and I have
> not seen an "Applied" message from you.  What is the status of the
> overlay patches?
> 
> Comments in the patch below.
> 
> 
> On 02/22/18 05:13, Laurent Pinchart wrote:
>> The internal LVDS encoders now have their own DT bindings. Before
>> switching the driver infrastructure to those new bindings, implement
>> backward-compatibility through live DT patching.
>>
>> Patching is disabled and will be enabled along with support for the new
>> DT bindings in the DU driver.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>
>> ---
>> Changes since v5:
>>
>> - Use a private copy of rcar_du_of_changeset_add_property()
>>
>> Changes since v3:
>>
>> - Use the OF changeset API
>> - Use of_graph_get_endpoint_by_regs()
>> - Replace hardcoded constants by sizeof()
>>
>> Changes since v2:
>>
>> - Update the SPDX headers to use C-style comments in header files
>> - Removed the manually created __local_fixups__ node
>> - Perform manual fixups on live DT instead of overlay
>>
>> Changes since v1:
>>
>> - Select OF_FLATTREE
>> - Compile LVDS DT bindings patch code when DRM_RCAR_LVDS is selected
>> - Update the SPDX headers to use GPL-2.0 instead of GPL-2.0-only
>> - Turn __dtb_rcar_du_of_lvds_(begin|end) from u8 to char
>> - Pass void begin and end pointers to rcar_du_of_get_overlay()
>> - Use of_get_parent() instead of accessing the parent pointer directly
>> - Find the LVDS endpoints nodes based on the LVDS node instead of the
>>   root of the overlay
>> - Update to the -lvds compatible string format
>> ---
>>  drivers/gpu/drm/rcar-du/Kconfig|   2 +
>>  drivers/gpu/drm/rcar-du/Makefile   |   7 +-
>>  drivers/gpu/drm/rcar-du/rcar_du_of.c   | 342 
>> +
>>  drivers/gpu/drm/rcar-du/rcar_du_of.h   |  20 ++
>>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts|  79 +
>>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts|  53 
>>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts|  53 
>>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts|  53 
>>  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts|  53 
>>  9 files changed, 661 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.c
>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.h
>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts
>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts
>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts
>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts
>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts
>>
>> diff --git a/drivers/gpu/drm/rcar-du/Kconfig 
>> b/drivers/gpu/drm/rcar-du/Kconfig
>> index 5d0b4b7119af..3f83352a7313 100644
>> --- a/drivers/gpu/drm/rcar-du/Kconfig
>> +++ b/drivers/gpu/drm/rcar-du/Kconfig

< snip >

> 
> becomes:
> 
>   ret = of_overlay_fdt_apply(dtb->begin, _id);
> 
> If my overlay patches do not exist, there are other small errors
> in the code block above.  I'll ignore them for the moment.
> 
> A quick scan of the rest of the code looks OK.  I'll read through it
> more carefully, but wanted to get this reply out without further
> delay.

< snip >

I was hoping to be able to convert the .dts files to use sugar syntax
instead of hand coding the fragment nodes, but for this specific set
of files I failed, since the labels that would have been required do
not already exist in the base .dts files that that overlays would be
applied against.

Oh well.  It was an interesting exercise anyway, trying to be crafty.

-Frank
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 00/16] R-Car DU: Convert LVDS code to bridge driver

[Frank Rowand]

On 02/20/18 15:10, Laurent Pinchart wrote:
> Hello,
> 
> This patch series addresses a design mistake that dates back from the initial
> DU support. Support for the LVDS encoders, which are IP cores separate from
> the DU, was bundled in the DU driver. Worse, both the DU and LVDS were
> described through a single DT node.
> 
> To fix the, patches 01/16 and 02/16 define new DT bindings for the LVDS
> encoders, and deprecate their description inside the DU bindings. To retain
> backward compatibility with existing DT, patches 03/16 to 08/16 then patch the
> device tree at runtime to convert the legacy bindings to the new ones.
> 
> With the DT side addressed, patch 09/16 converts the LVDS support code to a
> separate bridge driver. Patches 11/16 to 16/16 then update all the device tree
> sources to the new DU and LVDS encoders bindings.
> 
> I decided to go for live DT patching in patch 08/16 because implementing
> support for both the legacy and new bindings in the driver would have been
> very intrusive, and prevented further cleanups. This version relies more
> heavily on overlays to avoid touching the internals of the OF core compared to
> v2, even if manual fixes to the device tree are still needed.
> 
> Compared to v3, this series uses the OF changeset API to update properties
> instead of accessing the internals of the property structure. This removes the
> local implementation of functions to look up nodes by path and update
> properties. In order to do this, I pulled in Pantelis' patch series titled
> "[PATCH v2 0/5] of: dynamic: Changesets helpers & fixes" at Rob's request, and
> rebased it while taking two small review comments into account.

Wait a minute!  Why are you putting a patch set to modify core devicetree
in the middle of a driver series.  Please pull it out to a separate series.

I'll try to look at the patches, as they are in this series, sometime
tomorrow.  I have a vague memory of unresolved issues from the last
time they were proposed.

Thanks,

Frank


> 
> Rob, I'd like this series to be merged in v4.17. As the changeset helpers are
> now a dependency, I'd need you to merge them early (ideally on top of
> v4.16-rc1) and provide a stable branch, or get your ack to merge them through
> Dave's tree if they don't conflict with what you have and will queue for
> v4.17.
> 
> This version also drops the small fix to the Porter board device tree that has
> been queued for v4.17 already.
> 
> Compared to v2, the biggest change is in patch 03/16. Following Rob's and
> Frank's reviews it was clear that modifying the unflattened DT structure of
> the overlay before applying it wasn't popular. I have thus decided to use one
> overlay source per SoC to move as much of the DT changes to the overlay as
> possible, and only perform manual modifications (that are still needed as some
> of the information is board-specific) on the system DT after applying the
> overlay. As a result the overlay is parsed and applied without being modified.
> 
> Compared to v1, this series update the r8a7792 and r8a7794 device tree sources
> and incorporate review feedback as described by the changelogs of individual
> patches.
> 
> 
> Laurent Pinchart (11):
>   dt-bindings: display: renesas: Add R-Car LVDS encoder DT bindings
>   dt-bindings: display: renesas: Deprecate LVDS support in the DU
> bindings
>   drm: rcar-du: Fix legacy DT to create LVDS encoder nodes
>   drm: rcar-du: Convert LVDS encoder code to bridge driver
>   ARM: dts: r8a7790: Convert to new LVDS DT bindings
>   ARM: dts: r8a7791: Convert to new LVDS DT bindings
>   ARM: dts: r8a7792: Convert to new DU DT bindings
>   ARM: dts: r8a7793: Convert to new LVDS DT bindings
>   ARM: dts: r8a7794: Convert to new DU DT bindings
>   arm64: dts: renesas: r8a7795: Convert to new LVDS DT bindings
>   arm64: dts: renesas: r8a7796: Convert to new LVDS DT bindings
> 
> Pantelis Antoniou (5):
>   of: dynamic: Add __of_node_dupv()
>   of: changesets: Introduce changeset helper methods
>   of: changeset: Add of_changeset_node_move method
>   of: unittest: changeset helpers
>   i2c: demux: Use changeset helpers for clarity
> 
>  .../bindings/display/bridge/renesas,lvds.txt   |  56 +++
>  .../devicetree/bindings/display/renesas,du.txt |  31 +-
>  MAINTAINERS|   1 +
>  arch/arm/boot/dts/r8a7790-lager.dts|  22 +-
>  arch/arm/boot/dts/r8a7790.dtsi |  64 ++-
>  arch/arm/boot/dts/r8a7791-koelsch.dts  |  10 +-
>  arch/arm/boot/dts/r8a7791-porter.dts   |  16 +-
>  arch/arm/boot/dts/r8a7791.dtsi |  36 +-
>  arch/arm/boot/dts/r8a7792.dtsi |   1 -
>  arch/arm/boot/dts/r8a7793-gose.dts |  10 +-
>  arch/arm/boot/dts/r8a7793.dtsi |  37 +-
>  arch/arm/boot/dts/r8a7794.dtsi |   1 -
>  .../boot/dts/renesas/r8a7795-es1-salvator-x.dts|   3 +-
>  

Re: [PATCH v2 03/12] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes

[Frank Rowand]

On 01/15/18 15:46, Frank Rowand wrote:
> On 01/15/18 12:29, Laurent Pinchart wrote:
>> Hi Frank,
>>
>> On Monday, 15 January 2018 22:12:33 EET Frank Rowand wrote:
>>> On 01/15/18 11:22, Laurent Pinchart wrote:
>>>> On Monday, 15 January 2018 21:12:44 EET Frank Rowand wrote:
>>>>> On 01/15/18 09:09, Rob Herring wrote:
>>>>>> +Frank
>>>>>>
>>>>>> On Fri, Jan 12, 2018 at 5:14 PM, Laurent Pinchart wrote:
>>>>>>> The internal LVDS encoders now have their own DT bindings. Before
>>>>>>> switching the driver infrastructure to those new bindings, implement
>>>>>>> backward-compatibility through live DT patching.
>>>>>>
>>>>>> Uhh, we just got rid of TI's patching and now adding this one. I guess
>>>
>>> Let me first answer the question that you ask later.  You ask "Can we work
>>> on this together to find a solution that would suit us both ?"
>>>
>>> My answer to that is emphatically YES.  I will definitely work with you to
>>> try to find a good solution.
>>
>> \o/
>>
>>>>> Please no.  What we just got rid of was making it difficult for me to
>>>>> make changes to the overlay infrastructure.  There are issues with
>>>>> overlays that need to be fixed before overlays become really usable.
>>>>> I am about to propose the next change, which involves removing a
>>>>> chunk of code that I will not be able to remove if this patch is
>>>>> accepted (the proposal is awaiting me collecting some data about
>>>>> the impact of the change, which I expect to complete this week).
>>>
>>> I should have thought just a little bit more before I hit send.  The
>>> situation is even worse than I wrote.  One of the next steps (in
>>> addition to what I wrote about above) is to change the overlay apply
>>> function to accept a flattened device tree (FDT), not an expanded
>>> device tree.  In this changed model, the unflattened overlay is
>>> not available to be modified before it is applied.
>>
>> That makes sense if we consider overlays to be immutable objects that we 
>> apply 
>> without offering a way to modify them. I won't challenge that API decision, 
>> as 
>> my use of an overlay here is a bit of a hack indeed.
>>
>>> It is important for the devicetree infrastructure to have ownership
>>> of the FDT that is used to create the unflattened tree.  (Notice
>>> that in the proposed patch, rcar_du_of_get_overlay() follows the
>>> TI example of doing a kmemdup of the blob (FDT), then uses the
>>> copy for the unflatten.  The kmemdup() in this case is to create
>>> a persistent copy of the FDT.)  The driver having ownership of
>>> this copy, and having the ability to free it is one of the many
>>> problems with the current overlay implementation.
>>
>> Yes, that's something I've identified as well. Lots of work has been done to 
>> clean up the OF core and we're clearly not done yet. I'm happy to see all 
>> the 
>> improvements you're working on.
>>
>>>>> Can you please handle both the old and new bindings through driver
>>>>> code instead?
>>>>
>>>> I could, but it would be pointless. The point here is to allow cleanups in
>>>> the driver. The LVDS encoder handling code is very intrusive in its
>>>> current form and I need to get rid of it. There would be zero point in
>>>> moving to the new infrastructure, as the main point is to get rid of the
>>>> old code which prevents moving forward. As a consequence that would block
>>>> new boards from receiving proper upstream support. An easy option is to
>>>> break backward compatibility. I'm personally fine with that, but I assume
>>>> other people would complain :-)
>>>>
>>>> I can, on the other hand, work with you to see how live DT patching could
>>>> be implemented in this driver without blocking your code. When developing
>>>> this patch series I start by patching the device tree manually without
>>>> relying on overlays at all, but got blocked by the fact that I need to
>>>> allocate phandles for new nodes I create. If there was an API to allocate
>>>> an unused phandle I could avoid using the overlay infrastructure at all.
>>>> Or there could be other
>>>
>>> It seems reasonable to provide a way to autogenerate a phandle (if
&g

Re: [PATCH v2 03/12] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes

[Frank Rowand]

On 01/15/18 12:29, Laurent Pinchart wrote:
> Hi Frank,
> 
> On Monday, 15 January 2018 22:12:33 EET Frank Rowand wrote:
>> On 01/15/18 11:22, Laurent Pinchart wrote:
>>> On Monday, 15 January 2018 21:12:44 EET Frank Rowand wrote:
>>>> On 01/15/18 09:09, Rob Herring wrote:
>>>>> +Frank
>>>>>
>>>>> On Fri, Jan 12, 2018 at 5:14 PM, Laurent Pinchart wrote:
>>>>>> The internal LVDS encoders now have their own DT bindings. Before
>>>>>> switching the driver infrastructure to those new bindings, implement
>>>>>> backward-compatibility through live DT patching.
>>>>>
>>>>> Uhh, we just got rid of TI's patching and now adding this one. I guess
>>
>> Let me first answer the question that you ask later.  You ask "Can we work
>> on this together to find a solution that would suit us both ?"
>>
>> My answer to that is emphatically YES.  I will definitely work with you to
>> try to find a good solution.
> 
> \o/
> 
>>>> Please no.  What we just got rid of was making it difficult for me to
>>>> make changes to the overlay infrastructure.  There are issues with
>>>> overlays that need to be fixed before overlays become really usable.
>>>> I am about to propose the next change, which involves removing a
>>>> chunk of code that I will not be able to remove if this patch is
>>>> accepted (the proposal is awaiting me collecting some data about
>>>> the impact of the change, which I expect to complete this week).
>>
>> I should have thought just a little bit more before I hit send.  The
>> situation is even worse than I wrote.  One of the next steps (in
>> addition to what I wrote about above) is to change the overlay apply
>> function to accept a flattened device tree (FDT), not an expanded
>> device tree.  In this changed model, the unflattened overlay is
>> not available to be modified before it is applied.
> 
> That makes sense if we consider overlays to be immutable objects that we 
> apply 
> without offering a way to modify them. I won't challenge that API decision, 
> as 
> my use of an overlay here is a bit of a hack indeed.
> 
>> It is important for the devicetree infrastructure to have ownership
>> of the FDT that is used to create the unflattened tree.  (Notice
>> that in the proposed patch, rcar_du_of_get_overlay() follows the
>> TI example of doing a kmemdup of the blob (FDT), then uses the
>> copy for the unflatten.  The kmemdup() in this case is to create
>> a persistent copy of the FDT.)  The driver having ownership of
>> this copy, and having the ability to free it is one of the many
>> problems with the current overlay implementation.
> 
> Yes, that's something I've identified as well. Lots of work has been done to 
> clean up the OF core and we're clearly not done yet. I'm happy to see all the 
> improvements you're working on.
> 
>>>> Can you please handle both the old and new bindings through driver
>>>> code instead?
>>>
>>> I could, but it would be pointless. The point here is to allow cleanups in
>>> the driver. The LVDS encoder handling code is very intrusive in its
>>> current form and I need to get rid of it. There would be zero point in
>>> moving to the new infrastructure, as the main point is to get rid of the
>>> old code which prevents moving forward. As a consequence that would block
>>> new boards from receiving proper upstream support. An easy option is to
>>> break backward compatibility. I'm personally fine with that, but I assume
>>> other people would complain :-)
>>>
>>> I can, on the other hand, work with you to see how live DT patching could
>>> be implemented in this driver without blocking your code. When developing
>>> this patch series I start by patching the device tree manually without
>>> relying on overlays at all, but got blocked by the fact that I need to
>>> allocate phandles for new nodes I create. If there was an API to allocate
>>> an unused phandle I could avoid using the overlay infrastructure at all.
>>> Or there could be other
>>
>> It seems reasonable to provide a way to autogenerate a phandle (if
>> requested) by the devicetree code that creates a new node.  Were you using
>> a function from drivers/of/dynamic.c to create the node?
> 
> Not to allocate the node, no. I allocated the device_node structure manually 
> with kzalloc(), and inserted it in the device tree with of_attach_node(). Is 
> that the right approach ? I haven't be

Re: [PATCH v2 03/12] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes

[Frank Rowand]

On 01/15/18 11:22, Laurent Pinchart wrote:
> Hi Frank,
> 
> On Monday, 15 January 2018 21:12:44 EET Frank Rowand wrote:
>> On 01/15/18 09:09, Rob Herring wrote:
>>> +Frank
>>>
>>> On Fri, Jan 12, 2018 at 5:14 PM, Laurent Pinchart wrote:
>>>> The internal LVDS encoders now have their own DT bindings. Before
>>>> switching the driver infrastructure to those new bindings, implement
>>>> backward-compatibility through live DT patching.
>>>
>>> Uhh, we just got rid of TI's patching and now adding this one. I guess
>>

Let me first answer the question that you ask later.  You ask "Can we work on
this together to find a solution that would suit us both ?"

My answer to that is emphatically YES.  I will definitely work with you to
try to find a good solution.


>> Please no.  What we just got rid of was making it difficult for me to
>> make changes to the overlay infrastructure.  There are issues with
>> overlays that need to be fixed before overlays become really usable.
>> I am about to propose the next change, which involves removing a
>> chunk of code that I will not be able to remove if this patch is
>> accepted (the proposal is awaiting me collecting some data about
>> the impact of the change, which I expect to complete this week).

I should have thought just a little bit more before I hit send.  The
situation is even worse than I wrote.  One of the next steps (in
addition to what I wrote about above) is to change the overlay apply
function to accept a flattened device tree (FDT), not an expanded
device tree.  In this changed model, the unflattened overlay is
not available to be modified before it is applied.

It is important for the devicetree infrastructure to have ownership
of the FDT that is used to create the unflattened tree.  (Notice
that in the proposed patch, rcar_du_of_get_overlay() follows the
TI example of doing a kmemdup of the blob (FDT), then uses the
copy for the unflatten.  The kmemdup() in this case is to create
a persistent copy of the FDT.)  The driver having ownership of
this copy, and having the ability to free it is one of the many
problems with the current overlay implementation.

>>
>> Can you please handle both the old and new bindings through driver
>> code instead?
> 
> I could, but it would be pointless. The point here is to allow cleanups in 
> the 
> driver. The LVDS encoder handling code is very intrusive in its current form 
> and I need to get rid of it. There would be zero point in moving to the new 
> infrastructure, as the main point is to get rid of the old code which 
> prevents 
> moving forward. As a consequence that would block new boards from receiving 
> proper upstream support. An easy option is to break backward compatibility. 
> I'm personally fine with that, but I assume other people would complain :-)
> 
> I can, on the other hand, work with you to see how live DT patching could be 
> implemented in this driver without blocking your code. When developing this 
> patch series I start by patching the device tree manually without relying on 
> overlays at all, but got blocked by the fact that I need to allocate phandles 
> for new nodes I create. If there was an API to allocate an unused phandle I 
> could avoid using the overlay infrastructure at all. Or there could be other 

It seems reasonable to provide a way to autogenerate a phandle (if requested)
by the devicetree code that creates a new node.  Were you using a function
from drivers/of/dynamic.c to create the node?


> options I'm not thinking of as I don't know what the changes you're working 
> on 
> are. Can we work on this together to find a solution that would suit us both ?

Again, yes, I would be glad to work with you on this.

-Frank

> 
>>> it's necessary, but I'd like to know how long we need to keep this?
>>>
>>> How many overlays would you need if everything is static (i.e.
>>> removing all your fixup code)?
>>>
>>>> Patching is disabled and will be enabled along with support for the new
>>>> DT bindings in the DU driver.
>>>>
>>>> Signed-off-by: Laurent Pinchart
>>>> <laurent.pinchart+rene...@ideasonboard.com>
>>>> ---
>>>> Changes since v1:
>>>>
>>>> - Select OF_FLATTREE
>>>> - Compile LVDS DT bindings patch code when DRM_RCAR_LVDS is selected
>>>> - Update the SPDX headers to use GPL-2.0 instead of GPL-2.0-only
>>>> - Turn __dtb_rcar_du_of_lvds_(begin|end) from u8 to char
>>>> - Pass void begin and end pointers to rcar_du_of_get_overlay()
>>>> - Use of_get_parent() instead of accessing the parent pointer directly
>>>> - Find

Re: [PATCH v2 03/12] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes

[Frank Rowand]

On 01/15/18 09:09, Rob Herring wrote:
> +Frank
> 
> On Fri, Jan 12, 2018 at 5:14 PM, Laurent Pinchart
>  wrote:
>> The internal LVDS encoders now have their own DT bindings. Before
>> switching the driver infrastructure to those new bindings, implement
>> backward-compatibility through live DT patching.
> 
> Uhh, we just got rid of TI's patching and now adding this one. I guess

Please no.  What we just got rid of was making it difficult for me to
make changes to the overlay infrastructure.  There are issues with
overlays that need to be fixed before overlays become really usable.
I am about to propose the next change, which involves removing a
chunk of code that I will not be able to remove if this patch is
accepted (the proposal is awaiting me collecting some data about
the impact of the change, which I expect to complete this week).

Can you please handle both the old and new bindings through driver
code instead?

Thanks,

Frank


> it's necessary, but I'd like to know how long we need to keep this?
> 
> How many overlays would you need if everything is static (i.e.
> removing all your fixup code)?
> 
>> Patching is disabled and will be enabled along with support for the new
>> DT bindings in the DU driver.
>>
>> Signed-off-by: Laurent Pinchart 
>> ---
>> Changes since v1:
>>
>> - Select OF_FLATTREE
>> - Compile LVDS DT bindings patch code when DRM_RCAR_LVDS is selected
>> - Update the SPDX headers to use GPL-2.0 instead of GPL-2.0-only
>> - Turn __dtb_rcar_du_of_lvds_(begin|end) from u8 to char
>> - Pass void begin and end pointers to rcar_du_of_get_overlay()
>> - Use of_get_parent() instead of accessing the parent pointer directly
>> - Find the LVDS endpoints nodes based on the LVDS node instead of the
>>   root of the overlay
>> - Update to the -lvds compatible string format
>> ---
>>  drivers/gpu/drm/rcar-du/Kconfig |   2 +
>>  drivers/gpu/drm/rcar-du/Makefile|   3 +-
>>  drivers/gpu/drm/rcar-du/rcar_du_of.c| 451 
>> 
>>  drivers/gpu/drm/rcar-du/rcar_du_of.h|  16 +
>>  drivers/gpu/drm/rcar-du/rcar_du_of_lvds.dts |  82 +
>>  5 files changed, 553 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.c
>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.h
>>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds.dts
>>
>> diff --git a/drivers/gpu/drm/rcar-du/Kconfig 
>> b/drivers/gpu/drm/rcar-du/Kconfig
>> index 5d0b4b7119af..3f83352a7313 100644
>> --- a/drivers/gpu/drm/rcar-du/Kconfig
>> +++ b/drivers/gpu/drm/rcar-du/Kconfig
>> @@ -22,6 +22,8 @@ config DRM_RCAR_LVDS
>> bool "R-Car DU LVDS Encoder Support"
>> depends on DRM_RCAR_DU
>> select DRM_PANEL
>> +   select OF_FLATTREE
>> +   select OF_OVERLAY
> 
> OF_OVERLAY should probably select OF_FLATTREE. I guess in theory, we
> could have an overlay from a non-FDT source...
> 
>> help
>>   Enable support for the R-Car Display Unit embedded LVDS encoders.
>>
>> diff --git a/drivers/gpu/drm/rcar-du/Makefile 
>> b/drivers/gpu/drm/rcar-du/Makefile
>> index 0cf5c11030e8..8ed569a0f462 100644
>> --- a/drivers/gpu/drm/rcar-du/Makefile
>> +++ b/drivers/gpu/drm/rcar-du/Makefile
>> @@ -5,10 +5,11 @@ rcar-du-drm-y := rcar_du_crtc.o \
>>  rcar_du_group.o \
>>  rcar_du_kms.o \
>>  rcar_du_lvdscon.o \
>> +rcar_du_of.o \
>>  rcar_du_plane.o
>>
>>  rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS)+= rcar_du_lvdsenc.o
>> -
>> +rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS)+= rcar_du_of_lvds.dtb.o
>>  rcar-du-drm-$(CONFIG_DRM_RCAR_VSP) += rcar_du_vsp.o
>>
>>  obj-$(CONFIG_DRM_RCAR_DU)  += rcar-du-drm.o
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_of.c 
>> b/drivers/gpu/drm/rcar-du/rcar_du_of.c
>> new file mode 100644
>> index ..2bf91201fc93
>> --- /dev/null
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_of.c
>> @@ -0,0 +1,451 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * rcar_du_of.c - Legacy DT bindings compatibility
>> + *
>> + * Copyright (C) 2018 Laurent Pinchart 
>> + *
>> + * Based on work from Jyri Sarha 
>> + * Copyright (C) 2015 Texas Instruments
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include "rcar_du_crtc.h"
>> +#include "rcar_du_drv.h"
>> +
>> +#ifdef CONFIG_DRM_RCAR_LVDS
> 
> Why not make compiling this file conditional in the Makefile?
> 
>> +
>> +struct rcar_du_of_overlay {
>> +   struct device_node *np;
>> +   void *data;
>> +   void *mem;
>> +};
>> +
>> +static void __init rcar_du_of_free_overlay(struct rcar_du_of_overlay 
>> *overlay)
>> +{
>> +   of_node_put(overlay->np);
>> +   kfree(overlay->data);
>> +   kfree(overlay->mem);
>> +}
>> +
>> +static int 

Re: linux-next: manual merge of the devicetree tree with the drm tree

[Frank Rowand]

Hi Jyri,

On 11/13/17 07:40, Jyri Sarha wrote:
> On 11/13/17 07:58, Stephen Rothwell wrote:
>> Hi all,
>>
>> On Mon, 30 Oct 2017 20:37:56 + Mark Brown  wrote:
>>>
>>> Today's linux-next merge of the devicetree tree got a conflict in:
>>>
>>>   drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>>
>>> between commit:
>>>
>>>   44cd3939c111b7 ("drm/tilcdc: Remove redundant OF_DETACHED flag setting")
>>>
>>> from the drm tree and commit:
>>>
>>>   f948d6d8b792bb ("of: overlay: avoid race condition between applying 
>>> multiple overlays")
>>>
>>> from the devicetree tree.
>>>
>>> I fixed it up (see below) and can carry the fix as necessary. This
>>> is now fixed as far as linux-next is concerned, but any non trivial
>>> conflicts should be mentioned to your upstream maintainer when your tree
>>> is submitted for merging.  You may also want to consider cooperating
>>> with the maintainer of the conflicting tree to minimise any particularly
>>> complex conflicts.
>>>
>>> diff --cc drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>> index 482299a6f3b0,54025af534d4..
>>> --- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>> @@@ -163,12 -162,8 +162,6 @@@ static struct device_node * __init tilc
>>> return NULL;
>>> }
>>>   
>>> -   ret = of_resolve_phandles(overlay);
>>> -   if (ret) {
>>> -   pr_err("%s: Failed to resolve phandles: %d\n", __func__, ret);
>>> -   return NULL;
>>> -   }
>>>  -  of_node_set_flag(overlay, OF_DETACHED);
>>> --
>>> return overlay;
>>>   }
>>>   
>>
>> Just a reminder that this conflict still exists.
>>
> 
> After some consideration, I think we can drop the dts backward
> compatibility code from drm/tilcdc. It seems that it is causing a lot of
> trouble and I do not even know if anybody uses it anymore. Here is a
> patch for dropping it:
> 
> https://lists.freedesktop.org/archives/dri-devel/2017-November/157394.html
> 
> I did not plan to merge it before v4.16, but if this is a bigger problem
> I can push is right now for v4.15.
> 
> Best regards,
> Jyri
> 

The dropping patch is much appreciated.

It would make life easier for me if the patch gets into v4.15-rc.

-Frank
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 06/12] of: overlay: detect cases where device tree may become corrupt

[Frank Rowand]

On 10/19/17 12:04, Alan Tull wrote:
> On Tue, Oct 17, 2017 at 6:36 PM,   wrote:
> 
>>  static int overlay_notify(struct overlay_changeset *ovcs,
>> enum of_overlay_notify_action action)
>>  {
>> @@ -86,8 +109,14 @@ static int overlay_notify(struct overlay_changeset *ovcs,
>>
>> ret = blocking_notifier_call_chain(_notify_chain,
>>action, );
>> -   if (ret)
>> -   return notifier_to_errno(ret);
>> +   if (ret == NOTIFY_STOP)
>> +   return 0;
>> +   if (ret) {
>> +   ret = notifier_to_errno(ret);
>> +   pr_err("overlay changeset %s notifier error %d, 
>> target: %pOF\n",
>> +  of_overlay_action_name[action], ret, 
>> nd.target);
>> +   return ret;
>> +
> 
> Hi Frank,
> 
> This will spew lots of "error 0" messages for every notifier that
> returns NOTIFY_OK.
> 
> rmdir /sys/kernel/config/device-tree/overlays/1
> [  131.972505] OF: overlay: overlay changeset pre-remove notifier
> error 0, target: /soc/base_fpga_region/fpga_pr_region0
> [  131.987879] OF: overlay: overlay changeset post-remove notifier
> error 0, target: /soc/base_fpga_region/fpga_pr_region0

Thanks for finding that.  I'll send a patch so that NOTIFY_OK does
not spew an error.  (And overlay_notify() should also not be
returning an error in that case.)

-Frank


> 
> I could change fpga-region.c to return NOTIFY_STOP if it is accepting
> the overlay, but it will still want to return NOTIFY_OK for every case
> where it doesn't have an opinion.
> 
> Alan
> 
>> }
>>
>> return 0;
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 03/12] of: overlay: rename identifiers to more reflect what they do

[Frank Rowand]

On 10/17/17 07:38, Rob Herring wrote:
> On Mon, Oct 16, 2017 at 8:17 PM,  <frowand.l...@gmail.com> wrote:
>> From: Frank Rowand <frank.row...@sony.com>
>>
>> This patch is aimed primarily at drivers/of/overlay.c, but those
>> changes also have a small impact in a few other files.
>>
>> overlay.c is difficult to read and maintain.  Improve readability:
>>   - Rename functions, types and variables to better reflect what
>> they do and to be consistent with names in other places,
>> such as the device tree overlay FDT (flattened device tree),
>> and make the algorithms more clear
>>   - Use the same names consistently throughout the file
>>   - Update comments for name changes
>>   - Fix incorrect comments
>>
>> This patch is intended to not introduce any functional change.
>>
>> Signed-off-by: Frank Rowand <frank.row...@sony.com>
>> ---
>>  Documentation/devicetree/overlay-notes.txt   |  12 +-
>>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c |   5 +-
>>  drivers/of/dynamic.c |   2 +-
>>  drivers/of/overlay.c | 506 
>> ++-
>>  drivers/of/unittest.c|  20 +-
>>  include/linux/of.h   |  12 +-
>>  6 files changed, 294 insertions(+), 263 deletions(-)
> 
> Doesn't build:
> 
> ../drivers/of/overlay.c:397:41: warning: ‘struct of_overlay_info’
> declared inside parameter list will not be visible outside of this
> definition or declaration
>struct device_node *info_node, struct of_overlay_info *ovinfo)
>  ^~~
> ../drivers/of/overlay.c:396:40: warning: ‘struct of_overlay’ declared
> inside parameter list will not be visible outside of this definition
> or declaration
>  static int of_fill_overlay_info(struct of_overlay *ov,
> ^~
> ../drivers/of/overlay.c: In function ‘of_fill_overlay_info’:
> ../drivers/of/overlay.c:399:8: error: dereferencing pointer to
> incomplete type ‘struct of_overlay_info’
>   ovinfo->overlay = of_get_child_by_name(info_node, "__overlay__");
> ^~
> ../drivers/of/overlay.c: In function ‘init_overlay_changeset’:
> ../drivers/of/overlay.c:450:30: error: passing argument 1 of
> ‘of_fill_overlay_info’ from incompatible pointer type
> [-Werror=incompatible-pointer-types]
>ret = of_fill_overlay_info(ovcs, node, [cnt]);
>   ^~~~
> ../drivers/of/overlay.c:396:12: note: expected ‘struct of_overlay *’
> but argument is of type ‘struct overlay_changeset *’
>  static int of_fill_overlay_info(struct of_overlay *ov,
> ^~~~
> ../drivers/of/overlay.c:450:42: error: passing argument 3 of
> ‘of_fill_overlay_info’ from incompatible pointer type
> [-Werror=incompatible-pointer-types]
>ret = of_fill_overlay_info(ovcs, node, [cnt]);
>   ^
> ../drivers/of/overlay.c:396:12: note: expected ‘struct of_overlay_info
> *’ but argument is of type ‘struct fragment *’
>  static int of_fill_overlay_info(struct of_overlay *ov,
> ^~~~
> 
> 
> I could have messed something up as every commit so far conflicts with
> "of: overlay: fix memory leak related to duplicated property". Can you
> rebase on to my dt/next branch too.
> 
> Rob
> 

OK, I'll rebase.

-Frank
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 09/12] of: overlay: avoid race condition between applying multiple overlays

[Frank Rowand]

On 10/16/17 18:17, frowand.l...@gmail.com wrote:
> From: Frank Rowand <frank.row...@sony.com>
> 
> The process of applying an overlay consists of:
>   - unflatten an overlay FDT (flattened device tree) into an
> EDT (expanded device tree)
>   - fixup the phandle values in the overlay EDT to fit in a
> range above the phandle values in the live device tree
>   - create the overlay changeset to reflect the contents of
> the overlay EDT
>   - apply the overlay changeset, to modify the live device tree,
> potentially changing the maximum phandle value in the live
> device tree
> 
> There is currently no protection against two overlay applies
> concurrently determining what range of phandle values are in use
> in the live device tree, and subsequently changing that range.
> Add a mutex to prevent multiple overlay applies from occurring
> simultaneously.
> 
> Move of_resolve_phandles() into of_overlay_apply() so that it does not
> have to be duplicated by each caller of of_overlay_apply().
> 
> The test in of_resolve_phandles() that the overlay tree is detached is
> temporarily disabled so that old style overlay unittests do not fail.
> 
> Signed-off-by: Frank Rowand <frank.row...@sony.com>
> ---

Brown paper bag time.

Changes from version 1:

squash "[PATCH] of: overlay: move resolve phandles into of_overlay_apply()"
into "[PATCH 09/12] of: overlay: avoid race condition between applying multiple 
overlays"


>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c |  6 --
>  drivers/of/of_private.h  | 12 +++
>  drivers/of/overlay.c | 32 
> 
>  drivers/of/resolver.c|  7 ++
>  drivers/of/unittest.c| 22 +--
>  include/linux/of.h   |  3 ---
>  6 files changed, 67 insertions(+), 15 deletions(-)
> 

< snip >

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 00/12] of: overlay: clean up device tree overlay code

[Frank Rowand]

On 10/16/17 18:17, frowand.l...@gmail.com wrote:
> From: Frank Rowand <frank.row...@sony.com>
> 
> I have found the device tree overlay code to be difficult to read and
> maintain.  This patch series attempts to improve that situation.
> 
> The cleanup includes some changes visible to users of overlays.  The
> only in kernel user of overlays is fixed up for those changes.  The
> in kernel user is:
> 
>drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> 
> Following the cleanup patches are a set of patches to fix various
> issues.
> 
> The first five patches are intended to not make any functional
> changes, and are segrated to ease review.
> 
> Frank Rowand (12):
>   of: overlay.c: Remove comments that state the obvious, to reduce
> clutter
>   of: overlay.c: Convert comparisons to zero or NULL to logical
> expressions
>   of: overlay: rename identifiers to more reflect what they do
>   of: overlay: rename identifiers in dup_and_fixup_symbol_prop()
>   of: overlay: minor restructuring
>   of: overlay: detect cases where device tree may become corrupt
>   of: overlay: expand check of whether overlay changeset can be removed
>   of: overlay: loosen overly strict phandle clash check
>   of: overlay: avoid race condition between applying multiple overlays
>   of: overlay: simplify applying symbols from an overlay
>   of: overlay: remove a dependency on device node full_name
>   of: overlay: remove unneeded check for NULL kbasename()
> 
>  Documentation/devicetree/overlay-notes.txt   |   12 +-
>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c |   14 +-
>  drivers/of/base.c|2 +-
>  drivers/of/dynamic.c |  137 +++-
>  drivers/of/of_private.h  |   22 +-
>  drivers/of/overlay.c | 1034 
> --
>  drivers/of/resolver.c|7 +
>  drivers/of/unittest.c|   81 +-
>  include/linux/of.h   |   17 +-
>  9 files changed, 869 insertions(+), 457 deletions(-)
> 

Brown paper bag time.

Changes from version 1:

  squash "[PATCH] of: overlay: move resolve phandles into of_overlay_apply()"
  into "[PATCH 09/12] of: overlay: avoid race condition between applying 
multiple overlays"

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] of: overlay: move resolve phandles into of_overlay_apply()

[Frank Rowand]

On 10/16/17 13:46, Rob Herring wrote:
> On Tue, Oct 10, 2017 at 8:02 PM,  <frowand.l...@gmail.com> wrote:
>> From: Frank Rowand <frank.row...@sony.com>
>>
>> Move more code into of_overlay_apply() so that it does not have
>> to be duplicated by each caller of of_overlay_apply().
>>
>> The test in of_resolve_phandles() that the overlay tree is detached is
>> temporarily disabled so that old style overlay unittests do not fail.
>>
>> Signed-off-by: Frank Rowand <frank.row...@sony.com>
>> ---
>>
>> This patch applies on top of the series: "[PATCH 00/12] of: overlay: clean
>> up device tree overlay code".
>>
>> While reviewing "[PATCH 09/12] of: overlay: avoid race condition between
>> applying multiple overlays", Rob asked if of_resolve_phandles() could be
>> moved into of_overlay_apply().  This patch is what is involved in doing so.
> 
> Looks fine. Can you squash this into patch 9 and resend the series (or
> at least 9-12). The rest all looks fine to me.
> 
> Rob
> 

Thanks, I'll do that.

-Frank
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 00/12] of: overlay: clean up device tree overlay code

[Frank Rowand]

Hi Rob,

On 10/02/17 20:53, frowand.l...@gmail.com wrote:
> From: Frank Rowand <frank.row...@sony.com>
> 
> I have found the device tree overlay code to be difficult to read and
> maintain.  This patch series attempts to improve that situation.
> 
> The cleanup includes some changes visible to users of overlays.  The
> only in kernel user of overlays is fixed up for those changes.  The
> in kernel user is:
> 
>drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> 
> Following the cleanup patches are a set of patches to fix various
> issues.
> 
> The first five patches are intended to not make any functional
> changes, and are segrated to ease review.
> 
> Frank Rowand (12):
>   of: overlay.c: Remove comments that state the obvious, to reduce
> clutter
>   of: overlay.c: Convert comparisons to zero or NULL to logical
> expressions
>   of: overlay: rename identifiers to more reflect what they do
>   of: overlay: rename identifiers in dup_and_fixup_symbol_prop()
>   of: overlay: minor restructuring
>   of: overlay: detect cases where device tree may become corrupt
>   of: overlay: expand check of whether overlay changeset can be removed
>   of: overlay: loosen overly strict phandle clash check
>   of: overlay: avoid race condition between applying multiple overlays
>   of: overlay: simplify applying symbols from an overlay
>   of: overlay: remove a dependency on device node full_name
>   of: overlay: remove unneeded check for NULL kbasename()
> 
>  Documentation/devicetree/overlay-notes.txt   |   12 +-
>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c |   15 +-
>  drivers/of/base.c|2 +-
>  drivers/of/dynamic.c |  137 +++-
>  drivers/of/of_private.h  |   10 +-
>  drivers/of/overlay.c | 1024 
> --
>  drivers/of/unittest.c|   80 +-
>  include/linux/of.h   |   33 +-
>  8 files changed, 871 insertions(+), 442 deletions(-)
> 

What is the status on this series?  Did I resolve all of the issues that
you found?  Is there anything else I need to do?

The last issue that I recall, was a question about "[PATCH 09/12] of: overlay: 
avoid race condition between applying multiple overlays", were you asked if
of_resolve_phandles() could be moved into of_overlay_apply().  I sent an
additional patch "[PATCH] of: overlay: move resolve phandles into
of_overlay_apply()" [1] that applies on top of this series to do so.

There is a trickle of new patches against the same files as in my
series, so I would like to get my series applied sooner that later,
if possible.

Thanks,

Frank

[1] https://lkml.org/lkml/2017/10/10/1389
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 09/12] of: overlay: avoid race condition between applying multiple overlays

[Frank Rowand]

On 10/10/17 14:06, Rob Herring wrote:
> On Tue, Oct 10, 2017 at 2:39 PM, Frank Rowand <frowand.l...@gmail.com> wrote:
>> On 10/10/17 11:40, Rob Herring wrote:
>>> On Wed, Oct 04, 2017 at 08:29:59PM -0700, Frank Rowand wrote:
>>>> On 10/04/17 08:19, Rob Herring wrote:
>>>>> On Mon, Oct 2, 2017 at 10:53 PM,  <frowand.l...@gmail.com> wrote:
>>>>>> From: Frank Rowand <frank.row...@sony.com>
>>>>>>
>>>>>> The process of applying an overlay consists of:
>>>>>>   - unflatten an overlay FDT (flattened device tree) into an
>>>>>> EDT (expanded device tree)
>>>>>>   - fixup the phandle values in the overlay EDT to fit in a
>>>>>> range above the phandle values in the live device tree
>>>>>>   - create the overlay changeset to reflect the contents of
>>>>>> the overlay EDT
>>>>>>   - apply the overlay changeset, to modify the live device tree,
>>>>>> potentially changing the maximum phandle value in the live
>>>>>> device tree
>>>>>>
>>>>>> There is currently no protection against two overlay applies
>>>>>> concurrently determining what range of phandle values are in use
>>>>>> in the live device tree, and subsequently changing that range.
>>>>>> Add a mutex to prevent multiple overlay applies from occurring
>>>>>> simultaneously.
>>>>>>
>>>>>> Ignoring 2 checkpatch warnings: Prefer using '"%s...", __func__'
>>>>>> so that the WARN() string will be more easily grepped.
>>>>>>
>>>>>> Signed-off-by: Frank Rowand <frank.row...@sony.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c |  7 +++
>>>>>>  drivers/of/overlay.c | 22 ++
>>>>>>  drivers/of/unittest.c| 21 +
>>>>>>  include/linux/of.h   | 19 +++
>>>>>>  4 files changed, 69 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c 
>>>>>> b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>>>>> index 7a7be0515bfd..c99f7924b1c6 100644
>>>>>> --- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>>>>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>>>>> @@ -221,6 +221,11 @@ static void __init tilcdc_convert_slave_node(void)
>>>>>> goto out;
>>>>>> }
>>>>>>
>>>>>> +   /*
>>>>>> +* protect from of_resolve_phandles() through of_overlay_apply()
>>>>>> +*/
>>>>>> +   of_overlay_mutex_lock();
>>>>>> +
>>>>>
>>>>> We can't be relying on callers to get the locking right...
>>>>
>>>> Agreed.
>>>>
>>>>
>>>>>
>>>>>> overlay = tilcdc_get_overlay();
>>>>>> if (!overlay)
>>>>>> goto out;
>>>>>> @@ -256,6 +261,8 @@ static void __init tilcdc_convert_slave_node(void)
>>>>>> pr_info("%s: ti,tilcdc,slave node successfully 
>>>>>> converted\n",
>>>>>> __func__);
>>>>>>  out:
>>>>>> +   of_overlay_mutex_unlock();
>>>>>> +
>>>>>> kfree_table_free();
>>>>>> of_node_put(i2c);
>>>>>> of_node_put(slave);
>>>>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>>>>> index a0d3222febdc..4ed372af6ce7 100644
>>>>>> --- a/drivers/of/overlay.c
>>>>>> +++ b/drivers/of/overlay.c
>>>>>> @@ -71,6 +71,28 @@ static int build_changeset_next_level(struct 
>>>>>> overlay_changeset *ovcs,
>>>>>> const struct device_node *overlay_node,
>>>>>> bool is_symbols_node);
>>>>>>
>>>>>> +/*
>>>>>> + * of_resolve_phandles() finds the largest phandle in the live tree.
>>>>>> + * of_overlay_apply() may add a larger phandle to the live

Re: [PATCH 09/12] of: overlay: avoid race condition between applying multiple overlays

[Frank Rowand]

On 10/10/17 11:40, Rob Herring wrote:
> On Wed, Oct 04, 2017 at 08:29:59PM -0700, Frank Rowand wrote:
>> On 10/04/17 08:19, Rob Herring wrote:
>>> On Mon, Oct 2, 2017 at 10:53 PM,  <frowand.l...@gmail.com> wrote:
>>>> From: Frank Rowand <frank.row...@sony.com>
>>>>
>>>> The process of applying an overlay consists of:
>>>>   - unflatten an overlay FDT (flattened device tree) into an
>>>> EDT (expanded device tree)
>>>>   - fixup the phandle values in the overlay EDT to fit in a
>>>> range above the phandle values in the live device tree
>>>>   - create the overlay changeset to reflect the contents of
>>>> the overlay EDT
>>>>   - apply the overlay changeset, to modify the live device tree,
>>>> potentially changing the maximum phandle value in the live
>>>> device tree
>>>>
>>>> There is currently no protection against two overlay applies
>>>> concurrently determining what range of phandle values are in use
>>>> in the live device tree, and subsequently changing that range.
>>>> Add a mutex to prevent multiple overlay applies from occurring
>>>> simultaneously.
>>>>
>>>> Ignoring 2 checkpatch warnings: Prefer using '"%s...", __func__'
>>>> so that the WARN() string will be more easily grepped.
>>>>
>>>> Signed-off-by: Frank Rowand <frank.row...@sony.com>
>>>> ---
>>>>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c |  7 +++
>>>>  drivers/of/overlay.c | 22 ++
>>>>  drivers/of/unittest.c| 21 +
>>>>  include/linux/of.h   | 19 +++
>>>>  4 files changed, 69 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c 
>>>> b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>>> index 7a7be0515bfd..c99f7924b1c6 100644
>>>> --- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>>>> @@ -221,6 +221,11 @@ static void __init tilcdc_convert_slave_node(void)
>>>> goto out;
>>>> }
>>>>
>>>> +   /*
>>>> +* protect from of_resolve_phandles() through of_overlay_apply()
>>>> +*/
>>>> +   of_overlay_mutex_lock();
>>>> +
>>>
>>> We can't be relying on callers to get the locking right...
>>
>> Agreed.
>>
>>
>>>
>>>> overlay = tilcdc_get_overlay();
>>>> if (!overlay)
>>>> goto out;
>>>> @@ -256,6 +261,8 @@ static void __init tilcdc_convert_slave_node(void)
>>>> pr_info("%s: ti,tilcdc,slave node successfully 
>>>> converted\n",
>>>> __func__);
>>>>  out:
>>>> +   of_overlay_mutex_unlock();
>>>> +
>>>> kfree_table_free();
>>>> of_node_put(i2c);
>>>> of_node_put(slave);
>>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>>> index a0d3222febdc..4ed372af6ce7 100644
>>>> --- a/drivers/of/overlay.c
>>>> +++ b/drivers/of/overlay.c
>>>> @@ -71,6 +71,28 @@ static int build_changeset_next_level(struct 
>>>> overlay_changeset *ovcs,
>>>> const struct device_node *overlay_node,
>>>> bool is_symbols_node);
>>>>
>>>> +/*
>>>> + * of_resolve_phandles() finds the largest phandle in the live tree.
>>>> + * of_overlay_apply() may add a larger phandle to the live tree.
>>>> + * Do not allow race between two overlays being applied simultaneously:
>>>> + *mutex_lock(_overlay_phandle_mutex)
>>>> + *of_resolve_phandles()
>>>> + *of_overlay_apply()
>>>> + *mutex_unlock(_overlay_phandle_mutex)
>>>
>>> Why do these need to be separate functions? I think I mentioned it
>>> before, but essentially overlay_data_add() should be part of the
>>> overlay API. We may need to allow for callers to do each step, but
>>> generally I think the interface should just be "apply this fdt blob".
>>
>> Yes, that is where I want to end up.
> 
> So, is that not doable now? To put it another way, why does 
> of_resolve_phandles need to be a separate call? Seems like an internal 
> detail of how you apply an overlay to me.
> 
> Rob

Yes, of_resolve_phandles() should become an internal call made from
the "apply this fdt blob" function.

The biggest obstacle is drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
using overlays in a convoluted manner.  The second obstacle will
probably be the older overlay tests in drivers/of/unittest.c.  I
need to look at how to convert them to using actual overlays.

There are other fixes and improvements to the overlay code that
need to occur, but it is like pulling on a loose thread in a
sweater - it just goes on and on.  I'd like to get this set of
patches in, with whatever changes are absolutely essential,
then continue on with more patch sets.  This code will be
much easier for me to modify in the future if this patch set
is applied.

-Frank







___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 09/12] of: overlay: avoid race condition between applying multiple overlays

[Frank Rowand]

On 10/10/17 14:06, Rob Herring wrote:
> On Tue, Oct 10, 2017 at 2:39 PM, Frank Rowand <frowand.l...@gmail.com> wrote:

< snip >

> 
> AFAICT, I don't think anything between of_resolve_phandles and
> of_overlay_apply calls in tilcdc depends on the phandles being fixed
> up.

I think you are correct, after another quick scan of that code.

< snip >

-Frank
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 09/12] of: overlay: avoid race condition between applying multiple overlays

[Frank Rowand]

On 10/04/17 08:19, Rob Herring wrote:
> On Mon, Oct 2, 2017 at 10:53 PM,  <frowand.l...@gmail.com> wrote:
>> From: Frank Rowand <frank.row...@sony.com>
>>
>> The process of applying an overlay consists of:
>>   - unflatten an overlay FDT (flattened device tree) into an
>> EDT (expanded device tree)
>>   - fixup the phandle values in the overlay EDT to fit in a
>> range above the phandle values in the live device tree
>>   - create the overlay changeset to reflect the contents of
>> the overlay EDT
>>   - apply the overlay changeset, to modify the live device tree,
>> potentially changing the maximum phandle value in the live
>> device tree
>>
>> There is currently no protection against two overlay applies
>> concurrently determining what range of phandle values are in use
>> in the live device tree, and subsequently changing that range.
>> Add a mutex to prevent multiple overlay applies from occurring
>> simultaneously.
>>
>> Ignoring 2 checkpatch warnings: Prefer using '"%s...", __func__'
>> so that the WARN() string will be more easily grepped.
>>
>> Signed-off-by: Frank Rowand <frank.row...@sony.com>
>> ---
>>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c |  7 +++
>>  drivers/of/overlay.c | 22 ++
>>  drivers/of/unittest.c| 21 +
>>  include/linux/of.h   | 19 +++
>>  4 files changed, 69 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c 
>> b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>> index 7a7be0515bfd..c99f7924b1c6 100644
>> --- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
>> @@ -221,6 +221,11 @@ static void __init tilcdc_convert_slave_node(void)
>> goto out;
>> }
>>
>> +   /*
>> +* protect from of_resolve_phandles() through of_overlay_apply()
>> +*/
>> +   of_overlay_mutex_lock();
>> +
> 
> We can't be relying on callers to get the locking right...

Agreed.


> 
>> overlay = tilcdc_get_overlay();
>> if (!overlay)
>> goto out;
>> @@ -256,6 +261,8 @@ static void __init tilcdc_convert_slave_node(void)
>> pr_info("%s: ti,tilcdc,slave node successfully converted\n",
>> __func__);
>>  out:
>> +   of_overlay_mutex_unlock();
>> +
>> kfree_table_free();
>> of_node_put(i2c);
>> of_node_put(slave);
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index a0d3222febdc..4ed372af6ce7 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -71,6 +71,28 @@ static int build_changeset_next_level(struct 
>> overlay_changeset *ovcs,
>> const struct device_node *overlay_node,
>> bool is_symbols_node);
>>
>> +/*
>> + * of_resolve_phandles() finds the largest phandle in the live tree.
>> + * of_overlay_apply() may add a larger phandle to the live tree.
>> + * Do not allow race between two overlays being applied simultaneously:
>> + *mutex_lock(_overlay_phandle_mutex)
>> + *of_resolve_phandles()
>> + *of_overlay_apply()
>> + *mutex_unlock(_overlay_phandle_mutex)
> 
> Why do these need to be separate functions? I think I mentioned it
> before, but essentially overlay_data_add() should be part of the
> overlay API. We may need to allow for callers to do each step, but
> generally I think the interface should just be "apply this fdt blob".

Yes, that is where I want to end up.

> 
> Rob
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/5] drm: of: introduce drm_of_find_panel_or_bridge

[Frank Rowand]

On 02/03/17 19:36, Rob Herring wrote:
> Many drivers have a common pattern of searching the OF graph for either an
> attached panel or bridge and then finding the DRM struct for the panel
> or bridge. Also, most drivers need to handle deferred probing when the
> DRM device is not yet instantiated. Create a common function,
> drm_of_find_panel_or_bridge, to find the connected node and the
> associated DRM panel or bridge device.
> 
> Signed-off-by: Rob Herring 
> ---
>  drivers/gpu/drm/drm_of.c | 50 
> 
>  include/drm/drm_of.h | 13 +
>  2 files changed, 63 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 47848ed8ca48..b29ce2f52113 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -3,7 +3,9 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
> +#include 
>  #include 
>  
>  static void drm_release_of(struct device *dev, void *data)
> @@ -207,3 +209,51 @@ int drm_of_encoder_active_endpoint(struct device_node 
> *node,
>   return -EINVAL;
>  }
>  EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint);
> +
> +/*
> + * drm_of_find_panel_or_bridge - return connected panel or bridge device
> + * @np: device tree node containing encoder input ports

  @port: 
  @endpoint: 


> + * @panel: pointer to hold returned drm_panel
> + * @bridge: pointer to hold returned drm_bridge
> + *
> + * Given a DT node's port and endpoint number, find the connected node and
> + * return either the associated struct drm_panel or drm_bridge device.
> + *
> + * Returns zero if successful, or one of the standard error codes if it 
> fails.
> + */
> +int drm_of_find_panel_or_bridge(const struct device_node *np,
> + int port, int endpoint,
> + struct drm_panel **panel,
> + struct drm_bridge **bridge)
> +{

< snip >

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Frank Rowand]

On 11/23/16 10:13, Frank Rowand wrote:
> On 11/22/16 21:55, Sekhar Nori wrote:
>> On Tuesday 22 November 2016 11:51 PM, Frank Rowand wrote:
>>> Please note that the compatible property might contain several strings, not 
>>> just
>>> a single string.
>>
>> So I guess the best thing to do is to use
>> of_property_read_string_index() and print the sting at index 0.
>>
>> Thanks,
>> Sekhar
> 
> If you want to print just one compatible value, you could use that method.
> 
> To give all of the information needed to understand the problem, the error
> message would need to include all of the strings contained in the compatible
> property and all of the .board values in the da8xx_ddrctl_board_confs[] array
> (currently only one entry, but coded to allow additional entries in the
> future).
> 
> It is hard to justify an error message that complex.
> 
> I would just print an error that no match was found.
> 
> -Frank

I just needed to read some more emails.  I see this approach was taken
in the "[PATCH v4 0/2] da8xx: fix section mismatch in new drivers"
series.

-Frank

[PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Frank Rowand]

On 11/22/16 21:55, Sekhar Nori wrote:
> On Tuesday 22 November 2016 11:51 PM, Frank Rowand wrote:
>> Please note that the compatible property might contain several strings, not 
>> just
>> a single string.
> 
> So I guess the best thing to do is to use
> of_property_read_string_index() and print the sting at index 0.
> 
> Thanks,
> Sekhar

If you want to print just one compatible value, you could use that method.

To give all of the information needed to understand the problem, the error
message would need to include all of the strings contained in the compatible
property and all of the .board values in the da8xx_ddrctl_board_confs[] array
(currently only one entry, but coded to allow additional entries in the
future).

It is hard to justify an error message that complex.

I would just print an error that no match was found.

-Frank

[PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Frank Rowand]

On 11/21/16 22:25, Sekhar Nori wrote:
> Hi Frank,
> 
> On Tuesday 22 November 2016 07:13 AM, Frank Rowand wrote:
>> On 11/21/16 08:33, Sekhar Nori wrote:
>>> On Monday 31 October 2016 08:15 PM, Bartosz Golaszewski wrote:
>>>> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
>>>> +{
>>>> +  const struct da8xx_ddrctl_config_knob *knob;
>>>> +  const struct da8xx_ddrctl_setting *setting;
>>>> +  struct device_node *node;
>>>> +  struct resource *res;
>>>> +  void __iomem *ddrctl;
>>>> +  struct device *dev;
>>>> +  u32 reg;
>>>> +
>>>> +  dev = >dev;
>>>> +  node = dev->of_node;
>>>> +
>>>> +  setting = da8xx_ddrctl_get_board_settings();
>>>> +  if (!setting) {
>>>> +  dev_err(dev, "no settings for board '%s'\n",
>>>> +  of_flat_dt_get_machine_name());
>>>> +  return -EINVAL;
>>>> +  }
>>>
>>> This causes a section mismatch because of_flat_dt_get_machine_name() 
>>> has an __init annotation. I did not notice that before, sorry.
>>>
>>> It can be fixed with a patch like below:
>>>
>>> ---8<---
>>> diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
>>> index a20e7bbbcbe0..9ca5aab3ac54 100644
>>> --- a/drivers/memory/da8xx-ddrctl.c
>>> +++ b/drivers/memory/da8xx-ddrctl.c
>>> @@ -102,6 +102,18 @@ static const struct da8xx_ddrctl_setting 
>>> *da8xx_ddrctl_get_board_settings(void)
>>> return NULL;
>>>  }
>>>  
>>> +static const char* da8xx_ddrctl_get_machine_name(void)
>>> +{
>>> +   const char *str;
>>> +   int ret;
>>> +
>>> +   ret = of_property_read_string(of_root, "model", );
>>> +   if (ret)
>>> +   ret = of_property_read_string(of_root, "compatible", );
>>> +
>>> +   return str;
>>> +}
>>> +
>>>  static int da8xx_ddrctl_probe(struct platform_device *pdev)
>>>  {
>>> const struct da8xx_ddrctl_config_knob *knob;
>>> @@ -118,7 +130,7 @@ static int da8xx_ddrctl_probe(struct platform_device 
>>> *pdev)
>>> setting = da8xx_ddrctl_get_board_settings();
>>> if (!setting) {
>>> dev_err(dev, "no settings for board '%s'\n",
>>> -   of_flat_dt_get_machine_name());
>>
>> da8xx_ddrctl_get_board_settings() tries to match based on the "compatible"
>> property in the root node.  The "model" property in the root node has
>> nothing to do with the failure to match. So creating and then using
>> da8xx_ddrctl_get_machine_name() to potentially report model is not useful.
>>
>> It should be sufficient to simply report that no compatible matched.
> 
> I agree with you on this. Even if model name is printed, you will have
> to go back and check the compatible anyway. But I think it will be
> useful to print the compatible instead of just reporting that nothing
> matched.
> 
> Bartosz, if you agree too, could you send a fix patch just printing the
> compatible?

Please note that the compatible property might contain several strings, not just
a single string.

> 
> Thanks,
> Sekhar
> 

[PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Frank Rowand]

Hi Sekhar,

(And adding Sudeep since he becomes involved in this further
down thread and at that point says he will re-work this
proposed work around in a manner that is incorrect in a
manner that is similar to this proposed work around.)

On 11/21/16 08:33, Sekhar Nori wrote:
> On Monday 31 October 2016 08:15 PM, Bartosz Golaszewski wrote:
>> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
>> +{
>> +const struct da8xx_ddrctl_config_knob *knob;
>> +const struct da8xx_ddrctl_setting *setting;
>> +struct device_node *node;
>> +struct resource *res;
>> +void __iomem *ddrctl;
>> +struct device *dev;
>> +u32 reg;
>> +
>> +dev = >dev;
>> +node = dev->of_node;
>> +
>> +setting = da8xx_ddrctl_get_board_settings();
>> +if (!setting) {
>> +dev_err(dev, "no settings for board '%s'\n",
>> +of_flat_dt_get_machine_name());
>> +return -EINVAL;
>> +}
> 
> This causes a section mismatch because of_flat_dt_get_machine_name() 
> has an __init annotation. I did not notice that before, sorry.
> 
> It can be fixed with a patch like below:
> 
> ---8<---
> diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
> index a20e7bbbcbe0..9ca5aab3ac54 100644
> --- a/drivers/memory/da8xx-ddrctl.c
> +++ b/drivers/memory/da8xx-ddrctl.c
> @@ -102,6 +102,18 @@ static const struct da8xx_ddrctl_setting 
> *da8xx_ddrctl_get_board_settings(void)
>   return NULL;
>  }
>  
> +static const char* da8xx_ddrctl_get_machine_name(void)
> +{
> + const char *str;
> + int ret;
> +
> + ret = of_property_read_string(of_root, "model", );
> + if (ret)
> + ret = of_property_read_string(of_root, "compatible", );
> +
> + return str;
> +}
> +
>  static int da8xx_ddrctl_probe(struct platform_device *pdev)
>  {
>   const struct da8xx_ddrctl_config_knob *knob;
> @@ -118,7 +130,7 @@ static int da8xx_ddrctl_probe(struct platform_device 
> *pdev)
>   setting = da8xx_ddrctl_get_board_settings();
>   if (!setting) {
>   dev_err(dev, "no settings for board '%s'\n",
> - of_flat_dt_get_machine_name());

da8xx_ddrctl_get_board_settings() tries to match based on the "compatible"
property in the root node.  The "model" property in the root node has
nothing to do with the failure to match. So creating and then using
da8xx_ddrctl_get_machine_name() to potentially report model is not useful.

It should be sufficient to simply report that no compatible matched.


> + da8xx_ddrctl_get_machine_name());
>   return -EINVAL;
>   }
> ---8<--- 
> 
> A similar fix is required for the other driver in this series (patch 
> 2/5). I need some advise on whether I should introduce a common 
> function to get the machine name post kernel boot-up (I cannot see an 
> existing one). If yes, any advise on which file it should go into?
> 
> Thanks,
> Sekhar
> 
> 

[GIT PULL] On-demand device probing

[Frank Rowand]

On 10/22/2015 7:44 AM, Greg Kroah-Hartman wrote:
> 
> 
> On Thu, Oct 22, 2015 at 11:05:11AM +0200, Tomeu Vizoso wrote:
>> But that's moot currently because Greg believes that the time spent
>> probing devices at boot time could be reduced enough so that the order
>> in which devices are probed becomes irrelevant. IME that would have to
>> be under 200ms so that the user doesn't notice and that's unicorn-far
>> from any bootlog I have ever seen.
> 
> But as no one has actually produced a bootlog, how do you know that?
> Where exactly is your time being spent?  What driver is causing long
> delays?  Why is the long-delay-drivers not being done in their own
> thread?  And most importantly, why are you ignoring the work that people
> did back in 2008 to solve the issue on other hardware platforms?
> 
>> Given that downstreams are already carrying as many hacks as they
>> could think of to speed total boot up, I think this is effectively
>> telling them to go away.
> 
> No I'm not, I'm asking for real data, not hand-wavy-this-is-going-to
> solve-the-random-issue-i'm-having type patch by putting random calls in
> semi-random subsystems all over the kernel.
> 
> And when I ask for real data, you respond with the fact that you aren't
> trying to speed up boot time here at all, so what am I supposed to think

I also had the understanding that this patch series was about improving
boot time.  But I was kindly corrected that the behavior change was
getting the panel displaying stuff at an earlier point in the boot sequence,
_not_ completing the entire boot faster. 

The claim for the current series, in patch 0 in v7 is:

   With this series I get the kernel to output to the panel in 0.5s,
   instead of 2.8s.

Just to get side-tracked, one other approach at ordering to reduce
deferrals reported a modest boot time reduction for four boards and a
very slight boot time increase for one other board.) The report of boot
times with that approach was in:

  http://article.gmane.org/gmane.linux.drivers.devicetree/133010

from Alexander Holler.

I have not searched further to see if there is more data of boot time
reductions from any of the other attempts to change driver binding
order to move dependencies before use of a resource.  But whether
there is a performance improvement or not, there continues to be
a stream of developers creatively impacting the binding order for
their specific driver(s) or board.  So it seems that maybe there
is an underlying problem, or we don't have adequate documentation
explaining how to avoid a need to order bindings, or the
documentation exists and is not being read.

I have been defaulting to the position that has been asserted by
the device tree maintainters, that probe deferrals work just fine
for at least the majority of cases (and is the message I have been
sharing in my conference presentations about device tree).  But I
suspect that there is at least a small minority of cases that are not
well served by probe deferral.  (Not to be read as an endorsement of
this specific patch series, just a generic observation.)

-Frank

> other than that you don't care enough to do the real work and are trying
> to hack the driver core up instead.
> 
>> Sorry for the rant,
> 
> No apologies needed, it's cathartic at times :)
> 
> thanks,
> 
> greg k-h
> .
> 

Alternative approach to solve the deferred probe

[Frank Rowand]

On 10/21/2015 1:35 PM, Russell King - ARM Linux wrote:
> On Wed, Oct 21, 2015 at 08:36:23AM -0700, Frank Rowand wrote:
>> On 10/21/2015 1:18 AM, Russell King - ARM Linux wrote:
>>> On Tue, Oct 20, 2015 at 08:58:19PM -0700, Frank Rowand wrote:
>>>> On 10/20/2015 8:46 AM, Russell King - ARM Linux wrote:
>>
>> < snip >
>>
>>>>> +
>>>>>  static bool driver_deferred_probe_enable = false;
>>>>> +
>>>>>  /**
>>>>>   * driver_deferred_probe_trigger() - Kick off re-probing deferred devices
>>>>>   *
>>>>> @@ -188,6 +210,13 @@ static int deferred_probe_initcall(void)
>>>>>   driver_deferred_probe_trigger();
>>>>
>>>> Couldn't you put the "driver_deferred_probe_report = true" here?  And then
>>>> not add another round of probes.
>>>
>>> The idea is not to report anything for drivers that were deferred
>>> during the normal bootup.  The above is part of the normal bootup,
>>> and the deferred activity should not be warned about.
>>
>> The above is currently the last point for probe to succeed or defer
>> (until possibly, as you mentioned, module loading resolves the defer).
>> If a probe defers above, it will defer again below.  The set of defers
>> should be exactly the same above and below.
> 
> Why should it?

My assertion was incorrect.  A probe in the deferral processing can
result in the driver being placed on the new deferred list, then when
a later probe of another deferred driver succeeds, the first driver
will be moved to the active deferred list, and might succeed on
the second probe attempt (or with the current messages would result
in a second set of deferred messages).  So yes, placing
"driver_deferred_probe_report = true" where your patch put it and
running through the deferred probe processing again is correct.

-Frank

[GIT PULL] On-demand device probing

[Frank Rowand]

On 10/21/2015 2:12 PM, Rob Herring wrote:
> On Wed, Oct 21, 2015 at 1:18 PM, Frank Rowand  
> wrote:
>> On 10/21/2015 9:27 AM, Mark Brown wrote:
>>> On Wed, Oct 21, 2015 at 08:59:51AM -0700, Frank Rowand wrote:
>>>> On 10/19/2015 5:34 AM, Tomeu Vizoso wrote:
>>>
>>>>> To be clear, I was saying that this series should NOT affect total
>>>>> boot times much.
>>>
>>>> I'm confused.  If I understood correctly, improving boot time was
>>>> the key justification for accepting this patch set.  For example,
>>>> from "[PATCH v7 0/20] On-demand device probing":
>>>>
>>>>I have a problem with the panel on my Tegra Chromebook taking longer
>>>>than expected to be ready during boot (Stéphane Marchesin reported what
>>>>is basically the same issue in [0]), and have looked into ordered
>>>>probing as a better way of solving this than moving nodes around in the
>>>>DT or playing with initcall levels and linking order.
>>>>
>>>>...
>>>>
>>>>With this series I get the kernel to output to the panel in 0.5s,
>>>>instead of 2.8s.
>>>
>>> Overall boot time and time to get some individual built in component up
>>> and running aren't the same thing - what this'll do is get things up
>>> more in the link order of the leaf consumers rather than deferring those
>>> leaf consumers when their dependencies aren't ready yet.
>>
>> Thanks!  I read too much into what was being improved.
>>
>> So this patch series, which on other merits may be a good idea, is as
>> a by product solving a specific ordering issue, moving successful panel
>> initialization to an earlier point in the boot sequence, if I now
>> understand more correctly.
>>
>> In that context, this seems like yet another ad hoc way of causing the
>> probe order to change in a way to solves one specific issue?  Could
>> it just as likely move the boot order of some other driver on some
>> other board later, to the detriment of somebody else?
> 
> Time to display on is important for many products. Having the console
> up as early as possible is another case. CAN bus is another. This is a
> real problem that is not just bad drivers.

Yes, I agree.

What I am seeing is that there continues to be a need for the ability
to explicitly order at least some driver initialization (at some
granularity), despite the push back against explicit ordering that
has been present in the past.


> I don't think it is completely ad hoc. Given all devices are
> registered after drivers, drivers will still probe first in initcall
> level order and then link order AFAIK. We may not take (more) initcall
> level tweak hacks, but that is a much more simple change for
> downstream. Don't get me wrong, I'd really like to see a way to
> control order independent of initcall level.
> 
> Rob

Yep, it is not directly ad hoc, just a fortunate side effect in
this case.  So just accidently ad hoc. :-)

-Frank

[GIT PULL] On-demand device probing

[Frank Rowand]

On 10/21/2015 9:27 AM, Mark Brown wrote:
> On Wed, Oct 21, 2015 at 08:59:51AM -0700, Frank Rowand wrote:
>> On 10/19/2015 5:34 AM, Tomeu Vizoso wrote:
> 
>>> To be clear, I was saying that this series should NOT affect total
>>> boot times much.
> 
>> I'm confused.  If I understood correctly, improving boot time was
>> the key justification for accepting this patch set.  For example,
>> from "[PATCH v7 0/20] On-demand device probing":
>>
>>I have a problem with the panel on my Tegra Chromebook taking longer
>>than expected to be ready during boot (Stéphane Marchesin reported what
>>is basically the same issue in [0]), and have looked into ordered
>>probing as a better way of solving this than moving nodes around in the
>>DT or playing with initcall levels and linking order.
>>
>>...
>>
>>With this series I get the kernel to output to the panel in 0.5s,
>>instead of 2.8s.
> 
> Overall boot time and time to get some individual built in component up
> and running aren't the same thing - what this'll do is get things up
> more in the link order of the leaf consumers rather than deferring those
> leaf consumers when their dependencies aren't ready yet.

Thanks!  I read too much into what was being improved.

So this patch series, which on other merits may be a good idea, is as
a by product solving a specific ordering issue, moving successful panel
initialization to an earlier point in the boot sequence, if I now
understand more correctly.

In that context, this seems like yet another ad hoc way of causing the
probe order to change in a way to solves one specific issue?  Could
it just as likely move the boot order of some other driver on some
other board later, to the detriment of somebody else?


> 
>> While not as dramatic as your results, they are somewhat supportive.
>> What has changed your assessment that the on-demand device probing
>> patches will give a big boot performance increase?  Do you have
>> new data or analysis?
> 
> See above, my understanding was that the performance improvements were
> more around improved control/predictability/handwave of the boot
> ordering rather than total time.
> 

Alternative approach to solve the deferred probe

[Frank Rowand]

On 10/21/2015 9:55 AM, Grygorii Strashko wrote:
> On 10/21/2015 06:36 PM, Frank Rowand wrote:
>> On 10/21/2015 1:18 AM, Russell King - ARM Linux wrote:
>>> On Tue, Oct 20, 2015 at 08:58:19PM -0700, Frank Rowand wrote:
>>>> On 10/20/2015 8:46 AM, Russell King - ARM Linux wrote:
>>
>> < snip >
>>
>>>>> +
>>>>>   static bool driver_deferred_probe_enable = false;
>>>>> +
>>>>>   /**
>>>>>* driver_deferred_probe_trigger() - Kick off re-probing deferred 
>>>>> devices
>>>>>*
>>>>> @@ -188,6 +210,13 @@ static int deferred_probe_initcall(void)
>>>>>   driver_deferred_probe_trigger();
>>>>
>>>> Couldn't you put the "driver_deferred_probe_report = true" here?  And then
>>>> not add another round of probes.
>>>
>>> The idea is not to report anything for drivers that were deferred
>>> during the normal bootup.  The above is part of the normal bootup,
>>> and the deferred activity should not be warned about.
>>
>> The above is currently the last point for probe to succeed or defer
>> (until possibly, as you mentioned, module loading resolves the defer).
>> If a probe defers above, it will defer again below.  The set of defers
>> should be exactly the same above and below.
>>
> 
> Unfortunately this is not "the last point for probe to succeed or defer".
> There are still a bunch of drivers in Kernel which will be probed at 
> late_initcall() level.
> (like ./drivers/net/ethernet/ti/cpsw.c => late_initcall(cpsw_init);

Yes, cpsw_init() should _not_ be a late_initcall.  This is yet another
example of playing games with ordering probes that we have been trying
to eliminate.

Thanks for pointing out one of the resulting problems this causes for the
deferred probe mechanism.

> Yes - they probably need to be updated to use module_init(), but that's what
> we have now). Those drivers will re-trigger deferred device probing if their
> probe succeeded.

Yes, if cpsw_init() leads to a successful probe, then deferred device probing
will be re-triggered.  I do not know if cpsw_init() will be called before or
after deferred_probe_initcall().  The general initcall mechanism does not
provide any ordering guarantees between the two functions because they are
at the same initcall level.

> 
> As result, it is impossible to say when will it happen the 
> "final round of deferred device probing" :( and final list of drivers which
> was "deferred forever" will be know only when kernel exits to User space 
> ("deferred forever" - before loading modules).
> 
> May be, we also can consider adding debug_fs entry which can be used to 
> display
> actual state of deferred_probe_pending_list? 
> 
>>>
>>> If we have any devices still deferring after _this_ round, that must
>>> indicate that some resource they want is not available, and that
>>> should be warned about.
>>>
>>> Of course, modules can defer too - and I made some suggestions in my
>>> waffle above the patch about that.
>>>
>>
>> < adding back trimmed, for fuller context >
>>
>>>>>   /* Sort as many dependencies as possible before exiting 
>>>>> initcalls */
>>>>>   flush_workqueue(deferred_wq);
>>>>> +
>>>>> + /* Now one final round, reporting any devices that remain deferred */
>>>>> + driver_deferred_probe_report = true;
>>>>> + driver_deferred_probe_trigger();
>>>>> + /* Sort as many dependencies as possible before exiting initcalls */
>>>>> + flush_workqueue(deferred_wq);
>>>>> +
>>>>>   return 0;
>>>>>   }
> 
> 

[GIT PULL] On-demand device probing

[Frank Rowand]

On 10/19/2015 5:34 AM, Tomeu Vizoso wrote:
> On 18 October 2015 at 21:53, Mark Brown  wrote:
>> On Sun, Oct 18, 2015 at 12:37:57PM -0700, Greg Kroah-Hartman wrote:
>>> On Sun, Oct 18, 2015 at 08:29:31PM +0100, Mark Brown wrote:
 On Fri, Oct 16, 2015 at 11:57:50PM -0700, Greg Kroah-Hartman wrote:

< snip >

> hope you don't mind I summarize the points taken instead of replying
> to the individual emails. I tried to address all the concerns that
> have been raised again in the cover letter, but I guess I did a bad
> job at explaining myself, so here's another (more in-depth) go at it.

< snip >

> 3) Regarding total boot time, I don't expect this series to make much
> of a difference because though we would save a lot of matching and
> querying for resources, that's little time compared with how long we
> wait for hardware to react during probing. Async probing is more
> likely to help with drivers that take a long time to probe.

And then in your reply to Russell's reply to your email you say:

> To be clear, I was saying that this series should NOT affect total
> boot times much.

I'm confused.  If I understood correctly, improving boot time was
the key justification for accepting this patch set.  For example,
from "[PATCH v7 0/20] On-demand device probing":

   I have a problem with the panel on my Tegra Chromebook taking longer
   than expected to be ready during boot (Stéphane Marchesin reported what
   is basically the same issue in [0]), and have looked into ordered
   probing as a better way of solving this than moving nodes around in the
   DT or playing with initcall levels and linking order.

   ...

   With this series I get the kernel to output to the panel in 0.5s,
   instead of 2.8s.

Alexander Holler reported improved boot times for his patch set
in August, which is another approach to ordering probes
(http://article.gmane.org/gmane.linux.drivers.devicetree/133010).
His results for 5 boards was four booted faster, one slightly
slower:

   Some numbers (5 boots on each board, without and with ordering drivers),
   all times are seconds.

   Kirkwood (dockstar, armv5):

   Boot to "Freeing unused kernel memory" (includes mounting the rootfs),
   unordered:
   4.456016 3.937801 4.114788 4.114526 3.949480 (average 4.1145222)
   ordered:
   3.173054 3.164045 3.141418 3.480679 3.459298 (3.2836988)
   Time needed to sort (of_init_build_order()):
   0.003024
   Time needed to match drivers to the order (without calling them):
   0.002884

   Beagleboard (rev C4, armv7):

   unordered:
   6.706024 6.821746 6.696014 6.673675 6.769866 (6.733465)
   ordered:
   5.544860 5.514160 5.505859 5.527374 5.496795 (5.5178096)
   sorting: 0.021209
   matching: 0.006165

   Beaglebone Black (rev A5, armv7):

   unordered:
   3.826531 3.825662 3.826648 3.825434 3.825263 (3.8259076)
   ordered:
   2.838554 2.838322 2.839459 2.838467 2.838421 (2.8386446)
   sorting: 0.004769
   matching: 0.004860

   imx6q (armv7):

   unordered:
   3.451998 3.418864 3.446952 3.429974 3.440996 (3.4377568)
   ordered:
   3.538312 3.549019 3.538105 3.515916 3.555715 (3.5394134)
   sorting: 0.004622
   matching: 0.003868

While not as dramatic as your results, they are somewhat supportive.
What has changed your assessment that the on-demand device probing
patches will give a big boot performance increase?  Do you have
new data or analysis?

-Frank

Alternative approach to solve the deferred probe

[Frank Rowand]

On 10/21/2015 1:18 AM, Russell King - ARM Linux wrote:
> On Tue, Oct 20, 2015 at 08:58:19PM -0700, Frank Rowand wrote:
>> On 10/20/2015 8:46 AM, Russell King - ARM Linux wrote:

< snip >

>>> +
>>>  static bool driver_deferred_probe_enable = false;
>>> +
>>>  /**
>>>   * driver_deferred_probe_trigger() - Kick off re-probing deferred devices
>>>   *
>>> @@ -188,6 +210,13 @@ static int deferred_probe_initcall(void)
>>> driver_deferred_probe_trigger();
>>
>> Couldn't you put the "driver_deferred_probe_report = true" here?  And then
>> not add another round of probes.
> 
> The idea is not to report anything for drivers that were deferred
> during the normal bootup.  The above is part of the normal bootup,
> and the deferred activity should not be warned about.

The above is currently the last point for probe to succeed or defer
(until possibly, as you mentioned, module loading resolves the defer).
If a probe defers above, it will defer again below.  The set of defers
should be exactly the same above and below.

> 
> If we have any devices still deferring after _this_ round, that must
> indicate that some resource they want is not available, and that
> should be warned about.
> 
> Of course, modules can defer too - and I made some suggestions in my
> waffle above the patch about that.
> 

< adding back trimmed, for fuller context >

>>> /* Sort as many dependencies as possible before exiting initcalls */
>>> flush_workqueue(deferred_wq);
>>> +
>>> +   /* Now one final round, reporting any devices that remain deferred */
>>> +   driver_deferred_probe_report = true;
>>> +   driver_deferred_probe_trigger();
>>> +   /* Sort as many dependencies as possible before exiting initcalls */
>>> +   flush_workqueue(deferred_wq);
>>> +
>>> return 0;
>>>  }