[PATCH net v2 0/4] dpll: fix unordered unbind/bind registerer issues

[PATCH net v2 0/4] dpll: fix unordered unbind/bind registerer issues

[Arkadiusz Kubalewski]

Fix issues when performing unordered unbind/bind of a kernel modules
which are using a dpll device with DPLL_PIN_TYPE_MUX pins.
Currently only serialized bind/unbind of such use case works, fix
the issues and allow for unserialized kernel module bind order.

The issues are observed on the ice driver, i.e.,

$ echo 0000:af:00.0 > /sys/bus/pci/drivers/ice/unbind
$ echo 0000:af:00.1 > /sys/bus/pci/drivers/ice/unbind

results in:

ice 0000:af:00.0: Removed PTP clock
BUG: kernel NULL pointer dereference, address: 0000000000000010
PF: supervisor read access in kernel mode
PF: error_code(0x0000) - not-present page
PGD 0 P4D 0 
Oops: 0000 [#1] PREEMPT SMP PTI
CPU: 7 PID: 71848 Comm: bash Kdump: loaded Not tainted 6.6.0-rc5_next-queue_19th-Oct-2023-01625-g039e5d15e451 #1
Hardware name: Intel Corporation S2600STB/S2600STB, BIOS SE5C620.86B.02.01.0008.031920191559 03/19/2019
RIP: 0010:ice_dpll_rclk_state_on_pin_get+0x2f/0x90 [ice]
Code: 41 57 4d 89 cf 41 56 41 55 4d 89 c5 41 54 55 48 89 f5 53 4c 8b 66 08 48 89 cb 4d 8d b4 24 f0 49 00 00 4c 89 f7 e8 71 ec 1f c5 <0f> b6 5b 10 41 0f b6 84 24 30 4b 00 00 29 c3 41 0f b6 84 24 28 4b
RSP: 0018:ffffc902b179fb60 EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: ffff8882c1398000 RSI: ffff888c7435cc60 RDI: ffff888c7435cb90
RBP: ffff888c7435cc60 R08: ffffc902b179fbb0 R09: 0000000000000000
R10: ffff888ef1fc8050 R11: fffffffffff82700 R12: ffff888c743581a0
R13: ffffc902b179fbb0 R14: ffff888c7435cb90 R15: 0000000000000000
FS:  00007fdc7dae0740(0000) GS:ffff888c105c0000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000010 CR3: 0000000132c24002 CR4: 00000000007706e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
PKRU: 55555554
Call Trace:
 <TASK>
 ? __die+0x20/0x70
 ? page_fault_oops+0x76/0x170
 ? exc_page_fault+0x65/0x150
 ? asm_exc_page_fault+0x22/0x30
 ? ice_dpll_rclk_state_on_pin_get+0x2f/0x90 [ice]
 ? __pfx_ice_dpll_rclk_state_on_pin_get+0x10/0x10 [ice]
 dpll_msg_add_pin_parents+0x142/0x1d0
 dpll_pin_event_send+0x7d/0x150
 dpll_pin_on_pin_unregister+0x3f/0x100
 ice_dpll_deinit_pins+0xa1/0x230 [ice]
 ice_dpll_deinit+0x29/0xe0 [ice]
 ice_remove+0xcd/0x200 [ice]
 pci_device_remove+0x33/0xa0
 device_release_driver_internal+0x193/0x200
 unbind_store+0x9d/0xb0
 kernfs_fop_write_iter+0x128/0x1c0
 vfs_write+0x2bb/0x3e0
 ksys_write+0x5f/0xe0
 do_syscall_64+0x59/0x90
 ? filp_close+0x1b/0x30
 ? do_dup2+0x7d/0xd0
 ? syscall_exit_work+0x103/0x130
 ? syscall_exit_to_user_mode+0x22/0x40
 ? do_syscall_64+0x69/0x90
 ? syscall_exit_work+0x103/0x130
 ? syscall_exit_to_user_mode+0x22/0x40
 ? do_syscall_64+0x69/0x90
 entry_SYSCALL_64_after_hwframe+0x6e/0xd8
RIP: 0033:0x7fdc7d93eb97
Code: 0b 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24
RSP: 002b:00007fff2aa91028 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 000000000000000d RCX: 00007fdc7d93eb97
RDX: 000000000000000d RSI: 00005644814ec9b0 RDI: 0000000000000001
RBP: 00005644814ec9b0 R08: 0000000000000000 R09: 00007fdc7d9b14e0
R10: 00007fdc7d9b13e0 R11: 0000000000000246 R12: 000000000000000d
R13: 00007fdc7d9fb780 R14: 000000000000000d R15: 00007fdc7d9f69e0
 </TASK>
Modules linked in: uinput vfio_pci vfio_pci_core vfio_iommu_type1 vfio irqbypass ixgbevf snd_seq_dummy snd_hrtimer snd_seq snd_timer snd_seq_device snd soundcore overlay qrtr rfkill vfat fat xfs libcrc32c rpcrdma sunrpc rdma_ucm ib_srpt ib_isert iscsi_target_mod target_core_mod ib_iser libiscsi scsi_transport_iscsi rdma_cm iw_cm ib_cm intel_rapl_msr intel_rapl_common intel_uncore_frequency intel_uncore_frequency_common isst_if_common skx_edac nfit libnvdimm ipmi_ssif x86_pkg_temp_thermal intel_powerclamp coretemp irdma rapl intel_cstate ib_uverbs iTCO_wdt iTCO_vendor_support acpi_ipmi intel_uncore mei_me ipmi_si pcspkr i2c_i801 ib_core mei ipmi_devintf intel_pch_thermal ioatdma i2c_smbus ipmi_msghandler lpc_ich joydev acpi_power_meter acpi_pad ext4 mbcache jbd2 sd_mod t10_pi sg ast i2c_algo_bit drm_shmem_helper drm_kms_helper ice crct10dif_pclmul ixgbe crc32_pclmul drm crc32c_intel ahci i40e libahci ghash_clmulni_intel libata mdio dca gnss wmi fuse [last unloaded: iavf]
CR2: 0000000000000010

v2:
- added 4th patch: "[4/4] dpll: fix hide "zombie" pins for userspace"

Arkadiusz Kubalewski (4):
  dpll: fix pin dump crash after module unbind
  dpll: fix pin dump crash for rebound module
  dpll: fix register pin with unregistered parent pin
  dpll: fix hide "zombie" pins for userspace

 drivers/dpll/dpll_core.c    | 34 +++++++++++++++++++-----
 drivers/dpll/dpll_core.h    |  4 +--
 drivers/dpll/dpll_netlink.c | 53 ++++++++++++++++++++++++++-----------
 3 files changed, 67 insertions(+), 24 deletions(-)

-- 
2.38.1

[PATCH net v2 1/4] dpll: fix pin dump crash after module unbind

[Arkadiusz Kubalewski]

Disallow dump of unregistered parent pins, it is possible when parent
pin and dpll device registerer kernel module instance unbinds, and
other kernel module instances of the same dpll device have pins
registered with the parent pin. The user can invoke a pin-dump but as
the parent was unregistered, those shall not be accessed by the
userspace, prevent that by checking if parent pin is still registered.

Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions")
Reviewed-by: Jan Glaza <jan.glaza@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
 drivers/dpll/dpll_netlink.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index ce7cf736f020..b53478374a38 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c
@@ -328,6 +328,8 @@ dpll_msg_add_pin_parents(struct sk_buff *msg, struct dpll_pin *pin,
 		void *parent_priv;
 
 		ppin = ref->pin;
+		if (!xa_get_mark(&dpll_pin_xa, ppin->id, DPLL_REGISTERED))
+			continue;
 		parent_priv = dpll_pin_on_dpll_priv(dpll_ref->dpll, ppin);
 		ret = ops->state_on_pin_get(pin,
 					    dpll_pin_on_pin_priv(ppin, pin),
-- 
2.38.1

[PATCH net v2 2/4] dpll: fix pin dump crash for rebound module

[Arkadiusz Kubalewski]

When a kernel module is unbound but the pin resources were not entirely
freed (other kernel module instance have had kept the reference to that
pin), and kernel module is again bound, the pin properties would not be
updated (the properties are only assigned when memory for the pin is
allocated), prop pointer still points to the kernel module memory of
the kernel module which was deallocated on the unbind.

If the pin dump is invoked in this state, the result is a kernel crash.
Prevent the crash by storing persistent pin properties in dpll subsystem,
copy the content from the kernel module when pin is allocated, instead of
using memory of the kernel module.

Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions")
Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions")
Reviewed-by: Jan Glaza <jan.glaza@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
 drivers/dpll/dpll_core.c    | 29 ++++++++++++++++++++++++++---
 drivers/dpll/dpll_core.h    |  4 ++--
 drivers/dpll/dpll_netlink.c | 28 ++++++++++++++--------------
 3 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
index 3568149b9562..0b469096ef79 100644
--- a/drivers/dpll/dpll_core.c
+++ b/drivers/dpll/dpll_core.c
@@ -429,6 +429,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct module *module,
 	       const struct dpll_pin_properties *prop)
 {
 	struct dpll_pin *pin;
+	size_t freq_size;
 	int ret;
 
 	pin = kzalloc(sizeof(*pin), GFP_KERNEL);
@@ -440,9 +441,22 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct module *module,
 	if (WARN_ON(prop->type < DPLL_PIN_TYPE_MUX ||
 		    prop->type > DPLL_PIN_TYPE_MAX)) {
 		ret = -EINVAL;
-		goto err;
+		goto pin_free;
 	}
-	pin->prop = prop;
+	memcpy(&pin->prop, prop, sizeof(pin->prop));
+	if (prop->freq_supported && prop->freq_supported_num) {
+		freq_size = prop->freq_supported_num *
+			    sizeof(*pin->prop.freq_supported);
+		pin->prop.freq_supported = kmemdup(prop->freq_supported,
+						   freq_size, GFP_KERNEL);
+		if (!pin->prop.freq_supported) {
+			ret = -ENOMEM;
+			goto pin_free;
+		}
+	}
+	pin->prop.board_label = kstrdup(prop->board_label, GFP_KERNEL);
+	pin->prop.panel_label = kstrdup(prop->panel_label, GFP_KERNEL);
+	pin->prop.package_label = kstrdup(prop->package_label, GFP_KERNEL);
 	refcount_set(&pin->refcount, 1);
 	xa_init_flags(&pin->dpll_refs, XA_FLAGS_ALLOC);
 	xa_init_flags(&pin->parent_refs, XA_FLAGS_ALLOC);
@@ -451,8 +465,13 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct module *module,
 		goto err;
 	return pin;
 err:
+	kfree(pin->prop.package_label);
+	kfree(pin->prop.panel_label);
+	kfree(pin->prop.board_label);
+	kfree(pin->prop.freq_supported);
 	xa_destroy(&pin->dpll_refs);
 	xa_destroy(&pin->parent_refs);
+pin_free:
 	kfree(pin);
 	return ERR_PTR(ret);
 }
@@ -512,6 +531,10 @@ void dpll_pin_put(struct dpll_pin *pin)
 		xa_destroy(&pin->dpll_refs);
 		xa_destroy(&pin->parent_refs);
 		xa_erase(&dpll_pin_xa, pin->id);
+		kfree(pin->prop.board_label);
+		kfree(pin->prop.panel_label);
+		kfree(pin->prop.package_label);
+		kfree(pin->prop.freq_supported);
 		kfree(pin);
 	}
 	mutex_unlock(&dpll_lock);
@@ -634,7 +657,7 @@ int dpll_pin_on_pin_register(struct dpll_pin *parent, struct dpll_pin *pin,
 	unsigned long i, stop;
 	int ret;
 
-	if (WARN_ON(parent->prop->type != DPLL_PIN_TYPE_MUX))
+	if (WARN_ON(parent->prop.type != DPLL_PIN_TYPE_MUX))
 		return -EINVAL;
 
 	if (WARN_ON(!ops) ||
diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h
index 5585873c5c1b..717f715015c7 100644
--- a/drivers/dpll/dpll_core.h
+++ b/drivers/dpll/dpll_core.h
@@ -44,7 +44,7 @@ struct dpll_device {
  * @module:		module of creator
  * @dpll_refs:		hold referencees to dplls pin was registered with
  * @parent_refs:	hold references to parent pins pin was registered with
- * @prop:		pointer to pin properties given by registerer
+ * @prop:		pin properties copied from the registerer
  * @rclk_dev_name:	holds name of device when pin can recover clock from it
  * @refcount:		refcount
  **/
@@ -55,7 +55,7 @@ struct dpll_pin {
 	struct module *module;
 	struct xarray dpll_refs;
 	struct xarray parent_refs;
-	const struct dpll_pin_properties *prop;
+	struct dpll_pin_properties prop;
 	refcount_t refcount;
 };
 
diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index b53478374a38..3ce9995013f1 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c
@@ -278,17 +278,17 @@ dpll_msg_add_pin_freq(struct sk_buff *msg, struct dpll_pin *pin,
 	if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY, sizeof(freq), &freq,
 			  DPLL_A_PIN_PAD))
 		return -EMSGSIZE;
-	for (fs = 0; fs < pin->prop->freq_supported_num; fs++) {
+	for (fs = 0; fs < pin->prop.freq_supported_num; fs++) {
 		nest = nla_nest_start(msg, DPLL_A_PIN_FREQUENCY_SUPPORTED);
 		if (!nest)
 			return -EMSGSIZE;
-		freq = pin->prop->freq_supported[fs].min;
+		freq = pin->prop.freq_supported[fs].min;
 		if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MIN, sizeof(freq),
 				  &freq, DPLL_A_PIN_PAD)) {
 			nla_nest_cancel(msg, nest);
 			return -EMSGSIZE;
 		}
-		freq = pin->prop->freq_supported[fs].max;
+		freq = pin->prop.freq_supported[fs].max;
 		if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MAX, sizeof(freq),
 				  &freq, DPLL_A_PIN_PAD)) {
 			nla_nest_cancel(msg, nest);
@@ -304,9 +304,9 @@ static bool dpll_pin_is_freq_supported(struct dpll_pin *pin, u32 freq)
 {
 	int fs;
 
-	for (fs = 0; fs < pin->prop->freq_supported_num; fs++)
-		if (freq >= pin->prop->freq_supported[fs].min &&
-		    freq <= pin->prop->freq_supported[fs].max)
+	for (fs = 0; fs < pin->prop.freq_supported_num; fs++)
+		if (freq >= pin->prop.freq_supported[fs].min &&
+		    freq <= pin->prop.freq_supported[fs].max)
 			return true;
 	return false;
 }
@@ -398,7 +398,7 @@ static int
 dpll_cmd_pin_get_one(struct sk_buff *msg, struct dpll_pin *pin,
 		     struct netlink_ext_ack *extack)
 {
-	const struct dpll_pin_properties *prop = pin->prop;
+	const struct dpll_pin_properties *prop = &pin->prop;
 	struct dpll_pin_ref *ref;
 	int ret;
 
@@ -691,7 +691,7 @@ dpll_pin_on_pin_state_set(struct dpll_pin *pin, u32 parent_idx,
 	int ret;
 
 	if (!(DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE &
-	      pin->prop->capabilities)) {
+	      pin->prop.capabilities)) {
 		NL_SET_ERR_MSG(extack, "state changing is not allowed");
 		return -EOPNOTSUPP;
 	}
@@ -727,7 +727,7 @@ dpll_pin_state_set(struct dpll_device *dpll, struct dpll_pin *pin,
 	int ret;
 
 	if (!(DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE &
-	      pin->prop->capabilities)) {
+	      pin->prop.capabilities)) {
 		NL_SET_ERR_MSG(extack, "state changing is not allowed");
 		return -EOPNOTSUPP;
 	}
@@ -754,7 +754,7 @@ dpll_pin_prio_set(struct dpll_device *dpll, struct dpll_pin *pin,
 	int ret;
 
 	if (!(DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE &
-	      pin->prop->capabilities)) {
+	      pin->prop.capabilities)) {
 		NL_SET_ERR_MSG(extack, "prio changing is not allowed");
 		return -EOPNOTSUPP;
 	}
@@ -782,7 +782,7 @@ dpll_pin_direction_set(struct dpll_pin *pin, struct dpll_device *dpll,
 	int ret;
 
 	if (!(DPLL_PIN_CAPABILITIES_DIRECTION_CAN_CHANGE &
-	      pin->prop->capabilities)) {
+	      pin->prop.capabilities)) {
 		NL_SET_ERR_MSG(extack, "direction changing is not allowed");
 		return -EOPNOTSUPP;
 	}
@@ -812,8 +812,8 @@ dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr,
 	int ret;
 
 	phase_adj = nla_get_s32(phase_adj_attr);
-	if (phase_adj > pin->prop->phase_range.max ||
-	    phase_adj < pin->prop->phase_range.min) {
+	if (phase_adj > pin->prop.phase_range.max ||
+	    phase_adj < pin->prop.phase_range.min) {
 		NL_SET_ERR_MSG_ATTR(extack, phase_adj_attr,
 				    "phase adjust value not supported");
 		return -EINVAL;
@@ -997,7 +997,7 @@ dpll_pin_find(u64 clock_id, struct nlattr *mod_name_attr,
 	unsigned long i;
 
 	xa_for_each_marked(&dpll_pin_xa, i, pin, DPLL_REGISTERED) {
-		prop = pin->prop;
+		prop = &pin->prop;
 		cid_match = clock_id ? pin->clock_id == clock_id : true;
 		mod_match = mod_name_attr && module_name(pin->module) ?
 			!nla_strcmp(mod_name_attr,
-- 
2.38.1

[PATCH net v2 3/4] dpll: fix register pin with unregistered parent pin

[Arkadiusz Kubalewski]

In case of multiple kernel module instances using the same dpll device:
if only one registers dpll device, then only that one can register
directly connected pins with a dpll device. When unregistered parent is
responsible for determining if the muxed pin can be registered with it
or not, the drivers need to be loaded in serialized order to work
correctly - first the driver instance which registers the direct pins
needs to be loaded, then the other instances could register muxed type
pins.

Allow registration of a pin with a parent even if the parent was not
yet registered, thus allow ability for unserialized driver instance
load order.
Do not WARN_ON notification for unregistered pin, which can be invoked
for described case, instead just return error.

Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions")
Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions")
Reviewed-by: Jan Glaza <jan.glaza@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
 drivers/dpll/dpll_core.c    | 4 ----
 drivers/dpll/dpll_netlink.c | 2 +-
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
index 0b469096ef79..c8a2129f5699 100644
--- a/drivers/dpll/dpll_core.c
+++ b/drivers/dpll/dpll_core.c
@@ -28,8 +28,6 @@ static u32 dpll_xa_id;
 	WARN_ON_ONCE(!xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
 #define ASSERT_DPLL_NOT_REGISTERED(d)	\
 	WARN_ON_ONCE(xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
-#define ASSERT_PIN_REGISTERED(p)	\
-	WARN_ON_ONCE(!xa_get_mark(&dpll_pin_xa, (p)->id, DPLL_REGISTERED))
 
 struct dpll_device_registration {
 	struct list_head list;
@@ -664,8 +662,6 @@ int dpll_pin_on_pin_register(struct dpll_pin *parent, struct dpll_pin *pin,
 	    WARN_ON(!ops->state_on_pin_get) ||
 	    WARN_ON(!ops->direction_get))
 		return -EINVAL;
-	if (ASSERT_PIN_REGISTERED(parent))
-		return -EINVAL;
 
 	mutex_lock(&dpll_lock);
 	ret = dpll_xa_ref_pin_add(&pin->parent_refs, parent, ops, priv);
diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index 3ce9995013f1..f266db8da2f0 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c
@@ -553,7 +553,7 @@ dpll_pin_event_send(enum dpll_cmd event, struct dpll_pin *pin)
 	int ret = -ENOMEM;
 	void *hdr;
 
-	if (WARN_ON(!xa_get_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED)))
+	if (!xa_get_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED))
 		return -ENODEV;
 
 	msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
-- 
2.38.1

[PATCH net v2 4/4] dpll: hide "zombie" pins for userspace

[Arkadiusz Kubalewski]

If parent pin was unregistered but child pin was not, the userspace
would see the "zombie" pins - the ones that were registered with
parent pin (pins_pin_on_pin_register(..)).
Technically those are not available - as there is no dpll device in the
system. Do not dump those pins and prevent userspace from any
interaction with them.

Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions")
Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions")
Reviewed-by: Jan Glaza <jan.glaza@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
 drivers/dpll/dpll_netlink.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index f266db8da2f0..495dfc43c0be 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c
@@ -949,6 +949,19 @@ dpll_pin_parent_pin_set(struct dpll_pin *pin, struct nlattr *parent_nest,
 	return 0;
 }
 
+static bool dpll_pin_parents_registered(struct dpll_pin *pin)
+{
+	struct dpll_pin_ref *par_ref;
+	struct dpll_pin *p;
+	unsigned long i, j;
+
+	xa_for_each(&pin->parent_refs, i, par_ref)
+		xa_for_each_marked(&dpll_pin_xa, j, p, DPLL_REGISTERED)
+			if (par_ref->pin == p)
+				return true;
+	return false;
+}
+
 static int
 dpll_pin_set_from_nlattr(struct dpll_pin *pin, struct genl_info *info)
 {
@@ -1153,6 +1166,9 @@ int dpll_nl_pin_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 
 	xa_for_each_marked_start(&dpll_pin_xa, i, pin, DPLL_REGISTERED,
 				 ctx->idx) {
+		if (!xa_empty(&pin->parent_refs) &&
+		    !dpll_pin_parents_registered(pin))
+			continue;
 		hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
 				  cb->nlh->nlmsg_seq,
 				  &dpll_nl_family, NLM_F_MULTI,
@@ -1179,6 +1195,10 @@ int dpll_nl_pin_set_doit(struct sk_buff *skb, struct genl_info *info)
 {
 	struct dpll_pin *pin = info->user_ptr[0];
 
+	if (!xa_empty(&pin->parent_refs) &&
+	    !dpll_pin_parents_registered(pin))
+		return -ENODEV;
+
 	return dpll_pin_set_from_nlattr(pin, info);
 }
 
-- 
2.38.1

Re: [PATCH net v2 3/4] dpll: fix register pin with unregistered parent pin

[Jiri Pirko]

Thu, Jan 04, 2024 at 12:11:31PM CET, arkadiusz.kubalewski@intel.com wrote:
>In case of multiple kernel module instances using the same dpll device:
>if only one registers dpll device, then only that one can register
>directly connected pins with a dpll device. When unregistered parent is
>responsible for determining if the muxed pin can be registered with it
>or not, the drivers need to be loaded in serialized order to work
>correctly - first the driver instance which registers the direct pins
>needs to be loaded, then the other instances could register muxed type
>pins.
>
>Allow registration of a pin with a parent even if the parent was not
>yet registered, thus allow ability for unserialized driver instance
>load order.
>Do not WARN_ON notification for unregistered pin, which can be invoked
>for described case, instead just return error.
>
>Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions")
>Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions")
>Reviewed-by: Jan Glaza <jan.glaza@intel.com>
>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>---
> drivers/dpll/dpll_core.c    | 4 ----
> drivers/dpll/dpll_netlink.c | 2 +-
> 2 files changed, 1 insertion(+), 5 deletions(-)
>
>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>index 0b469096ef79..c8a2129f5699 100644
>--- a/drivers/dpll/dpll_core.c
>+++ b/drivers/dpll/dpll_core.c
>@@ -28,8 +28,6 @@ static u32 dpll_xa_id;
> 	WARN_ON_ONCE(!xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
> #define ASSERT_DPLL_NOT_REGISTERED(d)	\
> 	WARN_ON_ONCE(xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
>-#define ASSERT_PIN_REGISTERED(p)	\
>-	WARN_ON_ONCE(!xa_get_mark(&dpll_pin_xa, (p)->id, DPLL_REGISTERED))
> 
> struct dpll_device_registration {
> 	struct list_head list;
>@@ -664,8 +662,6 @@ int dpll_pin_on_pin_register(struct dpll_pin *parent, struct dpll_pin *pin,
> 	    WARN_ON(!ops->state_on_pin_get) ||
> 	    WARN_ON(!ops->direction_get))
> 		return -EINVAL;
>-	if (ASSERT_PIN_REGISTERED(parent))
>-		return -EINVAL;

This makes the pin-on-device and pin-on-pin register behaviour
different:
int
dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin,
                  const struct dpll_pin_ops *ops, void *priv)
{
	...
        if (ASSERT_DPLL_REGISTERED(dpll))
                return -EINVAL;

I think it is need to maintain the same set of restrictions and
behaviour the same for both.

With what you suggest, the user would just see couple of pins with no
parent (hidden one), no dpll devices (none would be registered).
That's odd.

PF0 is the owner of DPLL in your case.

From the user perspective, I think it should look like:
1) If PFn appears after PF0, it registers pins related to it, PF0
   created instances are there and valid. User sees them all.
2) If PF0 gets removed before PFn, it removes all dpll related entities,
   even those related to PFn. Users sees nothing.

So you have to make sure that the pin is hidden (not shown to the user)
in case the parent (device/pin) is not registered. Makes sense?



> 
> 	mutex_lock(&dpll_lock);
> 	ret = dpll_xa_ref_pin_add(&pin->parent_refs, parent, ops, priv);
>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>index 3ce9995013f1..f266db8da2f0 100644
>--- a/drivers/dpll/dpll_netlink.c
>+++ b/drivers/dpll/dpll_netlink.c
>@@ -553,7 +553,7 @@ dpll_pin_event_send(enum dpll_cmd event, struct dpll_pin *pin)
> 	int ret = -ENOMEM;
> 	void *hdr;
> 
>-	if (WARN_ON(!xa_get_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED)))
>+	if (!xa_get_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED))
> 		return -ENODEV;
> 
> 	msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>-- 
>2.38.1
>

Re: [PATCH net v2 4/4] dpll: hide "zombie" pins for userspace

[Jiri Pirko]

Thu, Jan 04, 2024 at 12:11:32PM CET, arkadiusz.kubalewski@intel.com wrote:
>If parent pin was unregistered but child pin was not, the userspace
>would see the "zombie" pins - the ones that were registered with
>parent pin (pins_pin_on_pin_register(..)).
>Technically those are not available - as there is no dpll device in the
>system. Do not dump those pins and prevent userspace from any
>interaction with them.

Ah, here it is :)


>
>Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions")
>Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions")
>Reviewed-by: Jan Glaza <jan.glaza@intel.com>
>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>---
> drivers/dpll/dpll_netlink.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>index f266db8da2f0..495dfc43c0be 100644
>--- a/drivers/dpll/dpll_netlink.c
>+++ b/drivers/dpll/dpll_netlink.c
>@@ -949,6 +949,19 @@ dpll_pin_parent_pin_set(struct dpll_pin *pin, struct nlattr *parent_nest,
> 	return 0;
> }
> 
>+static bool dpll_pin_parents_registered(struct dpll_pin *pin)
>+{
>+	struct dpll_pin_ref *par_ref;
>+	struct dpll_pin *p;
>+	unsigned long i, j;
>+
>+	xa_for_each(&pin->parent_refs, i, par_ref)
>+		xa_for_each_marked(&dpll_pin_xa, j, p, DPLL_REGISTERED)
>+			if (par_ref->pin == p)
>+				return true;

		if (xa_get_mark(..))
			return true;
?


>+	return false;


As I wrote in the reply to the other patch, could you unify the "hide"
behaviour for unregistered parent pin/device?


>+}
>+
> static int
> dpll_pin_set_from_nlattr(struct dpll_pin *pin, struct genl_info *info)
> {
>@@ -1153,6 +1166,9 @@ int dpll_nl_pin_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
> 
> 	xa_for_each_marked_start(&dpll_pin_xa, i, pin, DPLL_REGISTERED,
> 				 ctx->idx) {
>+		if (!xa_empty(&pin->parent_refs) &&

This empty check is redundant, remove it.


>+		    !dpll_pin_parents_registered(pin))
>+			continue;
> 		hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> 				  cb->nlh->nlmsg_seq,
> 				  &dpll_nl_family, NLM_F_MULTI,
>@@ -1179,6 +1195,10 @@ int dpll_nl_pin_set_doit(struct sk_buff *skb, struct genl_info *info)
> {
> 	struct dpll_pin *pin = info->user_ptr[0];
> 
>+	if (!xa_empty(&pin->parent_refs) &&

This empty check is redundant, remove it.


>+	    !dpll_pin_parents_registered(pin))
>+		return -ENODEV;
>+
> 	return dpll_pin_set_from_nlattr(pin, info);
> }
> 
>-- 
>2.38.1
>

Re: [PATCH net v2 4/4] dpll: hide "zombie" pins for userspace

[Jiri Pirko]

Thu, Jan 04, 2024 at 12:11:32PM CET, arkadiusz.kubalewski@intel.com wrote:

[...]


>@@ -1179,6 +1195,10 @@ int dpll_nl_pin_set_doit(struct sk_buff *skb, struct genl_info *info)


What about dpll_nl_pin_get_doit(), dpll_nl_pin_id_get_doit()?

I think it would be better to move the check to:
dpll_pin_pre_doit()


> {
> 	struct dpll_pin *pin = info->user_ptr[0];
> 
>+	if (!xa_empty(&pin->parent_refs) &&
>+	    !dpll_pin_parents_registered(pin))
>+		return -ENODEV;
>+
> 	return dpll_pin_set_from_nlattr(pin, info);
> }
> 
>-- 
>2.38.1
>

Re: [PATCH net v2 1/4] dpll: fix pin dump crash after module unbind

[Jiri Pirko]

Thu, Jan 04, 2024 at 12:11:29PM CET, arkadiusz.kubalewski@intel.com wrote:
>Disallow dump of unregistered parent pins, it is possible when parent
>pin and dpll device registerer kernel module instance unbinds, and
>other kernel module instances of the same dpll device have pins
>registered with the parent pin. The user can invoke a pin-dump but as
>the parent was unregistered, those shall not be accessed by the
>userspace, prevent that by checking if parent pin is still registered.
>
>Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions")
>Reviewed-by: Jan Glaza <jan.glaza@intel.com>
>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>---
> drivers/dpll/dpll_netlink.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>index ce7cf736f020..b53478374a38 100644
>--- a/drivers/dpll/dpll_netlink.c
>+++ b/drivers/dpll/dpll_netlink.c
>@@ -328,6 +328,8 @@ dpll_msg_add_pin_parents(struct sk_buff *msg, struct dpll_pin *pin,
> 		void *parent_priv;
> 
> 		ppin = ref->pin;
>+		if (!xa_get_mark(&dpll_pin_xa, ppin->id, DPLL_REGISTERED))
>+			continue;

If you make sure to hide the pin properly with the last patch, you don't
need this one. Drop it.


> 		parent_priv = dpll_pin_on_dpll_priv(dpll_ref->dpll, ppin);
> 		ret = ops->state_on_pin_get(pin,
> 					    dpll_pin_on_pin_priv(ppin, pin),
>-- 
>2.38.1
>

Re: [PATCH net v2 4/4] dpll: hide "zombie" pins for userspace

[Jiri Pirko]

Thu, Jan 04, 2024 at 12:11:32PM CET, arkadiusz.kubalewski@intel.com wrote:

[...]

>@@ -1179,6 +1195,10 @@ int dpll_nl_pin_set_doit(struct sk_buff *skb, struct genl_info *info)
> {
> 	struct dpll_pin *pin = info->user_ptr[0];
> 
>+	if (!xa_empty(&pin->parent_refs) &&
>+	    !dpll_pin_parents_registered(pin))
>+		return -ENODEV;
>+

Also make sure to prevent events from being send for this pin.


> 	return dpll_pin_set_from_nlattr(pin, info);
> }
> 
>-- 
>2.38.1
>

Re: [PATCH net v2 2/4] dpll: fix pin dump crash for rebound module

[Jiri Pirko]

Thu, Jan 04, 2024 at 12:11:30PM CET, arkadiusz.kubalewski@intel.com wrote:
>When a kernel module is unbound but the pin resources were not entirely
>freed (other kernel module instance have had kept the reference to that

Wait, you talk about module, yet you mean pci instance of the same
module, don't you?


>pin), and kernel module is again bound, the pin properties would not be
>updated (the properties are only assigned when memory for the pin is
>allocated), prop pointer still points to the kernel module memory of
>the kernel module which was deallocated on the unbind.
>
>If the pin dump is invoked in this state, the result is a kernel crash.
>Prevent the crash by storing persistent pin properties in dpll subsystem,
>copy the content from the kernel module when pin is allocated, instead of
>using memory of the kernel module.
>
>Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions")
>Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions")
>Reviewed-by: Jan Glaza <jan.glaza@intel.com>
>Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>---
> drivers/dpll/dpll_core.c    | 29 ++++++++++++++++++++++++++---
> drivers/dpll/dpll_core.h    |  4 ++--
> drivers/dpll/dpll_netlink.c | 28 ++++++++++++++--------------
> 3 files changed, 42 insertions(+), 19 deletions(-)
>
>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>index 3568149b9562..0b469096ef79 100644
>--- a/drivers/dpll/dpll_core.c
>+++ b/drivers/dpll/dpll_core.c
>@@ -429,6 +429,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct module *module,
> 	       const struct dpll_pin_properties *prop)
> {
> 	struct dpll_pin *pin;
>+	size_t freq_size;
> 	int ret;
> 
> 	pin = kzalloc(sizeof(*pin), GFP_KERNEL);
>@@ -440,9 +441,22 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct module *module,
> 	if (WARN_ON(prop->type < DPLL_PIN_TYPE_MUX ||
> 		    prop->type > DPLL_PIN_TYPE_MAX)) {
> 		ret = -EINVAL;
>-		goto err;
>+		goto pin_free;
> 	}
>-	pin->prop = prop;
>+	memcpy(&pin->prop, prop, sizeof(pin->prop));
>+	if (prop->freq_supported && prop->freq_supported_num) {
>+		freq_size = prop->freq_supported_num *
>+			    sizeof(*pin->prop.freq_supported);
>+		pin->prop.freq_supported = kmemdup(prop->freq_supported,
>+						   freq_size, GFP_KERNEL);
>+		if (!pin->prop.freq_supported) {
>+			ret = -ENOMEM;
>+			goto pin_free;
>+		}
>+	}
>+	pin->prop.board_label = kstrdup(prop->board_label, GFP_KERNEL);
>+	pin->prop.panel_label = kstrdup(prop->panel_label, GFP_KERNEL);
>+	pin->prop.package_label = kstrdup(prop->package_label, GFP_KERNEL);

Care to check the return values? Also don't kstrdup null pointers, does
not make much sense.

Could you perhaps move the prop dup/free to separate functions?


> 	refcount_set(&pin->refcount, 1);
> 	xa_init_flags(&pin->dpll_refs, XA_FLAGS_ALLOC);
> 	xa_init_flags(&pin->parent_refs, XA_FLAGS_ALLOC);
>@@ -451,8 +465,13 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct module *module,
> 		goto err;
> 	return pin;
> err:
>+	kfree(pin->prop.package_label);
>+	kfree(pin->prop.panel_label);
>+	kfree(pin->prop.board_label);
>+	kfree(pin->prop.freq_supported);
> 	xa_destroy(&pin->dpll_refs);
> 	xa_destroy(&pin->parent_refs);
>+pin_free:
> 	kfree(pin);
> 	return ERR_PTR(ret);
> }
>@@ -512,6 +531,10 @@ void dpll_pin_put(struct dpll_pin *pin)
> 		xa_destroy(&pin->dpll_refs);
> 		xa_destroy(&pin->parent_refs);
> 		xa_erase(&dpll_pin_xa, pin->id);
>+		kfree(pin->prop.board_label);
>+		kfree(pin->prop.panel_label);
>+		kfree(pin->prop.package_label);
>+		kfree(pin->prop.freq_supported);
> 		kfree(pin);
> 	}
> 	mutex_unlock(&dpll_lock);
>@@ -634,7 +657,7 @@ int dpll_pin_on_pin_register(struct dpll_pin *parent, struct dpll_pin *pin,
> 	unsigned long i, stop;
> 	int ret;
> 
>-	if (WARN_ON(parent->prop->type != DPLL_PIN_TYPE_MUX))
>+	if (WARN_ON(parent->prop.type != DPLL_PIN_TYPE_MUX))
> 		return -EINVAL;
> 
> 	if (WARN_ON(!ops) ||
>diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h
>index 5585873c5c1b..717f715015c7 100644
>--- a/drivers/dpll/dpll_core.h
>+++ b/drivers/dpll/dpll_core.h
>@@ -44,7 +44,7 @@ struct dpll_device {
>  * @module:		module of creator
>  * @dpll_refs:		hold referencees to dplls pin was registered with
>  * @parent_refs:	hold references to parent pins pin was registered with
>- * @prop:		pointer to pin properties given by registerer
>+ * @prop:		pin properties copied from the registerer
>  * @rclk_dev_name:	holds name of device when pin can recover clock from it
>  * @refcount:		refcount
>  **/
>@@ -55,7 +55,7 @@ struct dpll_pin {
> 	struct module *module;
> 	struct xarray dpll_refs;
> 	struct xarray parent_refs;
>-	const struct dpll_pin_properties *prop;
>+	struct dpll_pin_properties prop;
> 	refcount_t refcount;
> };
> 
>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>index b53478374a38..3ce9995013f1 100644
>--- a/drivers/dpll/dpll_netlink.c
>+++ b/drivers/dpll/dpll_netlink.c
>@@ -278,17 +278,17 @@ dpll_msg_add_pin_freq(struct sk_buff *msg, struct dpll_pin *pin,
> 	if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY, sizeof(freq), &freq,
> 			  DPLL_A_PIN_PAD))
> 		return -EMSGSIZE;
>-	for (fs = 0; fs < pin->prop->freq_supported_num; fs++) {
>+	for (fs = 0; fs < pin->prop.freq_supported_num; fs++) {
> 		nest = nla_nest_start(msg, DPLL_A_PIN_FREQUENCY_SUPPORTED);
> 		if (!nest)
> 			return -EMSGSIZE;
>-		freq = pin->prop->freq_supported[fs].min;
>+		freq = pin->prop.freq_supported[fs].min;
> 		if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MIN, sizeof(freq),
> 				  &freq, DPLL_A_PIN_PAD)) {
> 			nla_nest_cancel(msg, nest);
> 			return -EMSGSIZE;
> 		}
>-		freq = pin->prop->freq_supported[fs].max;
>+		freq = pin->prop.freq_supported[fs].max;
> 		if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MAX, sizeof(freq),
> 				  &freq, DPLL_A_PIN_PAD)) {
> 			nla_nest_cancel(msg, nest);
>@@ -304,9 +304,9 @@ static bool dpll_pin_is_freq_supported(struct dpll_pin *pin, u32 freq)
> {
> 	int fs;
> 
>-	for (fs = 0; fs < pin->prop->freq_supported_num; fs++)
>-		if (freq >= pin->prop->freq_supported[fs].min &&
>-		    freq <= pin->prop->freq_supported[fs].max)
>+	for (fs = 0; fs < pin->prop.freq_supported_num; fs++)
>+		if (freq >= pin->prop.freq_supported[fs].min &&
>+		    freq <= pin->prop.freq_supported[fs].max)
> 			return true;
> 	return false;
> }
>@@ -398,7 +398,7 @@ static int
> dpll_cmd_pin_get_one(struct sk_buff *msg, struct dpll_pin *pin,
> 		     struct netlink_ext_ack *extack)
> {
>-	const struct dpll_pin_properties *prop = pin->prop;
>+	const struct dpll_pin_properties *prop = &pin->prop;
> 	struct dpll_pin_ref *ref;
> 	int ret;
> 
>@@ -691,7 +691,7 @@ dpll_pin_on_pin_state_set(struct dpll_pin *pin, u32 parent_idx,
> 	int ret;
> 
> 	if (!(DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE &
>-	      pin->prop->capabilities)) {
>+	      pin->prop.capabilities)) {
> 		NL_SET_ERR_MSG(extack, "state changing is not allowed");
> 		return -EOPNOTSUPP;
> 	}
>@@ -727,7 +727,7 @@ dpll_pin_state_set(struct dpll_device *dpll, struct dpll_pin *pin,
> 	int ret;
> 
> 	if (!(DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE &
>-	      pin->prop->capabilities)) {
>+	      pin->prop.capabilities)) {
> 		NL_SET_ERR_MSG(extack, "state changing is not allowed");
> 		return -EOPNOTSUPP;
> 	}
>@@ -754,7 +754,7 @@ dpll_pin_prio_set(struct dpll_device *dpll, struct dpll_pin *pin,
> 	int ret;
> 
> 	if (!(DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE &
>-	      pin->prop->capabilities)) {
>+	      pin->prop.capabilities)) {
> 		NL_SET_ERR_MSG(extack, "prio changing is not allowed");
> 		return -EOPNOTSUPP;
> 	}
>@@ -782,7 +782,7 @@ dpll_pin_direction_set(struct dpll_pin *pin, struct dpll_device *dpll,
> 	int ret;
> 
> 	if (!(DPLL_PIN_CAPABILITIES_DIRECTION_CAN_CHANGE &
>-	      pin->prop->capabilities)) {
>+	      pin->prop.capabilities)) {
> 		NL_SET_ERR_MSG(extack, "direction changing is not allowed");
> 		return -EOPNOTSUPP;
> 	}
>@@ -812,8 +812,8 @@ dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr,
> 	int ret;
> 
> 	phase_adj = nla_get_s32(phase_adj_attr);
>-	if (phase_adj > pin->prop->phase_range.max ||
>-	    phase_adj < pin->prop->phase_range.min) {
>+	if (phase_adj > pin->prop.phase_range.max ||
>+	    phase_adj < pin->prop.phase_range.min) {
> 		NL_SET_ERR_MSG_ATTR(extack, phase_adj_attr,
> 				    "phase adjust value not supported");
> 		return -EINVAL;
>@@ -997,7 +997,7 @@ dpll_pin_find(u64 clock_id, struct nlattr *mod_name_attr,
> 	unsigned long i;
> 
> 	xa_for_each_marked(&dpll_pin_xa, i, pin, DPLL_REGISTERED) {
>-		prop = pin->prop;
>+		prop = &pin->prop;
> 		cid_match = clock_id ? pin->clock_id == clock_id : true;
> 		mod_match = mod_name_attr && module_name(pin->module) ?
> 			!nla_strcmp(mod_name_attr,
>-- 
>2.38.1
>

RE: [PATCH net v2 1/4] dpll: fix pin dump crash after module unbind

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Thursday, January 4, 2024 4:09 PM
>
>Thu, Jan 04, 2024 at 12:11:29PM CET, arkadiusz.kubalewski@intel.com wrote:
>>Disallow dump of unregistered parent pins, it is possible when parent
>>pin and dpll device registerer kernel module instance unbinds, and
>>other kernel module instances of the same dpll device have pins
>>registered with the parent pin. The user can invoke a pin-dump but as
>>the parent was unregistered, those shall not be accessed by the
>>userspace, prevent that by checking if parent pin is still registered.
>>
>>Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions")
>>Reviewed-by: Jan Glaza <jan.glaza@intel.com>
>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>---
>> drivers/dpll/dpll_netlink.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>>index ce7cf736f020..b53478374a38 100644
>>--- a/drivers/dpll/dpll_netlink.c
>>+++ b/drivers/dpll/dpll_netlink.c
>>@@ -328,6 +328,8 @@ dpll_msg_add_pin_parents(struct sk_buff *msg, struct
>>dpll_pin *pin,
>> 		void *parent_priv;
>>
>> 		ppin = ref->pin;
>>+		if (!xa_get_mark(&dpll_pin_xa, ppin->id, DPLL_REGISTERED))
>>+			continue;
>
>If you make sure to hide the pin properly with the last patch, you don't
>need this one. Drop it.
>

Makes sense, will do.

Thank you!
Arkadiusz

>
>> 		parent_priv = dpll_pin_on_dpll_priv(dpll_ref->dpll, ppin);
>> 		ret = ops->state_on_pin_get(pin,
>> 					    dpll_pin_on_pin_priv(ppin, pin),
>>--
>>2.38.1
>>

RE: [PATCH net v2 3/4] dpll: fix register pin with unregistered parent pin

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Thursday, January 4, 2024 3:48 PM
>
>Thu, Jan 04, 2024 at 12:11:31PM CET, arkadiusz.kubalewski@intel.com wrote:
>>In case of multiple kernel module instances using the same dpll device:
>>if only one registers dpll device, then only that one can register
>>directly connected pins with a dpll device. When unregistered parent is
>>responsible for determining if the muxed pin can be registered with it
>>or not, the drivers need to be loaded in serialized order to work
>>correctly - first the driver instance which registers the direct pins
>>needs to be loaded, then the other instances could register muxed type
>>pins.
>>
>>Allow registration of a pin with a parent even if the parent was not
>>yet registered, thus allow ability for unserialized driver instance
>>load order.
>>Do not WARN_ON notification for unregistered pin, which can be invoked
>>for described case, instead just return error.
>>
>>Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions")
>>Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base
>>functions")
>>Reviewed-by: Jan Glaza <jan.glaza@intel.com>
>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>---
>> drivers/dpll/dpll_core.c    | 4 ----
>> drivers/dpll/dpll_netlink.c | 2 +-
>> 2 files changed, 1 insertion(+), 5 deletions(-)
>>
>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c index
>>0b469096ef79..c8a2129f5699 100644
>>--- a/drivers/dpll/dpll_core.c
>>+++ b/drivers/dpll/dpll_core.c
>>@@ -28,8 +28,6 @@ static u32 dpll_xa_id;
>> 	WARN_ON_ONCE(!xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
>> #define ASSERT_DPLL_NOT_REGISTERED(d)	\
>> 	WARN_ON_ONCE(xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
>>-#define ASSERT_PIN_REGISTERED(p)	\
>>-	WARN_ON_ONCE(!xa_get_mark(&dpll_pin_xa, (p)->id, DPLL_REGISTERED))
>>
>> struct dpll_device_registration {
>> 	struct list_head list;
>>@@ -664,8 +662,6 @@ int dpll_pin_on_pin_register(struct dpll_pin *parent,
>>struct dpll_pin *pin,
>> 	    WARN_ON(!ops->state_on_pin_get) ||
>> 	    WARN_ON(!ops->direction_get))
>> 		return -EINVAL;
>>-	if (ASSERT_PIN_REGISTERED(parent))
>>-		return -EINVAL;
>
>This makes the pin-on-device and pin-on-pin register behaviour
>different:
>int
>dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin,
>                  const struct dpll_pin_ops *ops, void *priv) {
>	...
>        if (ASSERT_DPLL_REGISTERED(dpll))
>                return -EINVAL;
>
>I think it is need to maintain the same set of restrictions and behaviour
>the same for both.
>
>With what you suggest, the user would just see couple of pins with no
>parent (hidden one), no dpll devices (none would be registered).
>That's odd.
>
>PF0 is the owner of DPLL in your case.
>
>From the user perspective, I think it should look like:
>1) If PFn appears after PF0, it registers pins related to it, PF0
>   created instances are there and valid. User sees them all.
>2) If PF0 gets removed before PFn, it removes all dpll related entities,
>   even those related to PFn. Users sees nothing.
>
>So you have to make sure that the pin is hidden (not shown to the user) in
>case the parent (device/pin) is not registered. Makes sense?
>

Yes, perfect sense. Will do.

Thank you!
Arkadiusz

>
>
>>
>> 	mutex_lock(&dpll_lock);
>> 	ret = dpll_xa_ref_pin_add(&pin->parent_refs, parent, ops, priv); diff
>>--git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c index
>>3ce9995013f1..f266db8da2f0 100644
>>--- a/drivers/dpll/dpll_netlink.c
>>+++ b/drivers/dpll/dpll_netlink.c
>>@@ -553,7 +553,7 @@ dpll_pin_event_send(enum dpll_cmd event, struct
>>dpll_pin *pin)
>> 	int ret = -ENOMEM;
>> 	void *hdr;
>>
>>-	if (WARN_ON(!xa_get_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED)))
>>+	if (!xa_get_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED))
>> 		return -ENODEV;
>>
>> 	msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>>--
>>2.38.1
>>

RE: [PATCH net v2 2/4] dpll: fix pin dump crash for rebound module

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Thursday, January 4, 2024 4:41 PM
>
>Thu, Jan 04, 2024 at 12:11:30PM CET, arkadiusz.kubalewski@intel.com wrote:
>>When a kernel module is unbound but the pin resources were not entirely
>>freed (other kernel module instance have had kept the reference to that
>
>Wait, you talk about module, yet you mean pci instance of the same
>module, don't you?
>

Well, the kernel module can have multiple instances. I.e. for multiple PCI
devices, or one each Physical Function available on a single PCI device. Our case
is a kernel module instance of different PF but still a single PCI device.
In general, all the PCI devices controlled by given kernel module which use the
same dpll device would see this behavior.

But sure, can mention that those belong to single PCI device in the commit
message.

>
>>pin), and kernel module is again bound, the pin properties would not be
>>updated (the properties are only assigned when memory for the pin is
>>allocated), prop pointer still points to the kernel module memory of
>>the kernel module which was deallocated on the unbind.
>>
>>If the pin dump is invoked in this state, the result is a kernel crash.
>>Prevent the crash by storing persistent pin properties in dpll subsystem,
>>copy the content from the kernel module when pin is allocated, instead of
>>using memory of the kernel module.
>>
>>Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions")
>>Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions")
>>Reviewed-by: Jan Glaza <jan.glaza@intel.com>
>>Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>---
>> drivers/dpll/dpll_core.c    | 29 ++++++++++++++++++++++++++---
>> drivers/dpll/dpll_core.h    |  4 ++--
>> drivers/dpll/dpll_netlink.c | 28 ++++++++++++++--------------
>> 3 files changed, 42 insertions(+), 19 deletions(-)
>>
>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>index 3568149b9562..0b469096ef79 100644
>>--- a/drivers/dpll/dpll_core.c
>>+++ b/drivers/dpll/dpll_core.c
>>@@ -429,6 +429,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct
>>module *module,
>> 	       const struct dpll_pin_properties *prop)
>> {
>> 	struct dpll_pin *pin;
>>+	size_t freq_size;
>> 	int ret;
>>
>> 	pin = kzalloc(sizeof(*pin), GFP_KERNEL);
>>@@ -440,9 +441,22 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct
>>module *module,
>> 	if (WARN_ON(prop->type < DPLL_PIN_TYPE_MUX ||
>> 		    prop->type > DPLL_PIN_TYPE_MAX)) {
>> 		ret = -EINVAL;
>>-		goto err;
>>+		goto pin_free;
>> 	}
>>-	pin->prop = prop;
>>+	memcpy(&pin->prop, prop, sizeof(pin->prop));
>>+	if (prop->freq_supported && prop->freq_supported_num) {
>>+		freq_size = prop->freq_supported_num *
>>+			    sizeof(*pin->prop.freq_supported);
>>+		pin->prop.freq_supported = kmemdup(prop->freq_supported,
>>+						   freq_size, GFP_KERNEL);
>>+		if (!pin->prop.freq_supported) {
>>+			ret = -ENOMEM;
>>+			goto pin_free;
>>+		}
>>+	}
>>+	pin->prop.board_label = kstrdup(prop->board_label, GFP_KERNEL);
>>+	pin->prop.panel_label = kstrdup(prop->panel_label, GFP_KERNEL);
>>+	pin->prop.package_label = kstrdup(prop->package_label, GFP_KERNEL);
>
>Care to check the return values? Also don't kstrdup null pointers, does
>not make much sense.
>
>Could you perhaps move the prop dup/free to separate functions?
>

Sure, will do in v3.

Thank you!
Arkadiusz

>
>> 	refcount_set(&pin->refcount, 1);
>> 	xa_init_flags(&pin->dpll_refs, XA_FLAGS_ALLOC);
>> 	xa_init_flags(&pin->parent_refs, XA_FLAGS_ALLOC);
>>@@ -451,8 +465,13 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct
>>module *module,
>> 		goto err;
>> 	return pin;
>> err:
>>+	kfree(pin->prop.package_label);
>>+	kfree(pin->prop.panel_label);
>>+	kfree(pin->prop.board_label);
>>+	kfree(pin->prop.freq_supported);
>> 	xa_destroy(&pin->dpll_refs);
>> 	xa_destroy(&pin->parent_refs);
>>+pin_free:
>> 	kfree(pin);
>> 	return ERR_PTR(ret);
>> }
>>@@ -512,6 +531,10 @@ void dpll_pin_put(struct dpll_pin *pin)
>> 		xa_destroy(&pin->dpll_refs);
>> 		xa_destroy(&pin->parent_refs);
>> 		xa_erase(&dpll_pin_xa, pin->id);
>>+		kfree(pin->prop.board_label);
>>+		kfree(pin->prop.panel_label);
>>+		kfree(pin->prop.package_label);
>>+		kfree(pin->prop.freq_supported);
>> 		kfree(pin);
>> 	}
>> 	mutex_unlock(&dpll_lock);
>>@@ -634,7 +657,7 @@ int dpll_pin_on_pin_register(struct dpll_pin *parent,
>>struct dpll_pin *pin,
>> 	unsigned long i, stop;
>> 	int ret;
>>
>>-	if (WARN_ON(parent->prop->type != DPLL_PIN_TYPE_MUX))
>>+	if (WARN_ON(parent->prop.type != DPLL_PIN_TYPE_MUX))
>> 		return -EINVAL;
>>
>> 	if (WARN_ON(!ops) ||
>>diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h
>>index 5585873c5c1b..717f715015c7 100644
>>--- a/drivers/dpll/dpll_core.h
>>+++ b/drivers/dpll/dpll_core.h
>>@@ -44,7 +44,7 @@ struct dpll_device {
>>  * @module:		module of creator
>>  * @dpll_refs:		hold referencees to dplls pin was registered with
>>  * @parent_refs:	hold references to parent pins pin was registered
>>with
>>- * @prop:		pointer to pin properties given by registerer
>>+ * @prop:		pin properties copied from the registerer
>>  * @rclk_dev_name:	holds name of device when pin can recover clock
>>from it
>>  * @refcount:		refcount
>>  **/
>>@@ -55,7 +55,7 @@ struct dpll_pin {
>> 	struct module *module;
>> 	struct xarray dpll_refs;
>> 	struct xarray parent_refs;
>>-	const struct dpll_pin_properties *prop;
>>+	struct dpll_pin_properties prop;
>> 	refcount_t refcount;
>> };
>>
>>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>>index b53478374a38..3ce9995013f1 100644
>>--- a/drivers/dpll/dpll_netlink.c
>>+++ b/drivers/dpll/dpll_netlink.c
>>@@ -278,17 +278,17 @@ dpll_msg_add_pin_freq(struct sk_buff *msg, struct
>>dpll_pin *pin,
>> 	if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY, sizeof(freq), &freq,
>> 			  DPLL_A_PIN_PAD))
>> 		return -EMSGSIZE;
>>-	for (fs = 0; fs < pin->prop->freq_supported_num; fs++) {
>>+	for (fs = 0; fs < pin->prop.freq_supported_num; fs++) {
>> 		nest = nla_nest_start(msg, DPLL_A_PIN_FREQUENCY_SUPPORTED);
>> 		if (!nest)
>> 			return -EMSGSIZE;
>>-		freq = pin->prop->freq_supported[fs].min;
>>+		freq = pin->prop.freq_supported[fs].min;
>> 		if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MIN, sizeof(freq),
>> 				  &freq, DPLL_A_PIN_PAD)) {
>> 			nla_nest_cancel(msg, nest);
>> 			return -EMSGSIZE;
>> 		}
>>-		freq = pin->prop->freq_supported[fs].max;
>>+		freq = pin->prop.freq_supported[fs].max;
>> 		if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MAX, sizeof(freq),
>> 				  &freq, DPLL_A_PIN_PAD)) {
>> 			nla_nest_cancel(msg, nest);
>>@@ -304,9 +304,9 @@ static bool dpll_pin_is_freq_supported(struct dpll_pin
>>*pin, u32 freq)
>> {
>> 	int fs;
>>
>>-	for (fs = 0; fs < pin->prop->freq_supported_num; fs++)
>>-		if (freq >= pin->prop->freq_supported[fs].min &&
>>-		    freq <= pin->prop->freq_supported[fs].max)
>>+	for (fs = 0; fs < pin->prop.freq_supported_num; fs++)
>>+		if (freq >= pin->prop.freq_supported[fs].min &&
>>+		    freq <= pin->prop.freq_supported[fs].max)
>> 			return true;
>> 	return false;
>> }
>>@@ -398,7 +398,7 @@ static int
>> dpll_cmd_pin_get_one(struct sk_buff *msg, struct dpll_pin *pin,
>> 		     struct netlink_ext_ack *extack)
>> {
>>-	const struct dpll_pin_properties *prop = pin->prop;
>>+	const struct dpll_pin_properties *prop = &pin->prop;
>> 	struct dpll_pin_ref *ref;
>> 	int ret;
>>
>>@@ -691,7 +691,7 @@ dpll_pin_on_pin_state_set(struct dpll_pin *pin, u32
>>parent_idx,
>> 	int ret;
>>
>> 	if (!(DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE &
>>-	      pin->prop->capabilities)) {
>>+	      pin->prop.capabilities)) {
>> 		NL_SET_ERR_MSG(extack, "state changing is not allowed");
>> 		return -EOPNOTSUPP;
>> 	}
>>@@ -727,7 +727,7 @@ dpll_pin_state_set(struct dpll_device *dpll, struct
>>dpll_pin *pin,
>> 	int ret;
>>
>> 	if (!(DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE &
>>-	      pin->prop->capabilities)) {
>>+	      pin->prop.capabilities)) {
>> 		NL_SET_ERR_MSG(extack, "state changing is not allowed");
>> 		return -EOPNOTSUPP;
>> 	}
>>@@ -754,7 +754,7 @@ dpll_pin_prio_set(struct dpll_device *dpll, struct
>>dpll_pin *pin,
>> 	int ret;
>>
>> 	if (!(DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE &
>>-	      pin->prop->capabilities)) {
>>+	      pin->prop.capabilities)) {
>> 		NL_SET_ERR_MSG(extack, "prio changing is not allowed");
>> 		return -EOPNOTSUPP;
>> 	}
>>@@ -782,7 +782,7 @@ dpll_pin_direction_set(struct dpll_pin *pin, struct
>>dpll_device *dpll,
>> 	int ret;
>>
>> 	if (!(DPLL_PIN_CAPABILITIES_DIRECTION_CAN_CHANGE &
>>-	      pin->prop->capabilities)) {
>>+	      pin->prop.capabilities)) {
>> 		NL_SET_ERR_MSG(extack, "direction changing is not allowed");
>> 		return -EOPNOTSUPP;
>> 	}
>>@@ -812,8 +812,8 @@ dpll_pin_phase_adj_set(struct dpll_pin *pin, struct
>>nlattr *phase_adj_attr,
>> 	int ret;
>>
>> 	phase_adj = nla_get_s32(phase_adj_attr);
>>-	if (phase_adj > pin->prop->phase_range.max ||
>>-	    phase_adj < pin->prop->phase_range.min) {
>>+	if (phase_adj > pin->prop.phase_range.max ||
>>+	    phase_adj < pin->prop.phase_range.min) {
>> 		NL_SET_ERR_MSG_ATTR(extack, phase_adj_attr,
>> 				    "phase adjust value not supported");
>> 		return -EINVAL;
>>@@ -997,7 +997,7 @@ dpll_pin_find(u64 clock_id, struct nlattr
>>*mod_name_attr,
>> 	unsigned long i;
>>
>> 	xa_for_each_marked(&dpll_pin_xa, i, pin, DPLL_REGISTERED) {
>>-		prop = pin->prop;
>>+		prop = &pin->prop;
>> 		cid_match = clock_id ? pin->clock_id == clock_id : true;
>> 		mod_match = mod_name_attr && module_name(pin->module) ?
>> 			!nla_strcmp(mod_name_attr,
>>--
>>2.38.1
>>

RE: [PATCH net v2 4/4] dpll: hide "zombie" pins for userspace

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Thursday, January 4, 2024 4:09 PM
>
>Thu, Jan 04, 2024 at 12:11:32PM CET, arkadiusz.kubalewski@intel.com wrote:
>
>[...]
>
>>@@ -1179,6 +1195,10 @@ int dpll_nl_pin_set_doit(struct sk_buff *skb,
>>struct genl_info *info)
>> {
>> 	struct dpll_pin *pin = info->user_ptr[0];
>>
>>+	if (!xa_empty(&pin->parent_refs) &&
>>+	    !dpll_pin_parents_registered(pin))
>>+		return -ENODEV;
>>+
>
>Also make sure to prevent events from being send for this pin.
>

Sure, will do.

Thank you!
Arkadiusz

>
>> 	return dpll_pin_set_from_nlattr(pin, info);
>> }
>>
>>--
>>2.38.1
>>

RE: [PATCH net v2 4/4] dpll: hide "zombie" pins for userspace

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Thursday, January 4, 2024 4:07 PM
>
>Thu, Jan 04, 2024 at 12:11:32PM CET, arkadiusz.kubalewski@intel.com wrote:
>
>[...]
>
>
>>@@ -1179,6 +1195,10 @@ int dpll_nl_pin_set_doit(struct sk_buff *skb,
>struct genl_info *info)
>
>
>What about dpll_nl_pin_get_doit(), dpll_nl_pin_id_get_doit()?
>
>I think it would be better to move the check to:
>dpll_pin_pre_doit()
>

Yes, makes sense, will move it to dpll_pin_pre_doit().
Then won't be needed in dpll_nl_pin_get_doit() and
dpll_nl_pin_set_doit().

Plus, will add check in dpll_nl_pin_id_get_doit().


Thank you!
Arkadiusz

>
>> {
>> 	struct dpll_pin *pin = info->user_ptr[0];
>>
>>+	if (!xa_empty(&pin->parent_refs) &&
>>+	    !dpll_pin_parents_registered(pin))
>>+		return -ENODEV;
>>+
>> 	return dpll_pin_set_from_nlattr(pin, info);
>> }
>>
>>--
>>2.38.1
>>

RE: [PATCH net v2 4/4] dpll: hide "zombie" pins for userspace

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Thursday, January 4, 2024 4:05 PM
>To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>
>Cc: netdev@vger.kernel.org; vadim.fedorenko@linux.dev;
>michal.michalik@intel.com; Olech, Milena <milena.olech@intel.com>;
>pabeni@redhat.com; kuba@kernel.org; Glaza, Jan <jan.glaza@intel.com>
>Subject: Re: [PATCH net v2 4/4] dpll: hide "zombie" pins for userspace
>
>Thu, Jan 04, 2024 at 12:11:32PM CET, arkadiusz.kubalewski@intel.com wrote:
>>If parent pin was unregistered but child pin was not, the userspace
>>would see the "zombie" pins - the ones that were registered with
>>parent pin (pins_pin_on_pin_register(..)).
>>Technically those are not available - as there is no dpll device in the
>>system. Do not dump those pins and prevent userspace from any
>>interaction with them.
>
>Ah, here it is :)
>

:)

>
>>
>>Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions")
>>Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions")
>>Reviewed-by: Jan Glaza <jan.glaza@intel.com>
>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>---
>> drivers/dpll/dpll_netlink.c | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>>index f266db8da2f0..495dfc43c0be 100644
>>--- a/drivers/dpll/dpll_netlink.c
>>+++ b/drivers/dpll/dpll_netlink.c
>>@@ -949,6 +949,19 @@ dpll_pin_parent_pin_set(struct dpll_pin *pin, struct
>nlattr *parent_nest,
>> 	return 0;
>> }
>>
>>+static bool dpll_pin_parents_registered(struct dpll_pin *pin)
>>+{
>>+	struct dpll_pin_ref *par_ref;
>>+	struct dpll_pin *p;
>>+	unsigned long i, j;
>>+
>>+	xa_for_each(&pin->parent_refs, i, par_ref)
>>+		xa_for_each_marked(&dpll_pin_xa, j, p, DPLL_REGISTERED)
>>+			if (par_ref->pin == p)
>>+				return true;
>
>		if (xa_get_mark(..))
>			return true;
>?
>

Sure, will do.

>
>>+	return false;
>
>
>As I wrote in the reply to the other patch, could you unify the "hide"
>behaviour for unregistered parent pin/device?
>

Yes, in v3.

>
>>+}
>>+
>> static int
>> dpll_pin_set_from_nlattr(struct dpll_pin *pin, struct genl_info *info)
>> {
>>@@ -1153,6 +1166,9 @@ int dpll_nl_pin_get_dumpit(struct sk_buff *skb,
>>struct netlink_callback *cb)
>>
>> 	xa_for_each_marked_start(&dpll_pin_xa, i, pin, DPLL_REGISTERED,
>> 				 ctx->idx) {
>>+		if (!xa_empty(&pin->parent_refs) &&
>
>This empty check is redundant, remove it.
>

Ok.

>
>>+		    !dpll_pin_parents_registered(pin))
>>+			continue;
>> 		hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
>> 				  cb->nlh->nlmsg_seq,
>> 				  &dpll_nl_family, NLM_F_MULTI,
>>@@ -1179,6 +1195,10 @@ int dpll_nl_pin_set_doit(struct sk_buff *skb,
>>struct genl_info *info)
>> {
>> 	struct dpll_pin *pin = info->user_ptr[0];
>>
>>+	if (!xa_empty(&pin->parent_refs) &&
>
>This empty check is redundant, remove it.
>

Will do.
Thank you!
Arkadiusz

>
>>+	    !dpll_pin_parents_registered(pin))
>>+		return -ENODEV;
>>+
>> 	return dpll_pin_set_from_nlattr(pin, info);
>> }
>>
>>--
>>2.38.1
>>