[PATCH v4 net-next 0/3] Knobs for NPC default rule counters

[PATCH v4 net-next 0/3] Knobs for NPC default rule counters

[Linu Cherian]

Changelog from v3:
Add documentation for the new devlink param as well as the existing
ones.

Changelog from v2:
Split the patch into 2 as suggested by Simon.

Changelog from v1:
Removed wrong mutex_unlock invocations.

V3 is posted here,
https://lore.kernel.org/netdev/20241017084244.1654907-1-lcherian@marvell.com/

Patch 1 introduce _rvu_mcam_remove/add_counter_from/to_rule
by refactoring existing code

Patch 2 adds a devlink param to enable/disable counters for default
rules. Once enabled, counters can 

Patch 3 adds documentation for devlink params


Linu Cherian (3):
  octeontx2-af: Refactor few NPC mcam APIs
  octeontx2-af: Knobs for NPC default rule counters
  devlink: Add documenation for OcteonTx2 AF

 .../networking/devlink/octeontx2.rst          |  16 +++
 .../net/ethernet/marvell/octeontx2/af/rvu.h   |   8 +-
 .../marvell/octeontx2/af/rvu_devlink.c        |  32 +++++
 .../ethernet/marvell/octeontx2/af/rvu_npc.c   | 134 ++++++++++++++++--
 .../marvell/octeontx2/af/rvu_npc_fs.c         |  36 ++---
 5 files changed, 187 insertions(+), 39 deletions(-)

-- 
2.34.1

[PATCH v4 net-next 1/3] octeontx2-af: Refactor few NPC mcam APIs

[Linu Cherian]

Introduce lowlevel variant of rvu_mcam_remove/add_counter_from/to_rule
for better code reuse, which assumes necessary locks are taken at
higher level.

These low level functions would be used for implementing default rule
counter APIs in the subsequent patch.

Signed-off-by: Linu Cherian <lcherian@marvell.com>
Reviewed-by: Simon Horman <horms@kernel.org>
---
Changelog from v3:
Added Reviewed-by tag.

 .../net/ethernet/marvell/octeontx2/af/rvu.h   |  6 +-
 .../ethernet/marvell/octeontx2/af/rvu_npc.c   | 89 ++++++++++++++++---
 .../marvell/octeontx2/af/rvu_npc_fs.c         | 36 ++------
 3 files changed, 92 insertions(+), 39 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
index 5016ba82e142..d92a5f47a476 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
@@ -960,7 +960,11 @@ void rvu_npc_disable_default_entries(struct rvu *rvu, u16 pcifunc, int nixlf);
 void rvu_npc_enable_default_entries(struct rvu *rvu, u16 pcifunc, int nixlf);
 void rvu_npc_update_flowkey_alg_idx(struct rvu *rvu, u16 pcifunc, int nixlf,
 				    int group, int alg_idx, int mcam_index);
-
+void __rvu_mcam_remove_counter_from_rule(struct rvu *rvu, u16 pcifunc,
+					 struct rvu_npc_mcam_rule *rule);
+void __rvu_mcam_add_counter_to_rule(struct rvu *rvu, u16 pcifunc,
+				    struct rvu_npc_mcam_rule *rule,
+				    struct npc_install_flow_rsp *rsp);
 void rvu_npc_get_mcam_entry_alloc_info(struct rvu *rvu, u16 pcifunc,
 				       int blkaddr, int *alloc_cnt,
 				       int *enable_cnt);
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
index 97722ce8c4cb..c4ef1e83cc46 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
@@ -2975,9 +2975,9 @@ int rvu_mbox_handler_npc_mcam_shift_entry(struct rvu *rvu,
 	return rc;
 }
 
-int rvu_mbox_handler_npc_mcam_alloc_counter(struct rvu *rvu,
-			struct npc_mcam_alloc_counter_req *req,
-			struct npc_mcam_alloc_counter_rsp *rsp)
+static int __npc_mcam_alloc_counter(struct rvu *rvu,
+				    struct npc_mcam_alloc_counter_req *req,
+				    struct npc_mcam_alloc_counter_rsp *rsp)
 {
 	struct npc_mcam *mcam = &rvu->hw->mcam;
 	u16 pcifunc = req->hdr.pcifunc;
@@ -2998,11 +2998,9 @@ int rvu_mbox_handler_npc_mcam_alloc_counter(struct rvu *rvu,
 	if (!req->contig && req->count > NPC_MAX_NONCONTIG_COUNTERS)
 		return NPC_MCAM_INVALID_REQ;
 
-	mutex_lock(&mcam->lock);
 
 	/* Check if unused counters are available or not */
 	if (!rvu_rsrc_free_count(&mcam->counters)) {
-		mutex_unlock(&mcam->lock);
 		return NPC_MCAM_ALLOC_FAILED;
 	}
 
@@ -3035,12 +3033,27 @@ int rvu_mbox_handler_npc_mcam_alloc_counter(struct rvu *rvu,
 		}
 	}
 
-	mutex_unlock(&mcam->lock);
 	return 0;
 }
 
-int rvu_mbox_handler_npc_mcam_free_counter(struct rvu *rvu,
-		struct npc_mcam_oper_counter_req *req, struct msg_rsp *rsp)
+int rvu_mbox_handler_npc_mcam_alloc_counter(struct rvu *rvu,
+			struct npc_mcam_alloc_counter_req *req,
+			struct npc_mcam_alloc_counter_rsp *rsp)
+{
+	struct npc_mcam *mcam = &rvu->hw->mcam;
+	int err;
+
+	mutex_lock(&mcam->lock);
+
+	err = __npc_mcam_alloc_counter(rvu, req, rsp);
+
+	mutex_unlock(&mcam->lock);
+	return err;
+}
+
+static int __npc_mcam_free_counter(struct rvu *rvu,
+				   struct npc_mcam_oper_counter_req *req,
+				   struct msg_rsp *rsp)
 {
 	struct npc_mcam *mcam = &rvu->hw->mcam;
 	u16 index, entry = 0;
@@ -3050,10 +3063,8 @@ int rvu_mbox_handler_npc_mcam_free_counter(struct rvu *rvu,
 	if (blkaddr < 0)
 		return NPC_MCAM_INVALID_REQ;
 
-	mutex_lock(&mcam->lock);
 	err = npc_mcam_verify_counter(mcam, req->hdr.pcifunc, req->cntr);
 	if (err) {
-		mutex_unlock(&mcam->lock);
 		return err;
 	}
 
@@ -3077,10 +3088,66 @@ int rvu_mbox_handler_npc_mcam_free_counter(struct rvu *rvu,
 					      index, req->cntr);
 	}
 
-	mutex_unlock(&mcam->lock);
 	return 0;
 }
 
+int rvu_mbox_handler_npc_mcam_free_counter(struct rvu *rvu,
+		struct npc_mcam_oper_counter_req *req, struct msg_rsp *rsp)
+{
+	struct npc_mcam *mcam = &rvu->hw->mcam;
+	int err;
+
+	mutex_lock(&mcam->lock);
+
+	err = __npc_mcam_free_counter(rvu, req, rsp);
+
+	mutex_unlock(&mcam->lock);
+
+	return err;
+}
+
+void __rvu_mcam_remove_counter_from_rule(struct rvu *rvu, u16 pcifunc,
+					 struct rvu_npc_mcam_rule *rule)
+{
+	struct npc_mcam_oper_counter_req free_req = { 0 };
+	struct msg_rsp free_rsp;
+
+	if (!rule->has_cntr)
+		return;
+
+	free_req.hdr.pcifunc = pcifunc;
+	free_req.cntr = rule->cntr;
+
+	__npc_mcam_free_counter(rvu, &free_req, &free_rsp);
+	rule->has_cntr = false;
+}
+
+void __rvu_mcam_add_counter_to_rule(struct rvu *rvu, u16 pcifunc,
+				    struct rvu_npc_mcam_rule *rule,
+				    struct npc_install_flow_rsp *rsp)
+{
+	struct npc_mcam_alloc_counter_req cntr_req = { 0 };
+	struct npc_mcam_alloc_counter_rsp cntr_rsp = { 0 };
+	int err;
+
+	cntr_req.hdr.pcifunc = pcifunc;
+	cntr_req.contig = true;
+	cntr_req.count = 1;
+
+	/* we try to allocate a counter to track the stats of this
+	 * rule. If counter could not be allocated then proceed
+	 * without counter because counters are limited than entries.
+	 */
+	err = __npc_mcam_alloc_counter(rvu, &cntr_req, &cntr_rsp);
+	if (!err && cntr_rsp.count) {
+		rule->cntr = cntr_rsp.cntr;
+		rule->has_cntr = true;
+		rsp->counter = rule->cntr;
+	} else {
+		rsp->counter = err;
+	}
+}
+
 int rvu_mbox_handler_npc_mcam_unmap_counter(struct rvu *rvu,
 		struct npc_mcam_unmap_counter_req *req, struct msg_rsp *rsp)
 {
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
index 150635de2bd5..7a1c18b1486d 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
@@ -1081,44 +1081,26 @@ static void rvu_mcam_add_rule(struct npc_mcam *mcam,
 static void rvu_mcam_remove_counter_from_rule(struct rvu *rvu, u16 pcifunc,
 					      struct rvu_npc_mcam_rule *rule)
 {
-	struct npc_mcam_oper_counter_req free_req = { 0 };
-	struct msg_rsp free_rsp;
+	struct npc_mcam *mcam = &rvu->hw->mcam;
 
-	if (!rule->has_cntr)
-		return;
+	mutex_lock(&mcam->lock);
 
-	free_req.hdr.pcifunc = pcifunc;
-	free_req.cntr = rule->cntr;
+	__rvu_mcam_remove_counter_from_rule(rvu, pcifunc, rule);
 
-	rvu_mbox_handler_npc_mcam_free_counter(rvu, &free_req, &free_rsp);
-	rule->has_cntr = false;
+	mutex_unlock(&mcam->lock);
 }
 
 static void rvu_mcam_add_counter_to_rule(struct rvu *rvu, u16 pcifunc,
 					 struct rvu_npc_mcam_rule *rule,
 					 struct npc_install_flow_rsp *rsp)
 {
-	struct npc_mcam_alloc_counter_req cntr_req = { 0 };
-	struct npc_mcam_alloc_counter_rsp cntr_rsp = { 0 };
-	int err;
+	struct npc_mcam *mcam = &rvu->hw->mcam;
 
-	cntr_req.hdr.pcifunc = pcifunc;
-	cntr_req.contig = true;
-	cntr_req.count = 1;
+	mutex_lock(&mcam->lock);
 
-	/* we try to allocate a counter to track the stats of this
-	 * rule. If counter could not be allocated then proceed
-	 * without counter because counters are limited than entries.
-	 */
-	err = rvu_mbox_handler_npc_mcam_alloc_counter(rvu, &cntr_req,
-						      &cntr_rsp);
-	if (!err && cntr_rsp.count) {
-		rule->cntr = cntr_rsp.cntr;
-		rule->has_cntr = true;
-		rsp->counter = rule->cntr;
-	} else {
-		rsp->counter = err;
-	}
+	__rvu_mcam_add_counter_to_rule(rvu, pcifunc, rule, rsp);
+
+	mutex_unlock(&mcam->lock);
 }
 
 static int npc_mcast_update_action_index(struct rvu *rvu, struct npc_install_flow_req *req,
-- 
2.34.1

[PATCH v4 net-next 2/3] octeontx2-af: Knobs for NPC default rule counters

[Linu Cherian]

Add devlink knobs to enable/disable counters on NPC
default rule entries.

Sample command to enable default rule counters:
devlink dev param set <dev> name npc_def_rule_cntr value true cmode runtime

Sample command to read the counter:
cat /sys/kernel/debug/cn10k/npc/mcam_rules

Signed-off-by: Linu Cherian <lcherian@marvell.com>
---
Changelog from v3:
No changes.

 .../net/ethernet/marvell/octeontx2/af/rvu.h   |  2 +
 .../marvell/octeontx2/af/rvu_devlink.c        | 32 +++++++++++++
 .../ethernet/marvell/octeontx2/af/rvu_npc.c   | 45 +++++++++++++++++++
 3 files changed, 79 insertions(+)

diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
index d92a5f47a476..e8c6a6fe9bd5 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
@@ -525,6 +525,7 @@ struct rvu {
 	struct mutex		alias_lock; /* Serialize bar2 alias access */
 	int			vfs; /* Number of VFs attached to RVU */
 	u16			vf_devid; /* VF devices id */
+	bool			def_rule_cntr_en;
 	int			nix_blkaddr[MAX_NIX_BLKS];
 
 	/* Mbox */
@@ -989,6 +990,7 @@ void npc_set_mcam_action(struct rvu *rvu, struct npc_mcam *mcam,
 void npc_read_mcam_entry(struct rvu *rvu, struct npc_mcam *mcam,
 			 int blkaddr, u16 src, struct mcam_entry *entry,
 			 u8 *intf, u8 *ena);
+int npc_config_cntr_default_entries(struct rvu *rvu, bool enable);
 bool is_cgx_config_permitted(struct rvu *rvu, u16 pcifunc);
 bool is_mac_feature_supported(struct rvu *rvu, int pf, int feature);
 u32  rvu_cgx_get_fifolen(struct rvu *rvu);
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_devlink.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_devlink.c
index 7498ab429963..9c26e19a860b 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_devlink.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_devlink.c
@@ -1238,6 +1238,7 @@ enum rvu_af_dl_param_id {
 	RVU_AF_DEVLINK_PARAM_ID_DWRR_MTU,
 	RVU_AF_DEVLINK_PARAM_ID_NPC_MCAM_ZONE_PERCENT,
 	RVU_AF_DEVLINK_PARAM_ID_NPC_EXACT_FEATURE_DISABLE,
+	RVU_AF_DEVLINK_PARAM_ID_NPC_DEF_RULE_CNTR_ENABLE,
 	RVU_AF_DEVLINK_PARAM_ID_NIX_MAXLF,
 };
 
@@ -1358,6 +1359,32 @@ static int rvu_af_dl_npc_mcam_high_zone_percent_validate(struct devlink *devlink
 	return 0;
 }
 
+static int rvu_af_dl_npc_def_rule_cntr_get(struct devlink *devlink, u32 id,
+					   struct devlink_param_gset_ctx *ctx)
+{
+	struct rvu_devlink *rvu_dl = devlink_priv(devlink);
+	struct rvu *rvu = rvu_dl->rvu;
+
+	ctx->val.vbool = rvu->def_rule_cntr_en;
+
+	return 0;
+}
+
+static int rvu_af_dl_npc_def_rule_cntr_set(struct devlink *devlink, u32 id,
+					   struct devlink_param_gset_ctx *ctx,
+					   struct netlink_ext_ack *extack)
+{
+	struct rvu_devlink *rvu_dl = devlink_priv(devlink);
+	struct rvu *rvu = rvu_dl->rvu;
+	int err;
+
+	err = npc_config_cntr_default_entries(rvu, ctx->val.vbool);
+	if (!err)
+		rvu->def_rule_cntr_en = ctx->val.vbool;
+
+	return err;
+}
+
 static int rvu_af_dl_nix_maxlf_get(struct devlink *devlink, u32 id,
 				   struct devlink_param_gset_ctx *ctx)
 {
@@ -1444,6 +1471,11 @@ static const struct devlink_param rvu_af_dl_params[] = {
 			     rvu_af_dl_npc_mcam_high_zone_percent_get,
 			     rvu_af_dl_npc_mcam_high_zone_percent_set,
 			     rvu_af_dl_npc_mcam_high_zone_percent_validate),
+	DEVLINK_PARAM_DRIVER(RVU_AF_DEVLINK_PARAM_ID_NPC_DEF_RULE_CNTR_ENABLE,
+			     "npc_def_rule_cntr", DEVLINK_PARAM_TYPE_BOOL,
+			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
+			     rvu_af_dl_npc_def_rule_cntr_get,
+			     rvu_af_dl_npc_def_rule_cntr_set, NULL),
 	DEVLINK_PARAM_DRIVER(RVU_AF_DEVLINK_PARAM_ID_NIX_MAXLF,
 			     "nix_maxlf", DEVLINK_PARAM_TYPE_U16,
 			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
index c4ef1e83cc46..9e39c3149a4f 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
@@ -2691,6 +2691,51 @@ void npc_mcam_rsrcs_reserve(struct rvu *rvu, int blkaddr, int entry_idx)
 	npc_mcam_set_bit(mcam, entry_idx);
 }
 
+int npc_config_cntr_default_entries(struct rvu *rvu, bool enable)
+{
+	struct npc_install_flow_rsp rsp = { 0 };
+	struct npc_mcam *mcam = &rvu->hw->mcam;
+	struct rvu_npc_mcam_rule *rule;
+	int blkaddr;
+
+	blkaddr = rvu_get_blkaddr(rvu, BLKTYPE_NPC, 0);
+	if (blkaddr < 0)
+		return -EINVAL;
+
+	mutex_lock(&mcam->lock);
+	list_for_each_entry(rule, &mcam->mcam_rules, list) {
+		if (!is_mcam_entry_enabled(rvu, mcam, blkaddr, rule->entry))
+			continue;
+		if (!rule->default_rule)
+			continue;
+		if (enable && !rule->has_cntr) { /* Alloc and map new counter */
+			__rvu_mcam_add_counter_to_rule(rvu, rule->owner,
+						       rule, &rsp);
+			if (rsp.counter < 0) {
+				dev_err(rvu->dev, "%s: Err to allocate cntr for default rule (err=%d)\n",
+					__func__, rsp.counter);
+				break;
+			}
+			npc_map_mcam_entry_and_cntr(rvu, mcam, blkaddr,
+						    rule->entry, rsp.counter);
+		}
+
+		if (enable && rule->has_cntr) /* Reset counter before use */ {
+			rvu_write64(rvu, blkaddr,
+				    NPC_AF_MATCH_STATX(rule->cntr), 0x0);
+			continue;
+		}
+
+		if (!enable && rule->has_cntr) /* Free and unmap counter */ {
+			__rvu_mcam_remove_counter_from_rule(rvu, rule->owner,
+							    rule);
+		}
+	}
+	mutex_unlock(&mcam->lock);
+
+	return 0;
+}
+
 int rvu_mbox_handler_npc_mcam_alloc_entry(struct rvu *rvu,
 					  struct npc_mcam_alloc_entry_req *req,
 					  struct npc_mcam_alloc_entry_rsp *rsp)
-- 
2.34.1

[PATCH v4 net-next 3/3] devlink: Add documenation for OcteonTx2 AF

[Linu Cherian]

Add documenation for the following devlink params
- npc_mcam_high_zone_percent
- npc_def_rule_cntr
- nix_maxlf

Signed-off-by: Linu Cherian <lcherian@marvell.com>
---
Changelog from v3:
Newly added patch

 Documentation/networking/devlink/octeontx2.rst | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/networking/devlink/octeontx2.rst b/Documentation/networking/devlink/octeontx2.rst
index d33a90dd44bf..7808ab361692 100644
--- a/Documentation/networking/devlink/octeontx2.rst
+++ b/Documentation/networking/devlink/octeontx2.rst
@@ -40,6 +40,21 @@ The ``octeontx2 AF`` driver implements the following driver-specific parameters.
      - runtime
      - Use to set the quantum which hardware uses for scheduling among transmit queues.
        Hardware uses weighted DWRR algorithm to schedule among all transmit queues.
+   * - ``npc_mcam_high_zone_percent``
+     - u8
+     - runtime
+     - Use to set the number of high priority zone entries in NPC MCAM that can be allocated
+       by a user, out of the three priority zone categories high, mid and low.
+   * - ``npc_def_rule_cntr``
+     - bool
+     - runtime
+     - Use to enable or disable hit counters for the default rules in NPC MCAM.
+   * - ``nix_maxlf``
+     - u16
+     - runtime
+     - Use to set the maximum number of LFs in NIX hardware block. This would be useful
+       to increase the availability of default resources allocated to enabled LFs like
+       MCAM entries for example.
 
 The ``octeontx2 PF`` driver implements the following driver-specific parameters.
 
-- 
2.34.1

Re: [PATCH v4 net-next 3/3] devlink: Add documenation for OcteonTx2 AF

[Simon Horman]

On Tue, Oct 29, 2024 at 09:27:39AM +0530, Linu Cherian wrote:
> Add documenation for the following devlink params
> - npc_mcam_high_zone_percent
> - npc_def_rule_cntr
> - nix_maxlf
> 
> Signed-off-by: Linu Cherian <lcherian@marvell.com>
> ---
> Changelog from v3:
> Newly added patch

Thanks for addressing this, much appreciated.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH v4 net-next 2/3] octeontx2-af: Knobs for NPC default rule counters

[Simon Horman]

On Tue, Oct 29, 2024 at 09:27:38AM +0530, Linu Cherian wrote:
> Add devlink knobs to enable/disable counters on NPC
> default rule entries.
> 
> Sample command to enable default rule counters:
> devlink dev param set <dev> name npc_def_rule_cntr value true cmode runtime
> 
> Sample command to read the counter:
> cat /sys/kernel/debug/cn10k/npc/mcam_rules
> 
> Signed-off-by: Linu Cherian <lcherian@marvell.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH v4 net-next 2/3] octeontx2-af: Knobs for NPC default rule counters

[Jakub Kicinski]

On Tue, 29 Oct 2024 09:27:38 +0530 Linu Cherian wrote:
> +	struct npc_install_flow_rsp rsp = { 0 };

@rsp is reused in the loop, either it doesn't have to be inited at all,
or it has to be inited before every use

> +	struct npc_mcam *mcam = &rvu->hw->mcam;
> +	struct rvu_npc_mcam_rule *rule;
> +	int blkaddr;
> +
> +	blkaddr = rvu_get_blkaddr(rvu, BLKTYPE_NPC, 0);
> +	if (blkaddr < 0)
> +		return -EINVAL;
> +
> +	mutex_lock(&mcam->lock);
> +	list_for_each_entry(rule, &mcam->mcam_rules, list) {
> +		if (!is_mcam_entry_enabled(rvu, mcam, blkaddr, rule->entry))
> +			continue;
> +		if (!rule->default_rule)
> +			continue;
> +		if (enable && !rule->has_cntr) { /* Alloc and map new counter */
> +			__rvu_mcam_add_counter_to_rule(rvu, rule->owner,
> +						       rule, &rsp);
> +			if (rsp.counter < 0) {
> +				dev_err(rvu->dev, "%s: Err to allocate cntr for default rule (err=%d)\n",
> +					__func__, rsp.counter);
> +				break;

shouldn't you "unwind" in this case? We'll leave the counter enabled
for some rules and disabled for others

> +			}
> +			npc_map_mcam_entry_and_cntr(rvu, mcam, blkaddr,
> +						    rule->entry, rsp.counter);
> +		}
> +
> +		if (enable && rule->has_cntr) /* Reset counter before use */ {
> +			rvu_write64(rvu, blkaddr,
> +				    NPC_AF_MATCH_STATX(rule->cntr), 0x0);
> +			continue;

so setting to enabled while already enabled resets the value?
If so that's neither documented, nor.. usual.

> +		}
> +
> +		if (!enable && rule->has_cntr) /* Free and unmap counter */ {
> +			__rvu_mcam_remove_counter_from_rule(rvu, rule->owner,
> +							    rule);
> +		}

unnecesary parenthesis

> +	}
-- 
pw-bot: cr

Re: [PATCH v4 net-next 3/3] devlink: Add documenation for OcteonTx2 AF

[Jakub Kicinski]

On Tue, 29 Oct 2024 09:27:39 +0530 Linu Cherian wrote:
> +   * - ``npc_def_rule_cntr``
> +     - bool
> +     - runtime
> +     - Use to enable or disable hit counters for the default rules in NPC MCAM.

How are those counters accessible? ethtool -S? debugfs ? it should be
documented here. Plus please add examples of what such rules cover.
"default rules in NPC MCAM" requires too much familiarity with the
device.

Re: [PATCH v4 net-next 2/3] octeontx2-af: Knobs for NPC default rule counters

[Linu Cherian]

Hi Jakub,

On 2024-11-04 at 01:23:10, Jakub Kicinski (kuba@kernel.org) wrote:
> On Tue, 29 Oct 2024 09:27:38 +0530 Linu Cherian wrote:
> > +	struct npc_install_flow_rsp rsp = { 0 };
> 
> @rsp is reused in the loop, either it doesn't have to be inited at all,
> or it has to be inited before every use

Ack. It doesnt have to be inited. Will remove.

> 
> > +	struct npc_mcam *mcam = &rvu->hw->mcam;
> > +	struct rvu_npc_mcam_rule *rule;
> > +	int blkaddr;
> > +
> > +	blkaddr = rvu_get_blkaddr(rvu, BLKTYPE_NPC, 0);
> > +	if (blkaddr < 0)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&mcam->lock);
> > +	list_for_each_entry(rule, &mcam->mcam_rules, list) {
> > +		if (!is_mcam_entry_enabled(rvu, mcam, blkaddr, rule->entry))
> > +			continue;
> > +		if (!rule->default_rule)
> > +			continue;
> > +		if (enable && !rule->has_cntr) { /* Alloc and map new counter */
> > +			__rvu_mcam_add_counter_to_rule(rvu, rule->owner,
> > +						       rule, &rsp);
> > +			if (rsp.counter < 0) {
> > +				dev_err(rvu->dev, "%s: Err to allocate cntr for default rule (err=%d)\n",
> > +					__func__, rsp.counter);
> > +				break;
> 
> shouldn't you "unwind" in this case? We'll leave the counter enabled
> for some rules and disabled for others


Wanted to keep a best effort approach here, will make it clear in the documentation.

> 
> > +			}
> > +			npc_map_mcam_entry_and_cntr(rvu, mcam, blkaddr,
> > +						    rule->entry, rsp.counter);
> > +		}
> > +
> > +		if (enable && rule->has_cntr) /* Reset counter before use */ {
> > +			rvu_write64(rvu, blkaddr,
> > +				    NPC_AF_MATCH_STATX(rule->cntr), 0x0);
> > +			continue;
> 
> so setting to enabled while already enabled resets the value?
> If so that's neither documented, nor.. usual.


Will move this code under,  if (enable && !rule->has_cntr) case.
Infact, all enablement happens only from devlink control, hence the
above if check is really not required and can be confusing. 

> 
> > +		}
> > +
> > +		if (!enable && rule->has_cntr) /* Free and unmap counter */ {
> > +			__rvu_mcam_remove_counter_from_rule(rvu, rule->owner,
> > +							    rule);
> > +		}
> 
> unnecesary parenthesis

Ack.


Thanks,
Linu Cherian.

Re: [PATCH v4 net-next 3/3] devlink: Add documenation for OcteonTx2 AF

[Linu Cherian]

Hi Jakub,

On 2024-11-04 at 01:25:48, Jakub Kicinski (kuba@kernel.org) wrote:
> On Tue, 29 Oct 2024 09:27:39 +0530 Linu Cherian wrote:
> > +   * - ``npc_def_rule_cntr``
> > +     - bool
> > +     - runtime
> > +     - Use to enable or disable hit counters for the default rules in NPC MCAM.
> 
> How are those counters accessible? ethtool -S? debugfs ? it should be
> documented here. Plus please add examples of what such rules cover.
> "default rules in NPC MCAM" requires too much familiarity with the
> device.

Ack. Will address this.