On Tue, Feb 2, 2016 at 11:57 AM, Viresh Kumar viresh.kumar@linaro.org wrote:
Hi Rafael,
Sorry for doing this, I know you were also looking to fix this in a possibly different way. But I thought, it would be better if we fix that. We can scrap this version and take yours if that looks better.
That's not nice to be honest. At the very least you could have asked me about the status of my work before sending this.
Fortunately, though, when I started to look deeper into fixing this problem I thought I didn't like the overall design of things in the governor land, so I started to change that and my modifications turn out to be sort of complementary with respect to this patchset. Of course, they do conflict, but I can redo my patches on top of these if that's necessary. That said I'm going to post them in their current form anyway, at least to show the direction I want to take going forward.
The root cause of all the issues we were facing, was that we were taking policy->rwsem while accessing governor sysfs attributes. And that happened because we were sharing the show/store calls present in cpufreq.c.
I thought, perhaps the best way to fix it is to give separate sysfs-ops to governors. And that's what I did.
Yes, that's what I was talking about in the other thread.
My overall impression here is that the code changes make sense. Some details need to be improved (like the concurrent ->store for governor tunables pointed out by Juri). The patch changelogs suck, though.
If your hope was that this might go into 4.5, there is a small chance of that happening, but only if it can be made ready this week. Otherwise, I'd prefer it to be redone on top of my changes.
Let me comment on the individual patches.
Thanks, Rafael