| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
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