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_event_device_change(struct sk_buff *msg, struct dpll_device *dpll,
>+			 struct dpll_pin *pin, struct dpll_pin *parent,
>+			 enum dplla attr)
>+{
>+	int ret = dpll_msg_add_dev_handle(msg, dpll);
>+	struct dpll_pin_ref *ref = NULL;
>+	enum dpll_pin_state state;
>+
>+	if (ret)
>+		return ret;
>+	if (pin && nla_put_u32(msg, DPLL_A_PIN_IDX, pin->dev_driver_id))
>+		return -EMSGSIZE;
>+
>+	switch (attr) {
>+	case DPLL_A_MODE:
>+		ret = dpll_msg_add_mode(msg, dpll, NULL);
>+		break;
>+	case DPLL_A_SOURCE_PIN_IDX:
>+		ret = dpll_msg_add_source_pin_idx(msg, dpll, NULL);
>+		break;
>+	case DPLL_A_LOCK_STATUS:
>+		ret = dpll_msg_add_lock_status(msg, dpll, NULL);

On top of what I wrote about the notifications, I found another two
issues:
1) You don't take any lock calling this from drivers. You need to hold
   the xarray locks you have now.

   I have to repear, I think that we definitelly need to convert the
   overall locking scheme to have this per-instance, in a similar way
   we did that for devlink. I noted this in another email, but wanted
   to say that again.

2) You have possible race condition:
   1) -> driver gets a state change event
   2) -> driver calls into this function
   3) -> this code does call the driver op to get the state, driver
         queries the state again

   Between 1) and 3) state can easily change, multiple times. That might
   lead to oddities observed by the user (like getting a notification
   of change with the original values)

   I see only 1 solutions to this:
   Pass the value of changed item from the driver here and just pass
   it on over netlink without doing calling into driver again.


>+		break;
>+	case DPLL_A_TEMP:
>+		ret = dpll_msg_add_temp(msg, dpll, NULL);
>+		break;
>+	case DPLL_A_PIN_FREQUENCY:
>+		ret = dpll_msg_add_pin_freq(msg, pin, NULL, false);
>+		break;
>+	case DPLL_A_PIN_PRIO:
>+		ref = dpll_xa_ref_dpll_find(&pin->dpll_refs, dpll);
>+		if (!ref)
>+			return -EFAULT;
>+		ret = dpll_msg_add_pin_prio(msg, pin, ref, NULL);
>+		break;
>+	case DPLL_A_PIN_STATE:
>+		if (parent) {
>+			ref = dpll_xa_ref_pin_find(&pin->parent_refs, parent);
>+			if (!ref)
>+				return -EFAULT;
>+			if (!ref->ops || !ref->ops->state_on_pin_get)
>+				return -EOPNOTSUPP;
>+			ret = ref->ops->state_on_pin_get(pin, parent, &state,
>+							 NULL);
>+			if (ret)
>+				return ret;
>+			if (nla_put_u32(msg, DPLL_A_PIN_PARENT_IDX,
>+					parent->dev_driver_id))
>+				return -EMSGSIZE;
>+		} else {
>+			ref = dpll_xa_ref_dpll_find(&pin->dpll_refs, dpll);
>+			if (!ref)
>+				return -EFAULT;
>+			ret = dpll_msg_add_pin_on_dpll_state(msg, pin, ref,
>+							     NULL);
>+			if (ret)
>+				return ret;
>+		}
>+		break;
>+	default:
>+		break;
>+	}
>+
>+	return ret;
>+}
>+
>+static int
>+dpll_send_event_create(enum dpll_event event, struct dpll_device *dpll)
>+{
>+	struct sk_buff *msg;
>+	int ret = -EMSGSIZE;
>+	void *hdr;
>+
>+	msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>+	if (!msg)
>+		return -ENOMEM;
>+
>+	hdr = genlmsg_put(msg, 0, 0, &dpll_nl_family, 0, event);
>+	if (!hdr)
>+		goto out_free_msg;
>+
>+	ret = dpll_msg_add_dev_handle(msg, dpll);
>+	if (ret)
>+		goto out_cancel_msg;
>+	genlmsg_end(msg, hdr);
>+	genlmsg_multicast(&dpll_nl_family, msg, 0, 0, GFP_KERNEL);
>+
>+	return 0;
>+
>+out_cancel_msg:
>+	genlmsg_cancel(msg, hdr);
>+out_free_msg:
>+	nlmsg_free(msg);
>+
>+	return ret;
>+}
>+
>+static int
>+dpll_send_event_change(struct dpll_device *dpll, struct dpll_pin *pin,
>+		       struct dpll_pin *parent, enum dplla attr)
>+{
>+	struct sk_buff *msg;
>+	int ret = -EMSGSIZE;
>+	void *hdr;
>+
>+	msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>+	if (!msg)
>+		return -ENOMEM;
>+
>+	hdr = genlmsg_put(msg, 0, 0, &dpll_nl_family, 0,
>+			  DPLL_EVENT_DEVICE_CHANGE);
>+	if (!hdr)
>+		goto out_free_msg;
>+
>+	ret = dpll_event_device_change(msg, dpll, pin, parent, attr);
>+	if (ret)
>+		goto out_cancel_msg;
>+	genlmsg_end(msg, hdr);
>+	genlmsg_multicast(&dpll_nl_family, msg, 0, 0, GFP_KERNEL);
>+
>+	return 0;
>+
>+out_cancel_msg:
>+	genlmsg_cancel(msg, hdr);
>+out_free_msg:
>+	nlmsg_free(msg);
>+
>+	return ret;
>+}
>+
>+int dpll_notify_device_create(struct dpll_device *dpll)
>+{
>+	return dpll_send_event_create(DPLL_EVENT_DEVICE_CREATE, dpll);
>+}
>+
>+int dpll_notify_device_delete(struct dpll_device *dpll)

Please change the function names to "register/unregister" to be
consistent with the rest of the code.


>+{
>+	return dpll_send_event_create(DPLL_EVENT_DEVICE_DELETE, dpll);
>+}
>+
>+int dpll_device_notify(struct dpll_device *dpll, enum dplla attr)
>+{
>+	if (WARN_ON(!dpll))
>+		return -EINVAL;
>+
>+	return dpll_send_event_change(dpll, NULL, NULL, attr);
>+}
>+EXPORT_SYMBOL_GPL(dpll_device_notify);
>+
>+int dpll_pin_notify(struct dpll_device *dpll, struct dpll_pin *pin,
>+		    enum dplla attr)

The driver should be aware of netlink attributes. Should be
abstracted out.

just have per-item notification like:
dpll_pin_state_notify()
dpll_pin_prio_notify()
...

Then you can easily pass changed value that would allow solution to
the issue 2) I described above.



>+{
>+	return dpll_send_event_change(dpll, pin, NULL, attr);
>+}
>+

[...]

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