* [PATCH-RFC 6/7] drivers: of: ifdef out cmdline section
@ 2015-10-06 15:47 Daniel Walker
2015-10-06 17:14 ` Rob Herring
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Walker @ 2015-10-06 15:47 UTC (permalink / raw)
To: xe-kernel, Rob Herring, Frank Rowand, Grant Likely
Cc: Daniel Walker, devicetree, linux-kernel
It looks like there's some seepage of cmdline stuff into
the generic device tree code. This conflicts with the
generic cmdline implementation so I remove it in the case
when that's enabled.
Cc: xe-kernel@external.cisco.com
Cc: Daniel Walker <dwalker@fifo99.com>
Signed-off-by: Daniel Walker <danielwa@cisco.com>
---
drivers/of/fdt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 6e82bc42..80f1e48 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -953,7 +953,7 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
* managed to set the command line, unless CONFIG_CMDLINE_FORCE
* is set in which case we override whatever was found earlier.
*/
-#ifdef CONFIG_CMDLINE
+#if defined(CONFIG_CMDLINE) && !defined(CONFIG_GENERIC_CMDLINE)
#ifndef CONFIG_CMDLINE_FORCE
if (!((char *)data)[0])
#endif
--
2.1.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH-RFC 6/7] drivers: of: ifdef out cmdline section
2015-10-06 15:47 [PATCH-RFC 6/7] drivers: of: ifdef out cmdline section Daniel Walker
@ 2015-10-06 17:14 ` Rob Herring
2015-10-07 16:27 ` dwalker
0 siblings, 1 reply; 9+ messages in thread
From: Rob Herring @ 2015-10-06 17:14 UTC (permalink / raw)
To: Daniel Walker
Cc: xe-kernel, Frank Rowand, Grant Likely, Daniel Walker, devicetree,
linux-kernel
On Tue, Oct 6, 2015 at 10:47 AM, Daniel Walker <danielwa@cisco.com> wrote:
> It looks like there's some seepage of cmdline stuff into
> the generic device tree code. This conflicts with the
> generic cmdline implementation so I remove it in the case
> when that's enabled.
Nice series in general. I've had passing desire to do this every time
I run into the command line code.
The DT handling of the command line is generic across architectures.
The current design is working around that the kernel command line code
is not that way. I think we can take this a bit further by making the
generic DT code add the command line string directly rather than
relying on the arch to do that. Then we can remove all command line
handling from the arch code. I would also look at whether we can make
boot_command_line static rather than directly accessed. We might have
to leave it public for now, but could treat it as static for generic
cmdline case.
Rob
> Cc: xe-kernel@external.cisco.com
> Cc: Daniel Walker <dwalker@fifo99.com>
> Signed-off-by: Daniel Walker <danielwa@cisco.com>
> ---
> drivers/of/fdt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 6e82bc42..80f1e48 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -953,7 +953,7 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
> * managed to set the command line, unless CONFIG_CMDLINE_FORCE
> * is set in which case we override whatever was found earlier.
> */
> -#ifdef CONFIG_CMDLINE
> +#if defined(CONFIG_CMDLINE) && !defined(CONFIG_GENERIC_CMDLINE)
> #ifndef CONFIG_CMDLINE_FORCE
> if (!((char *)data)[0])
> #endif
> --
> 2.1.4
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-RFC 6/7] drivers: of: ifdef out cmdline section
2015-10-06 17:14 ` Rob Herring
@ 2015-10-07 16:27 ` dwalker
2015-10-07 21:48 ` Rob Herring
0 siblings, 1 reply; 9+ messages in thread
From: dwalker @ 2015-10-07 16:27 UTC (permalink / raw)
To: Rob Herring
Cc: Daniel Walker, xe-kernel, Frank Rowand, Grant Likely, devicetree,
linux-kernel
On Tue, Oct 06, 2015 at 12:14:43PM -0500, Rob Herring wrote:
> On Tue, Oct 6, 2015 at 10:47 AM, Daniel Walker <danielwa@cisco.com> wrote:
> > It looks like there's some seepage of cmdline stuff into
> > the generic device tree code. This conflicts with the
> > generic cmdline implementation so I remove it in the case
> > when that's enabled.
>
> Nice series in general. I've had passing desire to do this every time
> I run into the command line code.
>
> The DT handling of the command line is generic across architectures.
> The current design is working around that the kernel command line code
> is not that way. I think we can take this a bit further by making the
> generic DT code add the command line string directly rather than
> relying on the arch to do that. Then we can remove all command line
> handling from the arch code. I would also look at whether we can make
> boot_command_line static rather than directly accessed. We might have
> to leave it public for now, but could treat it as static for generic
> cmdline case.
>
Sorry I didn't respond sooner. I was waiting to see if there were more replies.
One of my colleague suggested something similar, I was reluctant to change anything
prior to sending it out so I could get more feedback on the direction.
So your suggesting this patch be something like,
#ifdef CONFIG_GENERIC_CMDLINE
// call generic cmdline functions
#else
// keep what's there currently
#endif
Then remove the arch specific generic cmdline changes in the architecture code for the DT path..
Most of them have two places where they add in the command line. One from the device tree, and
another place that's from the bootloader. For example , ARM has the device tree call, then another
for atags. I don't think these device tree changes would allow us to remove the atags version, also
the boot_command_line wouldn't be able to be static.
Daniel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-RFC 6/7] drivers: of: ifdef out cmdline section
2015-10-07 16:27 ` dwalker
@ 2015-10-07 21:48 ` Rob Herring
2015-10-13 20:13 ` Daniel Walker
0 siblings, 1 reply; 9+ messages in thread
From: Rob Herring @ 2015-10-07 21:48 UTC (permalink / raw)
To: dwalker
Cc: Daniel Walker, xe-kernel, Frank Rowand, Grant Likely, devicetree,
linux-kernel
On Wed, Oct 7, 2015 at 11:27 AM, <dwalker@fifo99.com> wrote:
> On Tue, Oct 06, 2015 at 12:14:43PM -0500, Rob Herring wrote:
>> On Tue, Oct 6, 2015 at 10:47 AM, Daniel Walker <danielwa@cisco.com> wrote:
>> > It looks like there's some seepage of cmdline stuff into
>> > the generic device tree code. This conflicts with the
>> > generic cmdline implementation so I remove it in the case
>> > when that's enabled.
>>
>> Nice series in general. I've had passing desire to do this every time
>> I run into the command line code.
>>
>> The DT handling of the command line is generic across architectures.
>> The current design is working around that the kernel command line code
>> is not that way. I think we can take this a bit further by making the
>> generic DT code add the command line string directly rather than
>> relying on the arch to do that. Then we can remove all command line
>> handling from the arch code. I would also look at whether we can make
>> boot_command_line static rather than directly accessed. We might have
>> to leave it public for now, but could treat it as static for generic
>> cmdline case.
>>
>
> Sorry I didn't respond sooner. I was waiting to see if there were more replies.
>
> One of my colleague suggested something similar, I was reluctant to change anything
> prior to sending it out so I could get more feedback on the direction.
>
> So your suggesting this patch be something like,
>
> #ifdef CONFIG_GENERIC_CMDLINE
> // call generic cmdline functions
> #else
> // keep what's there currently
> #endif
I think so yes, but I'd hope that the else case is empty. You've
converted the hard arches already. I'd guess the rest using DT would
be easy to convert as they either don't use DT for command line at all
or always use it.
> Then remove the arch specific generic cmdline changes in the architecture code for the DT path..
> Most of them have two places where they add in the command line. One from the device tree, and
> another place that's from the bootloader. For example , ARM has the device tree call, then another
> for atags. I don't think these device tree changes would allow us to remove the atags version, also
> the boot_command_line wouldn't be able to be static.
For ARM, yes, you will have to keep the ATAGS part, but ATAGS and DT
are mutually exclusive. I think that is generally true for other
arches.
Rob
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-RFC 6/7] drivers: of: ifdef out cmdline section
2015-10-07 21:48 ` Rob Herring
@ 2015-10-13 20:13 ` Daniel Walker
2015-10-13 21:17 ` Rob Herring
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Walker @ 2015-10-13 20:13 UTC (permalink / raw)
To: Rob Herring, dwalker
Cc: xe-kernel, Frank Rowand, Grant Likely, devicetree, linux-kernel
On 10/07/2015 02:48 PM, Rob Herring wrote:
> On Wed, Oct 7, 2015 at 11:27 AM, <dwalker@fifo99.com> wrote:
>> On Tue, Oct 06, 2015 at 12:14:43PM -0500, Rob Herring wrote:
>>> On Tue, Oct 6, 2015 at 10:47 AM, Daniel Walker <danielwa@cisco.com> wrote:
>>>> It looks like there's some seepage of cmdline stuff into
>>>> the generic device tree code. This conflicts with the
>>>> generic cmdline implementation so I remove it in the case
>>>> when that's enabled.
>>> Nice series in general. I've had passing desire to do this every time
>>> I run into the command line code.
>>>
>>> The DT handling of the command line is generic across architectures.
>>> The current design is working around that the kernel command line code
>>> is not that way. I think we can take this a bit further by making the
>>> generic DT code add the command line string directly rather than
>>> relying on the arch to do that. Then we can remove all command line
>>> handling from the arch code. I would also look at whether we can make
>>> boot_command_line static rather than directly accessed. We might have
>>> to leave it public for now, but could treat it as static for generic
>>> cmdline case.
>>>
>> Sorry I didn't respond sooner. I was waiting to see if there were more replies.
>>
>> One of my colleague suggested something similar, I was reluctant to change anything
>> prior to sending it out so I could get more feedback on the direction.
>>
>> So your suggesting this patch be something like,
>>
>> #ifdef CONFIG_GENERIC_CMDLINE
>> // call generic cmdline functions
>> #else
>> // keep what's there currently
>> #endif
> I think so yes, but I'd hope that the else case is empty. You've
> converted the hard arches already. I'd guess the rest using DT would
> be easy to convert as they either don't use DT for command line at all
> or always use it.
So I was thinking about doing a simplification like this,
932 int __init early_init_dt_scan_chosen(unsigned long node, const
char *uname,
933 int depth, void *data)
934 {
...
945
946 /* Retrieve command line */
947 p = of_get_flat_dt_prop(node, "bootargs", &l);
948 /*
949 * The builtin command line will be added here, or can
override
950 * the DT bootargs.
951 */
952 cmdline_add_builtin(data, p, COMMAND_LINE_SIZE);
The cmdline code would copy "p" over, or not if it's NULL. Then it would
either do the builtin command line override, or not. The thing that is
bothering me is that you have a length check in there "if (p != NULL &&
l > 0)" , is there a situation when "p" is not NULL, but "l" is 0 ?
I could also do this,
cmdline_add_builtin(data, (l > 0) ? p : NULL, COMMAND_LINE_SIZE);
but the strlcpy you have also only copies over "l" bytes, so there would
have to be a situation when the string is not NULL terminated or someone
only wants a section of it ?
Daniel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-RFC 6/7] drivers: of: ifdef out cmdline section
2015-10-13 20:13 ` Daniel Walker
@ 2015-10-13 21:17 ` Rob Herring
2015-10-14 14:48 ` Daniel Walker
0 siblings, 1 reply; 9+ messages in thread
From: Rob Herring @ 2015-10-13 21:17 UTC (permalink / raw)
To: Daniel Walker
Cc: Daniel Walker, xe-kernel, Frank Rowand, Grant Likely, devicetree,
linux-kernel
On Tue, Oct 13, 2015 at 3:13 PM, Daniel Walker <danielwa@cisco.com> wrote:
> On 10/07/2015 02:48 PM, Rob Herring wrote:
>>
>> On Wed, Oct 7, 2015 at 11:27 AM, <dwalker@fifo99.com> wrote:
>>>
>>> On Tue, Oct 06, 2015 at 12:14:43PM -0500, Rob Herring wrote:
>>>>
>>>> On Tue, Oct 6, 2015 at 10:47 AM, Daniel Walker <danielwa@cisco.com>
>>>> wrote:
>>>>>
>>>>> It looks like there's some seepage of cmdline stuff into
>>>>> the generic device tree code. This conflicts with the
>>>>> generic cmdline implementation so I remove it in the case
>>>>> when that's enabled.
>>>>
>>>> Nice series in general. I've had passing desire to do this every time
>>>> I run into the command line code.
>>>>
>>>> The DT handling of the command line is generic across architectures.
>>>> The current design is working around that the kernel command line code
>>>> is not that way. I think we can take this a bit further by making the
>>>> generic DT code add the command line string directly rather than
>>>> relying on the arch to do that. Then we can remove all command line
>>>> handling from the arch code. I would also look at whether we can make
>>>> boot_command_line static rather than directly accessed. We might have
>>>> to leave it public for now, but could treat it as static for generic
>>>> cmdline case.
>>>>
>>> Sorry I didn't respond sooner. I was waiting to see if there were more
>>> replies.
>>>
>>> One of my colleague suggested something similar, I was reluctant to
>>> change anything
>>> prior to sending it out so I could get more feedback on the direction.
>>>
>>> So your suggesting this patch be something like,
>>>
>>> #ifdef CONFIG_GENERIC_CMDLINE
>>> // call generic cmdline functions
>>> #else
>>> // keep what's there currently
>>> #endif
>>
>> I think so yes, but I'd hope that the else case is empty. You've
>> converted the hard arches already. I'd guess the rest using DT would
>> be easy to convert as they either don't use DT for command line at all
>> or always use it.
>
>
> So I was thinking about doing a simplification like this,
> 932 int __init early_init_dt_scan_chosen(unsigned long node, const char
> *uname,
> 933 int depth, void *data)
> 934 {
> ...
> 945
> 946 /* Retrieve command line */
> 947 p = of_get_flat_dt_prop(node, "bootargs", &l);
> 948 /*
> 949 * The builtin command line will be added here, or can
> override
> 950 * the DT bootargs.
> 951 */
> 952 cmdline_add_builtin(data, p, COMMAND_LINE_SIZE);
>
> The cmdline code would copy "p" over, or not if it's NULL. Then it would
> either do the builtin command line override, or not. The thing that is
> bothering me is that you have a length check in there "if (p != NULL && l >
> 0)" , is there a situation when "p" is not NULL, but "l" is 0 ?
Yes, you can have properties with no data (e.g. just "bootargs;"). I
don't think that would happen in this case. I generally don't think it
is the kernel's job to validate bindings, but given this comes from
the bootloader we probably should here.
> I could also do this,
> cmdline_add_builtin(data, (l > 0) ? p : NULL, COMMAND_LINE_SIZE);
>
> but the strlcpy you have also only copies over "l" bytes, so there would
> have to be a situation when the string is not NULL terminated or someone
> only wants a section of it ?
I don't think either would ever be true. If so, that should be the
caller's problem to deal with.
Rob
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-RFC 6/7] drivers: of: ifdef out cmdline section
2015-10-13 21:17 ` Rob Herring
@ 2015-10-14 14:48 ` Daniel Walker
2015-10-14 17:57 ` Rob Herring
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Walker @ 2015-10-14 14:48 UTC (permalink / raw)
To: Rob Herring
Cc: Daniel Walker, xe-kernel, Frank Rowand, Grant Likely, devicetree,
linux-kernel
There's one last little wrinkle .. In the current setup the defconfig
CONFIG_CMDLINE="" is used as a default in case the device tree has
nothing in it. In my changes, there is no identical functionality. The
only similar thing I have is the the CONFIG_CMDLINE_APPEND="" . The main
difference is that in the current implementation CONFIG_CMDLINE=""
doesn't get added at all if there is a device tree bootargs, but with my
implementation this line would be added unconditionally. It would
represent a subtle change where people would have to add into the DT
bootargs something to override what might be in the default command line.
For example,
if a config has CONFIG_CMDLINE_APPEND="debug" then they would have to
add a "loglevel=7" into the DT bootargs to get back to normal. I
wouldn't think people would want "debug" as the default, but oddly
enough some of the configs do have this. Some of them also have default
ip address setting, nfsroot= settings, and loglevel= settings.
What are your thoughts on this ? I think using the append type default
makes more sense because it's actually setting up global defaults. The
current complete replacement scheme seems to set the stage for people to
make an entirely custom default for a single development machine, which
IMO doesn't make sense. However, I'm not sure what the intent is with
the current setup.
Daniel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-RFC 6/7] drivers: of: ifdef out cmdline section
2015-10-14 14:48 ` Daniel Walker
@ 2015-10-14 17:57 ` Rob Herring
2015-10-14 19:16 ` Daniel Walker
0 siblings, 1 reply; 9+ messages in thread
From: Rob Herring @ 2015-10-14 17:57 UTC (permalink / raw)
To: Daniel Walker
Cc: Rob Herring, Daniel Walker, xe-kernel, Frank Rowand,
Grant Likely, devicetree, linux-kernel
On Wed, Oct 14, 2015 at 9:48 AM, Daniel Walker <danielwa@cisco.com> wrote:
>
>
> There's one last little wrinkle .. In the current setup the defconfig
> CONFIG_CMDLINE="" is used as a default in case the device tree has nothing
> in it. In my changes, there is no identical functionality. The only similar
> thing I have is the the CONFIG_CMDLINE_APPEND="" . The main difference is
> that in the current implementation CONFIG_CMDLINE="" doesn't get added at
> all if there is a device tree bootargs, but with my implementation this line
> would be added unconditionally. It would represent a subtle change where
> people would have to add into the DT bootargs something to override what
> might be in the default command line.
So CMDLINE_EXTEND would be equivalent to your version, but it looks
like CMDLINE_EXTEND is not used in the DT case. Perhaps you can add
the option? You already have OVERRIDE which is equivalent to FORCE.
> For example,
>
> if a config has CONFIG_CMDLINE_APPEND="debug" then they would have to add a
> "loglevel=7" into the DT bootargs to get back to normal. I wouldn't think
> people would want "debug" as the default, but oddly enough some of the
> configs do have this. Some of them also have default ip address setting,
> nfsroot= settings, and loglevel= settings.
Or they would have to remove the kernel default from their config.
That might be acceptable. You could have a case where you have 1
kernel binary and 2 different bootloaders where you expect the
bootloader's cmdline used in one case and the kernel's in the other.
Seems unlikely, but it would be an ABI break.
I would not judge people's choices of defaults making sense...
> What are your thoughts on this ? I think using the append type default makes
> more sense because it's actually setting up global defaults. The current
> complete replacement scheme seems to set the stage for people to make an
> entirely custom default for a single development machine, which IMO doesn't
> make sense. However, I'm not sure what the intent is with the current setup.
People will want a path to support up to the current 3 options (use
bootloader's cmdline, append bootloader cmdline to default, and force
kernel default) and you have to assume changing bootloader is not an
option.
Rob
>
> Daniel
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-RFC 6/7] drivers: of: ifdef out cmdline section
2015-10-14 17:57 ` Rob Herring
@ 2015-10-14 19:16 ` Daniel Walker
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Walker @ 2015-10-14 19:16 UTC (permalink / raw)
To: Rob Herring
Cc: Daniel Walker, xe-kernel, Frank Rowand, Grant Likely, devicetree,
linux-kernel
On 10/14/2015 10:57 AM, Rob Herring wrote:
> On Wed, Oct 14, 2015 at 9:48 AM, Daniel Walker <danielwa@cisco.com> wrote:
>>
>> There's one last little wrinkle .. In the current setup the defconfig
>> CONFIG_CMDLINE="" is used as a default in case the device tree has nothing
>> in it. In my changes, there is no identical functionality. The only similar
>> thing I have is the the CONFIG_CMDLINE_APPEND="" . The main difference is
>> that in the current implementation CONFIG_CMDLINE="" doesn't get added at
>> all if there is a device tree bootargs, but with my implementation this line
>> would be added unconditionally. It would represent a subtle change where
>> people would have to add into the DT bootargs something to override what
>> might be in the default command line.
> So CMDLINE_EXTEND would be equivalent to your version, but it looks
> like CMDLINE_EXTEND is not used in the DT case. Perhaps you can add
> the option? You already have OVERRIDE which is equivalent to FORCE.
I have better than CONFIG_CMDLINE_EXTEND.. My changes allow the
bootloader arguments to be appended or prepended to the default, which
ever works for you. It works in a slightly different way, your appending
or prepending the default values the bootloader arugments (or DT
bootargs), opposed to the other way around.
This is what I was suggesting we do, i.e. to use something like
CONFIG_CMDLINE_EXTEND. If we put everything from CONFIG_CMDLINE into
something like CONFIG_CMDLINE_EXTEND, then it ends up setting default I
think people won't expect.
>
>> For example,
>>
>> if a config has CONFIG_CMDLINE_APPEND="debug" then they would have to add a
>> "loglevel=7" into the DT bootargs to get back to normal. I wouldn't think
>> people would want "debug" as the default, but oddly enough some of the
>> configs do have this. Some of them also have default ip address setting,
>> nfsroot= settings, and loglevel= settings.
> Or they would have to remove the kernel default from their config.
> That might be acceptable. You could have a case where you have 1
> kernel binary and 2 different bootloaders where you expect the
> bootloader's cmdline used in one case and the kernel's in the other.
> Seems unlikely, but it would be an ABI break.
>
>
I suppose it would depend if it works or not with my changes. If we do
the EXTEND thing, then people might end up with "nfsroot=" lines they
didn't expend as defaults.
I'm not sure this is ABI breakage, because the kernel code isn't stuck
on ABI's internally. If someone wants specific boot arguments in there
kernel config this doesn't prevent it, it just changes the behavior.
>> What are your thoughts on this ? I think using the append type default makes
>> more sense because it's actually setting up global defaults. The current
>> complete replacement scheme seems to set the stage for people to make an
>> entirely custom default for a single development machine, which IMO doesn't
>> make sense. However, I'm not sure what the intent is with the current setup.
> People will want a path to support up to the current 3 options (use
> bootloader's cmdline, append bootloader cmdline to default, and force
> kernel default) and you have to assume changing bootloader is not an
> option.
>
Yeah we have those 3 with my changes. Maybe I'm drilling down too much
on this and we should just do the CONFIG_CMDLINE_EXTEND equivalent and
when they find there's a new "nfsroot=" line in there bootargs they can
just send a patch.
Daniel
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-10-14 19:16 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-06 15:47 [PATCH-RFC 6/7] drivers: of: ifdef out cmdline section Daniel Walker
2015-10-06 17:14 ` Rob Herring
2015-10-07 16:27 ` dwalker
2015-10-07 21:48 ` Rob Herring
2015-10-13 20:13 ` Daniel Walker
2015-10-13 21:17 ` Rob Herring
2015-10-14 14:48 ` Daniel Walker
2015-10-14 17:57 ` Rob Herring
2015-10-14 19:16 ` Daniel Walker
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).