On 28-11-16, 18:46, Stephen Boyd wrote:
That's a lot of lines for something that we want to backport to stable kernels!
Hmm, I agree.
The whole dev_list design seems fairly broken to me. Another solution would be to iterate the cpumask in reverse, but there doesn't seem to be a construct for that and adding one is probably not worth the effort.
Adding yet another member to the structure and doing accounting in different places seems to be papering over the problem as well. Now we want to have "inactive" devices in the list? That seems like a problem for cpufreq to solve. It can decide to not call OPP APIs when the cpu device isn't actually physically removed if it wants to.
It also exposes the OPP API's strong reliance on struct device for everything. Really we shouldn't be storing device pointers in the OPP core at all because we're not treating them like the reference counted objects they are. The dev_list should go probably go away and be replaced with some sort of counter. It would also be nice if struct device had a pointer to the OPP table(s) for a device so the lookup is direct.
If the struct device gets a pointer to the opp-table, then yes we can kill the dev-list completely. I will work on cleaning up OPP core a bit later on.
BTW, _dev_pm_opp_remove_table() calls _find_opp_dev() twice, once to find the opp_table for a device and then to find the opp_device inside the table that was used to match up the table in the first place. Madness!
Anyway, rant over, how about handing out the opp table pointer to the caller so they can pass it back in when they call the put side? That should fix the same problem if I understand correctly.
Yes, that can be a solution for the time being.
We should think about changing the API further so that callers have to "get" the OPP table cookie for their device and then pass that pointer to the dev_pm_*_set() APIs instead of passing a struct device pointer. That would save lots of cycles searching for something we already had.
Hmm, we need to do some cleanup soon I believe. Also note that we want to kill the RCU thing :)
-static inline void dev_pm_opp_put_regulator(struct device *dev) {} +static inline void dev_pm_opp_put_regulator(struct opp_table *opp_table) {}
We need to modify few more things as well. I will send a patch for that soon.