[PATCH net 0/2] dpll: fix a false notification and report interrupted dumps

[PATCH net 0/2] dpll: fix a false notification and report interrupted dumps

[Ivan Vecera]

Two small fixes for the DPLL core, both found by code review:

- Patch 1 stops sending a pin change notification when adding a reference
  sync pair fails. Reproduced on a ZL30732A board.
- Patch 2 sets NLM_F_DUMP_INTR on pin and device dumps that a registration
  or unregistration interrupted. Such dumps can silently miss an object.
  Reproduced with a test module.

Ivan Vecera (2):
  dpll: notify about a reference sync pair only if it was added
  dpll: report dumps interrupted by topology changes

 drivers/dpll/dpll_core.c    | 22 +++++++++++++++++++++-
 drivers/dpll/dpll_core.h    |  1 +
 drivers/dpll/dpll_netlink.c |  4 ++++
 3 files changed, 26 insertions(+), 1 deletion(-)


base-commit: 6baae996ecf6a3f57b08ea6f6786e29af9260f51
-- 
2.56.0

[PATCH net 1/2] dpll: notify about a reference sync pair only if it was added

[Ivan Vecera]

dpll_pin_ref_sync_pair_add() sends a pin change notification even when
xa_insert() fails and nothing has changed. zl3073x gets -EBUSY when
another channel has already added the same pair and ignores it, so user
space is notified about a change that did not happen.

Send the notification only after a successful insert.

Found by code review. Reproduced on a ZL30732A board with kprobes on
dpll_pin_ref_sync_pair_add() and __dpll_pin_change_ntf(): loading the
driver adds one pair twice and the add failing with -EBUSY sends a
notification. With this patch it does not, and the successful add still
does. A test module adding one pair twice shows the same on the monitor
group.

Fixes: 58256a26bfb3 ("dpll: add reference sync get/set")
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/dpll/dpll_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
index 8389983f425695..277692d12d3d1e 100644
--- a/drivers/dpll/dpll_core.c
+++ b/drivers/dpll/dpll_core.c
@@ -1081,7 +1081,8 @@ int dpll_pin_ref_sync_pair_add(struct dpll_pin *pin,
 	mutex_lock(&dpll_lock);
 	ret = xa_insert(&pin->ref_sync_pins, ref_sync_pin->id,
 			ref_sync_pin, GFP_KERNEL);
-	__dpll_pin_change_ntf(pin);
+	if (!ret)
+		__dpll_pin_change_ntf(pin);
 	mutex_unlock(&dpll_lock);
 
 	return ret;
-- 
2.56.0

[PATCH net 2/2] dpll: report dumps interrupted by topology changes

[Ivan Vecera]

Pin and device dumps release dpll_lock between parts and resume from the
id saved by the previous part. An object registered in between is missed
if its id is below that one, as is the case for an object allocated
before the dump started. User space gets an incomplete dump and has no
way to tell.

Count registrations and unregistrations in a generation counter and pass
it to genl_dump_check_consistent() to set NLM_F_DUMP_INTR. Check it only
after a message is built, a cancelled message would consume the change.

Found by code review. Reproduced with a test module registering a device
or a pin with a low id between parts of a raw netlink dump: the object was
missing and no message had the flag set. With this patch the flag is set
when an object is registered or unregistered during a dump, and is not
set on dumps without such a change. Tested in QEMU and on a ZL30732A
board, where unbinding the driver during a dump sets the flag as well.

Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions")
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/dpll/dpll_core.c    | 19 +++++++++++++++++++
 drivers/dpll/dpll_core.h    |  1 +
 drivers/dpll/dpll_netlink.c |  4 ++++
 3 files changed, 24 insertions(+)

diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
index 277692d12d3d1e..a68e8f5b609db8 100644
--- a/drivers/dpll/dpll_core.c
+++ b/drivers/dpll/dpll_core.c
@@ -22,6 +22,12 @@
 /* Mutex lock to protect DPLL subsystem devices and pins */
 DEFINE_MUTEX(dpll_lock);
 
+/* Changes with the topology of devices and pins, so that netlink dumps can
+ * detect it. Never 0, as netlink takes that for no sequence. Protected by
+ * dpll_lock.
+ */
+unsigned int dpll_generation = 1;
+
 DEFINE_XARRAY_FLAGS(dpll_device_xa, XA_FLAGS_ALLOC);
 DEFINE_XARRAY_FLAGS(dpll_pin_xa, XA_FLAGS_ALLOC);
 
@@ -53,6 +59,13 @@ struct dpll_pin_registration {
 	dpll_tracker tracker;
 };
 
+static void dpll_generation_inc(void)
+{
+	lockdep_assert_held(&dpll_lock);
+	while (++dpll_generation == 0)
+		;
+}
+
 static int call_dpll_notifiers(unsigned long action, void *info)
 {
 	lockdep_assert_held(&dpll_lock);
@@ -510,6 +523,7 @@ int dpll_device_register(struct dpll_device *dpll, enum dpll_type type,
 	}
 
 	xa_set_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED);
+	dpll_generation_inc();
 	dpll_device_create_ntf(dpll);
 	mutex_unlock(&dpll_lock);
 
@@ -549,6 +563,7 @@ void dpll_device_unregister(struct dpll_device *dpll,
 		return;
 	}
 	xa_clear_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED);
+	dpll_generation_inc();
 	mutex_unlock(&dpll_lock);
 }
 EXPORT_SYMBOL_GPL(dpll_device_unregister);
@@ -851,6 +866,7 @@ __dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin,
 	if (ret)
 		goto ref_pin_del;
 	xa_set_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED);
+	dpll_generation_inc();
 	dpll_pin_create_ntf(pin, dpll->clock_id);
 
 	return ret;
@@ -941,6 +957,7 @@ __dpll_pin_unregister(struct dpll_device *dpll, struct dpll_pin *pin,
 	dpll_pin_delete_ntf(pin, dpll->clock_id);
 	dpll_xa_ref_pin_del(&dpll->pin_refs, pin, ops, priv, cookie);
 	dpll_xa_ref_dpll_del(&pin->dpll_refs, dpll, ops, priv, cookie);
+	dpll_generation_inc();
 	if (xa_empty(&pin->dpll_refs)) {
 		dpll_pin_ref_sync_pair_del(pin->id);
 		xa_clear_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED);
@@ -1005,6 +1022,7 @@ int dpll_pin_on_pin_register(struct dpll_pin *parent, struct dpll_pin *pin,
 	ret = dpll_xa_ref_pin_add(&pin->parent_refs, parent, ops, priv, pin);
 	if (ret)
 		goto unlock;
+	dpll_generation_inc();
 	xa_for_each(&parent->dpll_refs, i, ref) {
 		ret = __dpll_pin_register(ref->dpll, pin, ops, priv, parent);
 		if (ret) {
@@ -1057,6 +1075,7 @@ void dpll_pin_on_pin_unregister(struct dpll_pin *parent, struct dpll_pin *pin,
 		__dpll_pin_unregister(ref->dpll, pin, ops, priv, parent);
 	}
 	dpll_xa_ref_pin_del(&pin->parent_refs, parent, ops, priv, pin);
+	dpll_generation_inc();
 	mutex_unlock(&dpll_lock);
 }
 EXPORT_SYMBOL_GPL(dpll_pin_on_pin_unregister);
diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h
index da8a369556ed11..c2ed0926de1b95 100644
--- a/drivers/dpll/dpll_core.h
+++ b/drivers/dpll/dpll_core.h
@@ -99,6 +99,7 @@ struct dpll_pin_ref *dpll_xa_ref_dpll_first(struct xarray *xa_refs);
 extern struct xarray dpll_device_xa;
 extern struct xarray dpll_pin_xa;
 extern struct mutex dpll_lock;
+extern unsigned int dpll_generation;
 
 void dpll_device_notify(struct dpll_device *dpll, unsigned long action);
 void dpll_pin_notify(struct dpll_pin *pin, u64 src_clock_id,
diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index e5380d95f238b3..87c8caef0f574b 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c
@@ -1768,6 +1768,7 @@ int dpll_nl_pin_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 	int ret = 0;
 
 	mutex_lock(&dpll_lock);
+	cb->seq = dpll_generation;
 	xa_for_each_marked_start(&dpll_pin_xa, i, pin, DPLL_REGISTERED,
 				 ctx->idx) {
 		if (!dpll_pin_available(pin))
@@ -1789,6 +1790,7 @@ int dpll_nl_pin_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 			}
 			break;
 		}
+		genl_dump_check_consistent(cb, hdr);
 		genlmsg_end(skb, hdr);
 	}
 	mutex_unlock(&dpll_lock);
@@ -1990,6 +1992,7 @@ int dpll_nl_device_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 	int ret = 0;
 
 	mutex_lock(&dpll_lock);
+	cb->seq = dpll_generation;
 	xa_for_each_marked_start(&dpll_device_xa, i, dpll, DPLL_REGISTERED,
 				 ctx->idx) {
 		hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
@@ -2004,6 +2007,7 @@ int dpll_nl_device_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 			genlmsg_cancel(skb, hdr);
 			break;
 		}
+		genl_dump_check_consistent(cb, hdr);
 		genlmsg_end(skb, hdr);
 	}
 	mutex_unlock(&dpll_lock);
-- 
2.56.0