Re: [PATCH net-next v7 4/9] devlink: Allow for devlink-rate nodes parent reassignment
From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Date: 2022-10-27 22:34:25
From: Michal Wilczynski <redacted> Date: Thu, 27 Oct 2022 15:00:44 +0200 [...]
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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