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

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

On Sat, Feb 03, 2024 at 02:15:19PM +0100, Jiri Pirko wrote:
> Fri, Feb 02, 2024 at 02:00:46PM CET, stanislaw.gruszka@linux.intel.com wrote:
> >On Fri, Feb 02, 2024 at 01:36:09PM +0100, Jiri Pirko wrote:
> >> Wed, Jan 31, 2024 at 01:05:35PM CET, stanislaw.gruszka@linux.intel.com wrote:
> >> 
> >> [...]
> >> 
> >> 
> >> >+static int hfi_netlink_notify(struct notifier_block *nb, unsigned long state,
> >> >+			      void *_notify)
> >> >+{
> >> >+	struct netlink_notify *notify = _notify;
> >> >+	struct hfi_instance *hfi_instance;
> >> >+	smp_call_func_t func;
> >> >+	unsigned int cpu;
> >> >+	int i;
> >> >+
> >> >+	if (notify->protocol != NETLINK_GENERIC)
> >> >+		return NOTIFY_DONE;
> >> >+
> >> >+	switch (state) {
> >> >+	case NETLINK_CHANGE:
> >> >+	case NETLINK_URELEASE:
> >> >+		mutex_lock(&hfi_instance_lock);
> >> >+
> >> 
> >> What's stopping other thread from mangling the listeners here?
> >
> >Nothing. But if the listeners will be changed, we will get next notify.
> >Serialization by the mutex is needed to assure that the last setting will win,
> >so we do not end with HFI disabled when there are listeners or vice versa.
> 
> Okay. Care to put a note somewhere?

I would if the flow would stay the same. But it was requested by Jakub to use
netlink bind/unbind, and this will not work the way described above any longer,
since bind() is before listeners change and unbind() after:

                if (optname == NETLINK_ADD_MEMBERSHIP && nlk->netlink_bind) {
                        err = nlk->netlink_bind(sock_net(sk), val);
                        if (err)
                                return err;
                }
                netlink_table_grab();
                netlink_update_socket_mc(nlk, val,
                                         optname == NETLINK_ADD_MEMBERSHIP);
                netlink_table_ungrab();
                if (optname == NETLINK_DROP_MEMBERSHIP && nlk->netlink_unbind)
                        nlk->netlink_unbind(sock_net(sk), val)

To avoid convoluted logic or new global lock, I'll use properly protected
local counter increased on bind and decreased on unbind.

Regards
Stanislaw