RE: [PATCH net-next v2 7/9] ice: implement dpll interface to control cgu

From: Jiri Pirko <jiri@resnulli.us>
Sent: Monday, August 7, 2023 9:08 AM

Fri, Aug 04, 2023 at 09:04:52PM CEST, vadim.fedorenko@linux.dev wrote:

quoted

From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

[...]

quoted

+/**
+ * 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;
+}
+

[...]

quoted

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

quoted

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

quoted

+	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);
+}

[...]

quoted

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c

b/drivers/net/ethernet/intel/ice/ice_main.c

quoted

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

quoted

+		ice_dpll_deinit(pf);
}

static void ice_init_wakeup(struct ice_pf *pf)
--
2.27.0