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: <DM6PR11MB46573D01E85D027150761CC99BCBA@DM6PR11MB4657.namprd11.prod.outlook.com> Date: Wed, 4 Oct 2023 09:13:23 +0000 From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com> To: Jiri Pirko <jiri@...nulli.us> CC: Vadim Fedorenko <vadim.fedorenko@...ux.dev>, "corbet@....net" <corbet@....net>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>, "Brandeburg, Jesse" <jesse.brandeburg@...el.com>, "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>, "kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>, "davem@...emloft.net" <davem@...emloft.net> Subject: RE: [Intel-wired-lan] [PATCH net-next 3/4] dpll: netlink/core: add support for pin-dpll signal phase offset/adjust >From: Jiri Pirko <jiri@...nulli.us> >Sent: Tuesday, October 3, 2023 7:19 PM > >Tue, Oct 03, 2023 at 04:29:13PM CEST, arkadiusz.kubalewski@...el.com wrote: >>>From: Jiri Pirko <jiri@...nulli.us> >>>Sent: Tuesday, October 3, 2023 8:27 AM >>>To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@...el.com> >>> >>>Tue, Oct 03, 2023 at 01:10:39AM CEST, arkadiusz.kubalewski@...el.com >>>wrote: >>>>>From: Intel-wired-lan <intel-wired-lan-bounces@...osl.org> On Behalf Of >>>>>Vadim Fedorenko >>>>>Sent: Monday, October 2, 2023 5:09 PM >>>>> >>>>>On 02/10/2023 16:04, Jiri Pirko wrote: >>>>>> Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalewski@...el.com >>>>>> wrote: >>>>>>>> From: Vadim Fedorenko <vadim.fedorenko@...ux.dev> >>>>>>>> Sent: Wednesday, September 27, 2023 8:09 PM >>>>>>>> >>>>>>>> On 27/09/2023 10:24, Arkadiusz Kubalewski wrote: >>>>>>>>> Add callback op (get) for pin-dpll phase-offset measurment. >>>>>>>>> Add callback ops (get/set) for pin signal phase adjustment. >>>>>>>>> Add min and max phase adjustment values to pin proprties. >>>>>>>>> Invoke get callbacks when filling up the pin details to provide user >>>>>>>>> with phase related attribute values. >>>>>>>>> Invoke phase-adjust set callback when phase-adjust value is provided >>>>>>>>> for >>>>>>>>> pin-set request. >>>>>>>>> >>>>>>>>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com> >>>>>>>> >>>>>>>> [...] >>>>>>>> >>>>>>>>> +static int >>>>>>>>> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr >>>>>>>>> *phase_adj_attr, >>>>>>>>> + struct netlink_ext_ack *extack) >>>>>>>>> +{ >>>>>>>>> + struct dpll_pin_ref *ref; >>>>>>>>> + unsigned long i; >>>>>>>>> + s32 phase_adj; >>>>>>>>> + int ret; >>>>>>>>> + >>>>>>>>> + phase_adj = nla_get_s32(phase_adj_attr); >>>>>>>>> + if (phase_adj > pin->prop->phase_range.max || >>>>>>>>> + phase_adj < pin->prop->phase_range.min) { >>>>>>>>> + NL_SET_ERR_MSG(extack, "phase adjust value not >>>>>>>>> supported"); >>>>>>>>> + return -EINVAL; >>>>>>>>> + } >>>>>>>>> + xa_for_each(&pin->dpll_refs, i, ref) { >>>>>>>>> + const struct dpll_pin_ops *ops = dpll_pin_ops(ref); >>>>>>>>> + struct dpll_device *dpll = ref->dpll; >>>>>>>>> + >>>>>>>>> + if (!ops->phase_adjust_set) >>>>>>>>> + return -EOPNOTSUPP; >>>>>>>> >>>>>>>> I'm thinking about this part. We can potentially have dpll devices >>>>>>>> with >>>>>>>> different expectations on phase adjustments, right? And if one of >>>>>>>> them >>>>>>>> won't be able to adjust phase (or will fail in the next line), then >>>>>>>> netlink will return EOPNOTSUPP while _some_ of the devices will be >>>>>>>> adjusted. Doesn't look great. Can we think about different way to >>>>>>>> apply >>>>>>>> the change? >>>>>>>> >>>>>>> >>>>>>> Well makes sense to me. >>>>>>> >>>>>>> Does following makes sense as a fix? >>>>>>> We would call op for all devices which has been provided with the op. >>>>>>> If device has no op -> add extack error, continue >>>>>> >>>>>> Is it real to expect some of the device support this and others don't? >>>>>> Is it true for ice? >>>>>> If not, I would got for all-or-nothing here. >>>>>> >>>>> >>>>>But nothing blocks vendors to provide such configuration. Should we >>>>>rollback the configuration? Otherwise we can easily make it >>>>>inconsistent. >>>> >>>>Good point, in such case rollback might be required. >>>> >>>>> >>>>>I'm more thinking of checking if all the devices returned error (or >>>>>absence of operation callback) and then return error instead of 0 with >>>>>extack filled in. >>>>> >>>> >>>>Well, what if different devices would return different errors? >>>>In general we would have to keep track of the error values returned in >>>>such case.. Assuming one is different than the other - still need to >>>>error >>>>extack them out? I guess it would be easier to return common error if >>>there >>> >>>In this case, it is common to return the first error hit and bail out, >>>not trying the rest. >>> >> >>OK, so now I see it like this: >>-> check if all device implement callback, if not return EOPNOTSUPP; >>-> get old phase_adjust >>-> if new == old, return EINVAL > >0 would be better, no? User has what he desired. > Yes, that makes sense. > >>-> for each device: call phase_adjust_set, if fails, rollback all previous >> successful attempts and return the failure code > >That would work. > Great, just sent v2. Thanks! Arkadiusz > >>? >> >>Thank you! >>Arkadiusz >> >>> >>>>were only failures and let the driver fill the errors on extack, smt >>>>like: >>>> >>>> int miss_cb_num = 0, dev_num = 0, err_num; >>>> >>>> xa_for_each(&pin->dpll_refs, i, ref) { >>>> const struct dpll_pin_ops *ops = dpll_pin_ops(ref); >>>> struct dpll_device *dpll = ref->dpll; >>>> >>>> dev_num++; >>>> if (!ops->phase_adjust_set) { >>>> miss_cb_num++; >>>> continue; >>>> } >>>> ret = ops->phase_adjust_set(pin, >>>> dpll_pin_on_dpll_priv(dpll, pin), >>>> dpll, dpll_priv(dpll), phase_adj, >>>> extack); >>>> if (ret) >>>> err_num++; >>>> } >>>> if (dev_num == miss_cb_num) >>>> return -EOPNOTSUPP; >>>> if (dev_num == err_num) >>>> return -EINVAL; >>>> __dpll_pin_change_ntf(pin); >>>> return 0; >>>> >>>>?? >>>> >>>>Thank you! >>>>Arkadiusz >>>> >>>>>> >>>>>>> If device fails to set -> add extack error, continue >>>>>>> Function always returns 0. >>>>>>> >>>>>>> Thank you! >>>>>>> Arkadiusz >>>>>>> >>>>>>>> >>>>>>>>> + ret = ops->phase_adjust_set(pin, >>>>>>>>> + dpll_pin_on_dpll_priv(dpll, pin), >>>>>>>>> + dpll, dpll_priv(dpll), phase_adj, >>>>>>>>> + extack); >>>>>>>>> + if (ret) >>>>>>>>> + return ret; >>>>>>>>> + } >>>>>>>>> + __dpll_pin_change_ntf(pin); >>>>>>>>> + >>>>>>>>> + return 0; >>>>>>>>> +} >>>>>>>>> + >>>>> >>>>>_______________________________________________ >>>>>Intel-wired-lan mailing list >>>>>Intel-wired-lan@...osl.org >>>>>https://lists.osuosl.org/mailman/listinfo/intel-wired-lan >>
Powered by blists - more mailing lists