netdev - Re: [PATCH v4 3/3] thermal: intel: hfi: Enable interface only when required

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: <Zdd8AT5+6oLX4eCk@linux.intel.com>
Date: Thu, 22 Feb 2024 17:53:21 +0100
From: Stanislaw Gruszka <stanislaw.gruszka@...ux.intel.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: linux-pm@...r.kernel.org,
	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
	Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Jiri Pirko <jiri@...nulli.us>,
	Johannes Berg <johannes@...solutions.net>,
	Florian Westphal <fw@...len.de>, netdev@...r.kernel.org
Subject: Re: [PATCH v4 3/3] thermal: intel: hfi: Enable interface only when
 required

On Tue, Feb 13, 2024 at 02:59:10PM +0100, Rafael J. Wysocki wrote:
> > -static void hfi_do_enable(void)
> > +/*
> > + * HFI enable/disable run in non-concurrent manner on boot CPU in syscore
> > + * callbacks or under protection of hfi_instance_lock.
> > + */
> 
> In the comment above I would say "If concurrency is not prevented by
> other means, the HFI enable/disable routines must be called under
> hfi_instance_lock." 

Ok. Will reword this way.

> and I would retain the comments below (they don't
> hurt IMO).

I found those comments somewhat confusing. FWICT at worst
what can happen when enable/resume race CPU online and
disable/suspend race with CPU offline is enable twice
or disable twice. What I think is fine, though plan to
check this (see below).

> > +static void hfi_do_enable(void *ptr)
> 
> I would call this hfi_enable_instance().
> 
> > +{
> > +       struct hfi_instance *hfi_instance = ptr;
> 
> Why is this variable needed ro even useful?  prt can be passed
> directly to hfi_set_hw_table().

Ok, will remove it.

> > +
> > +       hfi_set_hw_table(hfi_instance);
> > +       hfi_enable();
> > +}
> > +
> > +static void hfi_do_disable(void *ptr)
> 
> And I'd call this hfi_disable_instance().

Ok.

> > +static int hfi_thermal_notify(struct notifier_block *nb, unsigned long state,
> > +                             void *_notify)
> > +{
> > +       struct thermal_genl_notify *notify = _notify;
> > +       struct hfi_instance *hfi_instance;
> > +       smp_call_func_t func;
> > +       unsigned int cpu;
> > +       int i;
> > +
> > +       if (notify->mcgrp != THERMAL_GENL_EVENT_GROUP)
> > +               return NOTIFY_DONE;
> > +
> > +       if (state != THERMAL_NOTIFY_BIND && state != THERMAL_NOTIFY_UNBIND)
> > +               return NOTIFY_DONE;
> > +
> > +       mutex_lock(&hfi_instance_lock);
> > +
> > +       switch (state) {
> > +       case THERMAL_NOTIFY_BIND:
> > +               hfi_thermal_clients_num++;
> > +               break;
> > +
> > +       case THERMAL_NOTIFY_UNBIND:
> > +               hfi_thermal_clients_num--;
> > +               break;
> > +       }
> > +
> > +       if (hfi_thermal_clients_num > 0)
> > +               func = hfi_do_enable;
> > +       else
> > +               func = hfi_do_disable;
> > +
> > +       for (i = 0; i < max_hfi_instances; i++) {
> > +               hfi_instance = &hfi_instances[i];
> > +               if (cpumask_empty(hfi_instance->cpus))
> > +                       continue;
> > +
> > +               cpu = cpumask_any(hfi_instance->cpus);
> > +               smp_call_function_single(cpu, func, hfi_instance, true);
> > +       }
> > +
> > +       mutex_unlock(&hfi_instance_lock);
> 
> So AFAICS, one instance can be enabled multiple times because of this.
>   I guess that's OK?  In any case, it would be kind of nice to leave a
> note regarding it somewhere here.

It's write the same values to the same registers. So I think this 
should be fine. However after your comment I start to think there
perhaps could be some side-effect of writing the registers.
I'll double check (previously I verified that double enable works,
but only on MTL) or eventually rearrange code to do not enable already
enabled interface.

> > +
> > +       return NOTIFY_OK;
> > +}
> > +
> > +static struct notifier_block hfi_thermal_nb = {
> > +       .notifier_call = hfi_thermal_notify,
> >  };
> >
> >  void __init intel_hfi_init(void)
> > @@ -628,10 +697,16 @@ void __init intel_hfi_init(void)
> >         if (!hfi_updates_wq)
> >                 goto err_nomem;
> >
> > +       if (thermal_genl_register_notifier(&hfi_thermal_nb))
> > +               goto err_nl_notif;
> 
> Is it possible for any clients to be there before the notifier is
> registered?  If not, it would be good to add a comment about it.

HFI is build-in so it's started before any user space. I added note about that
in the cover letter but indeed it should be comment in the code. Will fix.  

Regards
Stanislaw