From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH] xen/dts: Don't translate invalid address Date: Mon, 06 Jan 2014 16:18:07 +0000 Message-ID: <52CAD73F.6050006@linaro.org> References: <1386901910-1016-1-git-send-email-julien.grall@linaro.org> <1389021866.31766.51.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1W0CsW-0006UM-GO for xen-devel@lists.xenproject.org; Mon, 06 Jan 2014 16:18:12 +0000 Received: by mail-lb0-f175.google.com with SMTP id w6so9847157lbh.6 for ; Mon, 06 Jan 2014 08:18:10 -0800 (PST) In-Reply-To: <1389021866.31766.51.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: xen-devel@lists.xenproject.org, tsahee@gmx.com, tim@xen.org, stefano.stabellini@citrix.com, patches@linaro.org List-Id: xen-devel@lists.xenproject.org On 01/06/2014 03:24 PM, Ian Campbell wrote: > On Fri, 2013-12-13 at 02:31 +0000, Julien Grall wrote: >> ePAR specifies that if the property "ranges" doesn't exist in a bus node: >> >> "it is assumed that no mapping exists between children of node and the parent >> address space". >> >> Modify dt_number_of_address to check if the list of ranges are valid. Return >> 0 (ie there is zero range) if the list is not valid. >> >> This patch has been tested on the Arndale where the bug can occur with the >> '/hdmi' node. >> >> Reported-by: >> Signed-off-by: Julien Grall >> >> --- >> >> This patch is a bug fix for Xen 4.4. Without it, Xen can't boot on the Arndale >> because it's unable to translate a wrong address. >> --- >> xen/common/device_tree.c | 31 +++++++++++++++++++++++++++---- >> 1 file changed, 27 insertions(+), 4 deletions(-) >> >> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c >> index 84e709d..9b9a348 100644 >> --- a/xen/common/device_tree.c >> +++ b/xen/common/device_tree.c >> @@ -93,7 +93,7 @@ struct dt_bus >> { >> const char *name; >> const char *addresses; >> - int (*match)(const struct dt_device_node *parent); >> + bool_t (*match)(const struct dt_device_node *parent); >> void (*count_cells)(const struct dt_device_node *child, >> int *addrc, int *sizec); >> u64 (*map)(__be32 *addr, const __be32 *range, int na, int ns, int pna); >> @@ -793,6 +793,18 @@ int dt_n_size_cells(const struct dt_device_node *np) >> /* >> * Default translator (generic bus) >> */ >> +static bool_t dt_bus_default_match(const struct dt_device_node *parent) >> +{ >> + /* Root node doesn't have "ranges" property */ >> + if ( parent->parent == NULL ) > > Calling the parameter "parent" led to me confusedly wondering why it was > the grandparent which mattered. I suppose "parent" in this sense means > the "parent bus" as opposed to parent node? Or does it just fall out of > the fact that in the caller it is the parent which is passed through? > > Can we call it something else, like "bus" or "node" or something else > appropriate? Right, the name is confusing. It took me few minutes, even if I wrote the code, to understand it :). I blindly copied the code from Linux of_bus structure. The best name would be "node", because the function is trying to find if the parameters is a bus or not. >> + return 1; >> + >> + /* The default bus is only used when the "ranges" property exists. >> + * Otherwise we can't translate the address >> + */ >> + return (dt_get_property(parent, "ranges", NULL) != NULL); >> +} >> + >> static void dt_bus_default_count_cells(const struct dt_device_node *dev, >> int *addrc, int *sizec) >> { >> @@ -819,7 +831,7 @@ static u64 dt_bus_default_map(__be32 *addr, const __be32 *range, >> * If the number of address cells is larger than 2 we assume the >> * mapping doesn't specify a physical address. Rather, the address >> * specifies an identifier that must match exactly. >> - */ >> + */ > > Spurious w/s change. > > Other that those two changes the patch looks good. I will fix it. -- Julien Grall