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: <20221027223414.11627-1-przemyslaw.kitszel@intel.com> Date: Fri, 28 Oct 2022 00:34:14 +0200 From: Przemek Kitszel <przemyslaw.kitszel@...el.com> To: netdev@...r.kernel.org Cc: Przemek Kitszel <przemyslaw.kitszel@...el.com>, alexandr.lobakin@...el.com, jacob.e.keller@...el.com, jesse.brandeburg@...el.com, anthony.l.nguyen@...el.com, kuba@...nel.org, ecree.xilinx@...il.com, jiri@...nulli.us, Michal Wilczynski <michal.wilczynski@...el.com> Subject: Re: [PATCH net-next v7 4/9] devlink: Allow for devlink-rate nodes parent reassignment From: Michal Wilczynski <michal.wilczynski@...el.com> Date: Thu, 27 Oct 2022 15:00:44 +0200 [...] > diff --git a/net/core/devlink.c b/net/core/devlink.c > index 864fa0967b7a..1e0c1b0376bf 100644 > --- a/net/core/devlink.c > +++ b/net/core/devlink.c > @@ -1875,10 +1875,9 @@ devlink_nl_rate_parent_node_set(struct devlink_rate *devlink_rate, > int err = -EOPNOTSUPP; > > parent = devlink_rate->parent; > - if (parent && len) { > - NL_SET_ERR_MSG_MOD(info->extack, "Rate object already has parent."); > - return -EBUSY; > - } else if (parent && !len) { > + > + /* if a parent is already set, just reassign the parent */ > + if (parent && !len) { Comment that you have added should be placed way below, here it is misleading. > if (devlink_rate_is_leaf(devlink_rate)) > err = ops->rate_leaf_parent_set(devlink_rate, NULL, > devlink_rate->priv, NULL, > @@ -1892,7 +1891,7 @@ devlink_nl_rate_parent_node_set(struct devlink_rate *devlink_rate, > > refcount_dec(&parent->refcnt); > devlink_rate->parent = NULL; > - } else if (!parent && len) { > + } else if (len) { > parent = devlink_rate_node_get_by_name(devlink, parent_name); > if (IS_ERR(parent)) > return -ENODEV; > @@ -1919,6 +1918,10 @@ devlink_nl_rate_parent_node_set(struct devlink_rate *devlink_rate, > if (err) > return err; > Comment above makes more sense here, likely combined with the one just below. > + if (devlink_rate->parent) > + /* we're reassigning to other parent in this case */ > + refcount_dec(&devlink_rate->parent->refcnt); > + > refcount_inc(&parent->refcnt); > devlink_rate->parent = parent; > } > -- Thanks for splitting this patch out of the other, change itself is easier to follow now. Code (modulo comments) is correct, you could add my Reviewed-by after comment fix. Side note: ops (rate_{leaf|node}_parent_set) lack documentation. There is also not much usage of them as of now, so maybe we could extend them to actually do refcount_inc + refcount_dec (if applicable) + set pointers. OTOH: As of now those are more of "on-event" callbacks, not "do-something", what is further confirmed in name (word "set" on the end, not begining). --PK
Powered by blists - more mailing lists