| Message ID | 20250403151303.2280-1-vulab@iscas.ac.cn (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | octeontx2-pf: Add error handling for cn10k_map_unmap_rq_policer(). | expand |
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
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; }
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 > >
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 >
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
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
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; }
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(-)