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

>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Monday, August 7, 2023 9:08 AM
>
>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!

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

Powered by blists - more mailing lists