netdev - Re: [PATCH net-next v3 5/5] dpll: netlink/core: change pin frequency set behavior

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: <ZR//DY6xMPi1AQ5i@nanopsycho>
Date: Fri, 6 Oct 2023 14:35:25 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
Cc: netdev@...r.kernel.org, vadim.fedorenko@...ux.dev, corbet@....net,
	davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
	jesse.brandeburg@...el.com, anthony.l.nguyen@...el.com,
	linux-doc@...r.kernel.org, intel-wired-lan@...ts.osuosl.org
Subject: Re: [PATCH net-next v3 5/5] dpll: netlink/core: change pin frequency
 set behavior

Fri, Oct 06, 2023 at 01:41:01PM CEST, arkadiusz.kubalewski@...el.com wrote:
>Align the aproach of pin frequency set behavior with the approach
>introduced with pin phase adjust set.
>Fail the request if any of devices did not registered the callback ops.
>If callback op on any pin's registered device fails, return error and
>rollback the value to previous one.
>
>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>---
> drivers/dpll/dpll_netlink.c | 50 +++++++++++++++++++++++++++++--------
> 1 file changed, 40 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>index 97319a9e4667..8e5fea74aec1 100644
>--- a/drivers/dpll/dpll_netlink.c
>+++ b/drivers/dpll/dpll_netlink.c
>@@ -615,30 +615,60 @@ static int
> dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
> 		  struct netlink_ext_ack *extack)
> {
>-	u64 freq = nla_get_u64(a);
>-	struct dpll_pin_ref *ref;
>+	u64 freq = nla_get_u64(a), old_freq;
>+	struct dpll_pin_ref *ref, *failed;
>+	const struct dpll_pin_ops *ops;
>+	struct dpll_device *dpll;
> 	unsigned long i;
> 	int ret;
> 
> 	if (!dpll_pin_is_freq_supported(pin, freq)) {
>-		NL_SET_ERR_MSG_ATTR(extack, a, "frequency is not supported by the device");
>+		NL_SET_ERR_MSG_ATTR(extack, a,
>+				    "frequency is not supported by the device");

No need for this wrapping. Seems unrelated to the rest of the patch
anyway.


> 		return -EINVAL;
> 	}
>-

No need for this too.



> 	xa_for_each(&pin->dpll_refs, i, ref) {
>-		const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
>-		struct dpll_device *dpll = ref->dpll;
>-
>-		if (!ops->frequency_set)
>+		ops = dpll_pin_ops(ref);
>+		if (!ops->frequency_set || !ops->frequency_get)
> 			return -EOPNOTSUPP;

Add an extack msg while you are at it - could be a separate patch.


>+	}
>+	ref = dpll_xa_ref_dpll_first(&pin->dpll_refs);
>+	ops = dpll_pin_ops(ref);
>+	dpll = ref->dpll;
>+	ret = ops->frequency_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
>+				 dpll_priv(dpll), &old_freq, extack);
>+	if (ret) {
>+		NL_SET_ERR_MSG(extack, "unable to get old frequency value");
>+		return ret;
>+	}
>+	if (freq == old_freq)
>+		return 0;
>+
>+	xa_for_each(&pin->dpll_refs, i, ref) {
>+		ops = dpll_pin_ops(ref);
>+		dpll = ref->dpll;
> 		ret = ops->frequency_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
> 					 dpll, dpll_priv(dpll), freq, extack);
>-		if (ret)
>-			return ret;
>+		if (ret) {
>+			failed = ref;

Extack msg.


>+			goto rollback;
>+		}
> 	}
> 	__dpll_pin_change_ntf(pin);
> 
> 	return 0;
>+
>+rollback:
>+	xa_for_each(&pin->dpll_refs, i, ref) {
>+		if (ref == failed)
>+			break;
>+		ops = dpll_pin_ops(ref);
>+		dpll = ref->dpll;
>+		if (ops->frequency_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
>+				       dpll, dpll_priv(dpll), old_freq, extack))
>+			NL_SET_ERR_MSG(extack, "set frequency rollback failed");
>+	}
>+	return ret;
> }
> 
> static int
>-- 
>2.38.1
>