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

[Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>]

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