linux-kernel - Re: [PATCH v2 4/4] thermal: intel: hfi: Add a suspend notifier

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZZV3CgnaWznmzFKF@linux.intel.com>
Date: Wed, 3 Jan 2024 16:02:34 +0100
From: Stanislaw Gruszka <stanislaw.gruszka@...ux.intel.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	Chen Yu <yu.c.chen@...el.com>, Len Brown <len.brown@...el.com>,
	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
	Zhang Rui <rui.zhang@...el.com>, Zhao Liu <zhao1.liu@...el.com>,
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/4] thermal: intel: hfi: Add a suspend notifier

On Wed, Jan 03, 2024 at 02:34:26PM +0100, Rafael J. Wysocki wrote:
> > +static int hfi_pm_notify(struct notifier_block *nb,
> > +                        unsigned long mode, void *unused)
> > +{
> > +       struct hfi_cpu_info *info = &per_cpu(hfi_cpu_info, 0);
> > +       struct hfi_instance *hfi = info->hfi_instance;
> > +       int ret = 0;
> > +
> > +       /* HFI may not be in use. */
> > +       if (!hfi)
> > +               return ret;
> > +
> > +       mutex_lock(&hfi_instance_lock);
> > +       /*
> > +        * Only handle the HFI instance of the package of the boot CPU. The
> > +        * instances of other packages are handled in the CPU hotplug callbacks.
> > +        */
> > +       switch (mode) {
> > +       case PM_HIBERNATION_PREPARE:
> > +       case PM_SUSPEND_PREPARE:
> > +       case PM_RESTORE_PREPARE:
> > +               ret = smp_call_function_single(0, hfi_do_disable, NULL, true);
> > +               break;
> > +
> > +       case PM_POST_RESTORE:
> > +       case PM_POST_HIBERNATION:
> > +       case PM_POST_SUSPEND:
> > +               ret = smp_call_function_single(0, hfi_do_enable, hfi, true);
> > +               break;
> 
> Because this handles the boot CPU only, one has to wonder if it should
> be a syscore op rather than a PM notifier.
> 
> It does not sleep AFAICS, so it can run in that context, and it is
> guaranteed to run on the boot CPU then, so it is not necessary to
> force that.  Moreover, syscore ops are guaranteed to be
> non-concurrent, so locking is not needed.

There are below warnings in smp_call_function_single() :

        /*
         * Can deadlock when called with interrupts disabled.
         * We allow cpu's that are not yet online though, as no one else can
         * send smp call function interrupt to this cpu and as such deadlocks
         * can't happen.
         */
        WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled()
                     && !oops_in_progress);

        /*
         * When @wait we can deadlock when we interrupt between llist_add() and
         * arch_send_call_function_ipi*(); when !@...t we can deadlock due to
         * csd_lock() on because the interrupt context uses the same csd
         * storage.
         */
        WARN_ON_ONCE(!in_task());

And this one in syscore_suspend(): 

        WARN_ONCE(!irqs_disabled(),
                "Interrupts enabled before system core suspend.\n");
                                                                     
So seems they are not compatible.

Regards
Stanislaw