[PATCH v3 0/3] of: support passing console options with stdout-path

[PATCH v3 0/3] of: support passing console options with stdout-path

[Leif Lindholm]

This set started its life as a small, self-contained, patch enabling
specifying console options in the stdout-path property, but then
grew into a mahoosive behemoth with one of the patches taking 1m42s
for get_maintainer to process. It has now once again reverted to a
smaller, more pleasant, state of being.

Changes since v2:
- Revert the invasive bit, creating a replacement wrapper function
  for of_get_node_by_path() callers.
- Make the of_get_node_opts_by_path() function return a const pointer
  to the argument.
- Added binding for /chosen node, with a description of stdout-path.

Changes since v1:
- Change interface of of_get_node_by_path() to take an additional
  argument, and update all of its callers to keep working.
- Rework original patch to use this interface.

Leif Lindholm (3):
  devicetree: of: Add bindings for chosen node, stdout-path
  of: add optional options parameter to of_find_node_by_path()
  of: support passing console options with stdout-path

 Documentation/devicetree/bindings/chosen.txt |   42 ++++++++++++++++++++++++++
 drivers/of/base.c                            |   30 ++++++++++++++----
 drivers/of/selftest.c                        |   12 ++++++++
 include/linux/of.h                           |   14 ++++++++-
 4 files changed, 91 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/chosen.txt

-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/3] devicetree: of: Add bindings for chosen node, stdout-path

[Leif Lindholm]

Add a global binding for the chosen node.
Include a description of the stdout-path, and an explicit statement on
its extra options in the context of a UART console.

Opening description stolen from www.devicetree.org, and part of the
remaining text provided by Mark Rutland.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 Documentation/devicetree/bindings/chosen.txt |   42 ++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/chosen.txt

diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
new file mode 100644
index 0000000..9cd74e9
--- /dev/null
+++ b/Documentation/devicetree/bindings/chosen.txt
@@ -0,0 +1,42 @@
+The chosen node
+---------------
+
+The chosen node does not represent a real device, but serves as a place
+for passing data between firmware and the operating system, like boot
+arguments. Data in the chosen node does not represent the hardware.
+
+
+stdout-path property
+--------------------
+
+Device trees may specify the device to be used for boot console output
+with a stdout-path property under /chosen, as described in ePAPR, e.g.
+
+/ {
+	chosen {
+		stdout-path = "/serial@f00:115200";
+	};
+
+	serial@f00 {
+		compatible = "vendor,some-uart";
+		reg = <0xf00 0x10>;
+	};
+};
+
+If the character ":" is present in the value, this terminates the path.
+The meaning of any characters following the ":" is device-specific, and
+must be specified in the relevant binding documentation.
+
+For UART devices, the format supported by uart_parse_options() is the
+expected one. In this case, the format of the string is:
+
+	<baud>{<parity>{<bits>{<flow>}}}
+
+where
+
+	baud	- baud rate in decimal
+	parity	- 'n' (none), 'o', (odd) or 'e' (even)
+	bits	- number of data bits
+	flow	- 'r' (rts)
+
+For example: 115200n8r
-- 
1.7.10.4

Re: [PATCH v3 1/3] devicetree: of: Add bindings for chosen node, stdout-path

[Mark Rutland]

On Thu, Nov 27, 2014 at 05:56:05PM +0000, Leif Lindholm wrote:
> Add a global binding for the chosen node.
> Include a description of the stdout-path, and an explicit statement on
> its extra options in the context of a UART console.
> 
> Opening description stolen from www.devicetree.org, and part of the
> remaining text provided by Mark Rutland.
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  Documentation/devicetree/bindings/chosen.txt |   42 ++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/chosen.txt
> 
> diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> new file mode 100644
> index 0000000..9cd74e9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/chosen.txt
> @@ -0,0 +1,42 @@
> +The chosen node
> +---------------
> +
> +The chosen node does not represent a real device, but serves as a place
> +for passing data between firmware and the operating system, like boot
> +arguments. Data in the chosen node does not represent the hardware.
> +
> +
> +stdout-path property
> +--------------------
> +
> +Device trees may specify the device to be used for boot console output
> +with a stdout-path property under /chosen, as described in ePAPR, e.g.
> +
> +/ {
> +	chosen {
> +		stdout-path = "/serial@f00:115200";
> +	};
> +
> +	serial@f00 {
> +		compatible = "vendor,some-uart";
> +		reg = <0xf00 0x10>;
> +	};
> +};
> +
> +If the character ":" is present in the value, this terminates the path.
> +The meaning of any characters following the ":" is device-specific, and
> +must be specified in the relevant binding documentation.
> +
> +For UART devices, the format supported by uart_parse_options() is the
> +expected one. In this case, the format of the string is:

Please drop the mention of uart_parse_options and just describe the
format. Linux internal details are irrelevant to the contract of the
binding.

Otherwise this looks good to me!

Thanks,
Mark.

> +
> +	<baud>{<parity>{<bits>{<flow>}}}
> +
> +where
> +
> +	baud	- baud rate in decimal
> +	parity	- 'n' (none), 'o', (odd) or 'e' (even)
> +	bits	- number of data bits
> +	flow	- 'r' (rts)
> +
> +For example: 115200n8r
> -- 
> 1.7.10.4
> 
> 

Re: [PATCH v3 1/3] devicetree: of: Add bindings for chosen node, stdout-path

[Grant Likely]

On Thu, 27 Nov 2014 18:41:33 +0000
, Mark Rutland <mark.rutland@arm.com>
 wrote:
> On Thu, Nov 27, 2014 at 05:56:05PM +0000, Leif Lindholm wrote:
> > Add a global binding for the chosen node.
> > Include a description of the stdout-path, and an explicit statement on
> > its extra options in the context of a UART console.
> > 
> > Opening description stolen from www.devicetree.org, and part of the
> > remaining text provided by Mark Rutland.
> > 
> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/chosen.txt |   42 ++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/chosen.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> > new file mode 100644
> > index 0000000..9cd74e9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/chosen.txt
> > @@ -0,0 +1,42 @@
> > +The chosen node
> > +---------------
> > +
> > +The chosen node does not represent a real device, but serves as a place
> > +for passing data between firmware and the operating system, like boot
> > +arguments. Data in the chosen node does not represent the hardware.
> > +
> > +
> > +stdout-path property
> > +--------------------
> > +
> > +Device trees may specify the device to be used for boot console output
> > +with a stdout-path property under /chosen, as described in ePAPR, e.g.
> > +
> > +/ {
> > +	chosen {
> > +		stdout-path = "/serial@f00:115200";
> > +	};
> > +
> > +	serial@f00 {
> > +		compatible = "vendor,some-uart";
> > +		reg = <0xf00 0x10>;
> > +	};
> > +};
> > +
> > +If the character ":" is present in the value, this terminates the path.
> > +The meaning of any characters following the ":" is device-specific, and
> > +must be specified in the relevant binding documentation.
> > +
> > +For UART devices, the format supported by uart_parse_options() is the
> > +expected one. In this case, the format of the string is:
> 
> Please drop the mention of uart_parse_options and just describe the
> format. Linux internal details are irrelevant to the contract of the
> binding.
> 
> Otherwise this looks good to me!

I've fixed it up and merged it. No need to respin. Thanks!

g.

Re: [PATCH v3 1/3] devicetree: of: Add bindings for chosen node, stdout-path

[Frank Rowand]

On 11/27/2014 9:56 AM, Leif Lindholm wrote:
> Add a global binding for the chosen node.
> Include a description of the stdout-path, and an explicit statement on
> its extra options in the context of a UART console.
> 
> Opening description stolen from www.devicetree.org, and part of the
> remaining text provided by Mark Rutland.
> 
> Signed-off-by: Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/chosen.txt |   42 ++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/chosen.txt
> 
> diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> new file mode 100644
> index 0000000..9cd74e9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/chosen.txt
> @@ -0,0 +1,42 @@
> +The chosen node
> +---------------
> +
> +The chosen node does not represent a real device, but serves as a place
> +for passing data between firmware and the operating system, like boot
> +arguments. Data in the chosen node does not represent the hardware.
> +
> +
> +stdout-path property

The code in patch 3/3 adds the extra options feature to the properties:

  stdout-path
  linux,stdout-path
  stdout  [if (IS_ENABLED(CONFIG_PPC) ... ]

> +--------------------
> +
> +Device trees may specify the device to be used for boot console output
> +with a stdout-path property under /chosen, as described in ePAPR, e.g.
> +
> +/ {
> +	chosen {
> +		stdout-path = "/serial@f00:115200";
> +	};
> +
> +	serial@f00 {
> +		compatible = "vendor,some-uart";
> +		reg = <0xf00 0x10>;
> +	};
> +};
> +
> +If the character ":" is present in the value, this terminates the path.
> +The meaning of any characters following the ":" is device-specific, and
> +must be specified in the relevant binding documentation.
> +
> +For UART devices, the format supported by uart_parse_options() is the
> +expected one. In this case, the format of the string is:
> +
> +	<baud>{<parity>{<bits>{<flow>}}}
> +
> +where
> +
> +	baud	- baud rate in decimal
> +	parity	- 'n' (none), 'o', (odd) or 'e' (even)
> +	bits	- number of data bits
> +	flow	- 'r' (rts)
> +
> +For example: 115200n8r
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] devicetree: of: Add bindings for chosen node, stdout-path

[Grant Likely]

On Wed, Dec 3, 2014 at 2:24 AM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 11/27/2014 9:56 AM, Leif Lindholm wrote:
>> Add a global binding for the chosen node.
>> Include a description of the stdout-path, and an explicit statement on
>> its extra options in the context of a UART console.
>>
>> Opening description stolen from www.devicetree.org, and part of the
>> remaining text provided by Mark Rutland.
>>
>> Signed-off-by: Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/chosen.txt |   42 ++++++++++++++++++++++++++
>>  1 file changed, 42 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/chosen.txt
>>
>> diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
>> new file mode 100644
>> index 0000000..9cd74e9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/chosen.txt
>> @@ -0,0 +1,42 @@
>> +The chosen node
>> +---------------
>> +
>> +The chosen node does not represent a real device, but serves as a place
>> +for passing data between firmware and the operating system, like boot
>> +arguments. Data in the chosen node does not represent the hardware.
>> +
>> +
>> +stdout-path property
>
> The code in patch 3/3 adds the extra options feature to the properties:
>
>   stdout-path
>   linux,stdout-path
>   stdout  [if (IS_ENABLED(CONFIG_PPC) ... ]

I don't understand what you mean here. Are you suggesting a change to
this patch? Is there something deficient in it?

g.

>
>> +--------------------
>> +
>> +Device trees may specify the device to be used for boot console output
>> +with a stdout-path property under /chosen, as described in ePAPR, e.g.
>> +
>> +/ {
>> +     chosen {
>> +             stdout-path = "/serial@f00:115200";
>> +     };
>> +
>> +     serial@f00 {
>> +             compatible = "vendor,some-uart";
>> +             reg = <0xf00 0x10>;
>> +     };
>> +};
>> +
>> +If the character ":" is present in the value, this terminates the path.
>> +The meaning of any characters following the ":" is device-specific, and
>> +must be specified in the relevant binding documentation.
>> +
>> +For UART devices, the format supported by uart_parse_options() is the
>> +expected one. In this case, the format of the string is:
>> +
>> +     <baud>{<parity>{<bits>{<flow>}}}
>> +
>> +where
>> +
>> +     baud    - baud rate in decimal
>> +     parity  - 'n' (none), 'o', (odd) or 'e' (even)
>> +     bits    - number of data bits
>> +     flow    - 'r' (rts)
>> +
>> +For example: 115200n8r
>>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] devicetree: of: Add bindings for chosen node, stdout-path

[Frank Rowand]

On 12/3/2014 7:12 AM, Grant Likely wrote:
> On Wed, Dec 3, 2014 at 2:24 AM, Frank Rowand <frowand.list@gmail.com> wrote:
>> On 11/27/2014 9:56 AM, Leif Lindholm wrote:
>>> Add a global binding for the chosen node.
>>> Include a description of the stdout-path, and an explicit statement on
>>> its extra options in the context of a UART console.
>>>
>>> Opening description stolen from www.devicetree.org, and part of the
>>> remaining text provided by Mark Rutland.
>>>
>>> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
>>> ---
>>>  Documentation/devicetree/bindings/chosen.txt |   42 ++++++++++++++++++++++++++
>>>  1 file changed, 42 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/chosen.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
>>> new file mode 100644
>>> index 0000000..9cd74e9
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/chosen.txt
>>> @@ -0,0 +1,42 @@
>>> +The chosen node
>>> +---------------
>>> +
>>> +The chosen node does not represent a real device, but serves as a place
>>> +for passing data between firmware and the operating system, like boot
>>> +arguments. Data in the chosen node does not represent the hardware.
>>> +
>>> +
>>> +stdout-path property
>>
>> The code in patch 3/3 adds the extra options feature to the properties:
>>
>>   stdout-path
>>   linux,stdout-path
>>   stdout  [if (IS_ENABLED(CONFIG_PPC) ... ]
> 
> I don't understand what you mean here. Are you suggesting a change to
> this patch? Is there something deficient in it?

Assuming that the code change in patch 3 is as desired, then the chosen.txt
documentation applies to all three properties, not just stdout-path.  So
yes, a change is suggested for the documentation.

-Frank

Re: [PATCH v3 1/3] devicetree: of: Add bindings for chosen node, stdout-path

[Grant Likely]

On Wed, Dec 3, 2014 at 7:46 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> On 12/3/2014 7:12 AM, Grant Likely wrote:
>> On Wed, Dec 3, 2014 at 2:24 AM, Frank Rowand <frowand.list@gmail.com> wrote:
>>> On 11/27/2014 9:56 AM, Leif Lindholm wrote:
>>>> Add a global binding for the chosen node.
>>>> Include a description of the stdout-path, and an explicit statement on
>>>> its extra options in the context of a UART console.
>>>>
>>>> Opening description stolen from www.devicetree.org, and part of the
>>>> remaining text provided by Mark Rutland.
>>>>
>>>> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
>>>> ---
>>>>  Documentation/devicetree/bindings/chosen.txt |   42 ++++++++++++++++++++++++++
>>>>  1 file changed, 42 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/chosen.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
>>>> new file mode 100644
>>>> index 0000000..9cd74e9
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/chosen.txt
>>>> @@ -0,0 +1,42 @@
>>>> +The chosen node
>>>> +---------------
>>>> +
>>>> +The chosen node does not represent a real device, but serves as a place
>>>> +for passing data between firmware and the operating system, like boot
>>>> +arguments. Data in the chosen node does not represent the hardware.
>>>> +
>>>> +
>>>> +stdout-path property
>>>
>>> The code in patch 3/3 adds the extra options feature to the properties:
>>>
>>>   stdout-path
>>>   linux,stdout-path
>>>   stdout  [if (IS_ENABLED(CONFIG_PPC) ... ]
>>
>> I don't understand what you mean here. Are you suggesting a change to
>> this patch? Is there something deficient in it?
>
> Assuming that the code change in patch 3 is as desired, then the chosen.txt
> documentation applies to all three properties, not just stdout-path.  So
> yes, a change is suggested for the documentation.

how about if this is added:

Implementation note: Linux will look for the property "linux,stdout-path" or
on PowerPC "stdout" if "stdout-path" is not found.  However, the
"linux,stdout-path" and "stdout" properties are deprecated. New platforms
should only use the "stdout-path" property.

g.

Re: [PATCH v3 1/3] devicetree: of: Add bindings for chosen node, stdout-path

[Frank Rowand]

On 12/3/2014 1:45 PM, Grant Likely wrote:
> On Wed, Dec 3, 2014 at 7:46 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On 12/3/2014 7:12 AM, Grant Likely wrote:
>>> On Wed, Dec 3, 2014 at 2:24 AM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> On 11/27/2014 9:56 AM, Leif Lindholm wrote:
>>>>> Add a global binding for the chosen node.
>>>>> Include a description of the stdout-path, and an explicit statement on
>>>>> its extra options in the context of a UART console.
>>>>>
>>>>> Opening description stolen from www.devicetree.org, and part of the
>>>>> remaining text provided by Mark Rutland.
>>>>>
>>>>> Signed-off-by: Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/chosen.txt |   42 ++++++++++++++++++++++++++
>>>>>  1 file changed, 42 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/chosen.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
>>>>> new file mode 100644
>>>>> index 0000000..9cd74e9
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/chosen.txt
>>>>> @@ -0,0 +1,42 @@
>>>>> +The chosen node
>>>>> +---------------
>>>>> +
>>>>> +The chosen node does not represent a real device, but serves as a place
>>>>> +for passing data between firmware and the operating system, like boot
>>>>> +arguments. Data in the chosen node does not represent the hardware.
>>>>> +
>>>>> +
>>>>> +stdout-path property
>>>>
>>>> The code in patch 3/3 adds the extra options feature to the properties:
>>>>
>>>>   stdout-path
>>>>   linux,stdout-path
>>>>   stdout  [if (IS_ENABLED(CONFIG_PPC) ... ]
>>>
>>> I don't understand what you mean here. Are you suggesting a change to
>>> this patch? Is there something deficient in it?
>>
>> Assuming that the code change in patch 3 is as desired, then the chosen.txt
>> documentation applies to all three properties, not just stdout-path.  So
>> yes, a change is suggested for the documentation.
> 
> how about if this is added:
> 
> Implementation note: Linux will look for the property "linux,stdout-path" or
> on PowerPC "stdout" if "stdout-path" is not found.  However, the
> "linux,stdout-path" and "stdout" properties are deprecated. New platforms
> should only use the "stdout-path" property.
> 
> g.
> 

Yep, perfect.

-Frank
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] devicetree: of: Add bindings for chosen node, stdout-path

[Grant Likely]

On Wed, 03 Dec 2014 15:07:49 -0800
, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
 wrote:
> On 12/3/2014 1:45 PM, Grant Likely wrote:
> > On Wed, Dec 3, 2014 at 7:46 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> On 12/3/2014 7:12 AM, Grant Likely wrote:
> >>> On Wed, Dec 3, 2014 at 2:24 AM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>>> On 11/27/2014 9:56 AM, Leif Lindholm wrote:
> >>>>> Add a global binding for the chosen node.
> >>>>> Include a description of the stdout-path, and an explicit statement on
> >>>>> its extra options in the context of a UART console.
> >>>>>
> >>>>> Opening description stolen from www.devicetree.org, and part of the
> >>>>> remaining text provided by Mark Rutland.
> >>>>>
> >>>>> Signed-off-by: Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>>>> ---
> >>>>>  Documentation/devicetree/bindings/chosen.txt |   42 ++++++++++++++++++++++++++
> >>>>>  1 file changed, 42 insertions(+)
> >>>>>  create mode 100644 Documentation/devicetree/bindings/chosen.txt
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> >>>>> new file mode 100644
> >>>>> index 0000000..9cd74e9
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/chosen.txt
> >>>>> @@ -0,0 +1,42 @@
> >>>>> +The chosen node
> >>>>> +---------------
> >>>>> +
> >>>>> +The chosen node does not represent a real device, but serves as a place
> >>>>> +for passing data between firmware and the operating system, like boot
> >>>>> +arguments. Data in the chosen node does not represent the hardware.
> >>>>> +
> >>>>> +
> >>>>> +stdout-path property
> >>>>
> >>>> The code in patch 3/3 adds the extra options feature to the properties:
> >>>>
> >>>>   stdout-path
> >>>>   linux,stdout-path
> >>>>   stdout  [if (IS_ENABLED(CONFIG_PPC) ... ]
> >>>
> >>> I don't understand what you mean here. Are you suggesting a change to
> >>> this patch? Is there something deficient in it?
> >>
> >> Assuming that the code change in patch 3 is as desired, then the chosen.txt
> >> documentation applies to all three properties, not just stdout-path.  So
> >> yes, a change is suggested for the documentation.
> > 
> > how about if this is added:
> > 
> > Implementation note: Linux will look for the property "linux,stdout-path" or
> > on PowerPC "stdout" if "stdout-path" is not found.  However, the
> > "linux,stdout-path" and "stdout" properties are deprecated. New platforms
> > should only use the "stdout-path" property.
> > 
> > g.
> > 
> 
> Yep, perfect.

Great, thanks. Merged.

g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/3] of: add optional options parameter to of_find_node_by_path()

[Leif Lindholm]

Update of_find_node_by_path():
1) Rename function to of_find_node_opts_by_path(), adding an optional
   pointer argument. Provide a static inline wrapper version of
   of_find_node_by_path() which calls the new function with NULL as
   the optional argument.
2) Ignore any part of the path beyond and including the ':' separator.
3) Set the new provided pointer argument to the beginning of the string
   following the ':' separator.
4: Add tests.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 drivers/of/base.c     |   21 +++++++++++++++++----
 drivers/of/selftest.c |   12 ++++++++++++
 include/linux/of.h    |   14 +++++++++++++-
 3 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 3823edf..7f0e5f7 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -699,10 +699,15 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
 {
 	struct device_node *child;
 	int len = strchrnul(path, '/') - path;
+	int term;
 
 	if (!len)
 		return NULL;
 
+	term = strchrnul(path, ':') - path;
+	if (term < len)
+		len = term;
+
 	__for_each_child_of_node(parent, child) {
 		const char *name = strrchr(child->full_name, '/');
 		if (WARN(!name, "malformed device_node %s\n", child->full_name))
@@ -715,11 +720,14 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
 }
 
 /**
- *	of_find_node_by_path - Find a node matching a full OF path
+ *	of_find_node_opts_by_path - Find a node matching a full OF path
  *	@path: Either the full path to match, or if the path does not
  *	       start with '/', the name of a property of the /aliases
  *	       node (an alias).  In the case of an alias, the node
  *	       matching the alias' value will be returned.
+ *	@opts: Address of a pointer into which to store the start of
+ *	       an options string appended to the end of the path with
+ *	       a ':' separator.
  *
  *	Valid paths:
  *		/foo/bar	Full path
@@ -729,19 +737,24 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
  *	Returns a node pointer with refcount incremented, use
  *	of_node_put() on it when done.
  */
-struct device_node *of_find_node_by_path(const char *path)
+struct device_node *of_find_node_opts_by_path(const char *path, const char **opts)
 {
 	struct device_node *np = NULL;
 	struct property *pp;
 	unsigned long flags;
+	char *separator;
 
 	if (strcmp(path, "/") == 0)
 		return of_node_get(of_allnodes);
 
+	separator = strchr(path, ':');
+	if (separator && opts)
+		*opts = separator + 1;
+
 	/* The path could begin with an alias */
 	if (*path != '/') {
 		char *p = strchrnul(path, '/');
-		int len = p - path;
+		int len = separator ? separator - path : p - path;
 
 		/* of_aliases must not be NULL */
 		if (!of_aliases)
@@ -770,7 +783,7 @@ struct device_node *of_find_node_by_path(const char *path)
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	return np;
 }
-EXPORT_SYMBOL(of_find_node_by_path);
+EXPORT_SYMBOL(of_find_node_opts_by_path);
 
 /**
  *	of_find_node_by_name - Find a node by its "name" property
diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
index e2d79af..c298065 100644
--- a/drivers/of/selftest.c
+++ b/drivers/of/selftest.c
@@ -43,6 +43,7 @@ static bool selftest_live_tree;
 static void __init of_selftest_find_node_by_name(void)
 {
 	struct device_node *np;
+	const char *options;
 
 	np = of_find_node_by_path("/testcase-data");
 	selftest(np && !strcmp("/testcase-data", np->full_name),
@@ -83,6 +84,17 @@ static void __init of_selftest_find_node_by_name(void)
 	np = of_find_node_by_path("testcase-alias/missing-path");
 	selftest(!np, "non-existent alias with relative path returned node %s\n", np->full_name);
 	of_node_put(np);
+
+	np = of_find_node_opts_by_path("/testcase-data:testoption", &options);
+	selftest(np && !strcmp("testoption", options),
+		 "option path test failed\n");
+	of_node_put(np);
+
+	np = of_find_node_opts_by_path("testcase-alias:testaliasoption",
+				       &options);
+	selftest(np && !strcmp("testaliasoption", options),
+		 "option alias path test failed\n");
+	of_node_put(np);
 }
 
 static void __init of_selftest_dynamic(void)
diff --git a/include/linux/of.h b/include/linux/of.h
index 29f0adc..a36be70 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -228,7 +228,13 @@ extern struct device_node *of_find_matching_node_and_match(
 	const struct of_device_id *matches,
 	const struct of_device_id **match);
 
-extern struct device_node *of_find_node_by_path(const char *path);
+extern struct device_node *of_find_node_opts_by_path(const char *path,
+	const char **opts);
+static inline struct device_node *of_find_node_by_path(const char *path)
+{
+	return of_find_node_opts_by_path(path, NULL);
+}
+
 extern struct device_node *of_find_node_by_phandle(phandle handle);
 extern struct device_node *of_get_parent(const struct device_node *node);
 extern struct device_node *of_get_next_parent(struct device_node *node);
@@ -385,6 +391,12 @@ static inline struct device_node *of_find_node_by_path(const char *path)
 	return NULL;
 }
 
+static inline struct device_node *of_find_node_opts_by_path(const char *path,
+	const char **opts)
+{
+	return NULL;
+}
+
 static inline struct device_node *of_get_parent(const struct device_node *node)
 {
 	return NULL;
-- 
1.7.10.4

Re: [PATCH v3 2/3] of: add optional options parameter to of_find_node_by_path()

[Grant Likely]

On Thu, 27 Nov 2014 17:56:06 +0000
, Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
 wrote:
> Update of_find_node_by_path():
> 1) Rename function to of_find_node_opts_by_path(), adding an optional
>    pointer argument. Provide a static inline wrapper version of
>    of_find_node_by_path() which calls the new function with NULL as
>    the optional argument.
> 2) Ignore any part of the path beyond and including the ':' separator.
> 3) Set the new provided pointer argument to the beginning of the string
>    following the ':' separator.
> 4: Add tests.
> 
> Signed-off-by: Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/of/base.c     |   21 +++++++++++++++++----
>  drivers/of/selftest.c |   12 ++++++++++++
>  include/linux/of.h    |   14 +++++++++++++-
>  3 files changed, 42 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 3823edf..7f0e5f7 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -699,10 +699,15 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
>  {
>  	struct device_node *child;
>  	int len = strchrnul(path, '/') - path;
> +	int term;
>  
>  	if (!len)
>  		return NULL;
>  
> +	term = strchrnul(path, ':') - path;
> +	if (term < len)
> +		len = term;
> +
>  	__for_each_child_of_node(parent, child) {
>  		const char *name = strrchr(child->full_name, '/');
>  		if (WARN(!name, "malformed device_node %s\n", child->full_name))
> @@ -715,11 +720,14 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
>  }
>  
>  /**
> - *	of_find_node_by_path - Find a node matching a full OF path
> + *	of_find_node_opts_by_path - Find a node matching a full OF path
>   *	@path: Either the full path to match, or if the path does not
>   *	       start with '/', the name of a property of the /aliases
>   *	       node (an alias).  In the case of an alias, the node
>   *	       matching the alias' value will be returned.
> + *	@opts: Address of a pointer into which to store the start of
> + *	       an options string appended to the end of the path with
> + *	       a ':' separator.
>   *
>   *	Valid paths:
>   *		/foo/bar	Full path
> @@ -729,19 +737,24 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
>   *	Returns a node pointer with refcount incremented, use
>   *	of_node_put() on it when done.
>   */
> -struct device_node *of_find_node_by_path(const char *path)
> +struct device_node *of_find_node_opts_by_path(const char *path, const char **opts)
>  {
>  	struct device_node *np = NULL;
>  	struct property *pp;
>  	unsigned long flags;
> +	char *separator;
>  
>  	if (strcmp(path, "/") == 0)
>  		return of_node_get(of_allnodes);
>  
> +	separator = strchr(path, ':');
> +	if (separator && opts)
> +		*opts = separator + 1;
> +

What about when there are no opts? Do we require the caller to make sure
opts is NULL before calling the function (which sounds like a good
source of bugs) or do we clear it on successful return?

I think if opts is passed in, but there are no options, then it should
set *opts = NULL.

There should be test cases for this also. Must set opts to NULL on
successful return, and (I think) should leave opts alone on an
unsuccessful search.

Otherwise the patch looks good. If it wasn't for the above ambiguity I
would merge it right now.

g.

>  	/* The path could begin with an alias */
>  	if (*path != '/') {
>  		char *p = strchrnul(path, '/');
> -		int len = p - path;
> +		int len = separator ? separator - path : p - path;
>  
>  		/* of_aliases must not be NULL */
>  		if (!of_aliases)
> @@ -770,7 +783,7 @@ struct device_node *of_find_node_by_path(const char *path)
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  	return np;
>  }
> -EXPORT_SYMBOL(of_find_node_by_path);
> +EXPORT_SYMBOL(of_find_node_opts_by_path);
>  
>  /**
>   *	of_find_node_by_name - Find a node by its "name" property
> diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
> index e2d79af..c298065 100644
> --- a/drivers/of/selftest.c
> +++ b/drivers/of/selftest.c
> @@ -43,6 +43,7 @@ static bool selftest_live_tree;
>  static void __init of_selftest_find_node_by_name(void)
>  {
>  	struct device_node *np;
> +	const char *options;
>  
>  	np = of_find_node_by_path("/testcase-data");
>  	selftest(np && !strcmp("/testcase-data", np->full_name),
> @@ -83,6 +84,17 @@ static void __init of_selftest_find_node_by_name(void)
>  	np = of_find_node_by_path("testcase-alias/missing-path");
>  	selftest(!np, "non-existent alias with relative path returned node %s\n", np->full_name);
>  	of_node_put(np);
> +
> +	np = of_find_node_opts_by_path("/testcase-data:testoption", &options);
> +	selftest(np && !strcmp("testoption", options),
> +		 "option path test failed\n");
> +	of_node_put(np);
> +
> +	np = of_find_node_opts_by_path("testcase-alias:testaliasoption",
> +				       &options);
> +	selftest(np && !strcmp("testaliasoption", options),
> +		 "option alias path test failed\n");
> +	of_node_put(np);
>  }
>  
>  static void __init of_selftest_dynamic(void)
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 29f0adc..a36be70 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -228,7 +228,13 @@ extern struct device_node *of_find_matching_node_and_match(
>  	const struct of_device_id *matches,
>  	const struct of_device_id **match);
>  
> -extern struct device_node *of_find_node_by_path(const char *path);
> +extern struct device_node *of_find_node_opts_by_path(const char *path,
> +	const char **opts);
> +static inline struct device_node *of_find_node_by_path(const char *path)
> +{
> +	return of_find_node_opts_by_path(path, NULL);
> +}
> +
>  extern struct device_node *of_find_node_by_phandle(phandle handle);
>  extern struct device_node *of_get_parent(const struct device_node *node);
>  extern struct device_node *of_get_next_parent(struct device_node *node);
> @@ -385,6 +391,12 @@ static inline struct device_node *of_find_node_by_path(const char *path)
>  	return NULL;
>  }
>  
> +static inline struct device_node *of_find_node_opts_by_path(const char *path,
> +	const char **opts)
> +{
> +	return NULL;
> +}
> +
>  static inline struct device_node *of_get_parent(const struct device_node *node)
>  {
>  	return NULL;
> -- 
> 1.7.10.4
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] of: add optional options parameter to of_find_node_by_path()

[Leif Lindholm]

On Fri, Nov 28, 2014 at 12:44:03AM +0000, Grant Likely wrote:
> On Thu, 27 Nov 2014 17:56:06 +0000
> , Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>  wrote:
> > Update of_find_node_by_path():
> > 1) Rename function to of_find_node_opts_by_path(), adding an optional
> >    pointer argument. Provide a static inline wrapper version of
> >    of_find_node_by_path() which calls the new function with NULL as
> >    the optional argument.
> > 2) Ignore any part of the path beyond and including the ':' separator.
> > 3) Set the new provided pointer argument to the beginning of the string
> >    following the ':' separator.
> > 4: Add tests.
> > 
> > Signed-off-by: Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > ---
> >  drivers/of/base.c     |   21 +++++++++++++++++----
> >  drivers/of/selftest.c |   12 ++++++++++++
> >  include/linux/of.h    |   14 +++++++++++++-
> >  3 files changed, 42 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index 3823edf..7f0e5f7 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -699,10 +699,15 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
> >  {
> >  	struct device_node *child;
> >  	int len = strchrnul(path, '/') - path;
> > +	int term;
> >  
> >  	if (!len)
> >  		return NULL;
> >  
> > +	term = strchrnul(path, ':') - path;
> > +	if (term < len)
> > +		len = term;
> > +
> >  	__for_each_child_of_node(parent, child) {
> >  		const char *name = strrchr(child->full_name, '/');
> >  		if (WARN(!name, "malformed device_node %s\n", child->full_name))
> > @@ -715,11 +720,14 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
> >  }
> >  
> >  /**
> > - *	of_find_node_by_path - Find a node matching a full OF path
> > + *	of_find_node_opts_by_path - Find a node matching a full OF path
> >   *	@path: Either the full path to match, or if the path does not
> >   *	       start with '/', the name of a property of the /aliases
> >   *	       node (an alias).  In the case of an alias, the node
> >   *	       matching the alias' value will be returned.
> > + *	@opts: Address of a pointer into which to store the start of
> > + *	       an options string appended to the end of the path with
> > + *	       a ':' separator.
> >   *
> >   *	Valid paths:
> >   *		/foo/bar	Full path
> > @@ -729,19 +737,24 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
> >   *	Returns a node pointer with refcount incremented, use
> >   *	of_node_put() on it when done.
> >   */
> > -struct device_node *of_find_node_by_path(const char *path)
> > +struct device_node *of_find_node_opts_by_path(const char *path, const char **opts)
> >  {
> >  	struct device_node *np = NULL;
> >  	struct property *pp;
> >  	unsigned long flags;
> > +	char *separator;
> >  
> >  	if (strcmp(path, "/") == 0)
> >  		return of_node_get(of_allnodes);
> >  
> > +	separator = strchr(path, ':');
> > +	if (separator && opts)
> > +		*opts = separator + 1;
> > +
> 
> What about when there are no opts? Do we require the caller to make sure
> opts is NULL before calling the function (which sounds like a good
> source of bugs) or do we clear it on successful return?
> 
> I think if opts is passed in, but there are no options, then it should
> set *opts = NULL.

Yeah, oops.

> There should be test cases for this also. Must set opts to NULL on
> successful return, and (I think) should leave opts alone on an
> unsuccessful search.

I would actually argue for always nuking the opts - since that could
(theoretically) prevent something working by accident in spite of
error conditions.

How about the below?

/
    Leif

>From 2e1a44e539967d96366d074ae158092900e0c822 Mon Sep 17 00:00:00 2001
From: Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Date: Thu, 27 Nov 2014 09:24:31 +0000
Subject: [PATCH] of: add optional options parameter to of_find_node_by_path()

Update of_find_node_by_path():
1) Rename function to of_find_node_opts_by_path(), adding an optional
   pointer argument. Provide a static inline wrapper version of
   of_find_node_by_path() which calls the new function with NULL as
   the optional argument.
2) Ignore any part of the path beyond and including the ':' separator.
3) Set the new provided pointer argument to the beginning of the string
   following the ':' separator.
4: Add tests.

Signed-off-by: Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/of/base.c     |   21 +++++++++++++++++----
 drivers/of/selftest.c |   17 +++++++++++++++++
 include/linux/of.h    |   14 +++++++++++++-
 3 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 3823edf..99f0fd9 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -699,10 +699,15 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
 {
 	struct device_node *child;
 	int len = strchrnul(path, '/') - path;
+	int term;
 
 	if (!len)
 		return NULL;
 
+	term = strchrnul(path, ':') - path;
+	if (term < len)
+		len = term;
+
 	__for_each_child_of_node(parent, child) {
 		const char *name = strrchr(child->full_name, '/');
 		if (WARN(!name, "malformed device_node %s\n", child->full_name))
@@ -715,11 +720,14 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
 }
 
 /**
- *	of_find_node_by_path - Find a node matching a full OF path
+ *	of_find_node_opts_by_path - Find a node matching a full OF path
  *	@path: Either the full path to match, or if the path does not
  *	       start with '/', the name of a property of the /aliases
  *	       node (an alias).  In the case of an alias, the node
  *	       matching the alias' value will be returned.
+ *	@opts: Address of a pointer into which to store the start of
+ *	       an options string appended to the end of the path with
+ *	       a ':' separator.
  *
  *	Valid paths:
  *		/foo/bar	Full path
@@ -729,19 +737,24 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
  *	Returns a node pointer with refcount incremented, use
  *	of_node_put() on it when done.
  */
-struct device_node *of_find_node_by_path(const char *path)
+struct device_node *of_find_node_opts_by_path(const char *path, const char **opts)
 {
 	struct device_node *np = NULL;
 	struct property *pp;
 	unsigned long flags;
+	char *separator;
 
 	if (strcmp(path, "/") == 0)
 		return of_node_get(of_allnodes);
 
+	separator = strchr(path, ':');
+	if (opts)
+		*opts = separator ? separator + 1 : NULL;
+
 	/* The path could begin with an alias */
 	if (*path != '/') {
 		char *p = strchrnul(path, '/');
-		int len = p - path;
+		int len = separator ? separator - path : p - path;
 
 		/* of_aliases must not be NULL */
 		if (!of_aliases)
@@ -770,7 +783,7 @@ struct device_node *of_find_node_by_path(const char *path)
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	return np;
 }
-EXPORT_SYMBOL(of_find_node_by_path);
+EXPORT_SYMBOL(of_find_node_opts_by_path);
 
 /**
  *	of_find_node_by_name - Find a node by its "name" property
diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
index e2d79af..721b2a4 100644
--- a/drivers/of/selftest.c
+++ b/drivers/of/selftest.c
@@ -43,6 +43,7 @@ static bool selftest_live_tree;
 static void __init of_selftest_find_node_by_name(void)
 {
 	struct device_node *np;
+	const char *options;
 
 	np = of_find_node_by_path("/testcase-data");
 	selftest(np && !strcmp("/testcase-data", np->full_name),
@@ -83,6 +84,22 @@ static void __init of_selftest_find_node_by_name(void)
 	np = of_find_node_by_path("testcase-alias/missing-path");
 	selftest(!np, "non-existent alias with relative path returned node %s\n", np->full_name);
 	of_node_put(np);
+
+	np = of_find_node_opts_by_path("/testcase-data:testoption", &options);
+	selftest(np && !strcmp("testoption", options),
+		 "option path test failed\n");
+	of_node_put(np);
+
+	np = of_find_node_opts_by_path("testcase-alias:testaliasoption",
+				       &options);
+	selftest(np && !strcmp("testaliasoption", options),
+		 "option alias path test failed\n");
+	of_node_put(np);
+
+	options = "testoption";
+	np = of_find_node_opts_by_path("testcase-alias", &options);
+	selftest(np && !options, "option clearing test failed\n");
+	of_node_put(np);
 }
 
 static void __init of_selftest_dynamic(void)
diff --git a/include/linux/of.h b/include/linux/of.h
index 29f0adc..a36be70 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -228,7 +228,13 @@ extern struct device_node *of_find_matching_node_and_match(
 	const struct of_device_id *matches,
 	const struct of_device_id **match);
 
-extern struct device_node *of_find_node_by_path(const char *path);
+extern struct device_node *of_find_node_opts_by_path(const char *path,
+	const char **opts);
+static inline struct device_node *of_find_node_by_path(const char *path)
+{
+	return of_find_node_opts_by_path(path, NULL);
+}
+
 extern struct device_node *of_find_node_by_phandle(phandle handle);
 extern struct device_node *of_get_parent(const struct device_node *node);
 extern struct device_node *of_get_next_parent(struct device_node *node);
@@ -385,6 +391,12 @@ static inline struct device_node *of_find_node_by_path(const char *path)
 	return NULL;
 }
 
+static inline struct device_node *of_find_node_opts_by_path(const char *path,
+	const char **opts)
+{
+	return NULL;
+}
+
 static inline struct device_node *of_get_parent(const struct device_node *node)
 {
 	return NULL;
-- 
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] of: add optional options parameter to of_find_node_by_path()

[Grant Likely]

On Fri, 28 Nov 2014 11:34:28 +0000
, Leif Lindholm <leif.lindholm@linaro.org>
 wrote:
> On Fri, Nov 28, 2014 at 12:44:03AM +0000, Grant Likely wrote:
> > > +	separator = strchr(path, ':');
> > > +	if (separator && opts)
> > > +		*opts = separator + 1;
> > > +
> > 
> > What about when there are no opts? Do we require the caller to make sure
> > opts is NULL before calling the function (which sounds like a good
> > source of bugs) or do we clear it on successful return?
> > 
> > I think if opts is passed in, but there are no options, then it should
> > set *opts = NULL.
> 
> Yeah, oops.
> 
> > There should be test cases for this also. Must set opts to NULL on
> > successful return, and (I think) should leave opts alone on an
> > unsuccessful search.
> 
> I would actually argue for always nuking the opts - since that could
> (theoretically) prevent something working by accident in spite of
> error conditions.
> 
> How about the below?

Perfect, applied with one fixup below...

> 
> /
>     Leif
> 
> From 2e1a44e539967d96366d074ae158092900e0c822 Mon Sep 17 00:00:00 2001
> From: Leif Lindholm <leif.lindholm@linaro.org>
> Date: Thu, 27 Nov 2014 09:24:31 +0000
> Subject: [PATCH] of: add optional options parameter to of_find_node_by_path()
> 
> Update of_find_node_by_path():
> 1) Rename function to of_find_node_opts_by_path(), adding an optional
>    pointer argument. Provide a static inline wrapper version of
>    of_find_node_by_path() which calls the new function with NULL as
>    the optional argument.
> 2) Ignore any part of the path beyond and including the ':' separator.
> 3) Set the new provided pointer argument to the beginning of the string
>    following the ':' separator.
> 4: Add tests.
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> @@ -729,19 +737,24 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
>   *	Returns a node pointer with refcount incremented, use
>   *	of_node_put() on it when done.
>   */
> -struct device_node *of_find_node_by_path(const char *path)
> +struct device_node *of_find_node_opts_by_path(const char *path, const char **opts)
>  {
>  	struct device_node *np = NULL;
>  	struct property *pp;
>  	unsigned long flags;
> +	char *separator;

const char *separator.

Thanks,
g.

Re: Re: [PATCH v3 2/3] of: add optional options parameter to of_find_node_by_path()

[Grant Likely]

On Fri, Nov 28, 2014 at 3:25 PM, Grant Likely <grant.likely@linaro.org> wrote:
> On Fri, 28 Nov 2014 11:34:28 +0000
> , Leif Lindholm <leif.lindholm@linaro.org>
>  wrote:
>> On Fri, Nov 28, 2014 at 12:44:03AM +0000, Grant Likely wrote:
>> > > + separator = strchr(path, ':');
>> > > + if (separator && opts)
>> > > +         *opts = separator + 1;
>> > > +
>> >
>> > What about when there are no opts? Do we require the caller to make sure
>> > opts is NULL before calling the function (which sounds like a good
>> > source of bugs) or do we clear it on successful return?
>> >
>> > I think if opts is passed in, but there are no options, then it should
>> > set *opts = NULL.
>>
>> Yeah, oops.
>>
>> > There should be test cases for this also. Must set opts to NULL on
>> > successful return, and (I think) should leave opts alone on an
>> > unsuccessful search.
>>
>> I would actually argue for always nuking the opts - since that could
>> (theoretically) prevent something working by accident in spite of
>> error conditions.
>>
>> How about the below?
>
> Perfect, applied with one fixup below...

And by the way, let me say well done on this patch. It is elegantly
implemented within the framework already there.

g.

Re: [PATCH v3 2/3] of: add optional options parameter to? of_find_node_by_path()

[Leif Lindholm]

On Fri, Nov 28, 2014 at 03:25:12PM +0000, Grant Likely wrote:
> On Fri, 28 Nov 2014 11:34:28 +0000
> , Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>  wrote:
> > On Fri, Nov 28, 2014 at 12:44:03AM +0000, Grant Likely wrote:
> > > > +	separator = strchr(path, ':');
> > > > +	if (separator && opts)
> > > > +		*opts = separator + 1;
> > > > +
> > > 
> > > What about when there are no opts? Do we require the caller to make sure
> > > opts is NULL before calling the function (which sounds like a good
> > > source of bugs) or do we clear it on successful return?
> > > 
> > > I think if opts is passed in, but there are no options, then it should
> > > set *opts = NULL.
> > 
> > Yeah, oops.
> > 
> > > There should be test cases for this also. Must set opts to NULL on
> > > successful return, and (I think) should leave opts alone on an
> > > unsuccessful search.
> > 
> > I would actually argue for always nuking the opts - since that could
> > (theoretically) prevent something working by accident in spite of
> > error conditions.
> > 
> > How about the below?
> 
> Perfect, applied with one fixup below...

Thanks!

And since it's Friday... *cough*

>From 5c469dad81961164c444da7d6c4e1c5b1c097aab Mon Sep 17 00:00:00 2001
From: Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Date: Fri, 28 Nov 2014 16:27:25 +0000
Subject: [PATCH] of: fix options clearing when path is "/"

The addition of the optional options extraction on
of_find_node_opts_by_path failed to clear incoming options pointer
if the specified path was "/".

Resolve this case, and add a test to detect it.

Signed-off-by: Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/of/base.c     |   11 ++++++-----
 drivers/of/selftest.c |    5 +++++
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 5e16c6b..32664d1 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -743,15 +743,16 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
 	struct device_node *np = NULL;
 	struct property *pp;
 	unsigned long flags;
-	char *separator;
+	char *separator = NULL;
+
+	if (opts) {
+		separator = strchr(path, ':');
+		*opts = separator ? separator + 1 : NULL;
+	}
 
 	if (strcmp(path, "/") == 0)
 		return of_node_get(of_allnodes);
 
-	separator = strchr(path, ':');
-	if (opts)
-		*opts = separator ? separator + 1 : NULL;
-
 	/* The path could begin with an alias */
 	if (*path != '/') {
 		char *p = strchrnul(path, '/');
diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
index 721b2a4..914f0ae 100644
--- a/drivers/of/selftest.c
+++ b/drivers/of/selftest.c
@@ -100,6 +100,11 @@ static void __init of_selftest_find_node_by_name(void)
 	np = of_find_node_opts_by_path("testcase-alias", &options);
 	selftest(np && !options, "option clearing test failed\n");
 	of_node_put(np);
+
+	options = "testoption";
+	np = of_find_node_opts_by_path("/", &options);
+	selftest(np && !options, "option clearing root node test failed\n");
+	of_node_put(np);
 }
 
 static void __init of_selftest_dynamic(void)
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] of: add optional options parameter to? of_find_node_by_path()

[Grant Likely]

On Fri, Nov 28, 2014 at 4:38 PM, Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Fri, Nov 28, 2014 at 03:25:12PM +0000, Grant Likely wrote:
>> On Fri, 28 Nov 2014 11:34:28 +0000
>> , Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>  wrote:
>> > On Fri, Nov 28, 2014 at 12:44:03AM +0000, Grant Likely wrote:
>> > > > +       separator = strchr(path, ':');
>> > > > +       if (separator && opts)
>> > > > +               *opts = separator + 1;
>> > > > +
>> > >
>> > > What about when there are no opts? Do we require the caller to make sure
>> > > opts is NULL before calling the function (which sounds like a good
>> > > source of bugs) or do we clear it on successful return?
>> > >
>> > > I think if opts is passed in, but there are no options, then it should
>> > > set *opts = NULL.
>> >
>> > Yeah, oops.
>> >
>> > > There should be test cases for this also. Must set opts to NULL on
>> > > successful return, and (I think) should leave opts alone on an
>> > > unsuccessful search.
>> >
>> > I would actually argue for always nuking the opts - since that could
>> > (theoretically) prevent something working by accident in spite of
>> > error conditions.
>> >
>> > How about the below?
>>
>> Perfect, applied with one fixup below...
>
> Thanks!

Fixups merged. Thanks.

g.

>
> And since it's Friday... *cough*
>
> From 5c469dad81961164c444da7d6c4e1c5b1c097aab Mon Sep 17 00:00:00 2001
> From: Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Date: Fri, 28 Nov 2014 16:27:25 +0000
> Subject: [PATCH] of: fix options clearing when path is "/"
>
> The addition of the optional options extraction on
> of_find_node_opts_by_path failed to clear incoming options pointer
> if the specified path was "/".
>
> Resolve this case, and add a test to detect it.
>
> Signed-off-by: Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/of/base.c     |   11 ++++++-----
>  drivers/of/selftest.c |    5 +++++
>  2 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 5e16c6b..32664d1 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -743,15 +743,16 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
>         struct device_node *np = NULL;
>         struct property *pp;
>         unsigned long flags;
> -       char *separator;
> +       char *separator = NULL;
> +
> +       if (opts) {
> +               separator = strchr(path, ':');
> +               *opts = separator ? separator + 1 : NULL;
> +       }
>
>         if (strcmp(path, "/") == 0)
>                 return of_node_get(of_allnodes);
>
> -       separator = strchr(path, ':');
> -       if (opts)
> -               *opts = separator ? separator + 1 : NULL;
> -
>         /* The path could begin with an alias */
>         if (*path != '/') {
>                 char *p = strchrnul(path, '/');
> diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
> index 721b2a4..914f0ae 100644
> --- a/drivers/of/selftest.c
> +++ b/drivers/of/selftest.c
> @@ -100,6 +100,11 @@ static void __init of_selftest_find_node_by_name(void)
>         np = of_find_node_opts_by_path("testcase-alias", &options);
>         selftest(np && !options, "option clearing test failed\n");
>         of_node_put(np);
> +
> +       options = "testoption";
> +       np = of_find_node_opts_by_path("/", &options);
> +       selftest(np && !options, "option clearing root node test failed\n");
> +       of_node_put(np);
>  }
>
>  static void __init of_selftest_dynamic(void)
> --
> 1.7.10.4
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] of: add optional options parameter to of_find_node_by_path()

[Peter Hurley]

Hi Leif,

On 11/27/2014 12:56 PM, Leif Lindholm wrote:
> Update of_find_node_by_path():
> 1) Rename function to of_find_node_opts_by_path(), adding an optional
>    pointer argument. Provide a static inline wrapper version of
>    of_find_node_by_path() which calls the new function with NULL as
>    the optional argument.
> 2) Ignore any part of the path beyond and including the ':' separator.
> 3) Set the new provided pointer argument to the beginning of the string
>    following the ':' separator.
> 4: Add tests.
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  drivers/of/base.c     |   21 +++++++++++++++++----
>  drivers/of/selftest.c |   12 ++++++++++++
>  include/linux/of.h    |   14 +++++++++++++-
>  3 files changed, 42 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 3823edf..7f0e5f7 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -699,10 +699,15 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
>  {
>  	struct device_node *child;
>  	int len = strchrnul(path, '/') - path;
> +	int term;
>  
>  	if (!len)
>  		return NULL;
>  
> +	term = strchrnul(path, ':') - path;
> +	if (term < len)
> +		len = term;
> +
>  	__for_each_child_of_node(parent, child) {
>  		const char *name = strrchr(child->full_name, '/');
>  		if (WARN(!name, "malformed device_node %s\n", child->full_name))
> @@ -715,11 +720,14 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
>  }
>  
>  /**
> - *	of_find_node_by_path - Find a node matching a full OF path
> + *	of_find_node_opts_by_path - Find a node matching a full OF path
>   *	@path: Either the full path to match, or if the path does not
>   *	       start with '/', the name of a property of the /aliases
>   *	       node (an alias).  In the case of an alias, the node
>   *	       matching the alias' value will be returned.
> + *	@opts: Address of a pointer into which to store the start of
> + *	       an options string appended to the end of the path with
> + *	       a ':' separator.
>   *
>   *	Valid paths:
>   *		/foo/bar	Full path
> @@ -729,19 +737,24 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
>   *	Returns a node pointer with refcount incremented, use
>   *	of_node_put() on it when done.
>   */
> -struct device_node *of_find_node_by_path(const char *path)
> +struct device_node *of_find_node_opts_by_path(const char *path, const char **opts)
>  {
>  	struct device_node *np = NULL;
>  	struct property *pp;
>  	unsigned long flags;
> +	char *separator;
>  
>  	if (strcmp(path, "/") == 0)
>  		return of_node_get(of_allnodes);
>  
> +	separator = strchr(path, ':');
> +	if (separator && opts)
> +		*opts = separator + 1;
> +
>  	/* The path could begin with an alias */
>  	if (*path != '/') {
>  		char *p = strchrnul(path, '/');
> -		int len = p - path;
> +		int len = separator ? separator - path : p - path;
>  
>  		/* of_aliases must not be NULL */
>  		if (!of_aliases)
> @@ -770,7 +783,7 @@ struct device_node *of_find_node_by_path(const char *path)
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  	return np;
>  }
> -EXPORT_SYMBOL(of_find_node_by_path);
> +EXPORT_SYMBOL(of_find_node_opts_by_path);
>  
>  /**
>   *	of_find_node_by_name - Find a node by its "name" property
> diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
> index e2d79af..c298065 100644
> --- a/drivers/of/selftest.c
> +++ b/drivers/of/selftest.c
> @@ -43,6 +43,7 @@ static bool selftest_live_tree;
>  static void __init of_selftest_find_node_by_name(void)
>  {
>  	struct device_node *np;
> +	const char *options;
>  
>  	np = of_find_node_by_path("/testcase-data");
>  	selftest(np && !strcmp("/testcase-data", np->full_name),
> @@ -83,6 +84,17 @@ static void __init of_selftest_find_node_by_name(void)
>  	np = of_find_node_by_path("testcase-alias/missing-path");
>  	selftest(!np, "non-existent alias with relative path returned node %s\n", np->full_name);
>  	of_node_put(np);
> +
> +	np = of_find_node_opts_by_path("/testcase-data:testoption", &options);
> +	selftest(np && !strcmp("testoption", options),
> +		 "option path test failed\n");
> +	of_node_put(np);
> +
> +	np = of_find_node_opts_by_path("testcase-alias:testaliasoption",
> +				       &options);
> +	selftest(np && !strcmp("testaliasoption", options),
> +		 "option alias path test failed\n");
> +	of_node_put(np);
>  }

The path parsing gets lost if the string after ':' contains '/'.

The selftests below fail with:
[    1.365528] ### dt-test ### FAIL of_selftest_find_node_by_name():99 option path test failed
[    1.365610] ### dt-test ### FAIL of_selftest_find_node_by_name():115 option alias path test failed

Regards,
Peter Hurley


--- >% ---
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 41a4a13..07ba5aa 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -94,6 +94,11 @@ static void __init of_selftest_find_node_by_name(void)
 		 "option path test failed\n");
 	of_node_put(np);
 
+	np = of_find_node_opts_by_path("/testcase-data:test/option", &options);
+	selftest(np && !strcmp("test/option", options),
+		 "option path test failed\n");
+	of_node_put(np);
+
 	np = of_find_node_opts_by_path("/testcase-data:testoption", NULL);
 	selftest(np, "NULL option path test failed\n");
 	of_node_put(np);
@@ -104,6 +109,12 @@ static void __init of_selftest_find_node_by_name(void)
 		 "option alias path test failed\n");
 	of_node_put(np);
 
+	np = of_find_node_opts_by_path("testcase-alias:test/alias/option",
+				       &options);
+	selftest(np && !strcmp("test/alias/option", options),
+		 "option alias path test failed\n");
+	of_node_put(np);
+
 	np = of_find_node_opts_by_path("testcase-alias:testaliasoption", NULL);
 	selftest(np, "NULL option alias path test failed\n");
 	of_node_put(np);



>  static void __init of_selftest_dynamic(void)
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 29f0adc..a36be70 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -228,7 +228,13 @@ extern struct device_node *of_find_matching_node_and_match(
>  	const struct of_device_id *matches,
>  	const struct of_device_id **match);
>  
> -extern struct device_node *of_find_node_by_path(const char *path);
> +extern struct device_node *of_find_node_opts_by_path(const char *path,
> +	const char **opts);
> +static inline struct device_node *of_find_node_by_path(const char *path)
> +{
> +	return of_find_node_opts_by_path(path, NULL);
> +}
> +
>  extern struct device_node *of_find_node_by_phandle(phandle handle);
>  extern struct device_node *of_get_parent(const struct device_node *node);
>  extern struct device_node *of_get_next_parent(struct device_node *node);
> @@ -385,6 +391,12 @@ static inline struct device_node *of_find_node_by_path(const char *path)
>  	return NULL;
>  }
>  
> +static inline struct device_node *of_find_node_opts_by_path(const char *path,
> +	const char **opts)
> +{
> +	return NULL;
> +}
> +
>  static inline struct device_node *of_get_parent(const struct device_node *node)
>  {
>  	return NULL;
> 

Re: [PATCH v3 2/3] of: add optional options parameter to of_find_node_by_path()

[Leif Lindholm]

Hi Peter,

On Wed, Mar 04, 2015 at 10:45:02AM -0500, Peter Hurley wrote:
> The path parsing gets lost if the string after ':' contains '/'.

Doh!
Thanks for reporting (and sorry for slow response).

> The selftests below fail with:
> [    1.365528] ### dt-test ### FAIL of_selftest_find_node_by_name():99 option path test failed
> [    1.365610] ### dt-test ### FAIL of_selftest_find_node_by_name():115 option alias path test failed
> 
> Regards,
> Peter Hurley
> 
> 
> --- >% ---
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index 41a4a13..07ba5aa 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -94,6 +94,11 @@ static void __init of_selftest_find_node_by_name(void)
>  		 "option path test failed\n");
>  	of_node_put(np);
>  
> +	np = of_find_node_opts_by_path("/testcase-data:test/option", &options);
> +	selftest(np && !strcmp("test/option", options),
> +		 "option path test failed\n");
> +	of_node_put(np);
> +
>  	np = of_find_node_opts_by_path("/testcase-data:testoption", NULL);
>  	selftest(np, "NULL option path test failed\n");
>  	of_node_put(np);
> @@ -104,6 +109,12 @@ static void __init of_selftest_find_node_by_name(void)
>  		 "option alias path test failed\n");
>  	of_node_put(np);
>  
> +	np = of_find_node_opts_by_path("testcase-alias:test/alias/option",
> +				       &options);
> +	selftest(np && !strcmp("test/alias/option", options),
> +		 "option alias path test failed\n");
> +	of_node_put(np);
> +
>  	np = of_find_node_opts_by_path("testcase-alias:testaliasoption", NULL);
>  	selftest(np, "NULL option alias path test failed\n");
>  	of_node_put(np);

Could you give the below a spin, and if it works for you, send me the
above tests as a full patch so that I can post both as a series?

Regards,

Leif

>From bf4ab0b2e33902ba88809a3c4a2cdf07efd02dde Mon Sep 17 00:00:00 2001
From: Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Date: Fri, 6 Mar 2015 16:38:54 +0000
Subject: [PATCH] of: fix handling of '/' in options for of_find_node_by_path()

Ensure proper handling of paths with appended options (after ':'),
where those options may contain a '/'.

Fixes: 7914a7c5651a ("of: support passing console options with stdout-path")
Reported-by: Peter Hurley <peter-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org>
Signed-off-by: Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/of/base.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 0a8aeb8..8b904e5 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -714,16 +714,17 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
 						const char *path)
 {
 	struct device_node *child;
-	int len = strchrnul(path, '/') - path;
-	int term;
+	int len;
+	const char *end;
 
+	end = strchr(path, ':');
+	if (!end)
+		end = strchrnul(path, '/');
+
+	len = end - path;
 	if (!len)
 		return NULL;
 
-	term = strchrnul(path, ':') - path;
-	if (term < len)
-		len = term;
-
 	__for_each_child_of_node(parent, child) {
 		const char *name = strrchr(child->full_name, '/');
 		if (WARN(!name, "malformed device_node %s\n", child->full_name))
@@ -768,8 +769,12 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
 
 	/* The path could begin with an alias */
 	if (*path != '/') {
-		char *p = strchrnul(path, '/');
-		int len = separator ? separator - path : p - path;
+		int len;
+		const char *p = separator;
+
+		if (!p)
+			p = strchrnul(path, '/');
+		len = p - path;
 
 		/* of_aliases must not be NULL */
 		if (!of_aliases)
@@ -794,6 +799,8 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
 		path++; /* Increment past '/' delimiter */
 		np = __of_find_node_by_path(np, path);
 		path = strchrnul(path, '/');
+		if (separator && separator < path)
+			break;
 	}
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	return np;
-- 
2.1.4


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] of: add optional options parameter to of_find_node_by_path()

[Peter Hurley]

On 03/06/2015 11:52 AM, Leif Lindholm wrote:
> Hi Peter,
> 
> On Wed, Mar 04, 2015 at 10:45:02AM -0500, Peter Hurley wrote:
>> The path parsing gets lost if the string after ':' contains '/'.
> 
> Doh!

Hardly.

I only noticed because I had to implement the corresponding algorithm
for earlycon and FDT, where the string scanning is obvious.

> Thanks for reporting (and sorry for slow response).

No worries :)

Rather, I'd like to thank you for implementing the options string so
that bootloader -> earlycon -> console works so seamlessly now.
Can't wait to see 3000000Mb/s console from boot.

And thanks for the quick patch. I'm still testing because, while there weren't
those failures, there were some other messages. So I just need to go back
and see if those are regressions.


>> The selftests below fail with:
>> [    1.365528] ### dt-test ### FAIL of_selftest_find_node_by_name():99 option path test failed
>> [    1.365610] ### dt-test ### FAIL of_selftest_find_node_by_name():115 option alias path test failed
>>
>> Regards,
>> Peter Hurley
>>
>>
>> --- >% ---
>> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
>> index 41a4a13..07ba5aa 100644
>> --- a/drivers/of/unittest.c
>> +++ b/drivers/of/unittest.c
>> @@ -94,6 +94,11 @@ static void __init of_selftest_find_node_by_name(void)
>>  		 "option path test failed\n");
>>  	of_node_put(np);
>>  
>> +	np = of_find_node_opts_by_path("/testcase-data:test/option", &options);
>> +	selftest(np && !strcmp("test/option", options),
>> +		 "option path test failed\n");
>> +	of_node_put(np);
>> +
>>  	np = of_find_node_opts_by_path("/testcase-data:testoption", NULL);
>>  	selftest(np, "NULL option path test failed\n");
>>  	of_node_put(np);
>> @@ -104,6 +109,12 @@ static void __init of_selftest_find_node_by_name(void)
>>  		 "option alias path test failed\n");
>>  	of_node_put(np);
>>  
>> +	np = of_find_node_opts_by_path("testcase-alias:test/alias/option",
>> +				       &options);
>> +	selftest(np && !strcmp("test/alias/option", options),
>> +		 "option alias path test failed\n");
>> +	of_node_put(np);
>> +
>>  	np = of_find_node_opts_by_path("testcase-alias:testaliasoption", NULL);
>>  	selftest(np, "NULL option alias path test failed\n");
>>  	of_node_put(np);
> 
> Could you give the below a spin, and if it works for you, send me the
> above tests as a full patch so that I can post both as a series?

Will do as soon as I finish testing.

Regards,
Peter Hurley


> From bf4ab0b2e33902ba88809a3c4a2cdf07efd02dde Mon Sep 17 00:00:00 2001
> From: Leif Lindholm <leif.lindholm@linaro.org>
> Date: Fri, 6 Mar 2015 16:38:54 +0000
> Subject: [PATCH] of: fix handling of '/' in options for of_find_node_by_path()
> 
> Ensure proper handling of paths with appended options (after ':'),
> where those options may contain a '/'.
> 
> Fixes: 7914a7c5651a ("of: support passing console options with stdout-path")
> Reported-by: Peter Hurley <peter@hurleysoftware.com>
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  drivers/of/base.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 0a8aeb8..8b904e5 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -714,16 +714,17 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
>  						const char *path)
>  {
>  	struct device_node *child;
> -	int len = strchrnul(path, '/') - path;
> -	int term;
> +	int len;
> +	const char *end;
>  
> +	end = strchr(path, ':');
> +	if (!end)
> +		end = strchrnul(path, '/');
> +
> +	len = end - path;
>  	if (!len)
>  		return NULL;
>  
> -	term = strchrnul(path, ':') - path;
> -	if (term < len)
> -		len = term;
> -
>  	__for_each_child_of_node(parent, child) {
>  		const char *name = strrchr(child->full_name, '/');
>  		if (WARN(!name, "malformed device_node %s\n", child->full_name))
> @@ -768,8 +769,12 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
>  
>  	/* The path could begin with an alias */
>  	if (*path != '/') {
> -		char *p = strchrnul(path, '/');
> -		int len = separator ? separator - path : p - path;
> +		int len;
> +		const char *p = separator;
> +
> +		if (!p)
> +			p = strchrnul(path, '/');
> +		len = p - path;
>  
>  		/* of_aliases must not be NULL */
>  		if (!of_aliases)
> @@ -794,6 +799,8 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
>  		path++; /* Increment past '/' delimiter */
>  		np = __of_find_node_by_path(np, path);
>  		path = strchrnul(path, '/');
> +		if (separator && separator < path)
> +			break;
>  	}
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  	return np;
> 

Re: [PATCH v3 2/3] of: add optional options parameter to of_find_node_by_path()

[Peter Hurley]

On 03/06/2015 01:11 PM, Peter Hurley wrote:
> On 03/06/2015 11:52 AM, Leif Lindholm wrote:

[...]

>> Could you give the below a spin, and if it works for you, send me the
>> above tests as a full patch so that I can post both as a series?
> 
> Will do as soon as I finish testing.

All passed with your patch; patch for testcases below.

Regards,
Peter Hurley

--- >% ---
From: Peter Hurley <peter@hurleysoftware.com>
Subject: [PATCH] of: unittest: Add options string testcase variants

Add testcase variants with '/' in the options string to test for
scan beyond end path name terminated by ':'.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/of/unittest.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 0cf9a23..b2d7547 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -92,6 +92,11 @@ static void __init of_selftest_find_node_by_name(void)
 		 "option path test failed\n");
 	of_node_put(np);
 
+	np = of_find_node_opts_by_path("/testcase-data:test/option", &options);
+	selftest(np && !strcmp("test/option", options),
+		 "option path test, subcase #1 failed\n");
+	of_node_put(np);
+
 	np = of_find_node_opts_by_path("/testcase-data:testoption", NULL);
 	selftest(np, "NULL option path test failed\n");
 	of_node_put(np);
@@ -102,6 +107,12 @@ static void __init of_selftest_find_node_by_name(void)
 		 "option alias path test failed\n");
 	of_node_put(np);
 
+	np = of_find_node_opts_by_path("testcase-alias:test/alias/option",
+				       &options);
+	selftest(np && !strcmp("test/alias/option", options),
+		 "option alias path test, subcase #1 failed\n");
+	of_node_put(np);
+
 	np = of_find_node_opts_by_path("testcase-alias:testaliasoption", NULL);
 	selftest(np, "NULL option alias path test failed\n");
 	of_node_put(np);
-- 
2.3.1

Re: [PATCH v3 2/3] of: add optional options parameter to of_find_node_by_path()

[Rob Herring]

On Fri, Mar 6, 2015 at 12:59 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 03/06/2015 01:11 PM, Peter Hurley wrote:
>> On 03/06/2015 11:52 AM, Leif Lindholm wrote:
>
> [...]
>
>>> Could you give the below a spin, and if it works for you, send me the
>>> above tests as a full patch so that I can post both as a series?
>>
>> Will do as soon as I finish testing.
>
> All passed with your patch; patch for testcases below.

Applied both for 4.0. Thanks.

Rob

>
> Regards,
> Peter Hurley
>
> --- >% ---
> From: Peter Hurley <peter@hurleysoftware.com>
> Subject: [PATCH] of: unittest: Add options string testcase variants
>
> Add testcase variants with '/' in the options string to test for
> scan beyond end path name terminated by ':'.
>
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  drivers/of/unittest.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index 0cf9a23..b2d7547 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -92,6 +92,11 @@ static void __init of_selftest_find_node_by_name(void)
>                  "option path test failed\n");
>         of_node_put(np);
>
> +       np = of_find_node_opts_by_path("/testcase-data:test/option", &options);
> +       selftest(np && !strcmp("test/option", options),
> +                "option path test, subcase #1 failed\n");
> +       of_node_put(np);
> +
>         np = of_find_node_opts_by_path("/testcase-data:testoption", NULL);
>         selftest(np, "NULL option path test failed\n");
>         of_node_put(np);
> @@ -102,6 +107,12 @@ static void __init of_selftest_find_node_by_name(void)
>                  "option alias path test failed\n");
>         of_node_put(np);
>
> +       np = of_find_node_opts_by_path("testcase-alias:test/alias/option",
> +                                      &options);
> +       selftest(np && !strcmp("test/alias/option", options),
> +                "option alias path test, subcase #1 failed\n");
> +       of_node_put(np);
> +
>         np = of_find_node_opts_by_path("testcase-alias:testaliasoption", NULL);
>         selftest(np, "NULL option alias path test failed\n");
>         of_node_put(np);
> --
> 2.3.1
>
>

[PATCH v3 3/3] of: support passing console options with stdout-path

[Leif Lindholm]

Support specifying console options (like with console=ttyXN,<options>)
by appending them to the stdout-path property after a separating ':'.

Example:
        stdout-path = "uart0:115200";

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 drivers/of/base.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 7f0e5f7..6d2d45e 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -37,6 +37,7 @@ EXPORT_SYMBOL(of_allnodes);
 struct device_node *of_chosen;
 struct device_node *of_aliases;
 struct device_node *of_stdout;
+static const char *of_stdout_options;
 
 struct kset *of_kset;
 
@@ -1844,7 +1845,7 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
 		if (IS_ENABLED(CONFIG_PPC) && !name)
 			name = of_get_property(of_aliases, "stdout", NULL);
 		if (name)
-			of_stdout = of_find_node_by_path(name);
+			of_stdout = of_find_node_opts_by_path(name, &of_stdout_options);
 	}
 
 	if (!of_aliases)
@@ -1968,9 +1969,13 @@ EXPORT_SYMBOL_GPL(of_prop_next_string);
  */
 bool of_console_check(struct device_node *dn, char *name, int index)
 {
+	char *console_options;
+
 	if (!dn || dn != of_stdout || console_set_on_cmdline)
 		return false;
-	return !add_preferred_console(name, index, NULL);
+
+	console_options = kstrdup(of_stdout_options, GFP_KERNEL);
+	return !add_preferred_console(name, index, console_options);
 }
 EXPORT_SYMBOL_GPL(of_console_check);
 
-- 
1.7.10.4

Re: [PATCH v3 3/3] of: support passing console options with stdout-path

[Grant Likely]

On Thu, 27 Nov 2014 17:56:07 +0000
, Leif Lindholm <leif.lindholm@linaro.org>
 wrote:
> Support specifying console options (like with console=ttyXN,<options>)
> by appending them to the stdout-path property after a separating ':'.
> 
> Example:
>         stdout-path = "uart0:115200";
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>

Applied with a slight change below.

Thanks!

> ---
>  drivers/of/base.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 7f0e5f7..6d2d45e 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -37,6 +37,7 @@ EXPORT_SYMBOL(of_allnodes);
>  struct device_node *of_chosen;
>  struct device_node *of_aliases;
>  struct device_node *of_stdout;
> +static const char *of_stdout_options;
>  
>  struct kset *of_kset;
>  
> @@ -1844,7 +1845,7 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
>  		if (IS_ENABLED(CONFIG_PPC) && !name)
>  			name = of_get_property(of_aliases, "stdout", NULL);
>  		if (name)
> -			of_stdout = of_find_node_by_path(name);
> +			of_stdout = of_find_node_opts_by_path(name, &of_stdout_options);
>  	}
>  
>  	if (!of_aliases)
> @@ -1968,9 +1969,13 @@ EXPORT_SYMBOL_GPL(of_prop_next_string);
>   */
>  bool of_console_check(struct device_node *dn, char *name, int index)
>  {
> +	char *console_options;
> +
>  	if (!dn || dn != of_stdout || console_set_on_cmdline)
>  		return false;
> -	return !add_preferred_console(name, index, NULL);
> +
> +	console_options = kstrdup(of_stdout_options, GFP_KERNEL);
> +	return !add_preferred_console(name, index, console_options);

I changed this to simply:
	return !add_preferred_console(name, index,
				      kstrdup(of_stdout_options, GFP_KERNEL));

Not a big deal, just makes for a slightly shorter function.

g.

Re: [PATCH v3 3/3] of: support passing console options with stdout-path

[Peter Hurley]

On 11/27/2014 12:56 PM, Leif Lindholm wrote:
> Support specifying console options (like with console=ttyXN,<options>)
> by appending them to the stdout-path property after a separating ':'.
> 
> Example:
>         stdout-path = "uart0:115200";

This format breaks DT earlycon because libfdt doesn't recognize ':' as a
path terminator.

It's simple enough to fix this directly in early_init_dt_scan_chosen_serial()
but perhaps it would better to teach fdt_path_offset() to recognize ':'?

Regards,
Peter Hurley

> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  drivers/of/base.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 7f0e5f7..6d2d45e 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -37,6 +37,7 @@ EXPORT_SYMBOL(of_allnodes);
>  struct device_node *of_chosen;
>  struct device_node *of_aliases;
>  struct device_node *of_stdout;
> +static const char *of_stdout_options;
>  
>  struct kset *of_kset;
>  
> @@ -1844,7 +1845,7 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
>  		if (IS_ENABLED(CONFIG_PPC) && !name)
>  			name = of_get_property(of_aliases, "stdout", NULL);
>  		if (name)
> -			of_stdout = of_find_node_by_path(name);
> +			of_stdout = of_find_node_opts_by_path(name, &of_stdout_options);
>  	}
>  
>  	if (!of_aliases)
> @@ -1968,9 +1969,13 @@ EXPORT_SYMBOL_GPL(of_prop_next_string);
>   */
>  bool of_console_check(struct device_node *dn, char *name, int index)
>  {
> +	char *console_options;
> +
>  	if (!dn || dn != of_stdout || console_set_on_cmdline)
>  		return false;
> -	return !add_preferred_console(name, index, NULL);
> +
> +	console_options = kstrdup(of_stdout_options, GFP_KERNEL);
> +	return !add_preferred_console(name, index, console_options);
>  }
>  EXPORT_SYMBOL_GPL(of_console_check);
>  
> 

Re: [PATCH v3 3/3] of: support passing console options with stdout-path

[Andrew Lunn]

On Thu, Feb 26, 2015 at 06:55:22AM -0500, Peter Hurley wrote:
> On 11/27/2014 12:56 PM, Leif Lindholm wrote:
> > Support specifying console options (like with console=ttyXN,<options>)
> > by appending them to the stdout-path property after a separating ':'.
> > 
> > Example:
> >         stdout-path = "uart0:115200";
> 
> This format breaks DT earlycon because libfdt doesn't recognize ':' as a
> path terminator.
> 
> It's simple enough to fix this directly in early_init_dt_scan_chosen_serial()
> but perhaps it would better to teach fdt_path_offset() to recognize ':'?

Hi Peter

ePAPR says for stdout-path in Chosen:

A string that specifies the full path to the node representing the
device to be used for boot console output. If the character ":" is
present in the value it terminates the path. The value may be an
alias.

So it is probably wise to implement this properly.

   Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/3] of: support passing console options with stdout-path

[Peter Hurley]

Hi Andrew,

On 02/26/2015 08:46 AM, Andrew Lunn wrote:
> On Thu, Feb 26, 2015 at 06:55:22AM -0500, Peter Hurley wrote:
>> On 11/27/2014 12:56 PM, Leif Lindholm wrote:
>>> Support specifying console options (like with console=ttyXN,<options>)
>>> by appending them to the stdout-path property after a separating ':'.
>>>
>>> Example:
>>>         stdout-path = "uart0:115200";
>>
>> This format breaks DT earlycon because libfdt doesn't recognize ':' as a
>> path terminator.
>>
>> It's simple enough to fix this directly in early_init_dt_scan_chosen_serial()
>> but perhaps it would better to teach fdt_path_offset() to recognize ':'?
> 
> Hi Peter
> 
> ePAPR says for stdout-path in Chosen:
> 
> A string that specifies the full path to the node representing the
> device to be used for boot console output. If the character ":" is
> present in the value it terminates the path. The value may be an
> alias.
> 
> So it is probably wise to implement this properly.

Sorry if I was unclear. My question was not _whether_ to fix this
for earlycon, but rather _how_.

IOW, is the ':' character accepted as a path terminator for
1. all nodes, so fix this in fdt_path_offset();
2. only chosen nodes;
3. unique to stdout-path, so fix this in early_init_dt_scan_chosen_serial()?

Regards,
Peter Hurley

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/3] of: support passing console options with stdout-path

[Andrew Lunn]

> Sorry if I was unclear. My question was not _whether_ to fix this
> for earlycon, but rather _how_.
> 
> IOW, is the ':' character accepted as a path terminator for
> 1. all nodes, so fix this in fdt_path_offset();
> 2. only chosen nodes;
> 3. unique to stdout-path, so fix this in early_init_dt_scan_chosen_serial()?

Ah, sorry.

The ':' character is accepted as a path terminator for stdout-path and
stdin-path within chosen node. It does not appear to be mentioned
anywhere else in the standard.

	 Andrew