linux-kernel - Re: [PATCH 3/3] PM / OPP: Parse 'opp-<prop>-<name>' bindings

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20151119030056.GM7336@ubuntu>
Date:	Thu, 19 Nov 2015 08:30:56 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	Stephen Boyd <sboyd@...eaurora.org>
Cc:	Rafael Wysocki <rjw@...ysocki.net>, linaro-kernel@...ts.linaro.org,
	linux-pm@...r.kernel.org, robh+dt@...nel.org, nm@...com,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
	Dmitry Torokhov <dtor@...omium.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Len Brown <len.brown@...el.com>,
	open list <linux-kernel@...r.kernel.org>,
	Pavel Machek <pavel@....cz>, Shawn Guo <shawnguo@...nel.org>
Subject: Re: [PATCH 3/3] PM / OPP: Parse 'opp-<prop>-<name>' bindings

On 18-11-15, 17:12, Stephen Boyd wrote:
> > +	/* Operations on OPP structures must be done from within rcu locks */
> > +	rcu_read_lock();
> > +
> > +	dev_opp = _add_device_opp(dev);
> > +	if (!dev_opp)
> > +		return -ENOMEM;
> > +
> > +	/* Do we already have a prop-name associated with dev_opp? */
> > +	if (dev_opp->prop_name) {
> > +		dev_err(dev, "%s: Already have prop-name %s\n", __func__,
> > +			dev_opp->prop_name);
> > +		ret = -EINVAL;
> > +		goto unlock;
> > +	}
> > +
> > +	dev_opp->prop_name = kstrdup(name, GFP_KERNEL);
> 
> I'm very confused now on the whole locking scheme. How can we be
> modifying the dev_opp under an rcu_read_lock? Don't we need to
> hold a stronger lock than a read lock because _add_device_opp()
> has already published the structure we're modifying here?

Yeah, it should be called from within the mutex lock we have.

> How is cpufreq-dt going to be changed to support this?

Maybe not at all :)

> I wonder
> if it would be better to hide these sorts of things behind a
> wrapper on top of the guts of dev_pm_opp_of_add_table(). That way
> the lifetime of the prop_name and hw_versions are contained to
> the same lifetime rules as the device's OPP table. Right now it
> seems like we're asking OPP initializers to call two or three
> functions in the right order to get the table initialized
> properly, which is not as easy as calling one function.

Yeah, so things should happen in order, but the caller for the new
routines can't be cpufreq-dt (well it can be, but then some
information is required via platform data).

These routines are supposed to be called by some sort of platform
specific code, as we need to read some efuses from hardware to know
about all this. And cpufreq-dt can't decide that.

Though, to keep things simple, we can put that information in platform
data, so that only cpufreq-dt does all the stuff in the correct order.

We can always add a wrapper which will add hardware-versions,
named-properties and finally initialize opp table. But I would like to
wait a bit for that.

Now that I need to update 2/3 again, due to your comments on this
patch, I will repost this series again instead of messing up here.

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