lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <SJ2PR11MB8452820F6BDF445F29D368C99B8BA@SJ2PR11MB8452.namprd11.prod.outlook.com>
Date: Thu, 8 May 2025 15:20:24 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Jiri Pirko <jiri@...nulli.us>
CC: "donald.hunter@...il.com" <donald.hunter@...il.com>, "kuba@...nel.org"
	<kuba@...nel.org>, "davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "pabeni@...hat.com"
	<pabeni@...hat.com>, "horms@...nel.org" <horms@...nel.org>,
	"vadim.fedorenko@...ux.dev" <vadim.fedorenko@...ux.dev>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@...el.com>, "andrew+netdev@...n.ch"
	<andrew+netdev@...n.ch>, "saeedm@...dia.com" <saeedm@...dia.com>,
	"leon@...nel.org" <leon@...nel.org>, "tariqt@...dia.com" <tariqt@...dia.com>,
	"jonathan.lemon@...il.com" <jonathan.lemon@...il.com>,
	"richardcochran@...il.com" <richardcochran@...il.com>, "Loktionov, Aleksandr"
	<aleksandr.loktionov@...el.com>, "Olech, Milena" <milena.olech@...el.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>
Subject: RE: [PATCH net-next v3 2/3] dpll: add phase_offset_monitor_get/set
 callback ops

>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Thursday, May 8, 2025 4:31 PM
>
>Thu, May 08, 2025 at 02:21:27PM +0200, arkadiusz.kubalewski@...el.com
>wrote:
>>Add new callback operations for a dpll device:
>>- phase_offset_monitor_get(..) - to obtain current state of phase offset
>>  monitor feature from dpll device,
>>- phase_offset_monitor_set(..) - to allow feature configuration.
>>
>>Obtain the feature state value using the get callback and provide it to
>>the user if the device driver implements callbacks.
>>
>>Execute the set callback upon user requests.
>>
>>Reviewed-by: Milena Olech <milena.olech@...el.com>
>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>>---
>>v3:
>>- remove feature flags and capabilities,
>>- add separated (per feature) callback ops,
>>- use callback ops to determine feature availability.
>>---
>> drivers/dpll/dpll_netlink.c | 76 ++++++++++++++++++++++++++++++++++++-
>> include/linux/dpll.h        |  8 ++++
>> 2 files changed, 82 insertions(+), 2 deletions(-)
>>
>>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>>index c130f87147fa..6d2980455a46 100644
>>--- a/drivers/dpll/dpll_netlink.c
>>+++ b/drivers/dpll/dpll_netlink.c
>>@@ -126,6 +126,26 @@ dpll_msg_add_mode_supported(struct sk_buff *msg,
>>struct dpll_device *dpll,
>> 	return 0;
>> }
>>
>>+static int
>>+dpll_msg_add_phase_offset_monitor(struct sk_buff *msg, struct dpll_device
>>*dpll,
>>+				  struct netlink_ext_ack *extack)
>>+{
>>+	const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>>+	enum dpll_feature_state state;
>>+	int ret;
>>+
>>+	if (ops->phase_offset_monitor_set && ops->phase_offset_monitor_get) {
>>+		ret = ops->phase_offset_monitor_get(dpll, dpll_priv(dpll),
>>+						    &state, extack);
>>+		if (ret)
>>+			return -EINVAL;
>
>Why you don't propagate "ret"?
>

My bad, will fix that.

>
>>+		if (nla_put_u32(msg, DPLL_A_PHASE_OFFSET_MONITOR, state))
>>+			return -EMSGSIZE;
>>+	}
>>+
>>+	return 0;
>>+}
>>+
>> static int
>> dpll_msg_add_lock_status(struct sk_buff *msg, struct dpll_device *dpll,
>> 			 struct netlink_ext_ack *extack)
>>@@ -591,6 +611,9 @@ dpll_device_get_one(struct dpll_device *dpll, struct
>>sk_buff *msg,
>> 		return ret;
>> 	if (nla_put_u32(msg, DPLL_A_TYPE, dpll->type))
>> 		return -EMSGSIZE;
>>+	ret = dpll_msg_add_phase_offset_monitor(msg, dpll, extack);
>>+	if (ret)
>>+		return ret;
>>
>> 	return 0;
>> }
>>@@ -746,6 +769,31 @@ int dpll_pin_change_ntf(struct dpll_pin *pin)
>> }
>> EXPORT_SYMBOL_GPL(dpll_pin_change_ntf);
>>
>>+static int
>>+dpll_phase_offset_monitor_set(struct dpll_device *dpll, struct nlattr *a,
>>+			      struct netlink_ext_ack *extack)
>>+{
>>+	const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>>+	enum dpll_feature_state state = nla_get_u32(a), old_state;
>>+	int ret;
>>+
>>+	if (!(ops->phase_offset_monitor_set && ops-
>>phase_offset_monitor_get)) {
>>+		NL_SET_ERR_MSG_ATTR(extack, a, "dpll device not capable of
>>phase offset monitor");
>>+		return -EOPNOTSUPP;
>>+	}
>>+	ret = ops->phase_offset_monitor_get(dpll, dpll_priv(dpll),
>>&old_state,
>>+					    extack);
>>+	if (ret) {
>>+		NL_SET_ERR_MSG(extack, "unable to get current state of phase
>>offset monitor");
>>+		return -EINVAL;
>>+	}
>>+	if (state == old_state)
>>+		return 0;
>>+
>>+	return ops->phase_offset_monitor_set(dpll, dpll_priv(dpll), state,
>>+					     extack);
>
>Why you need to do this get/set dance? I mean, just call the driver
>set() op and let it do what is needed there, no?
>

We did it this way from the beginning (during various pin-set related flows).

>
>>+}
>>+
>> static int
>> dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
>> 		  struct netlink_ext_ack *extack)
>>@@ -1533,10 +1581,34 @@ int dpll_nl_device_get_doit(struct sk_buff *skb,
>>struct genl_info *info)
>> 	return genlmsg_reply(msg, info);
>> }
>>
>>+static int
>>+dpll_set_from_nlattr(struct dpll_device *dpll, struct genl_info *info)
>>+{
>>+	struct nlattr *a;
>>+	int rem, ret;
>>+
>>+	nla_for_each_attr(a, genlmsg_data(info->genlhdr),
>>+			  genlmsg_len(info->genlhdr), rem) {
>
>Hmm, why you iterate? Why you just don't parse to attr array, as it is
>usually done?
>

Hmm, AFAIR there are issues when you parse nested stuff with the array
approach, here nothing is nested, but we already have the same approach on
parsing pin related stuff (dpll_pin_set_from_nlattr(..)), just did the same
here.

Thank you!
Arkadiusz

>
>>+		switch (nla_type(a)) {
>>+		case DPLL_A_PHASE_OFFSET_MONITOR:
>>+			ret = dpll_phase_offset_monitor_set(dpll, a,
>>+							    info->extack);
>>+			if (ret)
>>+				return ret;
>>+			break;
>>+		default:
>>+			break;
>>+		}
>>+	}
>>+
>>+	return 0;
>>+}
>>+
>> int dpll_nl_device_set_doit(struct sk_buff *skb, struct genl_info *info)
>> {
>>-	/* placeholder for set command */
>>-	return 0;
>>+	struct dpll_device *dpll = info->user_ptr[0];
>>+
>>+	return dpll_set_from_nlattr(dpll, info);
>> }
>>
>> int dpll_nl_device_get_dumpit(struct sk_buff *skb, struct
>>netlink_callback *cb)
>>diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>>index 5e4f9ab1cf75..6ad6c2968a28 100644
>>--- a/include/linux/dpll.h
>>+++ b/include/linux/dpll.h
>>@@ -30,6 +30,14 @@ struct dpll_device_ops {
>> 				       void *dpll_priv,
>> 				       unsigned long *qls,
>> 				       struct netlink_ext_ack *extack);
>>+	int (*phase_offset_monitor_set)(const struct dpll_device *dpll,
>>+					void *dpll_priv,
>>+					enum dpll_feature_state state,
>>+					struct netlink_ext_ack *extack);
>>+	int (*phase_offset_monitor_get)(const struct dpll_device *dpll,
>>+					void *dpll_priv,
>>+					enum dpll_feature_state *state,
>>+					struct netlink_ext_ack *extack);
>> };
>>
>> struct dpll_pin_ops {
>>--
>>2.38.1
>>


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ