[RFC PATCH v7 5/8] ice: implement dpll interface to control cgu
Kubalewski, Arkadiusz
arkadiusz.kubalewski at intel.com
Thu May 25 02:05:42 PDT 2023
>From: Paolo Abeni <pabeni at redhat.com>
>Sent: Friday, May 19, 2023 8:48 AM
>
>On Tue, 2023-05-16 at 08:26 +0200, Jiri Pirko wrote:
>> Tue, May 16, 2023 at 12:07:57AM CEST, arkadiusz.kubalewski at intel.com
>> > > wrote:
>> > > From: Jiri Pirko <jiri at resnulli.us>
>> > > Sent: Wednesday, May 3, 2023 2:19 PM
>> > >
>> > > Fri, Apr 28, 2023 at 02:20:06AM CEST, vadfed at meta.com wrote:
>> > > > From: Arkadiusz Kubalewski <arkadiusz.kubalewski at intel.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
Fixed.
Thank you,
Arkadiusz
More information about the linux-arm-kernel
mailing list