From patchwork Fri Jul 24 21:27:18 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rob Herring X-Patchwork-Id: 6862971 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id F35E8C05AC for ; Fri, 24 Jul 2015 21:29:47 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id F2EB8205B6 for ; Fri, 24 Jul 2015 21:29:46 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id F3805205B4 for ; Fri, 24 Jul 2015 21:29:45 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZIkVL-00078d-EO; Fri, 24 Jul 2015 21:27:43 +0000 Received: from mail-oi0-x22c.google.com ([2607:f8b0:4003:c06::22c]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZIkVI-00076p-Av for linux-arm-kernel@lists.infradead.org; Fri, 24 Jul 2015 21:27:41 +0000 Received: by oige126 with SMTP id e126so26310356oig.0 for ; Fri, 24 Jul 2015 14:27:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=jUOPg4NN/57G2Zafx1Sr4blRDTeeiiW8sJSfj0WukgU=; b=LtNg97bYQBKhY8vBPyUtZSdlMMs8SYKn8T3klMf8F/x8M8Jv2J9CRp5yUgcA0r0Q1l oUEPrtrw9aq8EgFMB/Nkkp+Oz+YBZxSbRIQB0hqfNvm/ALYx/9hggUsdIwM83gjptJws 32cShr1HR95ScenRorwztlgt2/DPfVbWJMCrFQ+T/kOvUY5bZFOYJEZTdIladNgjtZj+ Zz7VyHTr6cf2Jyx204W+x8LnZuGg3QW/s7dOAy1irAhV0faEjGkBreKT5Gh2fOsdCHwZ BHsusQDTGA5YRZWSiIbeo+Cdwv+rvvYD2wdCu2hALAoI/ZAB/i8phvBhrha3mvYbSPd1 hmcQ== X-Received: by 10.202.220.65 with SMTP id t62mr16468663oig.115.1437773239382; Fri, 24 Jul 2015 14:27:19 -0700 (PDT) Received: from [192.168.1.89] (72-48-98-129.dyn.grandenetworks.net. [72.48.98.129]) by smtp.googlemail.com with ESMTPSA id xp8sm5680517obc.10.2015.07.24.14.27.18 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 24 Jul 2015 14:27:18 -0700 (PDT) Message-ID: <55B2ADB6.3040307@gmail.com> Date: Fri, 24 Jul 2015 16:27:18 -0500 From: Rob Herring User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: Joerg Roedel Subject: Re: [PATCH 1/2] of: base: Allow more args than MAX_PHANDLE_ARGS if required References: <1437035444-13867-1-git-send-email-joro@8bytes.org> <1437035444-13867-2-git-send-email-joro@8bytes.org> <20150716102325.GC26390@arm.com> <20150716110900.GA30130@suse.de> In-Reply-To: <20150716110900.GA30130@suse.de> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20150724_142740_476595_3F558D83 X-CRM114-Status: GOOD ( 35.16 ) X-Spam-Score: -2.5 (--) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "devicetree@vger.kernel.org" , Joerg Roedel , Will Deacon , "linux-kernel@vger.kernel.org" , "iommu@lists.linux-foundation.org" , "grant.likely@linaro.org" , "linux-arm-kernel@lists.infradead.org" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-5.3 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Jul 16, 2015 at 6:09 AM, Joerg Roedel wrote: > Hi Will, > > On Thu, Jul 16, 2015 at 11:23:26AM +0100, Will Deacon wrote: >> On Thu, Jul 16, 2015 at 09:30:43AM +0100, Joerg Roedel wrote: >> > +struct of_phandle_args *of_alloc_phandle_args(int size) >> > +{ >> > + struct of_phandle_args *args; >> > + int e = max(0, size - MAX_PHANDLE_ARGS); >> > + >> > + args = kzalloc(sizeof(struct of_phandle_args) + e * sizeof(uint32_t), >> > + GFP_KERNEL); >> >> Should you also update args->args_count to reflect the extended array? > > The args_count member just tells us how many of the array elements are > used and not how many there are. So it doesn't need to be updated here. > >> That said, extending the fixed-size array member like this feels a bit >> fragile. Does GCC not complain about out-of-bounds accesses if you >> statically address args->args[MAX_PHANDLE_ARGS]? Admittedly, I can't >> think *why* this would be break (things like additional padding will be >> harmless), but I'm not intimate with the C standard. > > Yeah, I agree, it is not the best possible solution. But this way I > don't need to update all callers, and thus it works better with our > development model. Our development model is not to work-around kernel APIs last time I checked. > But I am open for suggestions on how to solve this problem better. In > fact, my main motivation in sending this was to get the discussion about > an upstreamable solution started :) > > Lets see what the device-tree maintainers have to say. A good number of callers and all iommu callers loop thru the list of phandles which are just open coded ATM. So we should do loop iterators here. With iterators, we can do the allocation within the iterators. This can be much more efficient as we don't iterate thru the list from the start every time. Normally, the list is not big enough to matter, but in your case it may be. I'm thinking something like this untested and not yet compiling patch. It still has the abuse of adding onto the end of the of_phandle_args struct which I don't really like. We could do a new struct, but I'd like to keep this code common. I also need to refactor the existing code to use __of_parse_one_phandle_with_args. Rob 8<-------------------------------------------------------------------------------------------- diff --git a/drivers/of/base.c b/drivers/of/base.c index 8b5a187..c1c5a43 100644 --- a/drivers/of/base.c +++ b/drivers/of/base.c @@ -1442,6 +1442,96 @@ void of_print_phandle_args(const char *msg, const struct of_phandle_args *args) printk("\n"); } + +static int __of_parse_one_phandle_with_args(const __be32 **list, + const char *cells_name, + int cell_count, + struct of_phandle_args **out_args) +{ + struct device_node *node; + phandle phandle; + struct of_phandle_args *args = *out_args; + const __be32 *start_list = *list; + u32 i, count = 0; + int ret; + + /* + * If phandle is 0, then it is an empty entry with no + * arguments. Skip forward to the next entry. + */ + phandle = be32_to_cpup(*list++); + if (phandle) { + /* + * Find the provider node and parse the #*-cells + * property to determine the argument length. + * + * This is not needed if the cell count is hard-coded + * (i.e. cells_name not set, but cell_count is set), + * except when we're going to return the found node + * below. + */ + node = of_find_node_by_phandle(phandle); + if (!node) + return -ENOENT; + + if (cells_name) { + ret = of_property_read_u32(node, cells_name, &count); + if (ret) { + pr_err("could not get %s for %s\n", + cells_name, node->full_name); + return ret; + } + } else { + count = cell_count; + } + } + + if (args && WARN_ON(count > MAX_PHANDLE_ARGS)) + count = MAX_PHANDLE_ARGS; + + if (!args) { + args = kzalloc(sizeof(*args) + (count * sizeof(uint32_t))), GFP_KERNEL); + *out_args = args; + } + if (!args) + return -ENOMEM; + + if (!phandle) { + memset(args, 0, sizeof(*args)); + return -ENOENT; + } + + args->np = node; + args->val = start_list; + args->args_count = count; + for (i = 0; i < count; i++) + args->args[i] = be32_to_cpup(*list++); + + return 0; +} + +of_phandle_args *of_prop_next_phandle_args(struct property *prop, + const char *cells_name, + int cell_count, + struct of_phandle_args *last) +{ + struct of_phandle_args *args = NULL; + const __be32 *start_list; + + if (!prop) + return NULL; + + if (!last) { + start_list = prop->value; + } else { + start_list = last->val; + kfree(last); + } + __of_parse_one_phandle_with_args(list, cells_name, cell_count, &args); + return args; +} +EXPORT_SYMBOL_GPL(of_prop_next_phandle_args); + static int __of_parse_phandle_with_args(const struct device_node *np, const char *list_name, const char *cells_name, diff --git a/include/linux/of.h b/include/linux/of.h index edc068d..a0f432c 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -879,6 +879,13 @@ static inline int of_property_read_s32(const struct device_node *np, s; \ s = of_prop_next_string(prop, s)) +#define of_property_for_each_phandle_with_args(np, propname, cells_name, cell_count, prop, arg) \ + for (prop = of_find_property(np, propname, NULL), \ + arg = of_prop_next_phandle_args(prop, NULL); \ + arg; \ + arg = of_prop_next_phandle_args(prop, arg)) + + #define for_each_node_by_name(dn, name) \ for (dn = of_find_node_by_name(NULL, name); dn; \ dn = of_find_node_by_name(dn, name))