Re: [PATCH net-next 05/10] dpll: convert dpll_lock to rwsem

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch turns dpll_lock from a mutex into an rwsem.

--

dpll: convert dpll_lock to rwsem

This commit converts dpll_lock from a mutex to an rwsem. Core functions
that change the topology take it for write. Netlink requests, dump parts
and change notifications take it for read, so callbacks of different
domains can run concurrently.

> diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
> index dd09f52817099..8eb7468234315 100644
> --- a/drivers/dpll/dpll_core.c
> +++ b/drivers/dpll/dpll_core.c
> @@ -19,8 +19,12 @@
>  #include "dpll_core.h"
>  #include "dpll_netlink.h"
>  
> -/* Mutex lock to protect DPLL subsystem devices and pins */
> -DEFINE_MUTEX(dpll_lock);
> +/* Protects the topology and lifetime of DPLL devices and pins. Taken for write
> + * to change them and for read for the whole netlink request, part of a dump or
> + * change notification that uses them. Driver callbacks are serialized by domain
> + * locks, which are only taken under this lock.
> + */
> +DECLARE_RWSEM(dpll_lock);

[Severity: Low]
Should the assertions in dpll_domain_get() and dpll_domain_put() be changed
to lockdep_assert_held_write() along with this conversion?

Both helpers still use the plain form:

drivers/dpll/dpll_core.c:dpll_domain_get() {
	lockdep_assert_held(&dpll_lock);

	xa_for_each(&dpll_domain_xa, i, domain) {
		if (domain->clock_id == clock_id && domain->module == module) {
			refcount_inc(&domain->refcount);
			return domain;
		}
	}
	...
	ret = xa_alloc(&dpll_domain_xa, &domain->id, domain, xa_limit_32b,
		       GFP_KERNEL);
	...
}

drivers/dpll/dpll_core.c:dpll_domain_put() {
	lockdep_assert_held(&dpll_lock);

	if (refcount_dec_and_test(&domain->refcount)) {
		xa_erase(&dpll_domain_xa, domain->id);
		mutex_destroy(&domain->lock);
		kfree(domain);
	}
}

On the old mutex, lockdep_assert_held() checked for exclusive ownership. On
an rwsem it also passes when only the read lock is held.

Every current caller holds dpll_lock for write: dpll_device_get() and
dpll_pin_get() via the alloc helpers, and dpll_device_put(), dpll_pin_put()
and the register/unregister paths via the __put helpers. So this is not a
runtime problem today.

However, the read-side holders added here (dpll_pre_doit(),
dpll_pin_pre_doit(), the dumpits, dpll_device_change_ntf() and
dpll_pin_change_ntf()) can now run concurrently. If one of them reached a
domain lookup-or-create, or a final put, lockdep would no longer warn about
the race on dpll_domain_xa or on the domain refcount.

call_dpll_notifiers() can keep the plain lockdep_assert_held(), because it
legitimately runs under the read lock.

These assertions are still unchanged at the end of the series.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106121005.1327603-1-ivecera%40redhat.com