Received-SPF: pass (google.com: domain of linux-kernel+bounces-15850-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240103041459.11113-1-ricardo.neri-calderon@linux.intel.com>
 <20240103041459.11113-5-ricardo.neri-calderon@linux.intel.com>
 <CAJZ5v0jSERoeMki9ZvWtTqiZidETeo1Xm_Qb0Oo2qRG0PMrSJQ@mail.gmail.com> <ZZV3CgnaWznmzFKF@linux.intel.com>
In-Reply-To: <ZZV3CgnaWznmzFKF@linux.intel.com>
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: Wed, 3 Jan 2024 19:21:10 +0100
Message-ID: <CAJZ5v0i5CwLJ7Fb-eZH+azxwnhMQ2QxyBAD58mGL151YpWqZXA@mail.gmail.com>
Subject: Re: [PATCH v2 4/4] thermal: intel: hfi: Add a suspend notifier
To: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>, Ricardo Neri <ricardo.neri-calderon@linux.intel.com>, 
	"Rafael J. Wysocki" <rafael.j.wysocki@intel.com>, Chen Yu <yu.c.chen@intel.com>, 
	Len Brown <len.brown@intel.com>, 
	Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>, Zhang Rui <rui.zhang@intel.com>, 
	Zhao Liu <zhao1.liu@intel.com>, linux-pm@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 3, 2024 at 6:26=E2=80=AFPM Stanislaw Gruszka
<stanislaw.gruszka@linux.intel.com> wrote:
>
> 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 =3D &per_cpu(hfi_cpu_info, 0);
> > > +       struct hfi_instance *hfi =3D info->hfi_instance;
> > > +       int ret =3D 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 CP=
U. 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 =3D smp_call_function_single(0, hfi_do_disable, N=
ULL, true);
> > > +               break;
> > > +
> > > +       case PM_POST_RESTORE:
> > > +       case PM_POST_HIBERNATION:
> > > +       case PM_POST_SUSPEND:
> > > +               ret =3D smp_call_function_single(0, hfi_do_enable, hf=
i, 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 deadl=
ocks
>          * 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 !@wait we can deadlock du=
e 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.

But smp_call_function_single() need not be used in syscore ops at all,
because they always run on the boot CPU.