'cached_raw_freq' is used to get the next frequency quickly but should
always be in sync with sg_policy->next_freq. There is a case where it is
not and in such cases it should be reset to avoid switching to incorrect
frequencies.
Consider this case for example:
- policy->cur is 1.2 GHz (Max)
- New request comes for 780 MHz and we store that in cached_raw_freq.
- Based on 780 MHz, we calculate the effective frequency as 800 MHz.
- We then see the CPU wasn't idle recently and choose to keep the next
freq as 1.2 GHz.
- Now we have cached_raw_freq is 780 MHz and sg_policy->next_freq is 1.2
GHz.
- Now if the utilization doesn't change in then next request, then the
next target frequency will still be 780 MHz and it will match with
cached_raw_freq. But we will choose 1.2 GHz instead of 800 MHz here.
Change-Id: I71bd31a1b59d27c26c0b4885301e4ba6155c2c51
Cc: <stable(a)vger.kernel.org> # 4.12
Fixes: b7eaf1aab9f8 ("cpufreq: schedutil: Avoid reducing frequency of busy CPUs prematurely")
Signed-off-by: Viresh Kumar <viresh.kumar(a)linaro.org>
---
kernel/sched/cpufreq_schedutil.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index ba0da243fdd8..2f52ec0f1539 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -282,8 +282,12 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
* Do not reduce the frequency if the CPU has not been idle
* recently, as the reduction is likely to be premature then.
*/
- if (busy && next_f < sg_policy->next_freq)
+ if (busy && next_f < sg_policy->next_freq) {
next_f = sg_policy->next_freq;
+
+ /* Reset cached freq as next_freq has changed */
+ sg_policy->cached_raw_freq = 0;
+ }
}
sugov_update_commit(sg_policy, time, next_f);
}
--
2.15.0.rc1.236.g92ea95045093
On Wed, Nov 8, 2017 at 12:06 AM, Rafael J. Wysocki <rjw(a)rjwysocki.net> wrote:
> On Tuesday, November 7, 2017 11:47:54 PM CET Rafael J. Wysocki wrote:
>> On Tue, Nov 7, 2017 at 10:08 PM, Ville Syrjala
>> <ville.syrjala(a)linux.intel.com> wrote:
>> > From: Ville Syrjälä <ville.syrjala(a)linux.intel.com>
>> >
>> > acpi_remove_pm_notifier() ends up calling flush_workqueue() while
>> > holding acpi_pm_notifier_lock, and that same lock is taken by
>> > by the work via acpi_pm_notify_handler(). This can deadlock.
>>
>> OK, good catch!
>>
>> [cut]
>>
>> >
>> > Cc: stable(a)vger.kernel.org
>> > Cc: Rafael J. Wysocki <rafael.j.wysocki(a)intel.com>
>> > Cc: Len Brown <lenb(a)kernel.org>
>> > Cc: Peter Zijlstra <peterz(a)infradead.org>
>> > Cc: Tejun Heo <tj(a)kernel.org>
>> > Cc: Ingo Molnar <mingo(a)kernel.org>
>> > Fixes: c072530f391e ("ACPI / PM: Revork the handling of ACPI device wakeup notifications")
>> > Signed-off-by: Ville Syrjälä <ville.syrjala(a)linux.intel.com>
>> > ---
>> > drivers/acpi/device_pm.c | 21 ++++++++++++---------
>> > 1 file changed, 12 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
>> > index fbcc73f7a099..18af71057b44 100644
>> > --- a/drivers/acpi/device_pm.c
>> > +++ b/drivers/acpi/device_pm.c
>> > @@ -387,6 +387,7 @@ EXPORT_SYMBOL(acpi_bus_power_manageable);
>> >
>> > #ifdef CONFIG_PM
>> > static DEFINE_MUTEX(acpi_pm_notifier_lock);
>> > +static DEFINE_MUTEX(acpi_pm_notifier_install_lock);
>> >
>> > void acpi_pm_wakeup_event(struct device *dev)
>> > {
>> > @@ -443,24 +444,25 @@ acpi_status acpi_add_pm_notifier(struct acpi_device *adev, struct device *dev,
>> > if (!dev && !func)
>> > return AE_BAD_PARAMETER;
>> >
>> > - mutex_lock(&acpi_pm_notifier_lock);
>> > + mutex_lock(&acpi_pm_notifier_install_lock);
>> >
>> > if (adev->wakeup.flags.notifier_present)
>> > goto out;
>> >
>> > - adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
>> > - adev->wakeup.context.dev = dev;
>> > - adev->wakeup.context.func = func;
>> > -
>>
>> But this doesn't look good to me.
>>
>> notifier_present should be checked under acpi_pm_notifier_lock.
>>
>> Actually, acpi_install_notify_handler() itself need not be called
>> under the lock, because it does sufficient checks of its own.
>>
>> So say you do
>>
>> mutex_lock(&acpi_pm_notifier_lock);
>>
>> if (adev->wakeup.context.dev)
>> goto out;
>>
>> adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
>> adev->wakeup.context.dev = dev;
>> adev->wakeup.context.func = func;
>>
>> mutex_unlock(&acpi_pm_notifier_lock);
>>
>> > status = acpi_install_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
>> > acpi_pm_notify_handler, NULL);
>> > if (ACPI_FAILURE(status))
>> > goto out;
>> >
>> > + mutex_lock(&acpi_pm_notifier_lock);
>>
>> And here you just set notifier_present under acpi_pm_notifier_lock.
>>
>> > + adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
>> > + adev->wakeup.context.dev = dev;
>> > + adev->wakeup.context.func = func;
>> > adev->wakeup.flags.notifier_present = true;
>> > + mutex_unlock(&acpi_pm_notifier_lock);
>> >
>> > out:
>> > - mutex_unlock(&acpi_pm_notifier_lock);
>> > + mutex_unlock(&acpi_pm_notifier_install_lock);
>> > return status;
>> > }
>>
>> Then on removal you can clear notifier_present first and drop the lock
>> around the acpi_remove_notify_handler() call and nothing bad will
>> happen.
>>
>> If you call acpi_add_pm_notifier() twice in parallel, the first
>> instance will set context.dev and the second one will see it set and
>> bail out. The first instance will then do the rest.
>>
>> If you call acpi_remove_pm_notifier() twice in a row, the first
>> instance will see notifier_present set and will clear it, so the
>> second one will see notifier_present unset and it will bail out.
>>
>> Now, if you call acpi_remove_pm_notifier() in parallel with
>> acpi_add_pm_notifier(), either the former will see notifier_present
>> unset and bail out, or the latter will see context.dev unset and bail
>> out.
>>
>> It doesn't look like the outer lock is needed, or have I missed anything?
>
> So something like the below (totally untested) should work too, shouldn't it?
There is a problem if a device is removed while acpi_add_pm_notifier()
is still in progress, in which case with my patch the
acpi_remove_pm_notifier() called from the removal path may bail out
prematurely (that doesn't seem likely to happen, but still I don't see
why it cannot happen), so I'll just use your patch. :-)
Thanks,
Rafael
Arnd Bergmann <arnd(a)arndb.de> wrote:
> We set rtlhal->last_suspend_sec to an uninitialized stack variable,
> but unfortunately gcc never warned about this, I only found it
> while working on another patch. I opened a gcc bug for this.
>
> Presumably the value of rtlhal->last_suspend_sec is not all that
> important, but it does get used, so we probably want the
> patch backported to stable kernels.
>
> Cc: stable(a)vger.kernel.org
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82839
> Signed-off-by: Arnd Bergmann <arnd(a)arndb.de>
> Acked-by: Larry Finger <Larry.Finger(a)lwfinger.net>
3 patches applied to wireless-drivers-next.git, thanks.
3f2a162fab15 rtlwifi: fix uninitialized rtlhal->last_suspend_sec time
3c92d5517af8 rtlwifi: use ktime_get_real_seconds() for suspend time
ac978dc79a91 rtlwifi: drop unused ppsc->last_wakeup_time
--
https://patchwork.kernel.org/patch/10043505/https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatc…
On Tue, Nov 7, 2017 at 11:47 PM, Rafael J. Wysocki <rafael(a)kernel.org> wrote:
> On Tue, Nov 7, 2017 at 10:08 PM, Ville Syrjala
> <ville.syrjala(a)linux.intel.com> wrote:
>> From: Ville Syrjälä <ville.syrjala(a)linux.intel.com>
>>
>> acpi_remove_pm_notifier() ends up calling flush_workqueue() while
>> holding acpi_pm_notifier_lock, and that same lock is taken by
>> by the work via acpi_pm_notify_handler(). This can deadlock.
>
> OK, good catch!
>
> [cut]
>
>>
>> Cc: stable(a)vger.kernel.org
>> Cc: Rafael J. Wysocki <rafael.j.wysocki(a)intel.com>
>> Cc: Len Brown <lenb(a)kernel.org>
>> Cc: Peter Zijlstra <peterz(a)infradead.org>
>> Cc: Tejun Heo <tj(a)kernel.org>
>> Cc: Ingo Molnar <mingo(a)kernel.org>
>> Fixes: c072530f391e ("ACPI / PM: Revork the handling of ACPI device wakeup notifications")
>> Signed-off-by: Ville Syrjälä <ville.syrjala(a)linux.intel.com>
>> ---
>> drivers/acpi/device_pm.c | 21 ++++++++++++---------
>> 1 file changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
>> index fbcc73f7a099..18af71057b44 100644
>> --- a/drivers/acpi/device_pm.c
>> +++ b/drivers/acpi/device_pm.c
>> @@ -387,6 +387,7 @@ EXPORT_SYMBOL(acpi_bus_power_manageable);
>>
>> #ifdef CONFIG_PM
>> static DEFINE_MUTEX(acpi_pm_notifier_lock);
>> +static DEFINE_MUTEX(acpi_pm_notifier_install_lock);
>>
>> void acpi_pm_wakeup_event(struct device *dev)
>> {
>> @@ -443,24 +444,25 @@ acpi_status acpi_add_pm_notifier(struct acpi_device *adev, struct device *dev,
>> if (!dev && !func)
>> return AE_BAD_PARAMETER;
>>
>> - mutex_lock(&acpi_pm_notifier_lock);
>> + mutex_lock(&acpi_pm_notifier_install_lock);
>>
>> if (adev->wakeup.flags.notifier_present)
>> goto out;
>>
>> - adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
>> - adev->wakeup.context.dev = dev;
>> - adev->wakeup.context.func = func;
>> -
>
> But this doesn't look good to me.
>
> notifier_present should be checked under acpi_pm_notifier_lock.
Well, not really, so the above is OK.
However, I still would prefer to avoid adding the outer lock.
Thanks,
Rafael
On 11/03/2017 11:56 PM, James Smart wrote:
> In test cases where an instance of the driver is detached and
> reattached, the driver will crash on reattachment. There is a
> compound if statement that will skip over the bar setup if
> the pci_resource_start call is not successful. The driver
> erroneously returns success to its bar setup in this scenario
> even though the bars aren't properly configured.
>
> Rework the offending code segment for proper initialization steps.
> If the pci_resource_start call fails, -ENOMEM is now returned.
>
> Sample stack:
>
> rport-5:0-10: blocked FC remote port time out: removing rport
> BUG: unable to handle kernel NULL pointer dereference at (null)
> ... lpfc_sli4_wait_bmbx_ready+0x32/0x70 [lpfc]
> ...
> ... RIP: 0010:... ... lpfc_sli4_wait_bmbx_ready+0x32/0x70 [lpfc]
> Call Trace:
> ... lpfc_sli4_post_sync_mbox+0x106/0x4d0 [lpfc]
> ... ? __alloc_pages_nodemask+0x176/0x420
> ... ? __kmalloc+0x2e/0x230
> ... lpfc_sli_issue_mbox_s4+0x533/0x720 [lpfc]
> ... ? mempool_alloc+0x69/0x170
> ... ? dma_generic_alloc_coherent+0x8f/0x140
> ... lpfc_sli_issue_mbox+0xf/0x20 [lpfc]
> ... lpfc_sli4_driver_resource_setup+0xa6f/0x1130 [lpfc]
> ... ? lpfc_pci_probe_one+0x23e/0x16f0 [lpfc]
> ... lpfc_pci_probe_one+0x445/0x16f0 [lpfc]
> ... local_pci_probe+0x45/0xa0
> ... work_for_cpu_fn+0x14/0x20
> ... process_one_work+0x17a/0x440
>
> Cc: <stable(a)vger.kernel.org> # 4.12+
> Signed-off-by: Dick Kennedy <dick.kennedy(a)broadcom.com>
> Signed-off-by: James Smart <james.smart(a)broadcom.com>
> ---
> drivers/scsi/lpfc/lpfc_init.c | 84 ++++++++++++++++++++++++++-----------------
> 1 file changed, 51 insertions(+), 33 deletions(-)
>
Reviewed-by: Hannes Reinecke <hare(a)suse.com>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare(a)suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)