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
| ||
|
Message-ID: <DM6PR11MB4657B0935B869AFCB3A87BBB9B0CA@DM6PR11MB4657.namprd11.prod.outlook.com> 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