On 01/13, Viresh Kumar wrote:

On 12-01-16, 11:45, Stephen Boyd wrote:

quoted

quoted

+	/*
+	 * Hold our list modification lock here as regulator_set_voltage_time()
+	 * can possibly take another mutex, which isn't allowed within rcu
+	 * locks.
+	 */
+	mutex_lock(&dev_opp_list_lock);

So now we take the list modification mutex. Why can't we
rcu_read_lock(), find the min and max voltage, and then release
the read lock and ask regulator framework for the voltage time?

That was possible before this series came in..

quoted

From what I can tell we're just trying to make sure that the list
is stable when iterating through it to find the min/max voltage.

Hmm, we have pointer to regulator within the dev-opp struct. If we
drop the lock, the dev-opp struct can get freed and regulator will be
put just before that. So, we may be using an already freed resource.

How do you suggest we fix it ?

Ok, first off, I don't understand why the regulator and clock
pointers are in the struct device_opp instead of the struct
device_list_opp. I thought we wanted to make it possible for two
devices to share the same OPP table (device_opp), but have
physically different clocks and regulators (non opp-shared case)?
If we put the clock and regulator handles in the device_opp then
the opp table is limited to one device or a set of devices that
all share the same clock and regulators.

BTW, these structure names confuse me all the time. I have to
rename dev_pm_opp to opp_entry, device_opp to opp_table, and
device_list_opp to opp_device_list in my head for anything to
make sense.

Gripes aside, the clock and regulator pointers should never be
'put' and go away until the device driver that's using the
dev_pm_opp_set_rate() API has called dev_pm_opp_remove(). So any
concerns about that happening during an OPP switch aren't the
concern of the OPP framework, but the concern of the consumer
drivers having the proper locking and tear down sequences.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project