Message-ID: <ZBw1dID2U9D7wMyy@nanopsycho>
Date:   Thu, 23 Mar 2023 12:18:12 +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:

[...]


>+/**
>+ * dpll_xa_ref_pin_del - remove reference of a pin from xarray
>+ * @xa_pins: dpll_pin_ref xarray holding pins
>+ * @pin: pointer to a pin
>+ *
>+ * Decrement refcount of existing pin reference on given xarray.
>+ * If all references are dropped, delete the reference and free its memory.
>+ *

Hmm, came to think about this, why do you do func docs even for static
function? It is customary to do that for exported function. For static
ones, not really needed.


>+ * Return:
>+ * * 0 on success
>+ * * -EINVAL if reference to a pin was not found
>+ */
>+static int dpll_xa_ref_pin_del(struct xarray *xa_pins, struct dpll_pin *pin)

Have this to return void, you don't check the return value anywhere.


>+{
>+	struct dpll_pin_ref *ref;
>+	unsigned long i;
>+
>+	xa_for_each(xa_pins, i, ref) {
>+		if (ref->pin == pin) {
>+			if (refcount_dec_and_test(&ref->refcount)) {
>+				xa_erase(xa_pins, i);
>+				kfree(ref);
>+			}
>+			return 0;
>+		}
>+	}
>+
>+	return -EINVAL;
>+}

[...]


>+/**
>+ * dpll_xa_ref_dpll_find - find dpll reference on xarray
>+ * @xa_dplls: dpll_pin_ref xarray holding dplls
>+ * @dpll: pointer to a dpll
>+ *
>+ * Search for dpll-pin ops reference struct of a given dpll on given xarray.
>+ *
>+ * Return:
>+ * * pin reference struct pointer on success
>+ * * NULL - reference to a pin was not found
>+ */
>+struct dpll_pin_ref *
>+dpll_xa_ref_dpll_find(struct xarray *xa_refs, const struct dpll_device *dpll)

Every caller of this function does fill the first arg by:
&pin->dpll_refs
Could you please change it to "struct dpll_pin *pin" and get the xarray
pointer in this function?

The same applies to other functions passing xarray pointer, like:
dpll_xa_ref_dpll_add
dpll_xa_ref_dpll_del
dpll_xa_ref_pin_find

The point is, always better and easier to read to pass
"struct dpll_device *" and "struct dpll_pin *" as function args.
Passing "struct xarray *" makes the reader uncertain about what
is going on.



>+{
>+	struct dpll_pin_ref *ref;
>+	unsigned long i;
>+
>+	xa_for_each(xa_refs, i, ref) {
>+		if (ref->dpll == dpll)
>+			return ref;
>+	}
>+
>+	return NULL;
>+}
>+
>+

[...]


>+/**
>+ * dpll_device_register - register the dpll device in the subsystem
>+ * @dpll: pointer to a dpll
>+ * @type: type of a dpll
>+ * @ops: ops for a dpll device
>+ * @priv: pointer to private information of owner
>+ * @owner: pointer to owner device
>+ *
>+ * Make dpll device available for user space.
>+ *
>+ * Return:
>+ * * 0 on success
>+ * * -EINVAL on failure

You return more than that. Do you really need to list the error
possiblities in the func docs?


>+ */
>+int dpll_device_register(struct dpll_device *dpll, enum dpll_type type,
>+			 struct dpll_device_ops *ops, void *priv,
>+			 struct device *owner)

[...]


>+static int
>+__dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin,
>+		    struct dpll_pin_ops *ops, void *priv,
>+		    const char *rclk_device_name)
>+{
>+	int ret;
>+
>+	if (rclk_device_name && !pin->rclk_dev_name) {
>+		pin->rclk_dev_name = kstrdup(rclk_device_name, GFP_KERNEL);
>+		if (!pin->rclk_dev_name)
>+			return -ENOMEM;
>+	}

Somewhere here, please add a check:
dpll->module == pin->module dpll->clock_id && pin->clock_id
For sanity sake.


>+	ret = dpll_xa_ref_pin_add(&dpll->pin_refs, pin, ops, priv);
>+	if (ret)
>+		goto rclk_free;
>+	ret = dpll_xa_ref_dpll_add(&pin->dpll_refs, dpll, ops, priv);
>+	if (ret)
>+		goto ref_pin_del;
>+	else
>+		dpll_pin_notify(dpll, pin, DPLL_A_PIN_IDX);

Pointless else.


>+
>+	return ret;
>+
>+ref_pin_del:
>+	dpll_xa_ref_pin_del(&dpll->pin_refs, pin);
>+rclk_free:
>+	kfree(pin->rclk_dev_name);
>+	return ret;
>+}

[...]


>+int
>+dpll_pin_on_pin_register(struct dpll_pin *parent, struct dpll_pin *pin,
>+			 struct dpll_pin_ops *ops, void *priv,
>+			 struct device *rclk_device)
>+{
>+	struct dpll_pin_ref *ref;
>+	unsigned long i, stop;
>+	int ret;
>+
>+	if (WARN_ON(!pin || !parent))
>+		return -EINVAL;
>+	if (WARN_ON(parent->prop.type != DPLL_PIN_TYPE_MUX))
>+		return -EPERM;
>+	mutex_lock(&dpll_pin_xa_lock);
>+	ret = dpll_xa_ref_pin_add(&pin->parent_refs, parent, ops, priv);
>+	if (ret)
>+		goto unlock;
>+	refcount_inc(&pin->refcount);
>+	xa_for_each(&parent->dpll_refs, i, ref) {
>+		mutex_lock(&dpll_device_xa_lock);
>+		ret = __dpll_pin_register(ref->dpll, pin, ops, priv,

Why exactly do you need to register the pin over to the dpll of a
parent? Isn't it enough to have the pin registered on a parent?
I mean, there is no direct connection between pin and dpll, the parent
is in the middle. So prio setup, and other things does not make sense to
configure on this child pin, isn't it?

Btw, what is stopping the driver from:
dpll register
pin1 register on dpll
pin2 register on pin1
pin1 unregister
?
The you would have pin2 registered to dpll incorrectly.


>+					  rclk_device ?
>+					  dev_name(rclk_device) : NULL);
>+		mutex_unlock(&dpll_device_xa_lock);
>+		if (ret) {
>+			stop = i;
>+			goto dpll_unregister;
>+		}
>+		dpll_pin_parent_notify(ref->dpll, pin, parent, DPLL_A_PIN_IDX);
>+	}
>+	mutex_unlock(&dpll_pin_xa_lock);
>+
>+	return ret;
>+
>+dpll_unregister:
>+	xa_for_each(&parent->dpll_refs, i, ref) {
>+		if (i < stop) {
>+			mutex_lock(&dpll_device_xa_lock);
>+			__dpll_pin_unregister(ref->dpll, pin);
>+			mutex_unlock(&dpll_device_xa_lock);
>+		}
>+	}
>+	refcount_dec(&pin->refcount);
>+	dpll_xa_ref_pin_del(&pin->parent_refs, parent);
>+unlock:
>+	mutex_unlock(&dpll_pin_xa_lock);
>+	return ret;
>+}

[...]


>+static int
>+dpll_pin_on_pin_state_set(struct dpll_device *dpll, struct dpll_pin *pin,
>+			  u32 parent_idx, enum dpll_pin_state state,
>+			  struct netlink_ext_ack *extack)
>+{
>+	struct dpll_pin_ref *ref;
>+	struct dpll_pin *parent;
>+
>+	if (!(DPLL_PIN_CAPS_STATE_CAN_CHANGE & pin->prop.capabilities))

Hmm, why is this capabilities are any good for internal purposes? I
understand the need to expose it to the user, but internally in kernel,
if the driver implements some _set() op, it is good enough indication of
a support of a certain setter. You can check if the relevant _set()
is not null and expose the appropriate capability to the user.



>+		return -EOPNOTSUPP;
>+	parent = dpll_pin_get_by_idx(dpll, parent_idx);

I don't follow. Why do you need dpll pointer to get the parent pin?
The same handle as pin should be used, you have clock_id and driver name
(in next patchsets implementation) that should be enough.
Pin is a separate entity, attached 0:N dplls.

Please remove dpll pointer from here. Also, please remove
dpll->pins_ref, as you are using this array only for this lookup (here
and in dpll_pin_pre_doit())


>+	if (!parent)
>+		return -EINVAL;
>+	ref = dpll_xa_ref_pin_find(&pin->parent_refs, parent);
>+	if (!ref)
>+		return -EINVAL;
>+	if (!ref->ops || !ref->ops->state_on_pin_set)
>+		return -EOPNOTSUPP;
>+	if (ref->ops->state_on_pin_set(pin, parent, state, extack))
>+		return -EFAULT;
>+	dpll_pin_parent_notify(dpll, pin, parent, DPLL_A_PIN_STATE);
>+
>+	return 0;
>+}
>+
>+static int
>+dpll_pin_state_set(struct dpll_device *dpll, struct dpll_pin *pin,
>+		   enum dpll_pin_state state,
>+		   struct netlink_ext_ack *extack)
>+{
>+	struct dpll_pin_ref *ref;
>+
>+	if (!(DPLL_PIN_CAPS_STATE_CAN_CHANGE & pin->prop.capabilities))
>+		return -EOPNOTSUPP;
>+	ref = dpll_xa_ref_dpll_find(&pin->dpll_refs, dpll);
>+	if (!ref)
>+		return -EFAULT;
>+	if (!ref->ops || !ref->ops->state_on_dpll_set)
>+		return -EOPNOTSUPP;
>+	if (ref->ops->state_on_dpll_set(pin, ref->dpll, state, extack))
>+		return -EINVAL;
>+	dpll_pin_notify(ref->dpll, pin, DPLL_A_PIN_STATE);
>+
>+	return 0;
>+}

[...]


>+static int
>+dpll_set_from_nlattr(struct dpll_device *dpll, struct genl_info *info)
>+{
>+	struct nlattr *attr;
>+	enum dpll_mode mode;
>+	int rem, ret = 0;
>+
>+	nla_for_each_attr(attr, genlmsg_data(info->genlhdr),
>+			  genlmsg_len(info->genlhdr), rem) {
>+		switch (nla_type(attr)) {
>+		case DPLL_A_MODE:
>+			mode = nla_get_u8(attr);
>+
>+			if (!dpll->ops || !dpll->ops->mode_set)

Remove the pointless check of ops. This cannot happen (checked in
dpll_device_register())


>+				return -EOPNOTSUPP;
>+			ret = dpll->ops->mode_set(dpll, mode, info->extack);
>+			if (ret)
>+				return ret;
>+			break;
>+		default:
>+			break;
>+		}
>+	}
>+
>+	return ret;
>+}
>+

[...]

Powered by blists - more mailing lists