Message-ID: <DM6PR11MB4657F542DD71F61FD2A1C20B9B7F9@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Thu, 18 May 2023 16:06:03 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Jiri Pirko <jiri@...nulli.us>
CC: Vadim Fedorenko <vadfed@...a.com>, Jakub Kicinski <kuba@...nel.org>,
	Jonathan Lemon <jonathan.lemon@...il.com>, Paolo Abeni <pabeni@...hat.com>,
	"Olech, Milena" <milena.olech@...el.com>, "Michalik, Michal"
	<michal.michalik@...el.com>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, poros <poros@...hat.com>, mschmidt
	<mschmidt@...hat.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>
Subject: RE: [RFC PATCH v7 5/8] ice: implement dpll interface to control cgu

>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Tuesday, May 16, 2023 8:26 AM
>
>Tue, May 16, 2023 at 12:07:57AM CEST, arkadiusz.kubalewski@...el.com wrote:
>>>From: Jiri Pirko <jiri@...nulli.us>
>>>Sent: Wednesday, May 3, 2023 2:19 PM
>>>
>>>Fri, Apr 28, 2023 at 02:20:06AM CEST, vadfed@...a.com wrote:
>>>>From: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>
>[...]
>
>
>>>>+ * 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.
>
>
>>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.
>

Ok, will propagate it.

>
>[...]
>
>
>>>>+/**
>>>>+ * 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.
>

Fixed.

>
>>>>+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.
>>
>>>
>>>
>>>>+	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.
>

I have some background from Automotive, MISRA etc.
Removed.

>
>[...]
>
>
>>>
>>>
>>>>+			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.
>

It doesn't have to, as release all would release all anyway.
Leaving it for now.

>
>
>
>
>>
>>>
>>>>+		}
>>>>+	}
>>>>+	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.
>>
>>>
>>>>+			}
>>>>+			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.
>

It works, though not all error paths were probably test..
Will take another look on it later.

>
>[...]
>
>
>>>>+	}
>>>>+
>>>>+	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 am not confused :) it wasn't removed as it should.

Thank you!
Arkadiusz

>
>>
>>>
>>>>+			goto put_pps;
>>>>+	}
>>>>+
>>>>+	return 0;
>>>>+
>>>>+put_pps:
>>>>+	dpll_device_put(pf->dplls.pps.dpll);
>>>>+	pf->dplls.pps.dpll = NULL;
>>>>+put_eec:
>>>>+	dpll_device_put(pf->dplls.eec.dpll);
>>>>+	pf->dplls.eec.dpll = NULL;
>>>>+
>>>>+	return ret;
>>>>+}
>
>[...]

Powered by blists - more mailing lists