[PATCH net-next v2 7/9] ice: implement dpll interface to control cgu
Kubalewski, Arkadiusz
arkadiusz.kubalewski at intel.com
Mon Aug 7 16:06:11 PDT 2023
>From: Jiri Pirko <jiri at resnulli.us>
>Sent: Monday, August 7, 2023 9:08 AM
>
>Fri, Aug 04, 2023 at 09:04:52PM CEST, vadim.fedorenko at linux.dev wrote:
>>From: Arkadiusz Kubalewski <arkadiusz.kubalewski at intel.com>
>
>[...]
>
>
>>+/**
>>+ * ice_dpll_deinit_worker - deinitialize dpll kworker
>>+ * @pf: board private structure
>>+ *
>>+ * Stop dpll's kworker, release it's resources.
>>+ */
>>+static void ice_dpll_deinit_worker(struct ice_pf *pf)
>>+{
>>+ struct ice_dplls *d = &pf->dplls;
>>+
>>+ kthread_cancel_delayed_work_sync(&d->work);
>>+ kthread_destroy_worker(d->kworker);
>>+}
>>+
>>+/**
>>+ * ice_dpll_init_worker - Initialize DPLLs periodic worker
>>+ * @pf: board private structure
>>+ *
>>+ * Create and start DPLLs periodic worker.
>>+ *
>>+ * Context: Shall be called after pf->dplls.lock is initialized and
>>+ * ICE_FLAG_DPLL flag is set.
>>+ * Return:
>>+ * * 0 - success
>>+ * * negative - create worker failure
>>+ */
>>+static int ice_dpll_init_worker(struct ice_pf *pf)
>>+{
>>+ struct ice_dplls *d = &pf->dplls;
>>+ struct kthread_worker *kworker;
>>+
>>+ ice_dpll_update_state(pf, &d->eec, true);
>>+ ice_dpll_update_state(pf, &d->pps, true);
>>+ kthread_init_delayed_work(&d->work, ice_dpll_periodic_work);
>>+ kworker = kthread_create_worker(0, "ice-dplls-%s",
>>+ dev_name(ice_pf_to_dev(pf)));
>>+ if (IS_ERR(kworker))
>>+ return PTR_ERR(kworker);
>>+ d->kworker = kworker;
>>+ d->cgu_state_acq_err_num = 0;
>>+ kthread_queue_delayed_work(d->kworker, &d->work, 0);
>>+
>>+ return 0;
>>+}
>>+
>
>[...]
>
>
>>+/**
>>+ * ice_dpll_deinit - Disable the driver/HW support for dpll subsystem
>>+ * the dpll device.
>>+ * @pf: board private structure
>>+ *
>>+ * Handles the cleanup work required after dpll initialization, freeing
>>+ * resources and unregistering the dpll, pin and all resources used for
>>+ * handling them.
>>+ *
>>+ * Context: Destroys pf->dplls.lock mutex.
>>+ */
>>+void ice_dpll_deinit(struct ice_pf *pf)
>>+{
>>+ bool cgu = ice_is_feature_supported(pf, ICE_F_CGU);
>>+
>>+ if (!test_bit(ICE_FLAG_DPLL, pf->flags))
>>+ return;
>>+ clear_bit(ICE_FLAG_DPLL, pf->flags);
>>+
>
>Please be symmetric with the init path and move ice_dpll_deinit_worker()
>call here.
>
>That would not only lead to nicer code, also, that will assure that the
>worker thread can only access initialized object.
>
>And as:
>1) worked thread can only access initialized objects
>2) dpll callbacks can only be called on initialized and registered objects
>
>You can remove the check for ICE_FLAG_DPLL flag from ice_dpll_cb_lock()
>as there would be no longer any possibility when this check could be
>evaluated as "true".
>
>Then, as an unexpected side effect (:O), ice_dpll_cb_lock() basically
>reduces to just calling mutex_lock(&pf->dplls.lock). So you can remove
>the thin wrappers of ice_dpll_cb_lock() and ice_dpll_cb_unlock() and
>instead of doing this obfuscation, you can call
>mutex_lock(&pf->dplls.lock) and mutex_unlock(&pf->dplls.lock) directly.
>
>That is what I'm trying to explain from the beginning. Is it clear now
>or do we need another iteration?
>
>Thanks!
Yeah, actually I removed ice_dpll_cb_lock() already, but this patch
was somehow lost when we were merging :s
Anyway, true will be fixed for v3.
>
>
>>+ ice_dpll_deinit_pins(pf, cgu);
>>+ ice_dpll_deinit_dpll(pf, &pf->dplls.pps, cgu);
>>+ ice_dpll_deinit_dpll(pf, &pf->dplls.eec, cgu);
>>+ ice_dpll_deinit_info(pf);
>>+ if (cgu)
>>+ ice_dpll_deinit_worker(pf);
>>+ mutex_destroy(&pf->dplls.lock);
>>+}
>>+
>>+/**
>>+ * ice_dpll_init - initialize support for dpll subsystem
>>+ * @pf: board private structure
>>+ *
>>+ * Set up the device dplls, register them and pins connected within Linux
>>dpll
>>+ * subsystem. Allow userspace to obtain state of DPLL and handling of
>>DPLL
>>+ * configuration requests.
>>+ *
>>+ * Context: Initializes pf->dplls.lock mutex.
>>+ */
>>+void ice_dpll_init(struct ice_pf *pf)
>>+{
>>+ bool cgu = ice_is_feature_supported(pf, ICE_F_CGU);
>>+ struct ice_dplls *d = &pf->dplls;
>>+ int err = 0;
>>+
>>+ err = ice_dpll_init_info(pf, cgu);
>>+ if (err)
>>+ goto err_exit;
>>+ err = ice_dpll_init_dpll(pf, &pf->dplls.eec, cgu, DPLL_TYPE_EEC);
>>+ if (err)
>>+ goto deinit_info;
>>+ err = ice_dpll_init_dpll(pf, &pf->dplls.pps, cgu, DPLL_TYPE_PPS);
>>+ if (err)
>>+ goto deinit_eec;
>>+ err = ice_dpll_init_pins(pf, cgu);
>>+ if (err)
>>+ goto deinit_pps;
>>+ mutex_init(&d->lock);
>>+ set_bit(ICE_FLAG_DPLL, pf->flags);
>
>Why can't you move the flag set to the end of this function and avoid
>calling clear_bi on the error path?
>
>If you can't, please fix the clear_bit() position (should be at the
>beginning of deinit_pins label section).
>
Yes, will be fixed in v3.
>
>>+ if (cgu) {
>>+ err = ice_dpll_init_worker(pf);
>>+ if (err)
>>+ goto deinit_pins;
>>+ }
>>+
>>+ return;
>>+
>>+deinit_pins:
>>+ ice_dpll_deinit_pins(pf, cgu);
>>+deinit_pps:
>>+ ice_dpll_deinit_dpll(pf, &pf->dplls.pps, cgu);
>>+deinit_eec:
>>+ ice_dpll_deinit_dpll(pf, &pf->dplls.eec, cgu);
>>+deinit_info:
>>+ ice_dpll_deinit_info(pf);
>>+err_exit:
>>+ clear_bit(ICE_FLAG_DPLL, pf->flags);
>>+ mutex_destroy(&d->lock);
>>+ dev_warn(ice_pf_to_dev(pf), "DPLLs init failure err:%d\n", err);
>>+}
>
>[...]
>
>
>>diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
>b/drivers/net/ethernet/intel/ice/ice_main.c
>>index 2e80d5cd9f56..4adc74f1cb1f 100644
>>--- a/drivers/net/ethernet/intel/ice/ice_main.c
>>+++ b/drivers/net/ethernet/intel/ice/ice_main.c
>>@@ -4627,6 +4627,10 @@ static void ice_init_features(struct ice_pf *pf)
>> if (ice_is_feature_supported(pf, ICE_F_GNSS))
>> ice_gnss_init(pf);
>>
>>+ if (ice_is_feature_supported(pf, ICE_F_CGU) ||
>>+ ice_is_feature_supported(pf, ICE_F_PHY_RCLK))
>>+ ice_dpll_init(pf);
>>+
>> /* Note: Flow director init failure is non-fatal to load */
>> if (ice_init_fdir(pf))
>> dev_err(dev, "could not initialize flow director\n");
>>@@ -4653,6 +4657,9 @@ static void ice_deinit_features(struct ice_pf *pf)
>> ice_gnss_exit(pf);
>> if (test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags))
>> ice_ptp_release(pf);
>>+ if (ice_is_feature_supported(pf, ICE_F_PHY_RCLK) ||
>>+ ice_is_feature_supported(pf, ICE_F_CGU))
>
>As you internally depend on ICE_FLAG_DPLL flag, this check is redundant.
>
Sure, will fix.
Thanks!
Arkadiusz
>
>>+ ice_dpll_deinit(pf);
>> }
>>
>> static void ice_init_wakeup(struct ice_pf *pf)
>>--
>>2.27.0
>>
More information about the linux-arm-kernel
mailing list