Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT

[Viresh Kumar]

On 02-02-16, 22:53, Rafael J. Wysocki wrote:
> First of all, this is effectively reverting commit 955ef4833574, so
> the subject should be "Revert commit 955ef4833574 (cpufreq: Drop rwsem
> lock around CPUFREQ_GOV_POLICY_EXIT)".
> 
> There should be a Fixes: tag pointing to commit 955ef4833574 and a
> Reported-by: for Juri.
> 
> If there is a link to a bug report related to this, it should be
> pointed to by a Link: tag.
> 
> The changelog should say why the original commit was there and why the
> way it attempted to solve the problem was incorrect.  It also should
> say that the original problem was addressed by a previous commit, so
> this one can be reverted without consequences.

How about this:

    Revert "cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT"
    
    Earlier, when the struct freq-attr was used to represent governor
    attributes, the standard cpufreq show/store sysfs attribute callbacks
    were applied to the governor tunable attributes and they always acquire
    the policy->rwsem lock before carrying out the operation.  That could
    have resulted in an ABBA deadlock if governor tunable attributes are
    removed under policy->rwsem while one of them is being accessed
    concurrently (if sysfs attributes removal wins the race, it will wait
    for the access to complete with policy->rwsem held while the attribute
    callback will block on policy->rwsem indefinitely).
    
    We attempted to address this issue by dropping policy->rwsem around
    governor tunable attributes removal (that is, around invocations of the
    ->governor callback with the event arg equal to CPUFREQ_GOV_POLICY_EXIT)
    in cpufreq_set_policy(), but that opened up race conditions that had not
    been possible with policy->rwsem held all the time.
    
    The previous commit, "cpufreq: governor: New sysfs show/store callbacks
    for governor tunables", fixed the original ABBA deadlock by adding new
    governor specific show/store callbacks.
    
    We don't have to drop rwsem around invocations of governor event
    CPUFREQ_GOV_POLICY_EXIT anymore, and original fix can be reverted now.
    
    Fixes: 955ef4833574 ("cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT")
    Reported-by: Juri Lelli <juri.lelli@xxxxxxx>
    Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>

> But I'm not going to write that changelog.  I actually am not going to
> write any changelogs for you any more, because I'm seriously tired of
> doing that.  Moreover, if I see a patch from you with a changelog
> that's not acceptable to me, it will immediately go to the "not
> applicable" trash bin no matter what the changes below look like.  You
> *have* *to* write useful changelogs.  This isn't optional or best
> effort.  This is mandatory and important.

Will try to improve, sorry about that (again).

> Now, I'm not really sure if the ordering of this patchset is right.
> Maybe we should just revert upfront with the "we'll address the
> original problem in the following commits" statement in the changelog
> and fix it in a different way?

Wouldn't that break things like 'git bisect'? People running kernels
after the reverted commits may start hitting lockdeps.

> It looks like patches [1-3/5] fix a
> problem that isn't there even, but would appear after the [4/5] if
> they were not applied previously, which doesn't sound really
> straightforward to me.

I am going to fight hard for it, if you feel 4/5 should be the first
patch here, I will do that.

-- 
viresh