Message-ID: <ZNCYPVX7p9Fe/lPY@nanopsycho>
Date: Mon, 7 Aug 2023 09:07:41 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Cc: Jakub Kicinski <kuba@...nel.org>,
	Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
	Jonathan Lemon <jonathan.lemon@...il.com>,
	Paolo Abeni <pabeni@...hat.com>,
	Milena Olech <milena.olech@...el.com>,
	Michal Michalik <michal.michalik@...el.com>,
	linux-arm-kernel@...ts.infradead.org, poros@...hat.com,
	mschmidt@...hat.com, netdev@...r.kernel.org,
	linux-clk@...r.kernel.org, Bart Van Assche <bvanassche@....org>,
	intel-wired-lan@...ts.osuosl.org
Subject: Re: [PATCH net-next v2 7/9] ice: implement dpll interface to control
 cgu

Fri, Aug 04, 2023 at 09:04:52PM CEST, vadim.fedorenko@...ux.dev wrote:
>From: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.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!


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


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


>+		ice_dpll_deinit(pf);
> }
> 
> static void ice_init_wakeup(struct ice_pf *pf)
>-- 
>2.27.0
>

Powered by blists - more mailing lists