Re: [RFC PATCH v7 5/8] ice: implement dpll interface to control cgu

On Tue, 2023-05-16 at 08:26 +0200, Jiri Pirko wrote:

Tue, May 16, 2023 at 12:07:57AM CEST, arkadiusz.kubalewski@intel.com wrote:

quoted

quoted

From: Jiri Pirko <jiri@resnulli.us>
Sent: Wednesday, May 3, 2023 2:19 PM

Fri, Apr 28, 2023 at 02:20:06AM CEST, vadfed@meta.com wrote:

quoted

From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

[...]

quoted

quoted

quoted

+ * ice_dpll_frequency_set - wrapper for pin callback for set frequency
+ * @pin: pointer to a pin
+ * @pin_priv: private data pointer passed on pin registration
+ * @dpll: pointer to dpll
+ * @frequency: frequency to be set
+ * @extack: error reporting
+ * @pin_type: type of pin being configured
+ *
+ * Wraps internal set frequency command on a pin.
+ *
+ * Return:
+ * * 0 - success
+ * * negative - error pin not found or couldn't set in hw  */ static
+int ice_dpll_frequency_set(const struct dpll_pin *pin, void *pin_priv,
+		       const struct dpll_device *dpll,
+		       const u32 frequency,
+		       struct netlink_ext_ack *extack,
+		       const enum ice_dpll_pin_type pin_type) {
+	struct ice_pf *pf = pin_priv;
+	struct ice_dpll_pin *p;
+	int ret = -EINVAL;
+
+	if (!pf)
+		return ret;
+	if (ice_dpll_cb_lock(pf))
+		return -EBUSY;
+	p = ice_find_pin(pf, pin, pin_type);

This does not make any sense to me. You should avoid the lookups and remove
ice_find_pin() function entirely. The purpose of having pin_priv is to
carry the struct ice_dpll_pin * directly. You should pass it down during
pin register.

pf pointer is stored in dpll_priv.

In this case dpll_priv is not passed, so cannot use it.

It should be passed. In general to every op where *dpll is passed, the
dpll_priv pointer should be passed along. Please, fix this.

quoted

But in general it makes sense I will hold pf inside of ice_dpll_pin
and fix this.

Nope, just use dpll_priv. That's why we have it.


[...]

quoted

quoted

quoted

+/**
+ * ice_dpll_pin_state_set - set pin's state on dpll
+ * @dpll: dpll being configured
+ * @pin: pointer to a pin
+ * @pin_priv: private data pointer passed on pin registration
+ * @state: state of pin to be set
+ * @extack: error reporting
+ * @pin_type: type of a pin
+ *
+ * Set pin state on a pin.
+ *
+ * Return:
+ * * 0 - OK or no change required
+ * * negative - error
+ */
+static int
+ice_dpll_pin_state_set(const struct dpll_device *dpll,
+		       const struct dpll_pin *pin, void *pin_priv,
+		       const enum dpll_pin_state state,

Why you use const with enums?

Just show usage intention explicitly.

Does not make any sense what so ever. Please avoid it.

quoted

quoted

quoted

+static int ice_dpll_rclk_state_on_pin_get(const struct dpll_pin *pin,
+					  void *pin_priv,
+					  const struct dpll_pin *parent_pin,
+					  enum dpll_pin_state *state,
+					  struct netlink_ext_ack *extack) {
+	struct ice_pf *pf = pin_priv;
+	u32 parent_idx, hw_idx = ICE_DPLL_PIN_IDX_INVALID, i;

Reverse christmas tree ordering please.

Fixed.

quoted

quoted

+	struct ice_dpll_pin *p;
+	int ret = -EFAULT;
+
+	if (!pf)

How exacly this can happen. My wild guess is it can't. Don't do such
pointless checks please, confuses the reader.

From driver perspective the pf pointer value is given by external entity,
why shouldn't it be valdiated?

What? You pass it during register, you get it back here. Nothing to
check. Please drop it. Non-sense checks like this have no place in
kernel, they only confuse reader as he/she assumes it is a valid case.


[...]

quoted

quoted

quoted

+			pins[i].pin = NULL;
+			return -ENOMEM;
+		}
+		if (cgu) {
+			ret = dpll_pin_register(pf->dplls.eec.dpll,
+						pins[i].pin,
+						ops, pf, NULL);
+			if (ret)
+				return ret;
+			ret = dpll_pin_register(pf->dplls.pps.dpll,
+						pins[i].pin,
+						ops, pf, NULL);
+			if (ret)
+				return ret;

You have to call dpll_pin_unregister(pf->dplls.eec.dpll, pins[i].pin, ..)
here.

No, in case of error, the caller releases everything ice_dpll_release_all(..).

How does ice_dpll_release_all() where you failed? If you need to
unregister one or both or none? I know that in ice you have odd ways to
handle error paths in general, but this one clearly seems to be broken.

quoted

quoted

quoted

+		}
+	}
+	if (cgu) {
+		ops = &ice_dpll_output_ops;
+		pins = pf->dplls.outputs;
+		for (i = 0; i < pf->dplls.num_outputs; i++) {
+			pins[i].pin = dpll_pin_get(pf->dplls.clock_id,
+						   i + pf->dplls.num_inputs,
+						   THIS_MODULE, &pins[i].prop);
+			if (IS_ERR_OR_NULL(pins[i].pin)) {
+				pins[i].pin = NULL;
+				return -ENOMEM;

Don't make up error values when you get them from the function you call:
	return PTR_ERR(pins[i].pin);

Fixed.

quoted

quoted

+			}
+			ret = dpll_pin_register(pf->dplls.eec.dpll, pins[i].pin,
+						ops, pf, NULL);
+			if (ret)
+				return ret;
+			ret = dpll_pin_register(pf->dplls.pps.dpll, pins[i].pin,
+						ops, pf, NULL);
+			if (ret)
+				return ret;

You have to call dpll_pin_unregister(pf->dplls.eec.dpll, pins[i].pin, ..)
here.

As above, in case of error, the caller releases everything.

As above, I don't think it works.


[...]

quoted

quoted

quoted

+	}
+
+	if (cgu) {
+		ret = dpll_device_register(pf->dplls.eec.dpll, DPLL_TYPE_EEC,
+					   &ice_dpll_ops, pf, dev);
+		if (ret)
+			goto put_pps;
+		ret = dpll_device_register(pf->dplls.pps.dpll, DPLL_TYPE_PPS,
+					   &ice_dpll_ops, pf, dev);
+		if (ret)

You are missing call to dpll_device_unregister(pf->dplls.eec.dpll,
DPLL_TYPE_EEC here. Fix the error path.

The caller shall do the clean up, but yeah will fix this as here clean up
is not expected.

:) Just make your error paths obvious and easy to follow to not to
confuse anybody, you included.

I agree with Jiri. The error paths here and in ice_dpll_init_info() are
quite confusing and IMHO error prone.

It will get more easy toread and more consistent if every
initialization function does return an error code would leave the state
clean in case of error. That is, in case of error, such function should
cleanup all the partially allocated/initialized resources.

Note that in ice_dpll_init_info() the situation is more mixed-up as
ice_dpll_release_info() is called on most error paths, except the last
one. Memory should not leaked due to later ice_dpll_release_all(), but
it's really confusing.

Cheers,

Paolo