[PATCH] octeontx2-pf: Add error handling for cn10k_map_unmap_rq_policer().

[PATCH] octeontx2-pf: Add error handling for cn10k_map_unmap_rq_policer().

[Wentao Liang]

The cn10k_free_matchall_ipolicer() calls the cn10k_map_unmap_rq_policer()
for each queue in a for loop without checking for any errors. A proper
implementation can be found in cn10k_set_matchall_ipolicer_rate().

Check the return value of the cn10k_map_unmap_rq_policer() function during
each loop. Jump to unlock function and return the error code if the
funciton fails to unmap policer.

Fixes: 2ca89a2c3752 ("octeontx2-pf: TC_MATCHALL ingress ratelimiting offload")
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c
index a15cc86635d6..ce58ad61198e 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c
@@ -353,11 +353,13 @@ int cn10k_free_matchall_ipolicer(struct otx2_nic *pfvf)
 
 	/* Remove RQ's policer mapping */
 	for (qidx = 0; qidx < hw->rx_queues; qidx++)
-		cn10k_map_unmap_rq_policer(pfvf, qidx,
-					   hw->matchall_ipolicer, false);
+		rc = cn10k_map_unmap_rq_policer(pfvf, qidx, hw->matchall_ipolicer, false);
+		if (rc)
+			goto out;
 
 	rc = cn10k_free_leaf_profile(pfvf, hw->matchall_ipolicer);
 
+out:
 	mutex_unlock(&pfvf->mbox.lock);
 	return rc;
 }
-- 
2.42.0.windows.2

Re: [PATCH] octeontx2-pf: Add error handling for cn10k_map_unmap_rq_policer().

[Michal Swiatkowski]

On Thu, Apr 03, 2025 at 11:13:03PM +0800, Wentao Liang wrote:
> The cn10k_free_matchall_ipolicer() calls the cn10k_map_unmap_rq_policer()
> for each queue in a for loop without checking for any errors. A proper
> implementation can be found in cn10k_set_matchall_ipolicer_rate().
> 
> Check the return value of the cn10k_map_unmap_rq_policer() function during
> each loop. Jump to unlock function and return the error code if the
> funciton fails to unmap policer.
> 
> Fixes: 2ca89a2c3752 ("octeontx2-pf: TC_MATCHALL ingress ratelimiting offload")
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> ---
>  drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c
> index a15cc86635d6..ce58ad61198e 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c
> @@ -353,11 +353,13 @@ int cn10k_free_matchall_ipolicer(struct otx2_nic *pfvf)
>  
>  	/* Remove RQ's policer mapping */
>  	for (qidx = 0; qidx < hw->rx_queues; qidx++)
> -		cn10k_map_unmap_rq_policer(pfvf, qidx,
> -					   hw->matchall_ipolicer, false);
> +		rc = cn10k_map_unmap_rq_policer(pfvf, qidx, hw->matchall_ipolicer, false);
> +		if (rc)
> +			goto out;

Didn't you forget about braces around for?

>  
>  	rc = cn10k_free_leaf_profile(pfvf, hw->matchall_ipolicer);
>  
> +out:
>  	mutex_unlock(&pfvf->mbox.lock);
>  	return rc;
>  }
> -- 
> 2.42.0.windows.2

RE: [PATCH] octeontx2-pf: Add error handling for cn10k_map_unmap_rq_policer().

[Subbaraya Sundeep Bhatta]

Hi,

From: Wentao Liang <vulab@iscas.ac.cn> 
Sent: Thursday, April 3, 2025 8:43 PM
To: Sunil Kovvuri Goutham <sgoutham@marvell.com>; Geethasowjanya Akula <gakula@marvell.com>; Subbaraya Sundeep Bhatta <sbhatta@marvell.com>; Hariprasad Kelam <hkelam@marvell.com>; andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com
Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Wentao Liang <vulab@iscas.ac.cn>
Subject: [PATCH] octeontx2-pf: Add error handling for cn10k_map_unmap_rq_policer().

The cn10k_free_matchall_ipolicer() calls the cn10k_map_unmap_rq_policer()
for each queue in a for loop without checking for any errors. A proper
implementation can be found in cn10k_set_matchall_ipolicer_rate().

Check the return value of the cn10k_map_unmap_rq_policer() function during
each loop. Jump to unlock function and return the error code if the
funciton fails to unmap policer.

Fixes: 2ca89a2c3752 ("octeontx2-pf: TC_MATCHALL ingress ratelimiting offload")
Signed-off-by: Wentao Liang <mailto:vulab@iscas.ac.cn>
---
 drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c
index a15cc86635d6..ce58ad61198e 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c
@@ -353,11 +353,13 @@ int cn10k_free_matchall_ipolicer(struct otx2_nic *pfvf)
 
 	/* Remove RQ's policer mapping */
 	for (qidx = 0; qidx < hw->rx_queues; qidx++)
-		cn10k_map_unmap_rq_policer(pfvf, qidx,
-					   hw->matchall_ipolicer, false);
+		rc = cn10k_map_unmap_rq_policer(pfvf, qidx, hw->matchall_ipolicer, false);
+		if (rc)
+			goto out;
 
Intentionally we do not bail out when unmapping one of the queues is failed. The reason is during teardown if one of the queues is failed then
we end up not tearing down rest of the queues and those queues cannot be used later which is bad. So leave whatever queues have failed and proceed
with tearing down the rest. Hence all we can do is print an error for the failed queue and continue.

Thanks,
Sundeep

 	rc = cn10k_free_leaf_profile(pfvf, hw->matchall_ipolicer);
 
+out:
 	mutex_unlock(&pfvf->mbox.lock);
 	return rc;
 }
-- 
2.42.0.windows.2

Re: [PATCH] octeontx2-pf: Add error handling for cn10k_map_unmap_rq_policer().

[Simon Horman]

On Thu, Apr 03, 2025 at 11:13:03PM +0800, Wentao Liang wrote:
> The cn10k_free_matchall_ipolicer() calls the cn10k_map_unmap_rq_policer()
> for each queue in a for loop without checking for any errors. A proper
> implementation can be found in cn10k_set_matchall_ipolicer_rate().
> 
> Check the return value of the cn10k_map_unmap_rq_policer() function during
> each loop. Jump to unlock function and return the error code if the
> funciton fails to unmap policer.
> 
> Fixes: 2ca89a2c3752 ("octeontx2-pf: TC_MATCHALL ingress ratelimiting offload")
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> ---
>  drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c
> index a15cc86635d6..ce58ad61198e 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c
> @@ -353,11 +353,13 @@ int cn10k_free_matchall_ipolicer(struct otx2_nic *pfvf)
>  
>  	/* Remove RQ's policer mapping */
>  	for (qidx = 0; qidx < hw->rx_queues; qidx++)
> -		cn10k_map_unmap_rq_policer(pfvf, qidx,
> -					   hw->matchall_ipolicer, false);
> +		rc = cn10k_map_unmap_rq_policer(pfvf, qidx, hw->matchall_ipolicer, false);
> +		if (rc)
> +			goto out;

I'm not sure that bailing out is the right thing to do here.
Won't it result in leaked resources?

>  
>  	rc = cn10k_free_leaf_profile(pfvf, hw->matchall_ipolicer);
>  
> +out:
>  	mutex_unlock(&pfvf->mbox.lock);
>  	return rc;
>  }
> -- 
> 2.42.0.windows.2
> 
> 

Re: [PATCH] octeontx2-pf: Add error handling for cn10k_map_unmap_rq_policer().

[Simon Horman]

On Fri, Apr 04, 2025 at 05:22:16AM +0000, Subbaraya Sundeep Bhatta wrote:
> Hi,
> 
> From: Wentao Liang <vulab@iscas.ac.cn> 
> Sent: Thursday, April 3, 2025 8:43 PM
> To: Sunil Kovvuri Goutham <sgoutham@marvell.com>; Geethasowjanya Akula <gakula@marvell.com>; Subbaraya Sundeep Bhatta <sbhatta@marvell.com>; Hariprasad Kelam <hkelam@marvell.com>; andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Wentao Liang <vulab@iscas.ac.cn>
> Subject: [PATCH] octeontx2-pf: Add error handling for cn10k_map_unmap_rq_policer().
> 
> The cn10k_free_matchall_ipolicer() calls the cn10k_map_unmap_rq_policer()
> for each queue in a for loop without checking for any errors. A proper
> implementation can be found in cn10k_set_matchall_ipolicer_rate().
> 
> Check the return value of the cn10k_map_unmap_rq_policer() function during
> each loop. Jump to unlock function and return the error code if the
> funciton fails to unmap policer.
> 
> Fixes: 2ca89a2c3752 ("octeontx2-pf: TC_MATCHALL ingress ratelimiting offload")
> Signed-off-by: Wentao Liang <mailto:vulab@iscas.ac.cn>
> ---
>  drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c
> index a15cc86635d6..ce58ad61198e 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c
> @@ -353,11 +353,13 @@ int cn10k_free_matchall_ipolicer(struct otx2_nic *pfvf)
>  
>  	/* Remove RQ's policer mapping */
>  	for (qidx = 0; qidx < hw->rx_queues; qidx++)
> -		cn10k_map_unmap_rq_policer(pfvf, qidx,
> -					   hw->matchall_ipolicer, false);
> +		rc = cn10k_map_unmap_rq_policer(pfvf, qidx, hw->matchall_ipolicer, false);
> +		if (rc)
> +			goto out;
>  
> Intentionally we do not bail out when unmapping one of the queues is failed. The reason is during teardown if one of the queues is failed then
> we end up not tearing down rest of the queues and those queues cannot be used later which is bad. So leave whatever queues have failed and proceed
> with tearing down the rest. Hence all we can do is print an error for the failed queue and continue.

Hi Sundeep,

Sorry that I didn't notice your response before sending my own to Wentao.

I do agree that bailing out here is not a good idea.  But I wonder if there
is any value in the function should propagate some error reporting if any
call to cn10k_map_unmap_rq_policer fails - e.g. the first failure - while
still iterating aver all elements.

Just an idea.

> 
> Thanks,
> Sundeep
> 
>  	rc = cn10k_free_leaf_profile(pfvf, hw->matchall_ipolicer);
>  
> +out:
>  	mutex_unlock(&pfvf->mbox.lock);
>  	return rc;
>  }
> -- 
> 2.42.0.windows.2
> 

Re: [PATCH] octeontx2-pf: Add error handling for cn10k_map_unmap_rq_policer().

[kernel test robot]

Hi Wentao,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.14 next-20250404]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Wentao-Liang/octeontx2-pf-Add-error-handling-for-cn10k_map_unmap_rq_policer/20250403-231435
base:   linus/master
patch link:    https://lore.kernel.org/r/20250403151303.2280-1-vulab%40iscas.ac.cn
patch subject: [PATCH] octeontx2-pf:  Add error handling for cn10k_map_unmap_rq_policer().
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20250405/202504050157.qdVzTVMM-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250405/202504050157.qdVzTVMM-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504050157.qdVzTVMM-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c: In function 'cn10k_free_matchall_ipolicer':
>> drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c:360:9: warning: this 'for' clause does not guard... [-Wmisleading-indentation]
     360 |         for (qidx = 0; qidx < hw->rx_queues; qidx++)
         |         ^~~
   drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c:362:17: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'for'
     362 |                 if (rc)
         |                 ^~


vim +/for +360 drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c

2ca89a2c375272 Sunil Goutham 2021-06-15  351  
2ca89a2c375272 Sunil Goutham 2021-06-15  352  int cn10k_free_matchall_ipolicer(struct otx2_nic *pfvf)
2ca89a2c375272 Sunil Goutham 2021-06-15  353  {
2ca89a2c375272 Sunil Goutham 2021-06-15  354  	struct otx2_hw *hw = &pfvf->hw;
2ca89a2c375272 Sunil Goutham 2021-06-15  355  	int qidx, rc;
2ca89a2c375272 Sunil Goutham 2021-06-15  356  
2ca89a2c375272 Sunil Goutham 2021-06-15  357  	mutex_lock(&pfvf->mbox.lock);
2ca89a2c375272 Sunil Goutham 2021-06-15  358  
2ca89a2c375272 Sunil Goutham 2021-06-15  359  	/* Remove RQ's policer mapping */
2ca89a2c375272 Sunil Goutham 2021-06-15 @360  	for (qidx = 0; qidx < hw->rx_queues; qidx++)
d85bdae93e8dbe Wentao Liang  2025-04-03  361  		rc = cn10k_map_unmap_rq_policer(pfvf, qidx, hw->matchall_ipolicer, false);
d85bdae93e8dbe Wentao Liang  2025-04-03  362  		if (rc)
d85bdae93e8dbe Wentao Liang  2025-04-03  363  			goto out;
2ca89a2c375272 Sunil Goutham 2021-06-15  364  
2ca89a2c375272 Sunil Goutham 2021-06-15  365  	rc = cn10k_free_leaf_profile(pfvf, hw->matchall_ipolicer);
2ca89a2c375272 Sunil Goutham 2021-06-15  366  
d85bdae93e8dbe Wentao Liang  2025-04-03  367  out:
2ca89a2c375272 Sunil Goutham 2021-06-15  368  	mutex_unlock(&pfvf->mbox.lock);
2ca89a2c375272 Sunil Goutham 2021-06-15  369  	return rc;
2ca89a2c375272 Sunil Goutham 2021-06-15  370  }
2ca89a2c375272 Sunil Goutham 2021-06-15  371  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] octeontx2-pf: Add error handling for cn10k_map_unmap_rq_policer().

[kernel test robot]

Hi Wentao,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.14 next-20250404]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Wentao-Liang/octeontx2-pf-Add-error-handling-for-cn10k_map_unmap_rq_policer/20250403-231435
base:   linus/master
patch link:    https://lore.kernel.org/r/20250403151303.2280-1-vulab%40iscas.ac.cn
patch subject: [PATCH] octeontx2-pf:  Add error handling for cn10k_map_unmap_rq_policer().
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20250405/202504050116.6a4iOEA7-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250405/202504050116.6a4iOEA7-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504050116.6a4iOEA7-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c:362:3: warning: misleading indentation; statement is not part of the previous 'for' [-Wmisleading-indentation]
     362 |                 if (rc)
         |                 ^
   drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c:360:2: note: previous statement is here
     360 |         for (qidx = 0; qidx < hw->rx_queues; qidx++)
         |         ^
   1 warning generated.


vim +/for +362 drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c

   351	
   352	int cn10k_free_matchall_ipolicer(struct otx2_nic *pfvf)
   353	{
   354		struct otx2_hw *hw = &pfvf->hw;
   355		int qidx, rc;
   356	
   357		mutex_lock(&pfvf->mbox.lock);
   358	
   359		/* Remove RQ's policer mapping */
   360		for (qidx = 0; qidx < hw->rx_queues; qidx++)
   361			rc = cn10k_map_unmap_rq_policer(pfvf, qidx, hw->matchall_ipolicer, false);
 > 362			if (rc)
   363				goto out;
   364	
   365		rc = cn10k_free_leaf_profile(pfvf, hw->matchall_ipolicer);
   366	
   367	out:
   368		mutex_unlock(&pfvf->mbox.lock);
   369		return rc;
   370	}
   371	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] octeontx2-pf: Add error handling for cn10k_map_unmap_rq_policer().

[Subbaraya Sundeep]

On 2025-04-04 at 11:11:38, Simon Horman (horms@kernel.org) wrote:
> On Fri, Apr 04, 2025 at 05:22:16AM +0000, Subbaraya Sundeep Bhatta wrote:
> > Hi,
> > 
> > From: Wentao Liang <vulab@iscas.ac.cn> 
> > Sent: Thursday, April 3, 2025 8:43 PM
> > To: Sunil Kovvuri Goutham <sgoutham@marvell.com>; Geethasowjanya Akula <gakula@marvell.com>; Subbaraya Sundeep Bhatta <sbhatta@marvell.com>; Hariprasad Kelam <hkelam@marvell.com>; andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com
> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Wentao Liang <vulab@iscas.ac.cn>
> > Subject: [PATCH] octeontx2-pf: Add error handling for cn10k_map_unmap_rq_policer().
> > 
> > The cn10k_free_matchall_ipolicer() calls the cn10k_map_unmap_rq_policer()
> > for each queue in a for loop without checking for any errors. A proper
> > implementation can be found in cn10k_set_matchall_ipolicer_rate().
> > 
> > Check the return value of the cn10k_map_unmap_rq_policer() function during
> > each loop. Jump to unlock function and return the error code if the
> > funciton fails to unmap policer.
> > 
> > Fixes: 2ca89a2c3752 ("octeontx2-pf: TC_MATCHALL ingress ratelimiting offload")
> > Signed-off-by: Wentao Liang <mailto:vulab@iscas.ac.cn>
> > ---
> >  drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c
> > index a15cc86635d6..ce58ad61198e 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c
> > @@ -353,11 +353,13 @@ int cn10k_free_matchall_ipolicer(struct otx2_nic *pfvf)
> >  
> >  	/* Remove RQ's policer mapping */
> >  	for (qidx = 0; qidx < hw->rx_queues; qidx++)
> > -		cn10k_map_unmap_rq_policer(pfvf, qidx,
> > -					   hw->matchall_ipolicer, false);
> > +		rc = cn10k_map_unmap_rq_policer(pfvf, qidx, hw->matchall_ipolicer, false);
> > +		if (rc)
> > +			goto out;
> >  
> > Intentionally we do not bail out when unmapping one of the queues is failed. The reason is during teardown if one of the queues is failed then
> > we end up not tearing down rest of the queues and those queues cannot be used later which is bad. So leave whatever queues have failed and proceed
> > with tearing down the rest. Hence all we can do is print an error for the failed queue and continue.
> 
> Hi Sundeep,
> 
> Sorry that I didn't notice your response before sending my own to Wentao.
> 
> I do agree that bailing out here is not a good idea.  But I wonder if there
> is any value in the function should propagate some error reporting if any
> call to cn10k_map_unmap_rq_policer fails - e.g. the first failure - while
> still iterating aver all elements.
> 
> Just an idea.
> 
Hi Simon,

We can do but it gets compilcated if more than one queue failed and
reasons are different. Hence just print error and continue.

Thanks,
Sundeep

> > 
> > Thanks,
> > Sundeep
> > 
> >  	rc = cn10k_free_leaf_profile(pfvf, hw->matchall_ipolicer);
> >  
> > +out:
> >  	mutex_unlock(&pfvf->mbox.lock);
> >  	return rc;
> >  }
> > -- 
> > 2.42.0.windows.2
> > 

Re: [PATCH] octeontx2-pf: Add error handling for cn10k_map_unmap_rq_policer().

[Simon Horman]

On Mon, Apr 07, 2025 at 05:57:14AM +0000, Subbaraya Sundeep wrote:
> On 2025-04-04 at 11:11:38, Simon Horman (horms@kernel.org) wrote:
> > On Fri, Apr 04, 2025 at 05:22:16AM +0000, Subbaraya Sundeep Bhatta wrote:
> > > Hi,
> > > 
> > > From: Wentao Liang <vulab@iscas.ac.cn> 
> > > Sent: Thursday, April 3, 2025 8:43 PM
> > > To: Sunil Kovvuri Goutham <sgoutham@marvell.com>; Geethasowjanya Akula <gakula@marvell.com>; Subbaraya Sundeep Bhatta <sbhatta@marvell.com>; Hariprasad Kelam <hkelam@marvell.com>; andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com
> > > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Wentao Liang <vulab@iscas.ac.cn>
> > > Subject: [PATCH] octeontx2-pf: Add error handling for cn10k_map_unmap_rq_policer().
> > > 
> > > The cn10k_free_matchall_ipolicer() calls the cn10k_map_unmap_rq_policer()
> > > for each queue in a for loop without checking for any errors. A proper
> > > implementation can be found in cn10k_set_matchall_ipolicer_rate().
> > > 
> > > Check the return value of the cn10k_map_unmap_rq_policer() function during
> > > each loop. Jump to unlock function and return the error code if the
> > > funciton fails to unmap policer.
> > > 
> > > Fixes: 2ca89a2c3752 ("octeontx2-pf: TC_MATCHALL ingress ratelimiting offload")
> > > Signed-off-by: Wentao Liang <mailto:vulab@iscas.ac.cn>
> > > ---
> > >  drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c
> > > index a15cc86635d6..ce58ad61198e 100644
> > > --- a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c
> > > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k.c
> > > @@ -353,11 +353,13 @@ int cn10k_free_matchall_ipolicer(struct otx2_nic *pfvf)
> > >  
> > >  	/* Remove RQ's policer mapping */
> > >  	for (qidx = 0; qidx < hw->rx_queues; qidx++)
> > > -		cn10k_map_unmap_rq_policer(pfvf, qidx,
> > > -					   hw->matchall_ipolicer, false);
> > > +		rc = cn10k_map_unmap_rq_policer(pfvf, qidx, hw->matchall_ipolicer, false);
> > > +		if (rc)
> > > +			goto out;
> > >  
> > > Intentionally we do not bail out when unmapping one of the queues is failed. The reason is during teardown if one of the queues is failed then
> > > we end up not tearing down rest of the queues and those queues cannot be used later which is bad. So leave whatever queues have failed and proceed
> > > with tearing down the rest. Hence all we can do is print an error for the failed queue and continue.
> > 
> > Hi Sundeep,
> > 
> > Sorry that I didn't notice your response before sending my own to Wentao.
> > 
> > I do agree that bailing out here is not a good idea.  But I wonder if there
> > is any value in the function should propagate some error reporting if any
> > call to cn10k_map_unmap_rq_policer fails - e.g. the first failure - while
> > still iterating aver all elements.
> > 
> > Just an idea.
> > 
> Hi Simon,
> 
> We can do but it gets compilcated if more than one queue failed and
> reasons are different. Hence just print error and continue.

Yes, understood. I did think about this some more and I think that
reporting an error is the best we can do, else as you say it gets complex.
So if there is already a log made, then I think that is sufficient.