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

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

Posted by Wentao Liang 10 months, 1 week ago

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().

Posted by kernel test robot 10 months, 1 week ago

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().

Posted by kernel test robot 10 months, 1 week ago

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().

Posted by Simon Horman 10 months, 1 week ago

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().

Posted by Michal Swiatkowski 10 months, 1 week ago

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