| 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: <DM6PR11MB4657175A833B5DD7B84D54269BBE9@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Tue, 14 Mar 2023 18:35:55 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Jiri Pirko <jiri@...nulli.us>, Vadim Fedorenko <vadfed@...a.com>
CC: Jakub Kicinski <kuba@...nel.org>,
Jonathan Lemon <jonathan.lemon@...il.com>,
Paolo Abeni <pabeni@...hat.com>,
Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
poros <poros@...hat.com>, mschmidt <mschmidt@...hat.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
"Olech, Milena" <milena.olech@...el.com>,
"Michalik, Michal" <michal.michalik@...el.com>
Subject: RE: [PATCH RFC v6 2/6] dpll: Add DPLL framework base functions
>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Tuesday, March 14, 2023 4:45 PM
>
>[...]
>
>
>>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/swit
>ch-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.
>
I understand suggestion on yaml, but here I am confused.
How do they relate to the supported frequency passed between driver and dpll
subsystem?
This bitfield is not visible to the userspace, and sure probably adding macro
can be useful.
>
>>+ 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()?
Yes, makes sense.
>
>
>>+ 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
>
Make sense to me.
>
>>+ 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
Why would we return here a driver return code, userspace can have this info
from extack. IMHO return values of dpll subsystem shall be not dependent on
what is returned from the driver.
>
>
>>+ 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.
>
This is the same as above (return code by driver) explanation.
>
>>+ return -EFAULT;
>>+ dpll_pin_notify(ref->dpll, pin, DPLL_A_PIN_DIRECTION);
>>+ }
>>+
>>+ return 0;
>
>[...]
Thanks,
Arkadiusz
Powered by blists - more mailing lists