| 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
| ||
|
Message-ID: <ZBCWkhRaUztjMapa@nanopsycho>
Date: Tue, 14 Mar 2023 16:45:22 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Vadim Fedorenko <vadfed@...a.com>
Cc: Jakub Kicinski <kuba@...nel.org>,
Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
Jonathan Lemon <jonathan.lemon@...il.com>,
Paolo Abeni <pabeni@...hat.com>,
Vadim Fedorenko <vadim.fedorenko@...ux.dev>, poros@...hat.com,
mschmidt@...hat.com, netdev@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org,
Milena Olech <milena.olech@...el.com>,
Michal Michalik <michal.michalik@...el.com>
Subject: Re: [PATCH RFC v6 2/6] dpll: Add DPLL framework base functions
Sun, Mar 12, 2023 at 03:28:03AM CET, vadfed@...a.com wrote:
[...]
>diff --git a/MAINTAINERS b/MAINTAINERS
>index edd3d562beee..0222b19af545 100644
>--- a/MAINTAINERS
>+++ b/MAINTAINERS
>@@ -6289,6 +6289,15 @@ F: Documentation/networking/device_drivers/ethernet/freescale/dpaa2/switch-drive
> F: drivers/net/ethernet/freescale/dpaa2/dpaa2-switch*
> F: drivers/net/ethernet/freescale/dpaa2/dpsw*
>
>+DPLL CLOCK SUBSYSTEM
Why "clock"? You don't mention "clock" anywhere else.
[...]
>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>new file mode 100644
>index 000000000000..3fc151e16751
>--- /dev/null
>+++ b/drivers/dpll/dpll_core.c
>@@ -0,0 +1,835 @@
>+// SPDX-License-Identifier: GPL-2.0
>+/*
>+ * dpll_core.c - Generic DPLL Management class support.
Why "class" ?
[...]
>+static int
>+dpll_msg_add_pin_freq(struct sk_buff *msg, const struct dpll_pin *pin,
>+ struct netlink_ext_ack *extack, bool dump_any_freq)
>+{
>+ enum dpll_pin_freq_supp fs;
>+ struct dpll_pin_ref *ref;
>+ unsigned long i;
>+ u32 freq;
>+
>+ xa_for_each((struct xarray *)&pin->dpll_refs, i, ref) {
>+ if (ref && ref->ops && ref->dpll)
>+ break;
>+ }
>+ if (!ref || !ref->ops || !ref->dpll)
>+ return -ENODEV;
>+ if (!ref->ops->frequency_get)
>+ return -EOPNOTSUPP;
>+ if (ref->ops->frequency_get(pin, ref->dpll, &freq, extack))
>+ return -EFAULT;
>+ if (nla_put_u32(msg, DPLL_A_PIN_FREQUENCY, freq))
>+ return -EMSGSIZE;
>+ if (!dump_any_freq)
>+ return 0;
>+ for (fs = DPLL_PIN_FREQ_SUPP_UNSPEC + 1;
>+ fs <= DPLL_PIN_FREQ_SUPP_MAX; fs++) {
>+ if (test_bit(fs, &pin->prop.freq_supported)) {
>+ if (nla_put_u32(msg, DPLL_A_PIN_FREQUENCY_SUPPORTED,
>+ dpll_pin_freq_value[fs]))
This is odd. As I suggested in the yaml patch, better to treat all
supported frequencies the same, no matter if it is range or not. The you
don't need this weird bitfield.
You can have a macro to help driver to assemble array of supported
frequencies and ranges.
>+ return -EMSGSIZE;
>+ }
>+ }
>+ if (pin->prop.any_freq_min && pin->prop.any_freq_max) {
>+ if (nla_put_u32(msg, DPLL_A_PIN_ANY_FREQUENCY_MIN,
>+ pin->prop.any_freq_min))
>+ return -EMSGSIZE;
>+ if (nla_put_u32(msg, DPLL_A_PIN_ANY_FREQUENCY_MAX,
>+ pin->prop.any_freq_max))
>+ return -EMSGSIZE;
>+ }
>+
>+ return 0;
>+}
>+
[...]
>+static int
>+dpll_cmd_pin_on_dpll_get(struct sk_buff *msg, struct dpll_pin *pin,
>+ struct dpll_device *dpll,
>+ struct netlink_ext_ack *extack)
>+{
>+ struct dpll_pin_ref *ref;
>+ int ret;
>+
>+ if (nla_put_u32(msg, DPLL_A_PIN_IDX, pin->dev_driver_id))
>+ return -EMSGSIZE;
>+ if (nla_put_string(msg, DPLL_A_PIN_DESCRIPTION, pin->prop.description))
>+ return -EMSGSIZE;
>+ if (nla_put_u8(msg, DPLL_A_PIN_TYPE, pin->prop.type))
>+ return -EMSGSIZE;
>+ if (nla_put_u32(msg, DPLL_A_PIN_DPLL_CAPS, pin->prop.capabilities))
>+ return -EMSGSIZE;
>+ ret = dpll_msg_add_pin_direction(msg, pin, extack);
>+ if (ret)
>+ return ret;
>+ ret = dpll_msg_add_pin_freq(msg, pin, extack, true);
>+ if (ret && ret != -EOPNOTSUPP)
>+ return ret;
>+ ref = dpll_xa_ref_dpll_find(&pin->dpll_refs, dpll);
>+ if (!ref)
How exactly this can happen? Looks to me like only in case of a bug.
WARN_ON() perhaps (put directly into dpll_xa_ref_dpll_find()?
>+ return -EFAULT;
>+ ret = dpll_msg_add_pin_prio(msg, pin, ref, extack);
>+ if (ret && ret != -EOPNOTSUPP)
>+ return ret;
>+ ret = dpll_msg_add_pin_on_dpll_state(msg, pin, ref, extack);
>+ if (ret && ret != -EOPNOTSUPP)
>+ return ret;
>+ ret = dpll_msg_add_pin_parents(msg, pin, extack);
>+ if (ret)
>+ return ret;
>+ if (pin->rclk_dev_name)
Use && and single if
>+ if (nla_put_string(msg, DPLL_A_PIN_RCLK_DEVICE,
>+ pin->rclk_dev_name))
>+ return -EMSGSIZE;
>+
>+ return 0;
>+}
>+
[...]
>+static int
>+dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
>+ struct netlink_ext_ack *extack)
>+{
>+ u32 freq = nla_get_u32(a);
>+ struct dpll_pin_ref *ref;
>+ unsigned long i;
>+ int ret;
>+
>+ if (!dpll_pin_is_freq_supported(pin, freq))
>+ return -EINVAL;
>+
>+ xa_for_each(&pin->dpll_refs, i, ref) {
>+ ret = ref->ops->frequency_set(pin, ref->dpll, freq, extack);
>+ if (ret)
>+ return -EFAULT;
return what the op returns: ret
>+ dpll_pin_notify(ref->dpll, pin, DPLL_A_PIN_FREQUENCY);
>+ }
>+
>+ return 0;
>+}
>+
[...]
>+static int
>+dpll_pin_direction_set(struct dpll_pin *pin, struct nlattr *a,
>+ struct netlink_ext_ack *extack)
>+{
>+ enum dpll_pin_direction direction = nla_get_u8(a);
>+ struct dpll_pin_ref *ref;
>+ unsigned long i;
>+
>+ if (!(DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE & pin->prop.capabilities))
>+ return -EOPNOTSUPP;
>+
>+ xa_for_each(&pin->dpll_refs, i, ref) {
>+ if (ref->ops->direction_set(pin, ref->dpll, direction, extack))
ret = ..
if (ret)
return ret;
Please use this pattern in other ops call code as well.
>+ return -EFAULT;
>+ dpll_pin_notify(ref->dpll, pin, DPLL_A_PIN_DIRECTION);
>+ }
>+
>+ return 0;
[...]
Powered by blists - more mailing lists