Re: [PATCH RFC v6 2/6] dpll: Add DPLL framework base functions

[Jiri Pirko <jiri@resnulli.us>]

Sun, Mar 12, 2023 at 03:28:03AM CET, vadfed@meta.com wrote:

[...]


>+static int
>+dpll_msg_add_pin_direction(struct sk_buff *msg, const struct dpll_pin *pin,
>+			   struct netlink_ext_ack *extack)
>+{
>+	enum dpll_pin_direction direction;
>+	struct dpll_pin_ref *ref;
>+	unsigned long i;
>+
>+	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->direction_get)
>+		return -EOPNOTSUPP;
>+	if (ref->ops->direction_get(pin, ref->dpll, &direction, extack))
>+		return -EFAULT;
>+	if (nla_put_u8(msg, DPLL_A_PIN_DIRECTION, direction))
>+		return -EMSGSIZE;
>+
>+	return 0;
>+}
>+
>+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)

Checking for "ref" is nonsense here, as xa_for_each fills it up
for every iteration.

ref->dpll is always filled. Also pointless check.

Does it make sense to register with ops==NULL? I think we should
forbid it and make this just xa_find(0) to get the first item in the
xarray.

I'm doing this in my patch, as it is dependency on some other patch I do
in this area.


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

Why -EOPNOTSUPP here is not ok, as for the others below?


>+		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 this can happen? I don't think it could.


>+		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)
>+		if (nla_put_string(msg, DPLL_A_PIN_RCLK_DEVICE,
>+				   pin->rclk_dev_name))
>+			return -EMSGSIZE;
>+
>+	return 0;
>+}
>+
>+static int
>+__dpll_cmd_pin_dump_one(struct sk_buff *msg, struct dpll_pin *pin,
>+			struct netlink_ext_ack *extack, bool dump_dpll)
>+{
>+	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;
>+	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;
>+	ret = dpll_msg_add_pins_on_pin(msg, pin, extack);
>+	if (ret)
>+		return ret;
>+	if (!xa_empty(&pin->dpll_refs) && dump_dpll) {

How dpll refs could be empty? I don't think it is possible.

Overall, whole the code has very odd habit of checking for conditions
that are obviously impossible to happen. Only confuses reader as he
naturally expects that the check is there for a reason.



>+		ret = dpll_msg_add_pin_dplls(msg, pin, extack);
>+		if (ret)
>+			return ret;
>+	}
>+	if (pin->rclk_dev_name)
>+		if (nla_put_string(msg, DPLL_A_PIN_RCLK_DEVICE,
>+				   pin->rclk_dev_name))
>+			return -EMSGSIZE;
>+
>+	return 0;
>+}
>+

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel