[net,2/3] dpll: fix pin dump crash for rebound module

Message ID	20231108103226.1168500-3-arkadiusz.kubalewski@intel.com (mailing list archive)
State	Changes Requested
Delegated to:	Netdev Maintainers
Headers	show Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id F03FD12E54 for <netdev@vger.kernel.org>; Wed, 8 Nov 2023 10:35:33 +0000 (UTC) From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> To: netdev@vger.kernel.org Cc: vadim.fedorenko@linux.dev, jiri@resnulli.us, michal.michalik@intel.com, milena.olech@intel.com, pabeni@redhat.com, kuba@kernel.org, Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> Subject: [PATCH net 2/3] dpll: fix pin dump crash for rebound module Date: Wed, 8 Nov 2023 11:32:25 +0100 Message-Id: <20231108103226.1168500-3-arkadiusz.kubalewski@intel.com> In-Reply-To: <20231108103226.1168500-1-arkadiusz.kubalewski@intel.com> References: <20231108103226.1168500-1-arkadiusz.kubalewski@intel.com> Precedence: bulk MIME-Version: 1.0 Content-Transfer-Encoding: 8bit
Series	dpll: fix unordered unbind/bind registerer issues \| expand [net,0/3] dpll: fix unordered unbind/bind registerer issues [net,1/3] dpll: fix pin dump crash after module unbind [net,2/3] dpll: fix pin dump crash for rebound module [net,3/3] dpll: fix register pin with unregistered parent pin

Context	Check	Description
netdev/series_format	success	Posting correctly formatted
netdev/tree_selection	success	Clearly marked for net
netdev/fixes_present	success	Fixes tag present in non-next series
netdev/header_inline	success	No static functions without inline keyword in header files
netdev/build_32bit	success	Errors and warnings before: 1314 this patch: 1314
netdev/cc_maintainers	fail	1 blamed authors not CCed: davem@davemloft.net; 1 maintainers not CCed: davem@davemloft.net
netdev/build_clang	success	Errors and warnings before: 1341 this patch: 1341
netdev/verify_signedoff	success	Signed-off-by tag matches author and committer
netdev/deprecated_api	success	None detected
netdev/check_selftest	success	No net selftest shell script
netdev/verify_fixes	success	Fixes tag looks correct
netdev/build_allmodconfig_warn	success	Errors and warnings before: 1342 this patch: 1342
netdev/checkpatch	success	total: 0 errors, 0 warnings, 0 checks, 122 lines checked
netdev/build_clang_rust	success	No Rust files in patch. Skipping build
netdev/kdoc	success	Errors and warnings before: 0 this patch: 0
netdev/source_inline	success	Was 0 now: 0

Kubalewski, Arkadiusz Nov. 8, 2023, 10:32 a.m. UTC

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")
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
 drivers/dpll/dpll_core.c    |  4 ++--
 drivers/dpll/dpll_core.h    |  4 ++--
 drivers/dpll/dpll_netlink.c | 28 ++++++++++++++--------------
 3 files changed, 18 insertions(+), 18 deletions(-)

Jiri Pirko Nov. 8, 2023, 2:30 p.m. UTC | #1

Wed, Nov 08, 2023 at 11:32:25AM 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
>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")
>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>---
> drivers/dpll/dpll_core.c    |  4 ++--
> drivers/dpll/dpll_core.h    |  4 ++--
> drivers/dpll/dpll_netlink.c | 28 ++++++++++++++--------------
> 3 files changed, 18 insertions(+), 18 deletions(-)
>
>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>index 3568149b9562..4077b562ba3b 100644
>--- a/drivers/dpll/dpll_core.c
>+++ b/drivers/dpll/dpll_core.c
>@@ -442,7 +442,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct module *module,
> 		ret = -EINVAL;
> 		goto err;
> 	}
>-	pin->prop = prop;
>+	memcpy(&pin->prop, prop, sizeof(pin->prop));

Odd, you don't care about the pointer within this structure?


> 	refcount_set(&pin->refcount, 1);
> 	xa_init_flags(&pin->dpll_refs, XA_FLAGS_ALLOC);
> 	xa_init_flags(&pin->parent_refs, XA_FLAGS_ALLOC);
>@@ -634,7 +634,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 93fc6c4b8a78..963bbbbe6660 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;
> }
>@@ -403,7 +403,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;
> 
>@@ -696,7 +696,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;
> 	}
>@@ -732,7 +732,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;
> 	}
>@@ -759,7 +759,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;
> 	}
>@@ -787,7 +787,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;
> 	}
>@@ -817,8 +817,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;
>@@ -999,7 +999,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
>

Kubalewski, Arkadiusz Nov. 9, 2023, 12:20 p.m. UTC | #2

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Wednesday, November 8, 2023 3:30 PM
>
>Wed, Nov 08, 2023 at 11:32:25AM 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
>>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")
>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>---
>> drivers/dpll/dpll_core.c    |  4 ++--
>> drivers/dpll/dpll_core.h    |  4 ++--
>> drivers/dpll/dpll_netlink.c | 28 ++++++++++++++--------------
>> 3 files changed, 18 insertions(+), 18 deletions(-)
>>
>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>index 3568149b9562..4077b562ba3b 100644
>>--- a/drivers/dpll/dpll_core.c
>>+++ b/drivers/dpll/dpll_core.c
>>@@ -442,7 +442,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct
>>module *module,
>> 		ret = -EINVAL;
>> 		goto err;
>> 	}
>>-	pin->prop = prop;
>>+	memcpy(&pin->prop, prop, sizeof(pin->prop));
>
>Odd, you don't care about the pointer within this structure?
>

Well, true. Need a fix.
Wondering if copying idea is better than just assigning prop pointer on
each call to dpll_pin_get(..) function (when pin already exists)?

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);
>>@@ -634,7 +634,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 93fc6c4b8a78..963bbbbe6660 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;
>> }
>>@@ -403,7 +403,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;
>>
>>@@ -696,7 +696,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;
>> 	}
>>@@ -732,7 +732,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;
>> 	}
>>@@ -759,7 +759,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;
>> 	}
>>@@ -787,7 +787,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;
>> 	}
>>@@ -817,8 +817,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;
>>@@ -999,7 +999,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
>>

Jiri Pirko Nov. 9, 2023, 1:19 p.m. UTC | #3

Thu, Nov 09, 2023 at 01:20:48PM CET, arkadiusz.kubalewski@intel.com wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Sent: Wednesday, November 8, 2023 3:30 PM
>>
>>Wed, Nov 08, 2023 at 11:32:25AM 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
>>>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")
>>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>>---
>>> drivers/dpll/dpll_core.c    |  4 ++--
>>> drivers/dpll/dpll_core.h    |  4 ++--
>>> drivers/dpll/dpll_netlink.c | 28 ++++++++++++++--------------
>>> 3 files changed, 18 insertions(+), 18 deletions(-)
>>>
>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>index 3568149b9562..4077b562ba3b 100644
>>>--- a/drivers/dpll/dpll_core.c
>>>+++ b/drivers/dpll/dpll_core.c
>>>@@ -442,7 +442,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct
>>>module *module,
>>> 		ret = -EINVAL;
>>> 		goto err;
>>> 	}
>>>-	pin->prop = prop;
>>>+	memcpy(&pin->prop, prop, sizeof(pin->prop));
>>
>>Odd, you don't care about the pointer within this structure?
>>
>
>Well, true. Need a fix.
>Wondering if copying idea is better than just assigning prop pointer on
>each call to dpll_pin_get(..) function (when pin already exists)?

Not sure what do you mean. Examples please.


>
>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);
>>>@@ -634,7 +634,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 93fc6c4b8a78..963bbbbe6660 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;
>>> }
>>>@@ -403,7 +403,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;
>>>
>>>@@ -696,7 +696,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;
>>> 	}
>>>@@ -732,7 +732,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;
>>> 	}
>>>@@ -759,7 +759,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;
>>> 	}
>>>@@ -787,7 +787,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;
>>> 	}
>>>@@ -817,8 +817,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;
>>>@@ -999,7 +999,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
>>>
>

Kubalewski, Arkadiusz Nov. 9, 2023, 4:30 p.m. UTC | #4

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Thursday, November 9, 2023 2:19 PM
>
>Thu, Nov 09, 2023 at 01:20:48PM CET, arkadiusz.kubalewski@intel.com wrote:
>>>From: Jiri Pirko <jiri@resnulli.us>
>>>Sent: Wednesday, November 8, 2023 3:30 PM
>>>
>>>Wed, Nov 08, 2023 at 11:32:25AM 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
>>>>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")
>>>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>>>---
>>>> drivers/dpll/dpll_core.c    |  4 ++--
>>>> drivers/dpll/dpll_core.h    |  4 ++--
>>>> drivers/dpll/dpll_netlink.c | 28 ++++++++++++++--------------
>>>> 3 files changed, 18 insertions(+), 18 deletions(-)
>>>>
>>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>>index 3568149b9562..4077b562ba3b 100644
>>>>--- a/drivers/dpll/dpll_core.c
>>>>+++ b/drivers/dpll/dpll_core.c
>>>>@@ -442,7 +442,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct
>>>>module *module,
>>>> 		ret = -EINVAL;
>>>> 		goto err;
>>>> 	}
>>>>-	pin->prop = prop;
>>>>+	memcpy(&pin->prop, prop, sizeof(pin->prop));
>>>
>>>Odd, you don't care about the pointer within this structure?
>>>
>>
>>Well, true. Need a fix.
>>Wondering if copying idea is better than just assigning prop pointer on
>>each call to dpll_pin_get(..) function (when pin already exists)?
>
>Not sure what do you mean. Examples please.
>

Sure,

Basically this change:

diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
index ae884b92d68c..06b72d5877c3 100644
--- a/drivers/dpll/dpll_core.c
+++ b/drivers/dpll/dpll_core.c
@@ -483,6 +483,7 @@ dpll_pin_get(u64 clock_id, u32 pin_idx, struct module *module,
                    pos->pin_idx == pin_idx &&
                    pos->module == module) {
                        ret = pos;
+                       pos->prop = prop;
                        refcount_inc(&ret->refcount);
                        break;
                }

would replace whole of this patch changes, although seems a bit hacky.

Thank you!
Arkadiusz

>
>>
>>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);
>>>>@@ -634,7 +634,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 93fc6c4b8a78..963bbbbe6660 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;
>>>> }
>>>>@@ -403,7 +403,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;
>>>>
>>>>@@ -696,7 +696,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;
>>>> 	}
>>>>@@ -732,7 +732,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;
>>>> 	}
>>>>@@ -759,7 +759,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;
>>>> 	}
>>>>@@ -787,7 +787,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;
>>>> 	}
>>>>@@ -817,8 +817,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;
>>>>@@ -999,7 +999,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
>>>>
>>

Jiri Pirko Nov. 9, 2023, 6:06 p.m. UTC | #5

Thu, Nov 09, 2023 at 05:30:20PM CET, arkadiusz.kubalewski@intel.com wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Sent: Thursday, November 9, 2023 2:19 PM
>>
>>Thu, Nov 09, 2023 at 01:20:48PM CET, arkadiusz.kubalewski@intel.com wrote:
>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>Sent: Wednesday, November 8, 2023 3:30 PM
>>>>
>>>>Wed, Nov 08, 2023 at 11:32:25AM 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
>>>>>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")
>>>>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>>>>---
>>>>> drivers/dpll/dpll_core.c    |  4 ++--
>>>>> drivers/dpll/dpll_core.h    |  4 ++--
>>>>> drivers/dpll/dpll_netlink.c | 28 ++++++++++++++--------------
>>>>> 3 files changed, 18 insertions(+), 18 deletions(-)
>>>>>
>>>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>>>index 3568149b9562..4077b562ba3b 100644
>>>>>--- a/drivers/dpll/dpll_core.c
>>>>>+++ b/drivers/dpll/dpll_core.c
>>>>>@@ -442,7 +442,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct
>>>>>module *module,
>>>>> 		ret = -EINVAL;
>>>>> 		goto err;
>>>>> 	}
>>>>>-	pin->prop = prop;
>>>>>+	memcpy(&pin->prop, prop, sizeof(pin->prop));
>>>>
>>>>Odd, you don't care about the pointer within this structure?
>>>>
>>>
>>>Well, true. Need a fix.
>>>Wondering if copying idea is better than just assigning prop pointer on
>>>each call to dpll_pin_get(..) function (when pin already exists)?
>>
>>Not sure what do you mean. Examples please.
>>
>
>Sure,
>
>Basically this change:
>
>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>index ae884b92d68c..06b72d5877c3 100644
>--- a/drivers/dpll/dpll_core.c
>+++ b/drivers/dpll/dpll_core.c
>@@ -483,6 +483,7 @@ dpll_pin_get(u64 clock_id, u32 pin_idx, struct module *module,
>                    pos->pin_idx == pin_idx &&
>                    pos->module == module) {
>                        ret = pos;
>+                       pos->prop = prop;
>                        refcount_inc(&ret->refcount);
>                        break;
>                }
>
>would replace whole of this patch changes, although seems a bit hacky.

Or event better, as I suggested in the other patch reply, resolve this
internally in the driver registering things only when they are valid.
Much better then to hack anything in dpll core.


>
>Thank you!
>Arkadiusz
>
>>
>>>
>>>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);
>>>>>@@ -634,7 +634,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 93fc6c4b8a78..963bbbbe6660 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;
>>>>> }
>>>>>@@ -403,7 +403,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;
>>>>>
>>>>>@@ -696,7 +696,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;
>>>>> 	}
>>>>>@@ -732,7 +732,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;
>>>>> 	}
>>>>>@@ -759,7 +759,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;
>>>>> 	}
>>>>>@@ -787,7 +787,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;
>>>>> 	}
>>>>>@@ -817,8 +817,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;
>>>>>@@ -999,7 +999,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
>>>>>
>>>

Kubalewski, Arkadiusz Nov. 9, 2023, 11:32 p.m. UTC | #6

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Thursday, November 9, 2023 7:06 PM
>
>Thu, Nov 09, 2023 at 05:30:20PM CET, arkadiusz.kubalewski@intel.com wrote:
>>>From: Jiri Pirko <jiri@resnulli.us>
>>>Sent: Thursday, November 9, 2023 2:19 PM
>>>
>>>Thu, Nov 09, 2023 at 01:20:48PM CET, arkadiusz.kubalewski@intel.com
>>>wrote:
>>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>>Sent: Wednesday, November 8, 2023 3:30 PM
>>>>>
>>>>>Wed, Nov 08, 2023 at 11:32:25AM 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
>>>>>>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")
>>>>>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>>>>>---
>>>>>> drivers/dpll/dpll_core.c    |  4 ++--
>>>>>> drivers/dpll/dpll_core.h    |  4 ++--
>>>>>> drivers/dpll/dpll_netlink.c | 28 ++++++++++++++--------------
>>>>>> 3 files changed, 18 insertions(+), 18 deletions(-)
>>>>>>
>>>>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>>>>index 3568149b9562..4077b562ba3b 100644
>>>>>>--- a/drivers/dpll/dpll_core.c
>>>>>>+++ b/drivers/dpll/dpll_core.c
>>>>>>@@ -442,7 +442,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct
>>>>>>module *module,
>>>>>> 		ret = -EINVAL;
>>>>>> 		goto err;
>>>>>> 	}
>>>>>>-	pin->prop = prop;
>>>>>>+	memcpy(&pin->prop, prop, sizeof(pin->prop));
>>>>>
>>>>>Odd, you don't care about the pointer within this structure?
>>>>>
>>>>
>>>>Well, true. Need a fix.
>>>>Wondering if copying idea is better than just assigning prop pointer on
>>>>each call to dpll_pin_get(..) function (when pin already exists)?
>>>
>>>Not sure what do you mean. Examples please.
>>>
>>
>>Sure,
>>
>>Basically this change:
>>
>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>index ae884b92d68c..06b72d5877c3 100644
>>--- a/drivers/dpll/dpll_core.c
>>+++ b/drivers/dpll/dpll_core.c
>>@@ -483,6 +483,7 @@ dpll_pin_get(u64 clock_id, u32 pin_idx, struct module
>>*module,
>>                    pos->pin_idx == pin_idx &&
>>                    pos->module == module) {
>>                        ret = pos;
>>+                       pos->prop = prop;
>>                        refcount_inc(&ret->refcount);
>>                        break;
>>                }
>>
>>would replace whole of this patch changes, although seems a bit hacky.
>
>Or event better, as I suggested in the other patch reply, resolve this
>internally in the driver registering things only when they are valid.
>Much better then to hack anything in dpll core.
>

This approach seemed to me hacky, that is why started with coping the
data.
It is not about registering, rather about unregistering on driver
unbind, which brakes things, and currently cannot be recovered in
described case.

Thank you!
Arkadiusz

>
>>
>>Thank you!
>>Arkadiusz
>>
>>>
>>>>
>>>>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);
>>>>>>@@ -634,7 +634,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 93fc6c4b8a78..963bbbbe6660 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;
>>>>>> }
>>>>>>@@ -403,7 +403,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;
>>>>>>
>>>>>>@@ -696,7 +696,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;
>>>>>> 	}
>>>>>>@@ -732,7 +732,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;
>>>>>> 	}
>>>>>>@@ -759,7 +759,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;
>>>>>> 	}
>>>>>>@@ -787,7 +787,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;
>>>>>> 	}
>>>>>>@@ -817,8 +817,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;
>>>>>>@@ -999,7 +999,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
>>>>>>
>>>>

Jiri Pirko Nov. 10, 2023, 6:45 a.m. UTC | #7

Fri, Nov 10, 2023 at 12:32:21AM CET, arkadiusz.kubalewski@intel.com wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Sent: Thursday, November 9, 2023 7:06 PM
>>
>>Thu, Nov 09, 2023 at 05:30:20PM CET, arkadiusz.kubalewski@intel.com wrote:
>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>Sent: Thursday, November 9, 2023 2:19 PM
>>>>
>>>>Thu, Nov 09, 2023 at 01:20:48PM CET, arkadiusz.kubalewski@intel.com
>>>>wrote:
>>>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>>>Sent: Wednesday, November 8, 2023 3:30 PM
>>>>>>
>>>>>>Wed, Nov 08, 2023 at 11:32:25AM 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
>>>>>>>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")
>>>>>>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>>>>>>---
>>>>>>> drivers/dpll/dpll_core.c    |  4 ++--
>>>>>>> drivers/dpll/dpll_core.h    |  4 ++--
>>>>>>> drivers/dpll/dpll_netlink.c | 28 ++++++++++++++--------------
>>>>>>> 3 files changed, 18 insertions(+), 18 deletions(-)
>>>>>>>
>>>>>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>>>>>index 3568149b9562..4077b562ba3b 100644
>>>>>>>--- a/drivers/dpll/dpll_core.c
>>>>>>>+++ b/drivers/dpll/dpll_core.c
>>>>>>>@@ -442,7 +442,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct
>>>>>>>module *module,
>>>>>>> 		ret = -EINVAL;
>>>>>>> 		goto err;
>>>>>>> 	}
>>>>>>>-	pin->prop = prop;
>>>>>>>+	memcpy(&pin->prop, prop, sizeof(pin->prop));
>>>>>>
>>>>>>Odd, you don't care about the pointer within this structure?
>>>>>>
>>>>>
>>>>>Well, true. Need a fix.
>>>>>Wondering if copying idea is better than just assigning prop pointer on
>>>>>each call to dpll_pin_get(..) function (when pin already exists)?
>>>>
>>>>Not sure what do you mean. Examples please.
>>>>
>>>
>>>Sure,
>>>
>>>Basically this change:
>>>
>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>index ae884b92d68c..06b72d5877c3 100644
>>>--- a/drivers/dpll/dpll_core.c
>>>+++ b/drivers/dpll/dpll_core.c
>>>@@ -483,6 +483,7 @@ dpll_pin_get(u64 clock_id, u32 pin_idx, struct module
>>>*module,
>>>                    pos->pin_idx == pin_idx &&
>>>                    pos->module == module) {
>>>                        ret = pos;
>>>+                       pos->prop = prop;
>>>                        refcount_inc(&ret->refcount);
>>>                        break;
>>>                }
>>>
>>>would replace whole of this patch changes, although seems a bit hacky.
>>
>>Or event better, as I suggested in the other patch reply, resolve this
>>internally in the driver registering things only when they are valid.
>>Much better then to hack anything in dpll core.
>>
>
>This approach seemed to me hacky, that is why started with coping the
>data.
>It is not about registering, rather about unregistering on driver
>unbind, which brakes things, and currently cannot be recovered in
>described case.

Sure it can. PF0 unbind-> internal notification-> PF1 unregisters all
related object. Very clean and simple.


>
>Thank you!
>Arkadiusz
>
>>
>>>
>>>Thank you!
>>>Arkadiusz
>>>
>>>>
>>>>>
>>>>>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);
>>>>>>>@@ -634,7 +634,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 93fc6c4b8a78..963bbbbe6660 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;
>>>>>>> }
>>>>>>>@@ -403,7 +403,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;
>>>>>>>
>>>>>>>@@ -696,7 +696,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;
>>>>>>> 	}
>>>>>>>@@ -732,7 +732,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;
>>>>>>> 	}
>>>>>>>@@ -759,7 +759,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;
>>>>>>> 	}
>>>>>>>@@ -787,7 +787,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;
>>>>>>> 	}
>>>>>>>@@ -817,8 +817,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;
>>>>>>>@@ -999,7 +999,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
>>>>>>>
>>>>>
>

Kubalewski, Arkadiusz Nov. 10, 2023, 9:01 a.m. UTC | #8

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Friday, November 10, 2023 7:46 AM
>
>Fri, Nov 10, 2023 at 12:32:21AM CET, arkadiusz.kubalewski@intel.com wrote:
>>>From: Jiri Pirko <jiri@resnulli.us>
>>>Sent: Thursday, November 9, 2023 7:06 PM
>>>
>>>Thu, Nov 09, 2023 at 05:30:20PM CET, arkadiusz.kubalewski@intel.com
>>>wrote:
>>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>>Sent: Thursday, November 9, 2023 2:19 PM
>>>>>
>>>>>Thu, Nov 09, 2023 at 01:20:48PM CET, arkadiusz.kubalewski@intel.com
>>>>>wrote:
>>>>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>>>>Sent: Wednesday, November 8, 2023 3:30 PM
>>>>>>>
>>>>>>>Wed, Nov 08, 2023 at 11:32:25AM 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
>>>>>>>>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")
>>>>>>>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>>>>>>>---
>>>>>>>> drivers/dpll/dpll_core.c    |  4 ++--
>>>>>>>> drivers/dpll/dpll_core.h    |  4 ++--
>>>>>>>> drivers/dpll/dpll_netlink.c | 28 ++++++++++++++--------------
>>>>>>>> 3 files changed, 18 insertions(+), 18 deletions(-)
>>>>>>>>
>>>>>>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>>>>>>index 3568149b9562..4077b562ba3b 100644
>>>>>>>>--- a/drivers/dpll/dpll_core.c
>>>>>>>>+++ b/drivers/dpll/dpll_core.c
>>>>>>>>@@ -442,7 +442,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct
>>>>>>>>module *module,
>>>>>>>> 		ret = -EINVAL;
>>>>>>>> 		goto err;
>>>>>>>> 	}
>>>>>>>>-	pin->prop = prop;
>>>>>>>>+	memcpy(&pin->prop, prop, sizeof(pin->prop));
>>>>>>>
>>>>>>>Odd, you don't care about the pointer within this structure?
>>>>>>>
>>>>>>
>>>>>>Well, true. Need a fix.
>>>>>>Wondering if copying idea is better than just assigning prop pointer
>>>>>>on
>>>>>>each call to dpll_pin_get(..) function (when pin already exists)?
>>>>>
>>>>>Not sure what do you mean. Examples please.
>>>>>
>>>>
>>>>Sure,
>>>>
>>>>Basically this change:
>>>>
>>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>>index ae884b92d68c..06b72d5877c3 100644
>>>>--- a/drivers/dpll/dpll_core.c
>>>>+++ b/drivers/dpll/dpll_core.c
>>>>@@ -483,6 +483,7 @@ dpll_pin_get(u64 clock_id, u32 pin_idx, struct
>>>>module
>>>>*module,
>>>>                    pos->pin_idx == pin_idx &&
>>>>                    pos->module == module) {
>>>>                        ret = pos;
>>>>+                       pos->prop = prop;
>>>>                        refcount_inc(&ret->refcount);
>>>>                        break;
>>>>                }
>>>>
>>>>would replace whole of this patch changes, although seems a bit hacky.
>>>
>>>Or event better, as I suggested in the other patch reply, resolve this
>>>internally in the driver registering things only when they are valid.
>>>Much better then to hack anything in dpll core.
>>>
>>
>>This approach seemed to me hacky, that is why started with coping the
>>data.
>>It is not about registering, rather about unregistering on driver
>>unbind, which brakes things, and currently cannot be recovered in
>>described case.
>
>Sure it can. PF0 unbind-> internal notification-> PF1 unregisters all
>related object. Very clean and simple.
>

What you are suggesting is:
- special purpose bus in the driver,
- dpll-related,
- not needed,
- prone for errors.

The dpll subsystem is here to make driver life easier.

Thank you!
Arkadiusz

>
>>
>>Thank you!
>>Arkadiusz
>>
>>>
>>>>
>>>>Thank you!
>>>>Arkadiusz
>>>>
>>>>>
>>>>>>
>>>>>>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);
>>>>>>>>@@ -634,7 +634,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 93fc6c4b8a78..963bbbbe6660 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;
>>>>>>>> }
>>>>>>>>@@ -403,7 +403,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;
>>>>>>>>
>>>>>>>>@@ -696,7 +696,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;
>>>>>>>> 	}
>>>>>>>>@@ -732,7 +732,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;
>>>>>>>> 	}
>>>>>>>>@@ -759,7 +759,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;
>>>>>>>> 	}
>>>>>>>>@@ -787,7 +787,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;
>>>>>>>> 	}
>>>>>>>>@@ -817,8 +817,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;
>>>>>>>>@@ -999,7 +999,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
>>>>>>>>
>>>>>>
>>

Jiri Pirko Nov. 10, 2023, 10:06 a.m. UTC | #9

Fri, Nov 10, 2023 at 10:01:50AM CET, arkadiusz.kubalewski@intel.com wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Sent: Friday, November 10, 2023 7:46 AM
>>
>>Fri, Nov 10, 2023 at 12:32:21AM CET, arkadiusz.kubalewski@intel.com wrote:
>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>Sent: Thursday, November 9, 2023 7:06 PM
>>>>
>>>>Thu, Nov 09, 2023 at 05:30:20PM CET, arkadiusz.kubalewski@intel.com
>>>>wrote:
>>>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>>>Sent: Thursday, November 9, 2023 2:19 PM
>>>>>>
>>>>>>Thu, Nov 09, 2023 at 01:20:48PM CET, arkadiusz.kubalewski@intel.com
>>>>>>wrote:
>>>>>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>>>>>Sent: Wednesday, November 8, 2023 3:30 PM
>>>>>>>>
>>>>>>>>Wed, Nov 08, 2023 at 11:32:25AM 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
>>>>>>>>>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")
>>>>>>>>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>>>>>>>>---
>>>>>>>>> drivers/dpll/dpll_core.c    |  4 ++--
>>>>>>>>> drivers/dpll/dpll_core.h    |  4 ++--
>>>>>>>>> drivers/dpll/dpll_netlink.c | 28 ++++++++++++++--------------
>>>>>>>>> 3 files changed, 18 insertions(+), 18 deletions(-)
>>>>>>>>>
>>>>>>>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>>>>>>>index 3568149b9562..4077b562ba3b 100644
>>>>>>>>>--- a/drivers/dpll/dpll_core.c
>>>>>>>>>+++ b/drivers/dpll/dpll_core.c
>>>>>>>>>@@ -442,7 +442,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct
>>>>>>>>>module *module,
>>>>>>>>> 		ret = -EINVAL;
>>>>>>>>> 		goto err;
>>>>>>>>> 	}
>>>>>>>>>-	pin->prop = prop;
>>>>>>>>>+	memcpy(&pin->prop, prop, sizeof(pin->prop));
>>>>>>>>
>>>>>>>>Odd, you don't care about the pointer within this structure?
>>>>>>>>
>>>>>>>
>>>>>>>Well, true. Need a fix.
>>>>>>>Wondering if copying idea is better than just assigning prop pointer
>>>>>>>on
>>>>>>>each call to dpll_pin_get(..) function (when pin already exists)?
>>>>>>
>>>>>>Not sure what do you mean. Examples please.
>>>>>>
>>>>>
>>>>>Sure,
>>>>>
>>>>>Basically this change:
>>>>>
>>>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>>>index ae884b92d68c..06b72d5877c3 100644
>>>>>--- a/drivers/dpll/dpll_core.c
>>>>>+++ b/drivers/dpll/dpll_core.c
>>>>>@@ -483,6 +483,7 @@ dpll_pin_get(u64 clock_id, u32 pin_idx, struct
>>>>>module
>>>>>*module,
>>>>>                    pos->pin_idx == pin_idx &&
>>>>>                    pos->module == module) {
>>>>>                        ret = pos;
>>>>>+                       pos->prop = prop;
>>>>>                        refcount_inc(&ret->refcount);
>>>>>                        break;
>>>>>                }
>>>>>
>>>>>would replace whole of this patch changes, although seems a bit hacky.
>>>>
>>>>Or event better, as I suggested in the other patch reply, resolve this
>>>>internally in the driver registering things only when they are valid.
>>>>Much better then to hack anything in dpll core.
>>>>
>>>
>>>This approach seemed to me hacky, that is why started with coping the
>>>data.
>>>It is not about registering, rather about unregistering on driver
>>>unbind, which brakes things, and currently cannot be recovered in
>>>described case.
>>
>>Sure it can. PF0 unbind-> internal notification-> PF1 unregisters all
>>related object. Very clean and simple.
>>
>
>What you are suggesting is:
>- special purpose bus in the driver,

No, it is a simple notificator. Very common infra over the whole
kernel code.


>- dpll-related,

Is this the only thing that PF0 is special with? Perhaps you can
utilize this for other features as well, since your fw design is like
this.


>- not needed,
>- prone for errors.
>
>The dpll subsystem is here to make driver life easier.

No, the subsystem is never here to handle device specific issues. And
your PF0 dependency is very clearly something device specific. Don't
pollute the dpll subsystem with workaround to handle specific device
needs. create/register the dplls objects from your driver only when it
is valid to do so. Make sure the lifetime of such object stays in
the scope of validity. Handle that in the driver. Very clear and simple.

Thanks!


>
>Thank you!
>Arkadiusz
>
>>
>>>
>>>Thank you!
>>>Arkadiusz
>>>
>>>>
>>>>>
>>>>>Thank you!
>>>>>Arkadiusz
>>>>>
>>>>>>
>>>>>>>
>>>>>>>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);
>>>>>>>>>@@ -634,7 +634,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 93fc6c4b8a78..963bbbbe6660 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;
>>>>>>>>> }
>>>>>>>>>@@ -403,7 +403,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;
>>>>>>>>>
>>>>>>>>>@@ -696,7 +696,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;
>>>>>>>>> 	}
>>>>>>>>>@@ -732,7 +732,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;
>>>>>>>>> 	}
>>>>>>>>>@@ -759,7 +759,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;
>>>>>>>>> 	}
>>>>>>>>>@@ -787,7 +787,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;
>>>>>>>>> 	}
>>>>>>>>>@@ -817,8 +817,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;
>>>>>>>>>@@ -999,7 +999,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
>>>>>>>>>
>>>>>>>
>>>

Kubalewski, Arkadiusz Nov. 10, 2023, 11:18 a.m. UTC | #10

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Friday, November 10, 2023 11:06 AM
>
>Fri, Nov 10, 2023 at 10:01:50AM CET, arkadiusz.kubalewski@intel.com wrote:
>>>From: Jiri Pirko <jiri@resnulli.us>
>>>Sent: Friday, November 10, 2023 7:46 AM
>>>
>>>Fri, Nov 10, 2023 at 12:32:21AM CET, arkadiusz.kubalewski@intel.com
>>>wrote:
>>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>>Sent: Thursday, November 9, 2023 7:06 PM
>>>>>
>>>>>Thu, Nov 09, 2023 at 05:30:20PM CET, arkadiusz.kubalewski@intel.com
>>>>>wrote:
>>>>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>>>>Sent: Thursday, November 9, 2023 2:19 PM
>>>>>>>
>>>>>>>Thu, Nov 09, 2023 at 01:20:48PM CET, arkadiusz.kubalewski@intel.com
>>>>>>>wrote:
>>>>>>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>>>>>>Sent: Wednesday, November 8, 2023 3:30 PM
>>>>>>>>>
>>>>>>>>>Wed, Nov 08, 2023 at 11:32:25AM 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
>>>>>>>>>>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")
>>>>>>>>>>Signed-off-by: Arkadiusz Kubalewski
>>>>>>>>>><arkadiusz.kubalewski@intel.com>
>>>>>>>>>>---
>>>>>>>>>> drivers/dpll/dpll_core.c    |  4 ++--
>>>>>>>>>> drivers/dpll/dpll_core.h    |  4 ++--
>>>>>>>>>> drivers/dpll/dpll_netlink.c | 28 ++++++++++++++--------------
>>>>>>>>>> 3 files changed, 18 insertions(+), 18 deletions(-)
>>>>>>>>>>
>>>>>>>>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>>>>>>>>index 3568149b9562..4077b562ba3b 100644
>>>>>>>>>>--- a/drivers/dpll/dpll_core.c
>>>>>>>>>>+++ b/drivers/dpll/dpll_core.c
>>>>>>>>>>@@ -442,7 +442,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx,
>>>>>>>>>>struct
>>>>>>>>>>module *module,
>>>>>>>>>> 		ret = -EINVAL;
>>>>>>>>>> 		goto err;
>>>>>>>>>> 	}
>>>>>>>>>>-	pin->prop = prop;
>>>>>>>>>>+	memcpy(&pin->prop, prop, sizeof(pin->prop));
>>>>>>>>>
>>>>>>>>>Odd, you don't care about the pointer within this structure?
>>>>>>>>>
>>>>>>>>
>>>>>>>>Well, true. Need a fix.
>>>>>>>>Wondering if copying idea is better than just assigning prop pointer
>>>>>>>>on
>>>>>>>>each call to dpll_pin_get(..) function (when pin already exists)?
>>>>>>>
>>>>>>>Not sure what do you mean. Examples please.
>>>>>>>
>>>>>>
>>>>>>Sure,
>>>>>>
>>>>>>Basically this change:
>>>>>>
>>>>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>>>>index ae884b92d68c..06b72d5877c3 100644
>>>>>>--- a/drivers/dpll/dpll_core.c
>>>>>>+++ b/drivers/dpll/dpll_core.c
>>>>>>@@ -483,6 +483,7 @@ dpll_pin_get(u64 clock_id, u32 pin_idx, struct
>>>>>>module
>>>>>>*module,
>>>>>>                    pos->pin_idx == pin_idx &&
>>>>>>                    pos->module == module) {
>>>>>>                        ret = pos;
>>>>>>+                       pos->prop = prop;
>>>>>>                        refcount_inc(&ret->refcount);
>>>>>>                        break;
>>>>>>                }
>>>>>>
>>>>>>would replace whole of this patch changes, although seems a bit hacky.
>>>>>
>>>>>Or event better, as I suggested in the other patch reply, resolve this
>>>>>internally in the driver registering things only when they are valid.
>>>>>Much better then to hack anything in dpll core.
>>>>>
>>>>
>>>>This approach seemed to me hacky, that is why started with coping the
>>>>data.
>>>>It is not about registering, rather about unregistering on driver
>>>>unbind, which brakes things, and currently cannot be recovered in
>>>>described case.
>>>
>>>Sure it can. PF0 unbind-> internal notification-> PF1 unregisters all
>>>related object. Very clean and simple.
>>>
>>
>>What you are suggesting is:
>>- special purpose bus in the driver,
>
>No, it is a simple notificator. Very common infra over the whole
>kernel code.
>

No difference if this is simple or not, it is special purpose.

>
>>- dpll-related,
>
>Is this the only thing that PF0 is special with? Perhaps you can
>utilize this for other features as well, since your fw design is like
>this.
>

None api user is allowed to the unordered driver bind when there are muxed
pins involved.
This requires a fix in the api not in the driver.

>
>>- not needed,
>>- prone for errors.
>>
>>The dpll subsystem is here to make driver life easier.
>
>No, the subsystem is never here to handle device specific issues. And
>your PF0 dependency is very clearly something device specific. Don't
>pollute the dpll subsystem with workaround to handle specific device
>needs. create/register the dplls objects from your driver only when it
>is valid to do so. Make sure the lifetime of such object stays in
>the scope of validity. Handle that in the driver. Very clear and simple.
>

In our case this is PF0 but this is broader issue. The muxed pins
infrastructure in now slightly broken and I am fixing it.
From the beginning it was designed to allow separated driver instances
to create a device and connect their pins with it.
Any driver which would use it would face this issue. What you are trying
to imply is that it is better to put traffic lights on each car instead
of putting them on crossroads.

>Thanks!
>

Thank you!
Arkadiusz

>
>>
>>Thank you!
>>Arkadiusz
>>
>>>
>>>>
>>>>Thank you!
>>>>Arkadiusz
>>>>
>>>>>
>>>>>>
>>>>>>Thank you!
>>>>>>Arkadiusz
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>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);
>>>>>>>>>>@@ -634,7 +634,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 93fc6c4b8a78..963bbbbe6660 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;
>>>>>>>>>> }
>>>>>>>>>>@@ -403,7 +403,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;
>>>>>>>>>>
>>>>>>>>>>@@ -696,7 +696,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;
>>>>>>>>>> 	}
>>>>>>>>>>@@ -732,7 +732,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;
>>>>>>>>>> 	}
>>>>>>>>>>@@ -759,7 +759,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;
>>>>>>>>>> 	}
>>>>>>>>>>@@ -787,7 +787,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;
>>>>>>>>>> 	}
>>>>>>>>>>@@ -817,8 +817,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;
>>>>>>>>>>@@ -999,7 +999,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
>>>>>>>>>>
>>>>>>>>
>>>>

Jiri Pirko Nov. 10, 2023, 11:44 a.m. UTC | #11

Fri, Nov 10, 2023 at 12:18:51PM CET, arkadiusz.kubalewski@intel.com wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Sent: Friday, November 10, 2023 11:06 AM
>>
>>Fri, Nov 10, 2023 at 10:01:50AM CET, arkadiusz.kubalewski@intel.com wrote:
>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>Sent: Friday, November 10, 2023 7:46 AM
>>>>
>>>>Fri, Nov 10, 2023 at 12:32:21AM CET, arkadiusz.kubalewski@intel.com
>>>>wrote:
>>>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>>>Sent: Thursday, November 9, 2023 7:06 PM
>>>>>>
>>>>>>Thu, Nov 09, 2023 at 05:30:20PM CET, arkadiusz.kubalewski@intel.com
>>>>>>wrote:
>>>>>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>>>>>Sent: Thursday, November 9, 2023 2:19 PM
>>>>>>>>
>>>>>>>>Thu, Nov 09, 2023 at 01:20:48PM CET, arkadiusz.kubalewski@intel.com
>>>>>>>>wrote:
>>>>>>>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>>>>>>>Sent: Wednesday, November 8, 2023 3:30 PM
>>>>>>>>>>
>>>>>>>>>>Wed, Nov 08, 2023 at 11:32:25AM 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
>>>>>>>>>>>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")
>>>>>>>>>>>Signed-off-by: Arkadiusz Kubalewski
>>>>>>>>>>><arkadiusz.kubalewski@intel.com>
>>>>>>>>>>>---
>>>>>>>>>>> drivers/dpll/dpll_core.c    |  4 ++--
>>>>>>>>>>> drivers/dpll/dpll_core.h    |  4 ++--
>>>>>>>>>>> drivers/dpll/dpll_netlink.c | 28 ++++++++++++++--------------
>>>>>>>>>>> 3 files changed, 18 insertions(+), 18 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>>>>>>>>>index 3568149b9562..4077b562ba3b 100644
>>>>>>>>>>>--- a/drivers/dpll/dpll_core.c
>>>>>>>>>>>+++ b/drivers/dpll/dpll_core.c
>>>>>>>>>>>@@ -442,7 +442,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx,
>>>>>>>>>>>struct
>>>>>>>>>>>module *module,
>>>>>>>>>>> 		ret = -EINVAL;
>>>>>>>>>>> 		goto err;
>>>>>>>>>>> 	}
>>>>>>>>>>>-	pin->prop = prop;
>>>>>>>>>>>+	memcpy(&pin->prop, prop, sizeof(pin->prop));
>>>>>>>>>>
>>>>>>>>>>Odd, you don't care about the pointer within this structure?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>Well, true. Need a fix.
>>>>>>>>>Wondering if copying idea is better than just assigning prop pointer
>>>>>>>>>on
>>>>>>>>>each call to dpll_pin_get(..) function (when pin already exists)?
>>>>>>>>
>>>>>>>>Not sure what do you mean. Examples please.
>>>>>>>>
>>>>>>>
>>>>>>>Sure,
>>>>>>>
>>>>>>>Basically this change:
>>>>>>>
>>>>>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>>>>>index ae884b92d68c..06b72d5877c3 100644
>>>>>>>--- a/drivers/dpll/dpll_core.c
>>>>>>>+++ b/drivers/dpll/dpll_core.c
>>>>>>>@@ -483,6 +483,7 @@ dpll_pin_get(u64 clock_id, u32 pin_idx, struct
>>>>>>>module
>>>>>>>*module,
>>>>>>>                    pos->pin_idx == pin_idx &&
>>>>>>>                    pos->module == module) {
>>>>>>>                        ret = pos;
>>>>>>>+                       pos->prop = prop;
>>>>>>>                        refcount_inc(&ret->refcount);
>>>>>>>                        break;
>>>>>>>                }
>>>>>>>
>>>>>>>would replace whole of this patch changes, although seems a bit hacky.
>>>>>>
>>>>>>Or event better, as I suggested in the other patch reply, resolve this
>>>>>>internally in the driver registering things only when they are valid.
>>>>>>Much better then to hack anything in dpll core.
>>>>>>
>>>>>
>>>>>This approach seemed to me hacky, that is why started with coping the
>>>>>data.
>>>>>It is not about registering, rather about unregistering on driver
>>>>>unbind, which brakes things, and currently cannot be recovered in
>>>>>described case.
>>>>
>>>>Sure it can. PF0 unbind-> internal notification-> PF1 unregisters all
>>>>related object. Very clean and simple.
>>>>
>>>
>>>What you are suggesting is:
>>>- special purpose bus in the driver,
>>
>>No, it is a simple notificator. Very common infra over the whole
>>kernel code.
>>
>
>No difference if this is simple or not, it is special purpose.
>
>>
>>>- dpll-related,
>>
>>Is this the only thing that PF0 is special with? Perhaps you can
>>utilize this for other features as well, since your fw design is like
>>this.
>>
>
>None api user is allowed to the unordered driver bind when there are muxed
>pins involved.
>This requires a fix in the api not in the driver.
>
>>
>>>- not needed,
>>>- prone for errors.
>>>
>>>The dpll subsystem is here to make driver life easier.
>>
>>No, the subsystem is never here to handle device specific issues. And
>>your PF0 dependency is very clearly something device specific. Don't
>>pollute the dpll subsystem with workaround to handle specific device
>>needs. create/register the dplls objects from your driver only when it
>>is valid to do so. Make sure the lifetime of such object stays in
>>the scope of validity. Handle that in the driver. Very clear and simple.
>>
>
>In our case this is PF0 but this is broader issue. The muxed pins
>infrastructure in now slightly broken and I am fixing it.
>From the beginning it was designed to allow separated driver instances
>to create a device and connect their pins with it.

That is true. That's exacly what we have implemented in mlx5. Each
instance registers dpll device and the pin related to the instance. No
problem.

The fact that you do register only in PF0 is a limitation in your
driver. Fix it there.


>Any driver which would use it would face this issue. What you are trying
>to imply is that it is better to put traffic lights on each car instead
>of putting them on crossroads.
>
>>Thanks!
>>
>
>Thank you!
>Arkadiusz
>
>>
>>>
>>>Thank you!
>>>Arkadiusz
>>>
>>>>
>>>>>
>>>>>Thank you!
>>>>>Arkadiusz
>>>>>
>>>>>>
>>>>>>>
>>>>>>>Thank you!
>>>>>>>Arkadiusz
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>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);
>>>>>>>>>>>@@ -634,7 +634,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 93fc6c4b8a78..963bbbbe6660 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;
>>>>>>>>>>> }
>>>>>>>>>>>@@ -403,7 +403,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;
>>>>>>>>>>>
>>>>>>>>>>>@@ -696,7 +696,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;
>>>>>>>>>>> 	}
>>>>>>>>>>>@@ -732,7 +732,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;
>>>>>>>>>>> 	}
>>>>>>>>>>>@@ -759,7 +759,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;
>>>>>>>>>>> 	}
>>>>>>>>>>>@@ -787,7 +787,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;
>>>>>>>>>>> 	}
>>>>>>>>>>>@@ -817,8 +817,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;
>>>>>>>>>>>@@ -999,7 +999,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
>>>>>>>>>>>
>>>>>>>>>
>>>>>

Kubalewski, Arkadiusz Nov. 10, 2023, 2:11 p.m. UTC | #12

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Friday, November 10, 2023 12:45 PM
>
>Fri, Nov 10, 2023 at 12:18:51PM CET, arkadiusz.kubalewski@intel.com wrote:
>>>From: Jiri Pirko <jiri@resnulli.us>
>>>Sent: Friday, November 10, 2023 11:06 AM
>>>
>>>Fri, Nov 10, 2023 at 10:01:50AM CET, arkadiusz.kubalewski@intel.com
>>>wrote:
>>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>>Sent: Friday, November 10, 2023 7:46 AM
>>>>>
>>>>>Fri, Nov 10, 2023 at 12:32:21AM CET, arkadiusz.kubalewski@intel.com
>>>>>wrote:
>>>>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>>>>Sent: Thursday, November 9, 2023 7:06 PM
>>>>>>>
>>>>>>>Thu, Nov 09, 2023 at 05:30:20PM CET, arkadiusz.kubalewski@intel.com
>>>>>>>wrote:
>>>>>>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>>>>>>Sent: Thursday, November 9, 2023 2:19 PM
>>>>>>>>>
>>>>>>>>>Thu, Nov 09, 2023 at 01:20:48PM CET, arkadiusz.kubalewski@intel.com
>>>>>>>>>wrote:
>>>>>>>>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>>>>>>>>Sent: Wednesday, November 8, 2023 3:30 PM
>>>>>>>>>>>
>>>>>>>>>>>Wed, Nov 08, 2023 at 11:32:25AM 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
>>>>>>>>>>>>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")
>>>>>>>>>>>>Signed-off-by: Arkadiusz Kubalewski
>>>>>>>>>>>><arkadiusz.kubalewski@intel.com>
>>>>>>>>>>>>---
>>>>>>>>>>>> drivers/dpll/dpll_core.c    |  4 ++--
>>>>>>>>>>>> drivers/dpll/dpll_core.h    |  4 ++--
>>>>>>>>>>>> drivers/dpll/dpll_netlink.c | 28 ++++++++++++++--------------
>>>>>>>>>>>> 3 files changed, 18 insertions(+), 18 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>>>>>>>>>>index 3568149b9562..4077b562ba3b 100644
>>>>>>>>>>>>--- a/drivers/dpll/dpll_core.c
>>>>>>>>>>>>+++ b/drivers/dpll/dpll_core.c
>>>>>>>>>>>>@@ -442,7 +442,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx,
>>>>>>>>>>>>struct
>>>>>>>>>>>>module *module,
>>>>>>>>>>>> 		ret = -EINVAL;
>>>>>>>>>>>> 		goto err;
>>>>>>>>>>>> 	}
>>>>>>>>>>>>-	pin->prop = prop;
>>>>>>>>>>>>+	memcpy(&pin->prop, prop, sizeof(pin->prop));
>>>>>>>>>>>
>>>>>>>>>>>Odd, you don't care about the pointer within this structure?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>Well, true. Need a fix.
>>>>>>>>>>Wondering if copying idea is better than just assigning prop
>>>>>>>>>>pointer
>>>>>>>>>>on
>>>>>>>>>>each call to dpll_pin_get(..) function (when pin already exists)?
>>>>>>>>>
>>>>>>>>>Not sure what do you mean. Examples please.
>>>>>>>>>
>>>>>>>>
>>>>>>>>Sure,
>>>>>>>>
>>>>>>>>Basically this change:
>>>>>>>>
>>>>>>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>>>>>>index ae884b92d68c..06b72d5877c3 100644
>>>>>>>>--- a/drivers/dpll/dpll_core.c
>>>>>>>>+++ b/drivers/dpll/dpll_core.c
>>>>>>>>@@ -483,6 +483,7 @@ dpll_pin_get(u64 clock_id, u32 pin_idx, struct
>>>>>>>>module
>>>>>>>>*module,
>>>>>>>>                    pos->pin_idx == pin_idx &&
>>>>>>>>                    pos->module == module) {
>>>>>>>>                        ret = pos;
>>>>>>>>+                       pos->prop = prop;
>>>>>>>>                        refcount_inc(&ret->refcount);
>>>>>>>>                        break;
>>>>>>>>                }
>>>>>>>>
>>>>>>>>would replace whole of this patch changes, although seems a bit
>>>>>>>hacky.
>>>>>>>
>>>>>>>Or event better, as I suggested in the other patch reply, resolve
>>>>>>>this
>>>>>>>internally in the driver registering things only when they are valid.
>>>>>>>Much better then to hack anything in dpll core.
>>>>>>>
>>>>>>
>>>>>>This approach seemed to me hacky, that is why started with coping the
>>>>>>data.
>>>>>>It is not about registering, rather about unregistering on driver
>>>>>>unbind, which brakes things, and currently cannot be recovered in
>>>>>>described case.
>>>>>
>>>>>Sure it can. PF0 unbind-> internal notification-> PF1 unregisters all
>>>>>related object. Very clean and simple.
>>>>>
>>>>
>>>>What you are suggesting is:
>>>>- special purpose bus in the driver,
>>>
>>>No, it is a simple notificator. Very common infra over the whole
>>>kernel code.
>>>
>>
>>No difference if this is simple or not, it is special purpose.
>>
>>>
>>>>- dpll-related,
>>>
>>>Is this the only thing that PF0 is special with? Perhaps you can
>>>utilize this for other features as well, since your fw design is like
>>>this.
>>>
>>
>>None api user is allowed to the unordered driver bind when there are muxed
>>pins involved.
>>This requires a fix in the api not in the driver.
>>
>>>
>>>>- not needed,
>>>>- prone for errors.
>>>>
>>>>The dpll subsystem is here to make driver life easier.
>>>
>>>No, the subsystem is never here to handle device specific issues. And
>>>your PF0 dependency is very clearly something device specific. Don't
>>>pollute the dpll subsystem with workaround to handle specific device
>>>needs. create/register the dplls objects from your driver only when it
>>>is valid to do so. Make sure the lifetime of such object stays in
>>>the scope of validity. Handle that in the driver. Very clear and simple.
>>>
>>
>>In our case this is PF0 but this is broader issue. The muxed pins
>>infrastructure in now slightly broken and I am fixing it.
>>From the beginning it was designed to allow separated driver instances
>>to create a device and connect their pins with it.
>
>That is true. That's exacly what we have implemented in mlx5. Each
>instance registers dpll device and the pin related to the instance. No
>problem.
>

I read this: "mlx5 is done correctly, you need to follow", funny again.

>The fact that you do register only in PF0 is a limitation in your
>driver. Fix it there.
>

A limitation when comparing to your design, makes it not the limitation at all.
Different design is just different.

According to documentation there is no such thing like a correct order of
registering/unregistering pins.
So a driver can register a parent pin, register a pin with that parent, and
remove the parent first. It shall not crash. Right now it is broken. The
dpll needs to handle such cases. I am fixing it. Simple.

Thank you!
Arkadiusz

>
>>Any driver which would use it would face this issue. What you are trying
>>to imply is that it is better to put traffic lights on each car instead
>>of putting them on crossroads.
>>
>>>Thanks!
>>>
>>
>>Thank you!
>>Arkadiusz
>>
>>>
>>>>
>>>>Thank you!
>>>>Arkadiusz
>>>>
>>>>>
>>>>>>
>>>>>>Thank you!
>>>>>>Arkadiusz
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>Thank you!
>>>>>>>>Arkadiusz
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>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);
>>>>>>>>>>>>@@ -634,7 +634,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 93fc6c4b8a78..963bbbbe6660 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;
>>>>>>>>>>>> }
>>>>>>>>>>>>@@ -403,7 +403,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;
>>>>>>>>>>>>
>>>>>>>>>>>>@@ -696,7 +696,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;
>>>>>>>>>>>> 	}
>>>>>>>>>>>>@@ -732,7 +732,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;
>>>>>>>>>>>> 	}
>>>>>>>>>>>>@@ -759,7 +759,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;
>>>>>>>>>>>> 	}
>>>>>>>>>>>>@@ -787,7 +787,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;
>>>>>>>>>>>> 	}
>>>>>>>>>>>>@@ -817,8 +817,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;
>>>>>>>>>>>>@@ -999,7 +999,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
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>

[net,2/3] dpll: fix pin dump crash for rebound module

Checks

Commit Message

Comments

Patch