On 01/13, Viresh Kumar wrote:
On 12-01-16, 11:45, Stephen Boyd wrote:
- /*
* 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..
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.