Re: [PATCH] media: Convert to using %pOF instead of full_name

[Frank Rowand]

On 07/19/17 09:02, Rob Herring wrote:
> On Wed, Jul 19, 2017 at 4:41 AM, Sylwester Nawrocki
>  wrote:
>> On 07/18/2017 11:43 PM, Rob Herring wrote:
>>> Now that we have a custom printf format specifier, convert users of
>>> full_name to use %pOF instead. This is preparation to remove storing
>>> of the full path string for each node.
>>>
>>> Signed-off-by: Rob Herring 
>>
>>> ---

< snip >

>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
>>> b/drivers/media/v4l2-core/v4l2-async.c
>>> index 851f128eba22..0a385d1ff28c 100644
>>> --- a/drivers/media/v4l2-core/v4l2-async.c
>>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>>> @@ -47,9 +47,7 @@ static bool match_fwnode(struct v4l2_subdev *sd, struct 
>>> v4l2_async_subdev *asd)
>>>   if (!is_of_node(sd->fwnode) || !is_of_node(asd->match.fwnode.fwnode))
>>>   return sd->fwnode == asd->match.fwnode.fwnode;
>>>
>>> - return !of_node_cmp(of_node_full_name(to_of_node(sd->fwnode)),
>>> - of_node_full_name(
>>> - to_of_node(asd->match.fwnode.fwnode)));
>>> + return to_of_node(sd->fwnode) == to_of_node(asd->match.fwnode.fwnode);
>>
>> I'm afraid this will not work, please see commit d2180e0cf77dc7a7049671d5d57d
>> "[media] v4l: async: make v4l2 coexist with devicetree nodes in a dt overlay"

Commit d2180e0cf77dc7a7049671d5d57d seems to have a fundamental
misunderstanding of overlays, if I understand the implications of that
commit.

When an overlay (1) is removed, all uses and references to the nodes and
properties in that overlay are no longer valid.  Any driver that uses any
information from the overlay _must_ stop using any data from the overlay.
Any driver that is bound to a new node in the overlay _must_ unbind.  Any
driver that became bound to a pre-existing node that was modified by the
overlay (became bound after the overlay was applied) _must_ adjust itself
to account for any changes to that node when the overlay is removed.  One
way to do this is to unbind when notified that the overlay is about to
be removed, then to re-bind after the overlay is completely removed.

If an overlay (2) is subsequently applied, a node with the same
full_name as from overlay (1) may exist.  There is no guarantee
that overlay (1) and overlay (2) are the same overlay, even if
that node has the same full_name in both cases.


> Maybe I'm missing something, but how does that work exactly? Before
> the overlay is applied, the remote endpoint node (and its parent)
> can't be resolved. In the commit example, the endpoint in the
> media_bridge would also have to be part of the overlay. If you apply
> and un-apply the overlay, then the of_node (and fw_node) in the
> overlay is once again invalid. IOW, you should be in the same state as
> before the overlay was applied. The node is still around because of
> paranoia that actually freeing nodes would break things. It seems this
> paranoia is real, so i think we need to do something to prevent this
> from spreading.

My understanding is that nodes from an un-applied overlay will be
freed if the expanded device tree that was used to create it has
its reference counts drop to zero.  But I wouldn't count on me
remembering this correctly without actually walking through all
the code.  And as far as I know, there is no example of this
in the mainline tree.


> Furthermore, it does not appear that any media driver supports
> overlays and we have no general way to apply them in mainline yet
> (other than an in kernel API). So really this scenario is not one we
> have to support yet.
> 
> Rob
> 

Re: [PATCH 0/2] of: overlay: Crash fix and improvement

[Frank Rowand]

On 12/08/17 05:13, Geert Uytterhoeven wrote:
>   Hi Pantelis, Rob, Frank,
> 
> This patch series fixes memory corruption when applying overlays.
> 
> I first noticed this when using OF configfs.  After lots of failed
> debugging attempts, I bisected it to "of: overlay: add per overlay sysfs
> attributes", which is not upstream.  But that was a red herring: that
> commit enlarged struct fragment to exactly 64-bytes, which just made it
> more likely to cause random corruption when writing beyond the end of an
> array of fragment structures.  With the smaller structure size before,
> such writes usually ended up in the unused holes between allocated
> blocks, causing no harm.
> 
> The first patch is the real fix, and applies to both v4.15-rc2 and Rob's
> for-next branch.
> The second patch is a small improvement, and applies to Rob's for-next
> branch only.

Overlay FDT files should not have invalid contents.  But they inevitably
will, so the code has to handle those cases.  Thanks for finding this
problem and making the code better!

For the full series:

Reviewed-by: Frank Rowand <frank.row...@sony.com>


> I've updated my topic/overlays and topic/renesas-overlays branches at
> git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git
> accordingly.
> 
> Thanks!
> 
> Geert Uytterhoeven (2):
>   of: overlay: Fix out-of-bounds write in init_overlay_changeset()
>   of: overlay: Make node skipping in init_overlay_changeset() clearer
> 
>  drivers/of/overlay.c | 22 --
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 

Re: [PATCH 0/2] of: overlay: Crash fix and improvement

[Frank Rowand]

On 12/09/17 01:04, Geert Uytterhoeven wrote:
> Hi Frank,
> 
> On Sat, Dec 9, 2017 at 7:01 AM, Frank Rowand <frowand.l...@gmail.com> wrote:
>> On 12/08/17 05:13, Geert Uytterhoeven wrote:
>>> This patch series fixes memory corruption when applying overlays.
>>> I first noticed this when using OF configfs.  After lots of failed
>>> debugging attempts, I bisected it to "of: overlay: add per overlay sysfs
>>> attributes", which is not upstream.  But that was a red herring: that
>>> commit enlarged struct fragment to exactly 64-bytes, which just made it
>>> more likely to cause random corruption when writing beyond the end of an
>>> array of fragment structures.  With the smaller structure size before,
>>> such writes usually ended up in the unused holes between allocated
>>> blocks, causing no harm.
>>>
>>> The first patch is the real fix, and applies to both v4.15-rc2 and Rob's
>>> for-next branch.
>>> The second patch is a small improvement, and applies to Rob's for-next
>>> branch only.
>>
>> Overlay FDT files should not have invalid contents.  But they inevitably
>> will, so the code has to handle those cases.  Thanks for finding this
>> problem and making the code better!
> 
> Sure, people can throw anything at it ;-)
> 
> In my case, I'm wondering if the dtbo was actually invalid?
> Simplification of the decompiled dtbo:
> 
> /dts-v1/;
> 
> / {
> 
> fragment-name {
> target-path = [2f 00];
> 
> __overlay__ {
> 
> node-name {
> compatible = "foo,bar";
> gpios = <0x 0x0 0x0>;
> };
> };
> };
> 
> __fixups__ {
> bank0 = "/fragment-name/__overlay__/node-name:gpios:0";
> };
> };
> 
> So it has __fixup__, but no __symbols__, which looks totally valid to me.

Yes, that is correct.  The bug would also be exposed if there was a 
__local_fixups__
node without a __symbols__ node.  Which is also a valid overlay.

My comment was triggered by another possible case, where a non-overlay node
occurs in an overlay, without a __symbols__ node.  I'm not positive, but I
don't think that dtc would find an error in that case. 


>> For the full series:
>>
>> Reviewed-by: Frank Rowand <frank.row...@sony.com>
> 
> Thanks!
> 
> 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
> 

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: [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 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 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: [RFC PATCH v2 0/1] of: easier debugging for node life cycle issues

[Frank Rowand]

On 01/21/18 06:31, Wolfram Sang wrote:
> I got a bug report for a DT node refcounting problem in the I2C subsystem. 
> This
> patch was a huge help in validating the bug report and the proposed solution.
> So, I thought I bring it to attention again. Thanks Tyrel, for the initial
> work!
> 
> Note that I did not test the dynamic updates, only of_node_{get|put} so far. I
> read that Tyrel checked dynamic updates extensively with this patch. And since
> DT overlays are also used within our Renesas dev team, this will help there, 
> as
> well.
> 
> Tested on a Renesas Salvator-XS board (R-Car H3).
> 
> Changes since RFC v1:
>   * rebased to v4.15-rc8
>   * fixed commit abbrev and one of the sysfs paths in commit desc
>   * removed trailing space and fixed pointer declaration in code
> 
> I consider all the remaining checkpatch issues irrelevant for this patch.
> 
> So what about applying it?
> 
> Kind regards,
> 
>Wolfram
> 
> 
> Tyrel Datwyler (1):
>   of: introduce event tracepoints for dynamic device_node lifecyle
> 
>  drivers/of/dynamic.c  | 32 ++--
>  include/trace/events/of.h | 93 
> +++
>  2 files changed, 105 insertions(+), 20 deletions(-)
>  create mode 100644 include/trace/events/of.h
> 

Please go back and read the thread for version 1.  Simply resubmitting a
forward port is ignoring that whole conversation.

There is a lot of good info in that thread.  I certainly learned stuff in it.

Thanks,

Frank

Re: [RFC PATCH v2 1/1] of: introduce event tracepoints for dynamic device_node lifecyle

[Frank Rowand]

On 01/21/18 06:31, Wolfram Sang wrote:
> From: Tyrel Datwyler 
> 
> This patch introduces event tracepoints for tracking a device_nodes
> reference cycle as well as reconfig notifications generated in response
> to node/property manipulations.
> 
> With the recent upstreaming of the refcount API several device_node
> underflows and leaks have come to my attention in the pseries (DLPAR)
> dynamic logical partitioning code (ie. POWER speak for hotplugging
> virtual and physcial resources at runtime such as cpus or IOAs). These
> tracepoints provide a easy and quick mechanism for validating the
> reference counting of device_nodes during their lifetime.
> 
> Further, when pseries lpars are migrated to a different machine we
> perform a live update of our device tree to bring it into alignment with
> the configuration of the new machine. The of_reconfig_notify trace point
> provides a mechanism that can be turned for debuging the device tree
> modifications with out having to build a custom kernel to get at the
> DEBUG code introduced by commit 00aa37206e1a54 ("of/reconfig: Add debug
> output for OF_RECONFIG notifiers").
> 
> The following trace events are provided: of_node_get, of_node_put,
> of_node_release, and of_reconfig_notify. These trace points require a
> kernel built with ftrace support to be enabled. In a typical environment
> where debugfs is mounted at /sys/kernel/debug the entire set of
> tracepoints can be set with the following:
> 
>   echo "of:*" > /sys/kernel/debug/tracing/set_event
> 
> or
> 
>   echo 1 > /sys/kernel/debug/tracing/events/of/enable
> 
> The following shows the trace point data from a DLPAR remove of a cpu
> from a pseries lpar:
> 
> cat /sys/kernel/debug/tracing/trace | grep "POWER8@10"
> 
> cpuhp/23-147   [023]    128.324827:
> of_node_put: refcount=5, dn->full_name=/cpus/PowerPC,POWER8@10
> cpuhp/23-147   [023]    128.324829:
> of_node_put: refcount=4, dn->full_name=/cpus/PowerPC,POWER8@10
> cpuhp/23-147   [023]    128.324829:
> of_node_put: refcount=3, dn->full_name=/cpus/PowerPC,POWER8@10
> cpuhp/23-147   [023]    128.324831:
> of_node_put: refcount=2, dn->full_name=/cpus/PowerPC,POWER8@10
>drmgr-7284  [009]    128.439000:
> of_node_put: refcount=1, dn->full_name=/cpus/PowerPC,POWER8@10
>drmgr-7284  [009]    128.439002:
> of_reconfig_notify: action=DETACH_NODE, 
> dn->full_name=/cpus/PowerPC,POWER8@10,
> prop->name=null, old_prop->name=null
>drmgr-7284  [009]    128.439015:
> of_node_put: refcount=0, dn->full_name=/cpus/PowerPC,POWER8@10
>drmgr-7284  [009]    128.439016:
> of_node_release: dn->full_name=/cpus/PowerPC,POWER8@10, dn->_flags=4
> 
> Signed-off-by: Tyrel Datwyler 
> [wsa: fixed commit abbrev and one of the sysfs paths in commit desc,
> removed trailing space and fixed pointer declaration in code]
> Signed-off-by: Wolfram Sang 
> ---
>  drivers/of/dynamic.c  | 32 ++--
>  include/trace/events/of.h | 93 
> +++
>  2 files changed, 105 insertions(+), 20 deletions(-)
>  create mode 100644 include/trace/events/of.h
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index ab988d88704da0..b0d6ab5a35b8c6 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -21,6 +21,9 @@ static struct device_node *kobj_to_device_node(struct 
> kobject *kobj)
>   return container_of(kobj, struct device_node, kobj);
>  }
>  
> +#define CREATE_TRACE_POINTS
> +#include 
> +
>  /**
>   * of_node_get() - Increment refcount of a node
>   * @node:Node to inc refcount, NULL is supported to simplify writing of
> @@ -30,8 +33,10 @@ static struct device_node *kobj_to_device_node(struct 
> kobject *kobj)
>   */
>  struct device_node *of_node_get(struct device_node *node)
>  {
> - if (node)
> + if (node) {
>   kobject_get(>kobj);
> + trace_of_node_get(refcount_read(>kobj.kref.refcount), 
> node->full_name);
> + }
>   return node;
>  }
>  EXPORT_SYMBOL(of_node_get);
> @@ -43,8 +48,10 @@ EXPORT_SYMBOL(of_node_get);
>   */
>  void of_node_put(struct device_node *node)
>  {
> - if (node)
> + if (node) {
> + trace_of_node_put(refcount_read(>kobj.kref.refcount) - 1, 
> node->full_name);
>   kobject_put(>kobj);
> + }
>  }
>  EXPORT_SYMBOL(of_node_put);
>  
> @@ -75,24 +82,7 @@ const char *action_names[] = {
>  int of_reconfig_notify(unsigned long action, struct of_reconfig_data *p)
>  {
>   int rc;
> -#ifdef DEBUG
> - struct of_reconfig_data *pr = p;
> -
> - switch (action) {
> - case OF_RECONFIG_ATTACH_NODE:
> - case OF_RECONFIG_DETACH_NODE:
> - pr_debug("notify %-15s %pOF\n", action_names[action],
> - pr->dn);
> - break;
> - case 

Re: [RFC PATCH v2 0/1] of: easier debugging for node life cycle issues

[Frank Rowand]

On 01/21/18 06:31, Wolfram Sang wrote:
> I got a bug report for a DT node refcounting problem in the I2C subsystem. 
> This
> patch was a huge help in validating the bug report and the proposed solution.
> So, I thought I bring it to attention again. Thanks Tyrel, for the initial
> work!
> 
> Note that I did not test the dynamic updates, only of_node_{get|put} so far. I
> read that Tyrel checked dynamic updates extensively with this patch. And since
> DT overlays are also used within our Renesas dev team, this will help there, 
> as
> well.

It's been nine months since version 1.  If you are going to include the
dynamic updates part of the patch then please test them.


> Tested on a Renesas Salvator-XS board (R-Car H3).
> 
> Changes since RFC v1:
>   * rebased to v4.15-rc8
>   * fixed commit abbrev and one of the sysfs paths in commit desc
>   * removed trailing space and fixed pointer declaration in code
> 

> I consider all the remaining checkpatch issues irrelevant for this patch.

I am OK with the line length warnings in this patch.

Why can't the macro error be fixed?

A file entry needs to be added to MAINTAINERS.


> 
> So what about applying it?
> 
> Kind regards,
> 
>Wolfram
> 
> 
> Tyrel Datwyler (1):
>   of: introduce event tracepoints for dynamic device_node lifecyle
> 
>  drivers/of/dynamic.c  | 32 ++--
>  include/trace/events/of.h | 93 
> +++
>  2 files changed, 105 insertions(+), 20 deletions(-)
>  create mode 100644 include/trace/events/of.h
> 

Re: [RFC PATCH v2 1/1] of: introduce event tracepoints for dynamic device_node lifecyle

[Frank Rowand]

On 01/21/18 06:31, Wolfram Sang wrote:
> From: Tyrel Datwyler 
> 
> This patch introduces event tracepoints for tracking a device_nodes
> reference cycle as well as reconfig notifications generated in response
> to node/property manipulations.
> 
> With the recent upstreaming of the refcount API several device_node
> underflows and leaks have come to my attention in the pseries (DLPAR)
> dynamic logical partitioning code (ie. POWER speak for hotplugging
> virtual and physcial resources at runtime such as cpus or IOAs). These
> tracepoints provide a easy and quick mechanism for validating the
> reference counting of device_nodes during their lifetime.
> 
> Further, when pseries lpars are migrated to a different machine we
> perform a live update of our device tree to bring it into alignment with
> the configuration of the new machine. The of_reconfig_notify trace point
> provides a mechanism that can be turned for debuging the device tree
> modifications with out having to build a custom kernel to get at the
> DEBUG code introduced by commit 00aa37206e1a54 ("of/reconfig: Add debug
> output for OF_RECONFIG notifiers").
> 
> The following trace events are provided: of_node_get, of_node_put,
> of_node_release, and of_reconfig_notify. These trace points require a

Please add a note that the of_reconfig_notify trace event is not an
added bit of debug info, but is instead replacing information that
was previously available via pr_debug() when DEBUG was defined.


> kernel built with ftrace support to be enabled. In a typical environment
> where debugfs is mounted at /sys/kernel/debug the entire set of
> tracepoints can be set with the following:
> 
>   echo "of:*" > /sys/kernel/debug/tracing/set_event
> 
> or
> 
>   echo 1 > /sys/kernel/debug/tracing/events/of/enable
> 
> The following shows the trace point data from a DLPAR remove of a cpu
> from a pseries lpar:
> 
> cat /sys/kernel/debug/tracing/trace | grep "POWER8@10"
> 
> cpuhp/23-147   [023]    128.324827:
> of_node_put: refcount=5, dn->full_name=/cpus/PowerPC,POWER8@10
> cpuhp/23-147   [023]    128.324829:
> of_node_put: refcount=4, dn->full_name=/cpus/PowerPC,POWER8@10
> cpuhp/23-147   [023]    128.324829:
> of_node_put: refcount=3, dn->full_name=/cpus/PowerPC,POWER8@10
> cpuhp/23-147   [023]    128.324831:
> of_node_put: refcount=2, dn->full_name=/cpus/PowerPC,POWER8@10
>drmgr-7284  [009]    128.439000:
> of_node_put: refcount=1, dn->full_name=/cpus/PowerPC,POWER8@10
>drmgr-7284  [009]    128.439002:
> of_reconfig_notify: action=DETACH_NODE, 
> dn->full_name=/cpus/PowerPC,POWER8@10,
> prop->name=null, old_prop->name=null
>drmgr-7284  [009]    128.439015:
> of_node_put: refcount=0, dn->full_name=/cpus/PowerPC,POWER8@10
>drmgr-7284  [009]    128.439016:
> of_node_release: dn->full_name=/cpus/PowerPC,POWER8@10, dn->_flags=4
> 
> Signed-off-by: Tyrel Datwyler 

The following belongs in a list of version 2 changes, below the "---" line:

> [wsa: fixed commit abbrev and one of the sysfs paths in commit desc,
> removed trailing space and fixed pointer declaration in code]

> Signed-off-by: Wolfram Sang 
> ---
>  drivers/of/dynamic.c  | 32 ++--
>  include/trace/events/of.h | 93 
> +++
>  2 files changed, 105 insertions(+), 20 deletions(-)
>  create mode 100644 include/trace/events/of.h

mode looks incorrect.  Existing files in include/trace/events/ are -rw-rw


> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index ab988d88704da0..b0d6ab5a35b8c6 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -21,6 +21,9 @@ static struct device_node *kobj_to_device_node(struct 
> kobject *kobj)
>   return container_of(kobj, struct device_node, kobj);
>  }
>  
> +#define CREATE_TRACE_POINTS
> +#include 
> +
>  /**
>   * of_node_get() - Increment refcount of a node
>   * @node:Node to inc refcount, NULL is supported to simplify writing of
> @@ -30,8 +33,10 @@ static struct device_node *kobj_to_device_node(struct 
> kobject *kobj)
>   */
>  struct device_node *of_node_get(struct device_node *node)
>  {
> - if (node)
> + if (node) {
>   kobject_get(>kobj);
> + trace_of_node_get(refcount_read(>kobj.kref.refcount), 
> node->full_name);

See the comment from Ron that I mentioned in my previous email.

Also, the path has been removed from node->full_name.  Does using it here
still give all of the information that is desired?  Same for all others uses
of full_name in this patch.

The trace point should have a single argument, node.  Accessing the two
fields can be done in the tracepoint assignment.  Or is there some
reason that can't be done?  Same for the trace_of_node_put() tracepoint.


> + }
>   return node;
>  }
>  

Re: [RFC PATCH v2 1/1] of: introduce event tracepoints for dynamic device_node lifecyle

[Frank Rowand]

On 01/24/18 22:48, Frank Rowand wrote:
> On 01/21/18 06:31, Wolfram Sang wrote:
>> From: Tyrel Datwyler <tyr...@linux.vnet.ibm.com>
>>
>> This patch introduces event tracepoints for tracking a device_nodes
>> reference cycle as well as reconfig notifications generated in response
>> to node/property manipulations.
>>
>> With the recent upstreaming of the refcount API several device_node
>> underflows and leaks have come to my attention in the pseries (DLPAR)
>> dynamic logical partitioning code (ie. POWER speak for hotplugging
>> virtual and physcial resources at runtime such as cpus or IOAs). These
>> tracepoints provide a easy and quick mechanism for validating the
>> reference counting of device_nodes during their lifetime.
>>
>> Further, when pseries lpars are migrated to a different machine we
>> perform a live update of our device tree to bring it into alignment with
>> the configuration of the new machine. The of_reconfig_notify trace point
>> provides a mechanism that can be turned for debuging the device tree
>> modifications with out having to build a custom kernel to get at the
>> DEBUG code introduced by commit 00aa37206e1a54 ("of/reconfig: Add debug
>> output for OF_RECONFIG notifiers").
>>
>> The following trace events are provided: of_node_get, of_node_put,
>> of_node_release, and of_reconfig_notify. These trace points require a
> 
> Please add a note that the of_reconfig_notify trace event is not an
> added bit of debug info, but is instead replacing information that
> was previously available via pr_debug() when DEBUG was defined.

I got a little carried away, "when DEBUG was defined" is extra
un-needed detail for the commit message.


> 
> 
>> kernel built with ftrace support to be enabled. In a typical environment
>> where debugfs is mounted at /sys/kernel/debug the entire set of
>> tracepoints can be set with the following:
>>
>>   echo "of:*" > /sys/kernel/debug/tracing/set_event
>>
>> or
>>
>>   echo 1 > /sys/kernel/debug/tracing/events/of/enable
>>
>> The following shows the trace point data from a DLPAR remove of a cpu
>> from a pseries lpar:
>>
>> cat /sys/kernel/debug/tracing/trace | grep "POWER8@10"
>>
>> cpuhp/23-147   [023]    128.324827:
>> of_node_put: refcount=5, dn->full_name=/cpus/PowerPC,POWER8@10
>> cpuhp/23-147   [023]    128.324829:
>> of_node_put: refcount=4, dn->full_name=/cpus/PowerPC,POWER8@10
>> cpuhp/23-147   [023]    128.324829:
>> of_node_put: refcount=3, dn->full_name=/cpus/PowerPC,POWER8@10
>> cpuhp/23-147   [023]    128.324831:
>> of_node_put: refcount=2, dn->full_name=/cpus/PowerPC,POWER8@10
>>drmgr-7284  [009]    128.439000:
>> of_node_put: refcount=1, dn->full_name=/cpus/PowerPC,POWER8@10
>>drmgr-7284  [009]    128.439002:
>> of_reconfig_notify: action=DETACH_NODE, 
>> dn->full_name=/cpus/PowerPC,POWER8@10,
>> prop->name=null, old_prop->name=null
>>drmgr-7284  [009]    128.439015:
>> of_node_put: refcount=0, dn->full_name=/cpus/PowerPC,POWER8@10
>>drmgr-7284  [009]    128.439016:
>> of_node_release: dn->full_name=/cpus/PowerPC,POWER8@10, dn->_flags=4
>>
>> Signed-off-by: Tyrel Datwyler <tyr...@linux.vnet.ibm.com>
> 
> The following belongs in a list of version 2 changes, below the "---" line:
> 
>> [wsa: fixed commit abbrev and one of the sysfs paths in commit desc,
>> removed trailing space and fixed pointer declaration in code]
> 
>> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
>> ---
>>  drivers/of/dynamic.c  | 32 ++--
>>  include/trace/events/of.h | 93 
>> +++
>>  2 files changed, 105 insertions(+), 20 deletions(-)
>>  create mode 100644 include/trace/events/of.h
> 
> mode looks incorrect.  Existing files in include/trace/events/ are -rw-rw
> 
> 
>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>> index ab988d88704da0..b0d6ab5a35b8c6 100644
>> --- a/drivers/of/dynamic.c
>> +++ b/drivers/of/dynamic.c
>> @@ -21,6 +21,9 @@ static struct device_node *kobj_to_device_node(struct 
>> kobject *kobj)
>>  return container_of(kobj, struct device_node, kobj);
>>  }
>>  
>> +#define CREATE_TRACE_POINTS
>> +#include 
>> +
>>  /**
>>   * of_node_get() - Increment refcount of a node
>>   * @node:   Nod

Re: [RFC PATCH v2 1/1] of: introduce event tracepoints for dynamic device_node lifecyle

[Frank Rowand]

On 01/25/18 00:01, Geert Uytterhoeven wrote:
> Hi Frank,
> 
> On Thu, Jan 25, 2018 at 7:48 AM, Frank Rowand <frowand.l...@gmail.com> wrote:
>>>  create mode 100644 include/trace/events/of.h
>>
>> mode looks incorrect.  Existing files in include/trace/events/ are -rw-rw
> 
> Not in my git clone ;-) 644 should be fine.
> 
> Gr{oetje,eeting}s,
> 
> Geert

Thanks!!!  I just learned something new about git.  Now I have to go experiment
a bit to make sure I learned properly.

-Frank

Re: [RFC PATCH v2 0/1] of: easier debugging for node life cycle issues

[Frank Rowand]

Hi Steve,

On 01/21/18 06:31, Wolfram Sang wrote:
> I got a bug report for a DT node refcounting problem in the I2C subsystem. 
> This
> patch was a huge help in validating the bug report and the proposed solution.
> So, I thought I bring it to attention again. Thanks Tyrel, for the initial
> work!
> 
> Note that I did not test the dynamic updates, only of_node_{get|put} so far. I
> read that Tyrel checked dynamic updates extensively with this patch. And since
> DT overlays are also used within our Renesas dev team, this will help there, 
> as
> well.
> 
> Tested on a Renesas Salvator-XS board (R-Car H3).
> 
> Changes since RFC v1:
>   * rebased to v4.15-rc8
>   * fixed commit abbrev and one of the sysfs paths in commit desc
>   * removed trailing space and fixed pointer declaration in code
> 
> I consider all the remaining checkpatch issues irrelevant for this patch.
> 
> So what about applying it?
> 
> Kind regards,
> 
>Wolfram
> 
> 
> Tyrel Datwyler (1):
>   of: introduce event tracepoints for dynamic device_node lifecyle
> 
>  drivers/of/dynamic.c  | 32 ++--
>  include/trace/events/of.h | 93 
> +++
>  2 files changed, 105 insertions(+), 20 deletions(-)
>  create mode 100644 include/trace/events/of.h
> 

Off the top of your head, can you tell me know early in the boot
process a trace_event can be called and successfully provide the
data to someone trying to debug early boot issues?

Also, way back when version 1 of this patch was being discussed,
a question about stacktrace triggers:

 >>> # echo stacktrace > /sys/kernel/debug/tracing/trace_options
 >>> # cat trace | grep -A6 "/pci@8002018"  
 >>
 >> Just to let you know that there is now stacktrace event triggers, where
 >> you don't need to stacktrace all events, you can pick and choose. And
 >> even filter the stack trace on specific fields of the event.  
 >
 > This is great, and I did figure that out this afternoon. One thing I was
 > still trying to determine though was whether its possible to set these
 > triggers at boot? As far as I could tell I'm still limited to
 > "trace_options=stacktrace" as a kernel boot parameter to get the stack
 > for event tracepoints.

 No not yet. But I'll add that to the todo list.

 Thanks,

 -- Steve

Is this still on your todo list, or is it now available?

Thanks,

Frank

Re: [RFC PATCH v2 0/1] of: easier debugging for node life cycle issues

[Frank Rowand]

Hi Wolfram,

On 01/25/18 03:03, Steven Rostedt wrote:
> On Wed, 24 Jan 2018 22:55:13 -0800
> Frank Rowand <frowand.l...@gmail.com> wrote:
> 
>> Hi Steve,
> 
>>
>> Off the top of your head, can you tell me know early in the boot
>> process a trace_event can be called and successfully provide the
>> data to someone trying to debug early boot issues?
> 
> The trace events are enabled by early_initcall().

< snip >

This means that ftrace can not be used for the of_node_get(),
of_node_put(), and of_node_release() debug info, because
these functions are called before early_initcall().  Please
use pr_debug() for these functions.

As far as I know, the of_reconfig_notify() could remain an
ftrace instrumented function.  But now that the only thing
that would be ftrace instrumented is of_reconfig_notify(),
I don't see a strong justification for changing the existing
pr_debug() calls to an ftrace alternative.  Though I suspect
the original author of the patch still might desire to have
the "#ifdef DEBUG" surrounding the pr_debug() calls removed
since one of his issues was having to recompile his kernel
to do his debugging.

-Frank

Re: [RFC PATCH v2 1/1] of: introduce event tracepoints for dynamic device_node lifecyle

[Frank Rowand]

On 01/25/18 14:40, Tyrel Datwyler wrote:
> On 01/24/2018 10:48 PM, Frank Rowand wrote:
>> On 01/21/18 06:31, Wolfram Sang wrote:
>>> From: Tyrel Datwyler <tyr...@linux.vnet.ibm.com>
>>>
>>> This patch introduces event tracepoints for tracking a device_nodes
>>> reference cycle as well as reconfig notifications generated in response
>>> to node/property manipulations.
>>>
>>> With the recent upstreaming of the refcount API several device_node
>>> underflows and leaks have come to my attention in the pseries (DLPAR)
>>> dynamic logical partitioning code (ie. POWER speak for hotplugging
>>> virtual and physcial resources at runtime such as cpus or IOAs). These
>>> tracepoints provide a easy and quick mechanism for validating the
>>> reference counting of device_nodes during their lifetime.
>>>
>>> Further, when pseries lpars are migrated to a different machine we
>>> perform a live update of our device tree to bring it into alignment with
>>> the configuration of the new machine. The of_reconfig_notify trace point
>>> provides a mechanism that can be turned for debuging the device tree
>>> modifications with out having to build a custom kernel to get at the
>>> DEBUG code introduced by commit 00aa37206e1a54 ("of/reconfig: Add debug
>>> output for OF_RECONFIG notifiers").
>>>
>>> The following trace events are provided: of_node_get, of_node_put,
>>> of_node_release, and of_reconfig_notify. These trace points require a
>>
>> Please add a note that the of_reconfig_notify trace event is not an
>> added bit of debug info, but is instead replacing information that
>> was previously available via pr_debug() when DEBUG was defined.
>>
>>
>>> kernel built with ftrace support to be enabled. In a typical environment
>>> where debugfs is mounted at /sys/kernel/debug the entire set of
>>> tracepoints can be set with the following:
>>>
>>>   echo "of:*" > /sys/kernel/debug/tracing/set_event
>>>
>>> or
>>>
>>>   echo 1 > /sys/kernel/debug/tracing/events/of/enable
>>>
>>> The following shows the trace point data from a DLPAR remove of a cpu
>>> from a pseries lpar:
>>>
>>> cat /sys/kernel/debug/tracing/trace | grep "POWER8@10"
>>>
>>> cpuhp/23-147   [023]    128.324827:
>>> of_node_put: refcount=5, dn->full_name=/cpus/PowerPC,POWER8@10
>>> cpuhp/23-147   [023]    128.324829:
>>> of_node_put: refcount=4, dn->full_name=/cpus/PowerPC,POWER8@10
>>> cpuhp/23-147   [023]    128.324829:
>>> of_node_put: refcount=3, dn->full_name=/cpus/PowerPC,POWER8@10
>>> cpuhp/23-147   [023]    128.324831:
>>> of_node_put: refcount=2, dn->full_name=/cpus/PowerPC,POWER8@10
>>>drmgr-7284  [009]    128.439000:
>>> of_node_put: refcount=1, dn->full_name=/cpus/PowerPC,POWER8@10
>>>drmgr-7284  [009]    128.439002:
>>> of_reconfig_notify: action=DETACH_NODE, 
>>> dn->full_name=/cpus/PowerPC,POWER8@10,
>>> prop->name=null, old_prop->name=null
>>>drmgr-7284  [009]    128.439015:
>>> of_node_put: refcount=0, dn->full_name=/cpus/PowerPC,POWER8@10
>>>drmgr-7284  [009]    128.439016:
>>> of_node_release: dn->full_name=/cpus/PowerPC,POWER8@10, dn->_flags=4
>>>
>>> Signed-off-by: Tyrel Datwyler <tyr...@linux.vnet.ibm.com>
>>
>> The following belongs in a list of version 2 changes, below the "---" line:
>>
>>> [wsa: fixed commit abbrev and one of the sysfs paths in commit desc,
>>> removed trailing space and fixed pointer declaration in code]
>>
>>> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
>>> ---
>>>  drivers/of/dynamic.c  | 32 ++--
>>>  include/trace/events/of.h | 93 
>>> +++
>>>  2 files changed, 105 insertions(+), 20 deletions(-)
>>>  create mode 100644 include/trace/events/of.h
>>
>> mode looks incorrect.  Existing files in include/trace/events/ are -rw-rw
>>
>>
>>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>>> index ab988d88704da0..b0d6ab5a35b8c6 100644
>>> --- a/drivers/of/dynamic.c
>>> +++ b/drivers/of/dynamic.c
>>> @@ -21,6 +21,9 @@ static struct device_node *kobj_to_device_node(struct 
&g

Re: [RFC PATCH v2 0/1] of: easier debugging for node life cycle issues

[Frank Rowand]

On 01/25/18 15:12, Wolfram Sang wrote:
> Frank,
> 
> here seems to be a misunderstanding going on. I don't want to push this
> patch upstream against all odds. I merely wanted to find out what the
> status of this patch is. Because one possibility was that it had just
> been forgotten...
> 
>>> So, I thought reposting would be a good way of finding out if your
>>> concerns were addressed in the discussion or not. If I overlooked

Marking a patch as RFC (as you mention just below here) is very different
than explicitly stating something like: Frank, you had concerns with
version 1, were your concerns addressed in the thread about version 1?

You mention below that adding RFC to the title was providing the
information that I needed.  I am saying that the communication was
not clear, was implied at best, and should have be more explicit.

I actually didn't even notice that the patch title had changed from
not an RFC, to being an RFC, so the subtle clue went right over my
head.  I just treated it as I would any RFC patch.


>> Then you should have stated that there were concerns raised in the
>> discussion and asked me if my concerns were addressed.
> 
> From my perspective, I have done that:
> 
> I marked the patch as RFC. I put you on the CC list. I asked about the
> possibility of applying it. It was not very elaborate, but hey, this is
> just a simple debugging patch :)

After reading through the original patch thread, I did not think that
the issues raised had been fully addressed.  You read the thread and
thought they had.  No big deal on coming to different conclusions.

I think you and I are talking past each other a little bit.  My
comments in the email you are responding to are because I don't
think that the previous emails have been as clear as you think.
I could read between the lines and see how you think that you
were being clear.  But from my perspective, I'm asking for more
explicit statements and less implied statements.

My first real response (the response that pointed out that Rob had
made an observation / suggestion that was not responded to) was coming
from the perspective that the issues in the first thread had not been
fully addressed.

As I was writing that response, I felt that I might as well do a review,
even though I thought the previous thread was dangling.  Which led to a
lot of issues and a few more emails pointing them out.


> I totally would have accepted a high level "No, that won't fly
> because...". Or a high level "This and that would need a change". Or
> something like that. I intentionally sent this out as RFC because I know
> there is some testing missing. I wanted to know if it is worth taking
> further steps with this patch.
> 
> So, I simply wanted to know if you (still) have fundamental issues with
> the patch?

It would have been good if you had simply stated so in exactly those
words.  I did not read the original email as saying that.  I read
the original email as saying (my paraphrase) "please apply it".  You
did _not_ use the words that I paraphrased, you actually said
"So what about applying it?".  I misunderstood what you were trying
to say.  I apologize for that.


> That needs to be discussed first before we go into coding
> details. I think it is fine to not apply it if there are reasons. I'd
> like to know them, however, for a better understanding.

Too late now. :-)  I've already done the reviewing and provided all
of the reasons that are show stoppers for the patch and how to fix.

One thing that I did not mention in this thread  is that I have an
aversion to using ftrace for what is purely debugging data (which
this is) because there is a danger that trace points become user
space ABI.  That is a whole long discussion that we do not have to
have because I am not subjecting this patch to that objection.


> For me, this is a super-super-side project, so if it causes too much
> hazzle, I just leave it here and know interested people can find it
> easier now. But if it could be applied with a sane amount of effort, I
> was offering that.
> 
> Was that understandable?

I think so.  And in return?  We can always talk more at the next
conference if you want.

-Frank

> 
> Kind regards,
> 
>Wolfram
> 

Re: [RFC PATCH v2 0/1] of: easier debugging for node life cycle issues

[Frank Rowand]

On 01/25/18 15:14, Wolfram Sang wrote:
> 
>> This means that ftrace can not be used for the of_node_get(),
>> of_node_put(), and of_node_release() debug info, because
>> these functions are called before early_initcall().
> 
> For the record: You can still unbind/bind devices. This is how I
> debugged an issue.

I wasn't implying that the data wasn't usable for any use case.

The point is that using ftrace means there are use cases for the
debug information where the information will not be available.

Re: [RFC PATCH v2 0/1] of: easier debugging for node life cycle issues

[Frank Rowand]

On 01/25/18 15:53, Tyrel Datwyler wrote:
> On 01/25/2018 01:49 PM, Frank Rowand wrote:
>> Hi Wolfram,
>>
>> On 01/25/18 03:03, Steven Rostedt wrote:
>>> On Wed, 24 Jan 2018 22:55:13 -0800
>>> Frank Rowand <frowand.l...@gmail.com> wrote:
>>>
>>>> Hi Steve,
>>>
>>>>
>>>> Off the top of your head, can you tell me know early in the boot
>>>> process a trace_event can be called and successfully provide the
>>>> data to someone trying to debug early boot issues?
>>>
>>> The trace events are enabled by early_initcall().
>>
>> < snip >
>>
>> This means that ftrace can not be used for the of_node_get(),
>> of_node_put(), and of_node_release() debug info, because
>> these functions are called before early_initcall().  Please
>> use pr_debug() for these functions.
> 
> I would argue that early boot debugging doesn't completely negate the
> usefulness of this tracing infrastructure.

I did not say or imply that it did.  I am pointing out that this
implementation does not meet the needs of other use cases.  And
potentially provides misleading information (or more precisely
misleading lack of information) in some other use cases.


> I get that no information
> is available in the trace up until ftrace is setup by its
> early_initcall, but I still found issues after early boot using this
> patch and I would hope that it would be somewhat obvious if
> references are out of whack once the ftrace data becomes available.
> In the dynamic case on Power we often do reconfig well after boot on
> live systems which produces a lot of reference put/gets. This patch
> made it easy to identify several reference leaks and underflows in
> our attach and detach logic with the added aid of being able to turn
> on the stacktrace for each call in the ftrace data.

Yes, you can get stacktraces relatively easily.  This is the strongest
argument for using ftrace.

My assumption has been that the stack trace is useful for of_node_get()
and of_node_put().  Is there _large_ value to the stack trace for
of_reconfig_notify()?


> Another thought is it would be nice if we could have the best of both
> worlds such that the tracepoints were pr_debugs up until the ftrace
> early_initcall. Or, I suppose we could ifdef it and make the ftrace
> tracepoints a configuration option, such that if it wasn't configured
> we implement the tracepoint functions as pr_debugs. This makes early
> boot an option. Just spit balling ideas.

An overly complex solution.  This is just debug.  Worst case alternative
is that the patches live on, out of tree.  So nope.


> 
> -Tyrel
> 
>>
>> As far as I know, the of_reconfig_notify() could remain an
>> ftrace instrumented function.  But now that the only thing
>> that would be ftrace instrumented is of_reconfig_notify(),
>> I don't see a strong justification for changing the existing
>> pr_debug() calls to an ftrace alternative.  Though I suspect
>> the original author of the patch still might desire to have
>> the "#ifdef DEBUG" surrounding the pr_debug() calls removed
>> since one of his issues was having to recompile his kernel
>> to do his debugging.
>>
>> -Frank
>>
> 
> 

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 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

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

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]

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 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

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 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.
> 

Re: [RFC PATCH v2 0/1] of: easier debugging for node life cycle issues

[Frank Rowand]

On 01/23/18 04:11, Michael Ellerman wrote:
> Wolfram Sang  writes:
> 
>> Hi Frank,
>>
>>> Please go back and read the thread for version 1.  Simply resubmitting a
>>> forward port is ignoring that whole conversation.
>>>
>>> There is a lot of good info in that thread.  I certainly learned stuff in 
>>> it.
>>
>> Yes, I did that and learned stuff, too. My summary of the discussion was:
>>
>> - you mentioned some drawbacks you saw (like the mixture of trace output
>>   and printk output)
>> - most of them look like addressed to me? (e.g. Steven showed a way to 
>> redirect
>>   printk to trace)
>> - you posted your version (which was, however, marked as "not user friendly"
>>   even by yourself)
>> - The discussion stalled over having two approaches
>>
>> So, I thought reposting would be a good way of finding out if your
>> concerns were addressed in the discussion or not. If I overlooked
>> something, I am sorry for that. Still, my intention is to continue the
>> discussion, not to ignore it. Because as it stands, we don't have such a
>> debugging mechanism in place currently, and with people working with DT
>> overlays, I'd think it would be nice to have.
> 
> Yeah I agree with all of that, I didn't think there were really any
> concerns left outstanding. These trace points are very useful, I've
> twice added them to a kernel to debug something, so it would be great
> for them to be in mainline.
> 
> cheers
> 

Yes, I believe there are concerns outstanding.  I'll try to read through
the whole thread today to make sure I'm not missing anything.

-Frank

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
> .
> 

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 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.

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

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

[Frank Rowand]

 
> +#include 
> +#include 
> +
> +#include "rcar_du_crtc.h"
> +#include "rcar_du_drv.h"
> +
> +/* 
> -
> + * Generic Overlay Handling
> + */
> +
> +struct rcar_du_of_overlay {
> + const char *compatible;
> + void *begin;
> + void *end;
> +};
> +
> +#define RCAR_DU_OF_DTB(type, soc)\
> + extern char __dtb_rcar_du_of_##type##_##soc##_begin[];  \
> + extern char __dtb_rcar_du_of_##type##_##soc##_end[]
> +
> +#define RCAR_DU_OF_OVERLAY(type, soc)
> \
> + {   \
> + .compatible = "renesas,du-" #soc,   \
> + .begin = __dtb_rcar_du_of_##type##_##soc##_begin,   \
> + .end = __dtb_rcar_du_of_##type##_##soc##_end,   \
> + }
> +
> +static int __init rcar_du_of_apply_overlay(const struct rcar_du_of_overlay 
> *dtbs,
> +const char *compatible)
> +{
> + const struct rcar_du_of_overlay *dtb = NULL;
> + unsigned int i;
> + int ovcs_id;
> +
> +     for (i = 0; dtbs[i].compatible; ++i) {
> + if (!strcmp(dtbs[i].compatible, compatible)) {
> + dtb = [i];
> + break;
> + }
> + }
> +
> + if (!dtb)
> + return -ENODEV;
> +
> + ovcs_id = 0;
> + return of_overlay_fdt_apply(dtb->begin, _id);

return of_overlay_fdt_apply(dtb->begin, dtb->end - dtb->begin, 
_id);



For version 5 of my patch series I added an FDT size argument, as you
suggested.


Reviewed-by: Frank Rowand <frank.row...@sony.com>

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

[Frank Rowand]

On 03/01/18 14:05, 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>
> ---

Reviewed-by: Frank Rowand <frank.row...@sony.com>