netdev - Re: [PATCH net-next 2/3] dpll: add phase_offset_avg_factor

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] [day] [month] [year] [list]

Message-ID: <3e3742a7-5cfd-4f7c-8ca9-65b1de08ec29@linux.dev>
Date: Fri, 26 Sep 2025 16:25:58 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Ivan Vecera <ivecera@...hat.com>, netdev@...r.kernel.org
Cc: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
 Jiri Pirko <jiri@...nulli.us>, Jonathan Corbet <corbet@....net>,
 Donald Hunter <donald.hunter@...il.com>, Jakub Kicinski <kuba@...nel.org>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
 Prathosh Satish <Prathosh.Satish@...rochip.com>,
 Chuck Lever <chuck.lever@...cle.com>, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, Michal Schmidt <mschmidt@...hat.com>,
 Petr Oros <poros@...hat.com>
Subject: Re: [PATCH net-next 2/3] dpll: add phase_offset_avg_factor_get/set
 callback ops

On 26/09/2025 15:21, Ivan Vecera wrote:
> Add new callback operations for a dpll device:
> - phase_offset_avg_factor_get(...) - to obtain current phase offset
>    averaging factor from dpll device,
> - phase_offset_avg_factor_set(...) - to set phase offset averaging factor
> 
> Obtain the factor value using the get callback and provide it to the user
> if the device driver implements callbacks. Execute the set callback upon
> user requests.
> 
> Signed-off-by: Ivan Vecera <ivecera@...hat.com>
> ---
>   drivers/dpll/dpll_netlink.c | 76 +++++++++++++++++++++++++++++++++----
>   include/linux/dpll.h        |  6 +++
>   2 files changed, 75 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
> index 0a852011653c4..55b3ffe08024b 100644
> --- a/drivers/dpll/dpll_netlink.c
> +++ b/drivers/dpll/dpll_netlink.c
> @@ -164,6 +164,28 @@ dpll_msg_add_phase_offset_monitor(struct sk_buff *msg, struct dpll_device *dpll,
>   	return 0;
>   }
>   
> +static int
> +dpll_msg_add_phase_offset_avg_factor(struct sk_buff *msg,
> +				     struct dpll_device *dpll,
> +				     struct netlink_ext_ack *extack)
> +{
> +	const struct dpll_device_ops *ops = dpll_device_ops(dpll);
> +	u32 factor;
> +	int ret;
> +
> +	if (ops->phase_offset_avg_factor_set &&
> +	    ops->phase_offset_avg_factor_get) {
> +		ret = ops->phase_offset_avg_factor_get(dpll, dpll_priv(dpll),
> +						       &factor, extack);

Well, correct me if I'm wrong, but the device can have offset average
factor as a constant, and it can technically report it. I would make
set/get callback optional and don't require both of them to be
implemented.


> +		if (ret)
> +			return ret;
> +		if (nla_put_u32(msg, DPLL_A_PHASE_OFFSET_AVG_FACTOR, factor))
> +			return -EMSGSIZE;
> +	}
> +
> +	return 0;
> +}
> +
>   static int
>   dpll_msg_add_lock_status(struct sk_buff *msg, struct dpll_device *dpll,
>   			 struct netlink_ext_ack *extack)
> @@ -675,6 +697,9 @@ dpll_device_get_one(struct dpll_device *dpll, struct sk_buff *msg,
>   	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;
> +	ret = dpll_msg_add_phase_offset_avg_factor(msg, dpll, extack);
>   	if (ret)
>   		return ret;
>   
> @@ -839,6 +864,32 @@ dpll_phase_offset_monitor_set(struct dpll_device *dpll, struct nlattr *a,
>   					     extack);
>   }
>   
> +static int
> +dpll_phase_offset_avg_factor_set(struct dpll_device *dpll, struct nlattr *a,
> +				 struct netlink_ext_ack *extack)
> +{
> +	const struct dpll_device_ops *ops = dpll_device_ops(dpll);
> +	u32 factor = nla_get_u32(a), old_factor;
> +	int ret;
> +
> +	if (!(ops->phase_offset_avg_factor_set &&
> +	      ops->phase_offset_avg_factor_get)) {
> +		NL_SET_ERR_MSG_ATTR(extack, a, "dpll device not capable of phase offset averaging");
> +		return -EOPNOTSUPP;
> +	}
> +	ret = ops->phase_offset_avg_factor_get(dpll, dpll_priv(dpll),
> +					       &old_factor, extack);
> +	if (ret) {
> +		NL_SET_ERR_MSG(extack, "unable to get current phase offset averaging factor");
> +		return ret;
> +	}
> +	if (factor == old_factor)
> +		return 0;

I don't think the core should do any checks here. If the user for some
reason wants to re-install the same value - give them a chance. Some
drivers may implement check logic if it's relevant to the hardware, but
not in general.

> +
> +	return ops->phase_offset_avg_factor_set(dpll, dpll_priv(dpll), factor,
> +						extack);
> +}
> +

[...]