Re: [PATCH-RFC 6/7] drivers: of: ifdef out cmdline section

[Daniel Walker <danielwa@cisco.com>]

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