On 10/28/2022 12:34 AM, Przemek Kitszel wrote:

From:   Michal Wilczynski <redacted>
Date:   Thu, 27 Oct 2022 15:00:44 +0200

[...]

quoted

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.

In my mind it made sense, cause we're handling an additional case here that
wasn't handled before. I think I'll just remove the comment then if you
find it misleading.

quoted

  		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.

They would duplicate in my mind, so I'll just remove the upper one.

quoted

+		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.

I'm not sure I follow you there, those are function pointers, and are 
supposed
to be implemented in the driver.

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