>From: Jiri Pirko <jiri at resnulli.us>
>Sent: Tuesday, May 16, 2023 8:26 AM
>
>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.
>
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;
>>>>+}
>
>[...]