From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932177AbbJMVST (ORCPT ); Tue, 13 Oct 2015 17:18:19 -0400 Received: from mail.kernel.org ([198.145.29.136]:42622 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752979AbbJMVSR (ORCPT ); Tue, 13 Oct 2015 17:18:17 -0400 MIME-Version: 1.0 In-Reply-To: <561D6603.4060006@cisco.com> References: <1444146434-12776-6-git-send-email-danielwa@cisco.com> <20151007162755.GA23283@fifo99.com> <561D6603.4060006@cisco.com> From: Rob Herring Date: Tue, 13 Oct 2015 16:17:54 -0500 Message-ID: Subject: Re: [PATCH-RFC 6/7] drivers: of: ifdef out cmdline section To: Daniel Walker Cc: Daniel Walker , xe-kernel@external.cisco.com, Frank Rowand , Grant Likely , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Oct 13, 2015 at 3:13 PM, Daniel Walker wrote: > On 10/07/2015 02:48 PM, Rob Herring wrote: >> >> On Wed, Oct 7, 2015 at 11:27 AM, 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 >>>> 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