| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20110223194352.GR14597@angua.secretlab.ca>
Date: Wed, 23 Feb 2011 12:43:52 -0700
From: Grant Likely <grant.likely@...retlab.ca>
To: Andres Salomon <dilinger@...ued.net>
Cc: Daniel Drake <dsd@...top.org>,
David Woodhouse <dwmw2@...radead.org>, cbou@...l.ru,
linux-kernel@...r.kernel.org, x86@...nel.org, tglx@...utronix.de,
mingo@...hat.com, hpa@...or.com, dmitry.torokhov@...il.com,
devicetree-discuss@...ts.ozlabs.org,
"David S. Miller" <davem@...emloft.net>, sparclinux@...r.kernel.org
Subject: Re: [PATCH] of/pdt: allow DT device matching by fixing 'name'
brokenness
On Fri, Feb 18, 2011 at 07:06:59PM -0800, Andres Salomon wrote:
> On Fri, 18 Feb 2011 23:42:57 +0000
> Daniel Drake <dsd@...top.org> wrote:
>
> > On 16 February 2011 22:44, David Woodhouse <dwmw2@...radead.org>
> > wrote:
> > > On Wed, 2011-02-16 at 22:28 +0000, Daniel Drake wrote:
> > >>
> > >> +static int __init add_common_platform_devices(void)
> > >> +{
> > >> + struct platform_device *pdev;
> > >> +
> > >> + pdev = platform_device_register_simple("olpc-battery", -1,
> > >> NULL, 0);
> > >> + if (IS_ERR(pdev))
> > >> + return PTR_ERR(pdev);
> > >> +
> > >> + return 0;
> > >> +}
> > >> +
> > >
> > > Still kind of sucks that you have to do this, and can't bind to
> > > something in the device-tree.
> >
> > OK, feel free to put this patch on hold for now. I started looking at
> > the device tree approach today. It looks doable but first we have to
> > fix a DT bug/inconsistency that is preventing us from correctly
> > binding to the tree's devices.
> >
> > Daniel
>
>
> Mea culpa. The patch below fixes a bug I introduced earlier.
> Cc'ing the sparc folks, as this probably affects them
> (although I would think that it fixes broken behavior for them..?)
Wait; why are you binding to a device based on name? Binding by name
and/or device_type is strongly discouraged for new code. Use compatible
instead.
As for this patch, comments below...
> From: Andres Salomon <dilinger@...ued.net>
>
> Commit e2f2a93b changed dp->name from using the 'name' property to
> using package-to-path. This fixed /proc/device-tree creation by
> eliminating conflicts between names (the 'name' property provides
> names like 'battery', whereas package-to-path provides names like
> '/foo/bar/battery@0', which we stripped to 'battery@0'). However,
> it also breaks of_device_id table matching.
>
> The fix that we _really_ wanted was to keep dp->name based upon
> the name property ('battery'), but based dp->full_name upon
> package-to-path ('battery@0'). This patch does just that.
>
> Signed-off-by: Andres Salomon <dilinger@...ued.net>
> Reported-by: Daniel Drake <dsd@...top.org>
>From what I can tell, this only affects OLPC, correct? It looks like
SPARC's implementation of of_pdt_node_name() will sidestep most of this
name retrieval code.
> ---
> drivers/of/pdt.c | 42 +++++++++++++++++++-----------------------
> 1 files changed, 19 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> index 28295d0..b39d584 100644
> --- a/drivers/of/pdt.c
> +++ b/drivers/of/pdt.c
> @@ -48,7 +48,7 @@ static inline void irq_trans_init(struct device_node *dp) { }
>
> static inline const char *of_pdt_node_name(struct device_node *dp)
> {
> - return dp->name;
> + return NULL;
> }
Rather than using this hook; perhaps the sparc .path_component_name
should be enabled for all architectures. It is a useful data item to
keep a pointer to, and it would simplify the of_pdt code.
>
> #endif /* !CONFIG_SPARC */
> @@ -156,23 +156,6 @@ static char * __init of_pdt_try_pkg2path(phandle node)
> return res+1;
> }
>
> -/*
> - * When fetching the node's name, first try using package-to-path; if
> - * that fails (either because the arch hasn't supplied a PROM callback,
> - * or some other random failure), fall back to just looking at the node's
> - * 'name' property.
> - */
> -static char * __init of_pdt_build_name(phandle node)
> -{
> - char *buf;
> -
> - buf = of_pdt_try_pkg2path(node);
> - if (!buf)
> - buf = of_pdt_get_one_property(node, "name");
> -
> - return buf;
> -}
> -
> static struct device_node * __init of_pdt_create_node(phandle node,
> struct device_node *parent)
> {
> @@ -187,7 +170,7 @@ static struct device_node * __init of_pdt_create_node(phandle node,
>
> kref_init(&dp->kref);
>
> - dp->name = of_pdt_build_name(node);
> + dp->name = of_pdt_get_one_property(node, "name");
> dp->type = of_pdt_get_one_property(node, "device_type");
> dp->phandle = node;
>
> @@ -198,13 +181,26 @@ static struct device_node * __init of_pdt_create_node(phandle node,
> return dp;
> }
>
> -static char * __init of_pdt_build_full_name(struct device_node *dp)
> +static char * __init of_pdt_build_full_name(struct device_node *dp,
> + phandle node)
> {
> int len, ourlen, plen;
> + const char *name;
> char *n;
>
> + /*
> + * When fetching the full name, rather than what we see with the
> + * name property (ie, 'battery'), we want the name we see with
> + * package-to-path (ie, 'battery@0').
> + */
> + name = of_pdt_node_name(dp);
> + if (!name)
> + name = of_pdt_try_pkg2path(node);
> + if (!name)
> + name = dp->name;
> +
Rather than this thrashing about looking for a way to get the path
component, it should be fairly certain how to obtain the node's name
before getting into of_pdt_build_full_name(). I'd follow SPARC's
lead and create an implementation of build_path_component();
However, considering that this is creating the full name; wouldn't the
raw output of pkg2path() provide exactly the full name string you need
here instead of breaking it down into components and adding it back up
again?
> plen = strlen(dp->parent->full_name);
> - ourlen = strlen(of_pdt_node_name(dp));
> + ourlen = strlen(name);
> len = ourlen + plen + 2;
>
> n = prom_early_alloc(len);
> @@ -213,7 +209,7 @@ static char * __init of_pdt_build_full_name(struct device_node *dp)
> strcpy(n + plen, "/");
> plen++;
> }
> - strcpy(n + plen, of_pdt_node_name(dp));
> + strcpy(n + plen, name);
>
> return n;
> }
> @@ -243,7 +239,7 @@ static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
> #if defined(CONFIG_SPARC)
> dp->path_component_name = build_path_component(dp);
> #endif
> - dp->full_name = of_pdt_build_full_name(dp);
> + dp->full_name = of_pdt_build_full_name(dp, node);
>
> dp->child = of_pdt_build_tree(dp,
> of_pdt_prom_ops->getchild(node), nextp);
> --
> 1.7.2.3
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists