Re: stable 3-10-3: strange output of "lsmod | grep ^acpi_cpufreq"

["Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>]

On 07/29/2013 05:19 PM, Rafael J. Wysocki wrote:
> On Monday, July 29, 2013 03:11:45 PM Srivatsa S. Bhat wrote:
>> On 07/29/2013 04:50 AM, Rafael J. Wysocki wrote:
>>> On Monday, July 29, 2013 12:43:59 AM Rafael J. Wysocki wrote:
>>>> On Monday, July 29, 2013 12:11:18 AM Rafael J. Wysocki wrote:
>>>>> On Sunday, July 28, 2013 12:21:22 PM Toralf Förster wrote:
>>>>>> On 07/28/2013 01:39 AM, Rafael J. Wysocki wrote:
>>>>>>> On Saturday, July 27, 2013 07:40:34 PM Toralf Förster wrote:
>>>>>>>> it gives at a ThinkPad T420:
>>>>>>>>
>>>>>>>> tfoerste@n22 ~/tmp $ lsmod | grep ^acpi_cpufreq
>>>>>>>> acpi_cpufreq           12902  2147483647
>>>>>>>
>>>>>>> That is -1, which indicates some module refcount woes.
>>>>>>
>>>>>> yes, ofc.
>>>>>>
>>>>>> The issue apears after 1 s2ram/resume cycle, before s2ram the refcount is 1.
>>>>>>
>>>>>>> I definitely can't see that with the mainline on my machines.
>>>>>>
>>>>>> It is in mainline too.
>>>>>
>>>>> Does the appended patch help?
>>>>
>>>> Actually, something as simple as this also should help:
>>>>
>>>> ---
>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>> Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume
>>>>
>>>> Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the
>>>> driver module refcount, __cpufreq_remove_dev() causes that refcount
>>>> to become negative after a suspend/resume cycle, for example.
>>>>
>>>> To prevent this from happening make __cpufreq_remove_dev() put
>>>> the policy kobject only instead of calling cpufreq_cpu_put().
>>>
>>> Having a deeper look at it, though, I see that in fact the whole
>>> cpufreq_cpu_put() is needed if the CPU is not the last one for the given
>>> policy and is not needed at all otherwise (as described in the changelog
>>> of the patch below).
>>>
>>> Srivatsa, does this make sense to you?
>>>
>>
>> Code-wise, your patch looks good to me. But one thing in the existing code
>> struck me as a little strange.
>>
>> I'm assuming that the module_get() is used in the cpufreq core to ensure that
>> until the cpufreq core is managing (atleast one) CPU(s), the cpufreq backend
>> driver module (eg: acpi-cpufreq) can't be removed.
> 
> Quite frankly, I'm not sure about that.  If that were the case,
> cpufreq_add_dev() would not call module_put() which it does.
> 
> That may be a bug, I agree, but that's not for the present release cycle.  For
> now, we need to ensure that the reference counts are *balanced* .
> 

Sure, in that case, I agree that your patch is the right fix at this point,
since it resolves the immediate problem that we have with the refcounts.

Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Regards,
Srivatsa S. Bhat