On Monday, July 29, 2013 05:14:25 PM Srivatsa S. Bhat wrote: > 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@xxxxxxxxx> > >>>> 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@xxxxxxxxxxxxxxxxxx> Great, thanks! Does your patchset avoiding the creation/removal of sysfs directories over suspend/resume need to be modified to take this patch into account? Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe cpufreq" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
