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: <DM6PR11MB4657F542DD71F61FD2A1C20B9B7F9@DM6PR11MB4657.namprd11.prod.outlook.com> Date: Thu, 18 May 2023 16:06:03 +0000 From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com> To: Jiri Pirko <jiri@...nulli.us> CC: Vadim Fedorenko <vadfed@...a.com>, 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> Subject: RE: [RFC PATCH v7 5/8] ice: implement dpll interface to control cgu >From: Jiri Pirko <jiri@...nulli.us> >Sent: Tuesday, May 16, 2023 8:26 AM > >Tue, May 16, 2023 at 12:07:57AM CEST, arkadiusz.kubalewski@...el.com wrote: >>>From: Jiri Pirko <jiri@...nulli.us> >>>Sent: Wednesday, May 3, 2023 2:19 PM >>> >>>Fri, Apr 28, 2023 at 02:20:06AM CEST, vadfed@...a.com wrote: >>>>From: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com> > >[...] > > >>>>+ * ice_dpll_frequency_set - wrapper for pin callback for set frequency >>>>+ * @pin: pointer to a pin >>>>+ * @pin_priv: private data pointer passed on pin registration >>>>+ * @dpll: pointer to dpll >>>>+ * @frequency: frequency to be set >>>>+ * @extack: error reporting >>>>+ * @pin_type: type of pin being configured >>>>+ * >>>>+ * Wraps internal set frequency command on a pin. >>>>+ * >>>>+ * Return: >>>>+ * * 0 - success >>>>+ * * negative - error pin not found or couldn't set in hw */ static >>>>+int ice_dpll_frequency_set(const struct dpll_pin *pin, void *pin_priv, >>>>+ const struct dpll_device *dpll, >>>>+ const u32 frequency, >>>>+ struct netlink_ext_ack *extack, >>>>+ const enum ice_dpll_pin_type pin_type) { >>>>+ struct ice_pf *pf = pin_priv; >>>>+ struct ice_dpll_pin *p; >>>>+ int ret = -EINVAL; >>>>+ >>>>+ if (!pf) >>>>+ return ret; >>>>+ if (ice_dpll_cb_lock(pf)) >>>>+ return -EBUSY; >>>>+ p = ice_find_pin(pf, pin, pin_type); >>> >>>This does not make any sense to me. You should avoid the lookups and remove >>>ice_find_pin() function entirely. The purpose of having pin_priv is to >>>carry the struct ice_dpll_pin * directly. You should pass it down during >>>pin register. >>> >>>pf pointer is stored in dpll_priv. >>> >> >>In this case dpll_priv is not passed, so cannot use it. > >It should be passed. In general to every op where *dpll is passed, the >dpll_priv pointer should be passed along. Please, fix this. > > >>But in general it makes sense I will hold pf inside of ice_dpll_pin >>and fix this. > >Nope, just use dpll_priv. That's why we have it. > Ok, will propagate it. > >[...] > > >>>>+/** >>>>+ * ice_dpll_pin_state_set - set pin's state on dpll >>>>+ * @dpll: dpll being configured >>>>+ * @pin: pointer to a pin >>>>+ * @pin_priv: private data pointer passed on pin registration >>>>+ * @state: state of pin to be set >>>>+ * @extack: error reporting >>>>+ * @pin_type: type of a pin >>>>+ * >>>>+ * Set pin state on a pin. >>>>+ * >>>>+ * Return: >>>>+ * * 0 - OK or no change required >>>>+ * * negative - error >>>>+ */ >>>>+static int >>>>+ice_dpll_pin_state_set(const struct dpll_device *dpll, >>>>+ const struct dpll_pin *pin, void *pin_priv, >>>>+ const enum dpll_pin_state state, >>> >>>Why you use const with enums? >>> >> >>Just show usage intention explicitly. > >Does not make any sense what so ever. Please avoid it. > Fixed. > >>>>+static int ice_dpll_rclk_state_on_pin_get(const struct dpll_pin *pin, >>>>+ void *pin_priv, >>>>+ const struct dpll_pin *parent_pin, >>>>+ enum dpll_pin_state *state, >>>>+ struct netlink_ext_ack *extack) { >>>>+ struct ice_pf *pf = pin_priv; >>>>+ u32 parent_idx, hw_idx = ICE_DPLL_PIN_IDX_INVALID, i; >>> >>>Reverse christmas tree ordering please. >> >>Fixed. >> >>> >>> >>>>+ struct ice_dpll_pin *p; >>>>+ int ret = -EFAULT; >>>>+ >>>>+ if (!pf) >>> >>>How exacly this can happen. My wild guess is it can't. Don't do such >>>pointless checks please, confuses the reader. >>> >> >>>From driver perspective the pf pointer value is given by external entity, >>why shouldn't it be valdiated? > >What? You pass it during register, you get it back here. Nothing to >check. Please drop it. Non-sense checks like this have no place in >kernel, they only confuse reader as he/she assumes it is a valid case. > I have some background from Automotive, MISRA etc. Removed. > >[...] > > >>> >>> >>>>+ pins[i].pin = NULL; >>>>+ return -ENOMEM; >>>>+ } >>>>+ if (cgu) { >>>>+ ret = dpll_pin_register(pf->dplls.eec.dpll, >>>>+ pins[i].pin, >>>>+ ops, pf, NULL); >>>>+ if (ret) >>>>+ return ret; >>>>+ ret = dpll_pin_register(pf->dplls.pps.dpll, >>>>+ pins[i].pin, >>>>+ ops, pf, NULL); >>>>+ if (ret) >>>>+ return ret; >>> >>>You have to call dpll_pin_unregister(pf->dplls.eec.dpll, pins[i].pin, ..) >>>here. >>> >> >>No, in case of error, the caller releases everything >ice_dpll_release_all(..). > > >How does ice_dpll_release_all() where you failed? If you need to >unregister one or both or none? I know that in ice you have odd ways to >handle error paths in general, but this one clearly seems to be broken. > It doesn't have to, as release all would release all anyway. Leaving it for now. > > > > >> >>> >>>>+ } >>>>+ } >>>>+ if (cgu) { >>>>+ ops = &ice_dpll_output_ops; >>>>+ pins = pf->dplls.outputs; >>>>+ for (i = 0; i < pf->dplls.num_outputs; i++) { >>>>+ pins[i].pin = dpll_pin_get(pf->dplls.clock_id, >>>>+ i + pf->dplls.num_inputs, >>>>+ THIS_MODULE, &pins[i].prop); >>>>+ if (IS_ERR_OR_NULL(pins[i].pin)) { >>>>+ pins[i].pin = NULL; >>>>+ return -ENOMEM; >>> >>>Don't make up error values when you get them from the function you call: >>> return PTR_ERR(pins[i].pin); >> >>Fixed. >> >>> >>>>+ } >>>>+ ret = dpll_pin_register(pf->dplls.eec.dpll, pins[i].pin, >>>>+ ops, pf, NULL); >>>>+ if (ret) >>>>+ return ret; >>>>+ ret = dpll_pin_register(pf->dplls.pps.dpll, pins[i].pin, >>>>+ ops, pf, NULL); >>>>+ if (ret) >>>>+ return ret; >>> >>>You have to call dpll_pin_unregister(pf->dplls.eec.dpll, pins[i].pin, ..) >>>here. >>> >> >>As above, in case of error, the caller releases everything. > >As above, I don't think it works. > It works, though not all error paths were probably test.. Will take another look on it later. > >[...] > > >>>>+ } >>>>+ >>>>+ if (cgu) { >>>>+ ret = dpll_device_register(pf->dplls.eec.dpll, DPLL_TYPE_EEC, >>>>+ &ice_dpll_ops, pf, dev); >>>>+ if (ret) >>>>+ goto put_pps; >>>>+ ret = dpll_device_register(pf->dplls.pps.dpll, DPLL_TYPE_PPS, >>>>+ &ice_dpll_ops, pf, dev); >>>>+ if (ret) >>> >>>You are missing call to dpll_device_unregister(pf->dplls.eec.dpll, >>>DPLL_TYPE_EEC here. Fix the error path. >>> >> >>The caller shall do the clean up, but yeah will fix this as here clean up >>is not expected. > >:) Just make your error paths obvious and easy to follow to not to >confuse anybody, you included. > I am not confused :) it wasn't removed as it should. Thank you! Arkadiusz > >> >>> >>>>+ goto put_pps; >>>>+ } >>>>+ >>>>+ return 0; >>>>+ >>>>+put_pps: >>>>+ dpll_device_put(pf->dplls.pps.dpll); >>>>+ pf->dplls.pps.dpll = NULL; >>>>+put_eec: >>>>+ dpll_device_put(pf->dplls.eec.dpll); >>>>+ pf->dplls.eec.dpll = NULL; >>>>+ >>>>+ return ret; >>>>+} > >[...]
Powered by blists - more mailing lists