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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <rwterkiyhdjcedboj773zc5s3d6purz6yaccfowco7m5zd7q3c@or4r33ay2dxh>
Date: Thu, 8 May 2025 16:30:49 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
Cc: donald.hunter@...il.com, kuba@...nel.org, davem@...emloft.net, 
	edumazet@...gle.com, pabeni@...hat.com, horms@...nel.org, vadim.fedorenko@...ux.dev, 
	anthony.l.nguyen@...el.com, przemyslaw.kitszel@...el.com, andrew+netdev@...n.ch, 
	saeedm@...dia.com, leon@...nel.org, tariqt@...dia.com, jonathan.lemon@...il.com, 
	richardcochran@...il.com, aleksandr.loktionov@...el.com, milena.olech@...el.com, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org, intel-wired-lan@...ts.osuosl.org, 
	linux-rdma@...r.kernel.org
Subject: Re: [PATCH net-next v3 2/3] dpll: add phase_offset_monitor_get/set
 callback ops

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"?


>+		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?


>+}
>+
> 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?


>+		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