| 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: <20120223035640.GC9141@shlinux2.ap.freescale.net>
Date: Thu, 23 Feb 2012 11:56:41 +0800
From: Dong Aisheng <aisheng.dong@...escale.com>
To: Stephen Warren <swarren@...dia.com>
CC: Linus Walleij <linus.walleij@...aro.org>,
Linus Walleij <linus.walleij@...ricsson.com>,
"B29396@...escale.com" <B29396@...escale.com>,
"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
"dongas86@...il.com" <dongas86@...il.com>,
"shawn.guo@...aro.org" <shawn.guo@...aro.org>,
"thomas.abraham@...aro.org" <thomas.abraham@...aro.org>,
"tony@...mide.com" <tony@...mide.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 08/20] pinctrl: Assume map table entries can't have a
NULL name field
On Wed, Feb 22, 2012 at 07:39:42PM -0800, Stephen Warren wrote:
> Dong Aisheng wrote at Wednesday, February 22, 2012 8:35 PM:
> > On Wed, Feb 22, 2012 at 10:05:20AM -0800, Stephen Warren wrote:
> > > Dong Aisheng wrote at Tuesday, February 21, 2012 11:35 PM:
> > > > On Tue, Feb 21, 2012 at 09:46:01AM -0800, Stephen Warren wrote:
> > > > ....
> > > > > > we might just
> > > > > >
> > > > > > #define PIN_MAP_NAME_DEFAULT "default"
> > > > > > In <linux/pinctrl/consumer.h> and <linux/pinctrl/machine.h> alike,
> > > > > > maybe in some <linux/pinctrl/mapnames.h> that both of these
> > > > > > include?
> > > > > >
> > > > > > the have the driver ask for:
> > > > > >
> > > > > > sirfport->p = pinctrl_get(&pdev->dev, PIN_MAP_NAME_DEFAULT);
> > > > > >
> > > > > > (Similar changes can be done for U300, naming all its map
> > > > > > "default".)
> > > > >
> > > > > I guess we could just modify pinmux_get() such that if NULL is passed as
> > > > > the state name, it uses "default" instead internally. The disadvantage I
> > > > > see here is that someone reading the client driver and writing the mapping
> > > > > table then has to know that pinmux_get() does that internally, rather than
> > > > > it being obvious right in the client driver code.
> > > >
> > > > It looks like a way out.
> > >
> > > > The left problem seems to be that we may force the mapping table writer to
> > > > specify "default" name for map at least.
> > > > Is there any way to avoid it?
> > >
> > > That specific restriction has been present since the very first patch
> > > that created the pinctrl subsystem.
> >
> > Since i saw some code in pinctrl subsystem formerly like:
> > /*
> > * If we're looking for a specific named map, this must match,
> > * else we loop and look for the next.
> > */
> > if (name != NULL) {
> > if (map->name == NULL)
> > continue;
> > if (strcmp(map->name, name))
> > continue;
> > }
> > So i guessed the map->name could be NULL in the original design.
> > Of course, if we used the convenience macro to define map, there's no
> > chance to make map name NULL.
>
> Admittedly, the code is a little inconsistent. I was thinking of:
>
> +int __init pinmux_register_mappings(struct pinmux_map const *maps,
> ...
> + for (i = 0; i < num_maps; i++) {
> + /* Sanity check the mapping */
> + if (!maps[i].name) {
> + pr_err("failed to register map %d: "
> + "no map name given\n", i);
> + return -EINVAL;
> + }
>
> > We did not force users to must convenience macros, right?
>
> No. In fact, my patches for Tegra's board file didn't always use them.
>
> > > > I just think it may be better if we do not have such restriction.
> > >
> > > Given that all map entries have a name, I'd really prefer that all drivers
> > > had to request a specific matching name, rather than saying "just give me
> > > whatever is in the mapping table".
> > >
> > > I don't see this as a restriction, but more of a correctness issue;
> > > every mapping table entry has a name and drivers (or device tree bindings)
> > > define what that name must be, so why shouldn't drivers be required to
> > > request that specific name, and experience an error if the mapping table
> > > author didn't create that name?
> >
> > Yes, i agree.
> > Based on this point, i feel maybe internally convert NULL to a "default" name for
> > pinctrl_get does not make too much sense.
> >
> > > regulator_get(dev, id) requires id != NULL.
> > >
> > > clk_get(dev, con_id) requires con_id != NULL in order to match a clock
> > > that has a non-NULL con_id defined (although clocks with a NULL con_id
> > > are also legal, in which case, the con_id parameter to clk_get is
> > > ignored)
> > >
> > > So pinctrl_get(dev, state) requiring state != NULL seems consistent
> > > with existing practice.
> >
> > Yes, it's true if given all map entries have a name.
> >
> > Now i'm intend to the way you mentioned at:
> > https://lkml.org/lkml/2012/2/21/213
> >
> > And i agree with the way you said:
> > If we continue to allow NULL names, I think we should change that to:
> > if name:
> > match = map->name && !strcmp(name, map->name)
> > else:
> > match = !map->name
> >
> > And i think allowing map name to be NULL will make life easy for those
> > devices who do not support multi states.
>
> It does make it a little convenient for board files, but isn't really
> practical to implement for device-tree unless we modify the binding to
> have something that means "no name". I'd rather not allow more options
> in the binding though.
>
> What's wrong with writing e.g. PINCTRL_DEFAULT instead of NULL when calling
> pinctrl_get() or pinctrl_lookup_state() though? Given pinctrl.h having:
>
> #define PINCTRL_DEFAULT "default"
>
First, map writer needs know this logic.
Second, all as you said:
> > > every mapping table entry has a name and drivers (or device tree bindings)
> > > define what that name must be, so why shouldn't drivers be required to
> > > request that specific name, and experience an error if the mapping table
> > > author didn't create that name?
if want to keep align with regulator and clock,
i just feel a little that it does not make too much sense to
internally convert NULL to "default", right?
Regards
Dong Aisheng
--
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