On 10/07/2015 02:48 PM, Rob Herring wrote:
On Wed, Oct 7, 2015 at 11:27 AM, <dwalker@xxxxxxxxxx> 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@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
