Date: Fri, 19 May 2023 08:47:42 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Jiri Pirko <jiri@...nulli.us>, "Kubalewski, Arkadiusz"
	 <arkadiusz.kubalewski@...el.com>
Cc: Vadim Fedorenko <vadfed@...a.com>, Jakub Kicinski <kuba@...nel.org>, 
 Jonathan Lemon <jonathan.lemon@...il.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

On Tue, 2023-05-16 at 08:26 +0200, Jiri Pirko wrote:
> 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.
> 
> 
> [...]
> 
> 
> > > > +/**
> > > > + * 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.
> 
> 
> > > > +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.
> 
> 
> [...]
> 
> 
> > > 
> > > 
> > > > +			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.
> 
> 
> 
> 
> 
> > 
> > > 
> > > > +		}
> > > > +	}
> > > > +	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.
> 
> 
> [...]
> 
> 
> > > > +	}
> > > > +
> > > > +	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

Powered by blists - more mailing lists