linux-kernel - RE: [PATCH 19/20] pinctrl: API changes to support multiple states per device

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <74CDBE0F657A3D45AFBB94109FB122FF17BD8BCFCD@HQMAIL01.nvidia.com>
Date:	Mon, 27 Feb 2012 10:37:16 -0800
From:	Stephen Warren <swarren@...dia.com>
To:	Dong Aisheng <aisheng.dong@...escale.com>
CC:	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 19/20] pinctrl: API changes to support multiple states
 per device

Dong Aisheng wrote at Monday, February 27, 2012 2:07 AM:
> On Sun, Feb 19, 2012 at 11:45:59PM -0700, Stephen Warren wrote:

> > -	.name = "POWERMAP"
> > +	.dev_name = "pinctrl-foo",
> > +	.name = "active",
>
> I guess if we really decide to use a fixed state name for hog functions,
> we'd better not let users to write the state name, at least provide a macro.

Yes, that's a good point for adding the special-case macros. I have
added such special-case macros to my local version of patch 20, so they'll
be there in the repost.

> > +static struct pinctrl *pinctrl_get_locked(struct device *dev)
> > +{
> > +	struct pinctrl *p;
> >
> > -error:
> > -	list_for_each_entry(setting, &p->settings, node)
> > -		pinmux_free_setting(setting);
> > +	if (WARN_ON(!dev))
> > +		return ERR_PTR(-EINVAL);
> >
> > -	kfree(p);
> > +	p = find_pinctrl(dev);
> > +	if (p == NULL)
> > +		p = create_pinctrl(dev);
> > +	if (IS_ERR(p))
> > +		return p;
> > +
> > +	p->usecount++;
>
> I still can not understand what's the purpose of p->usecount?
> For allowing multi times calling of pinctrl_get for on the same device?

pinctrl_get() could be called multiple times for the same device. Rather
than create a whole new struct pinctrl each time it's called, we just
reference count the object so that each call returns the same one, and
it won't be destroyed until all users have called pinctrl_put().

Hopefully it is true that multiple different pieces of code won't be
screwing with the same device's pinctrl settings, but it's simple enough
to do this so we may as well. This somewhat of a the moral equivalent of
the old code's p->usecount manipulations in pinctrl_enable()/disable(),
although admittedly a little different.

> > +static inline struct pinctrl * __must_check pinctrl_get_select(
> > +					struct device *dev, const char *name)
> > +{
> > +	struct pinctrl *p;
> > +	struct pinctrl_state *s;
> > +	int ret;
> > +
> > +	p = pinctrl_get(dev);
> > +	if (IS_ERR(p))
> > +		return p;
> > +
> > +	s = pinctrl_lookup_state(p, name);
> > +	if (IS_ERR(s)) {
> > +		pinctrl_put(p);
> > +		return ERR_PTR(PTR_ERR(s));
> > +	}
> > +
> > +	ret = pinctrl_select_state(p, s);
> > +	if (ret < 0) {
> > +		pinctrl_put(p);
> > +		return ERR_PTR(ret);
>
> s/ERR_PTR(ret)/ret ?

The function returns a pointer, whereas ret is an int. ERR_PTR() is used
to wrap the int error code into a pointer value so that the function can
return either a valid pointer, or an error-code. See include/linux/err.h.

I've fixed locally up the other issues you pointed out. Thanks.

-- 
nvpublic

--
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/