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

Powered by blists - more mailing lists