| Message ID | 20241016060605.11359-1-pvmohammedanees2003@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Ping-Ke Shih |
| Headers | show |
| Series | wifi: rtw88: Refactor looping in rtw_phy_store_tx_power_by_rate | expand |
Mohammed Anees <pvmohammedanees2003@gmail.com> wrote: > > The previous implementation performs check for the band > in each iteration, which is unnecessary and further more > there is a else condition which will never get triggered, I feel compilers can optimize the check for the band, and we can just remove the else condition. Or if (2ghz) foo_2g(); else foo_5g(); > since a check is done to see if the band is either 2G or > 5G earlier and the band either be any of those 2. We can > refactor this by assigning a pointer to the appropriate > power offset array based on the band before the loop and > updating this. > > Signed-off-by: Mohammed Anees <pvmohammedanees2003@gmail.com> > --- > drivers/net/wireless/realtek/rtw88/phy.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtw88/phy.c b/drivers/net/wireless/realtek/rtw88/phy.c > index 37ef80c9091d..17d61f1d9257 100644 > --- a/drivers/net/wireless/realtek/rtw88/phy.c > +++ b/drivers/net/wireless/realtek/rtw88/phy.c > @@ -1465,15 +1465,14 @@ static void rtw_phy_store_tx_power_by_rate(struct rtw_dev *rtwdev, > rate_num > RTW_RF_PATH_MAX)) > return; > > + s8 (*tx_pwr_by_rate_offset) = (band == PHY_BANK_2G) > + ? hal->tx_pwr_by_rate_offset_2g[rfpath] > + : hal->tx_pwr_by_rate_offset_5g[rfpath]; > + Though -Wdeclaration-after-statement was dropped, still recommend to place declarations at the beginning of function. The operands ? and : should place at the end of statement. x = y ? z0 : z1; > for (i = 0; i < rate_num; i++) { > offset = pwr_by_rate[i]; > rate = rates[i]; > - if (band == PHY_BAND_2G) > - hal->tx_pwr_by_rate_offset_2g[rfpath][rate] = offset; > - else if (band == PHY_BAND_5G) > - hal->tx_pwr_by_rate_offset_5g[rfpath][rate] = offset; > - else > - continue; > + tx_pwr_by_rate_offset[rate] = offset; > } > } > > -- > 2.47.0
Hi Mohammed, kernel test robot noticed the following build errors: [auto build test ERROR on wireless-next/main] [also build test ERROR on wireless/main linus/master v6.12-rc3 next-20241016] [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/Mohammed-Anees/wifi-rtw88-Refactor-looping-in-rtw_phy_store_tx_power_by_rate/20241016-140811 base: https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git main patch link: https://lore.kernel.org/r/20241016060605.11359-1-pvmohammedanees2003%40gmail.com patch subject: [PATCH] wifi: rtw88: Refactor looping in rtw_phy_store_tx_power_by_rate config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20241017/202410171143.OnFlgIwK-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 14.1.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241017/202410171143.OnFlgIwK-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/202410171143.OnFlgIwK-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/net/wireless/realtek/rtw88/phy.c: In function 'rtw_phy_store_tx_power_by_rate': >> drivers/net/wireless/realtek/rtw88/phy.c:1468:48: error: 'PHY_BANK_2G' undeclared (first use in this function); did you mean 'PHY_BAND_2G'? 1468 | s8 (*tx_pwr_by_rate_offset) = (band == PHY_BANK_2G) | ^~~~~~~~~~~ | PHY_BAND_2G drivers/net/wireless/realtek/rtw88/phy.c:1468:48: note: each undeclared identifier is reported only once for each function it appears in Kconfig warnings: (for reference only) WARNING: unmet direct dependencies detected for GET_FREE_REGION Depends on [n]: SPARSEMEM [=n] Selected by [m]: - RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m] vim +1468 drivers/net/wireless/realtek/rtw88/phy.c 1447 1448 static void rtw_phy_store_tx_power_by_rate(struct rtw_dev *rtwdev, 1449 u32 band, u32 rfpath, u32 txnum, 1450 u32 regaddr, u32 bitmask, u32 data) 1451 { 1452 struct rtw_hal *hal = &rtwdev->hal; 1453 u8 rate_num = 0; 1454 u8 rate; 1455 u8 rates[RTW_RF_PATH_MAX] = {0}; 1456 s8 offset; 1457 s8 pwr_by_rate[RTW_RF_PATH_MAX] = {0}; 1458 int i; 1459 1460 rtw_phy_get_rate_values_of_txpwr_by_rate(rtwdev, regaddr, bitmask, data, 1461 rates, pwr_by_rate, &rate_num); 1462 1463 if (WARN_ON(rfpath >= RTW_RF_PATH_MAX || 1464 (band != PHY_BAND_2G && band != PHY_BAND_5G) || 1465 rate_num > RTW_RF_PATH_MAX)) 1466 return; 1467 > 1468 s8 (*tx_pwr_by_rate_offset) = (band == PHY_BANK_2G) 1469 ? hal->tx_pwr_by_rate_offset_2g[rfpath] 1470 : hal->tx_pwr_by_rate_offset_5g[rfpath]; 1471 1472 for (i = 0; i < rate_num; i++) { 1473 offset = pwr_by_rate[i]; 1474 rate = rates[i]; 1475 tx_pwr_by_rate_offset[rate] = offset; 1476 } 1477 } 1478
Oops, I sent over the wrong patch with typo, I'll make sure to fix that in the next version. > I feel compilers can optimize the check for the band, and we can just remove > the else condition. Or > if (2ghz) > foo_2g(); > else > foo_5g(); I do agree with that but I feel, it would be better to make it independent of compiler optimization, thoughts? Let me know what you think is better, that is whether letting it be if - else, or using a pointer. thanks
Mohammed Anees <pvmohammedanees2003@gmail.com> wrote: > Oops, I sent over the wrong patch with typo, > I'll make sure to fix that in the next version. > > > I feel compilers can optimize the check for the band, and we can just remove > > the else condition. Or > > if (2ghz) > > foo_2g(); > > else > > foo_5g(); > > I do agree with that but I feel, it would be > better to make it independent of compiler > optimization, thoughts? > > Let me know what you think is better, that is > whether letting it be if - else, or using a > pointer. Using a pointer looks delicate and optimized. The if-else code is simple to read. Since this chunk is small, I don't bias one of them. If I must choose one, I vote if-else.
Thank you for your feedback! I appreciate your perspective on this, I'll stick to if-else then and remove the unnecessary else and send over the patch. Thanks!
diff --git a/drivers/net/wireless/realtek/rtw88/phy.c b/drivers/net/wireless/realtek/rtw88/phy.c index 37ef80c9091d..17d61f1d9257 100644 --- a/drivers/net/wireless/realtek/rtw88/phy.c +++ b/drivers/net/wireless/realtek/rtw88/phy.c @@ -1465,15 +1465,14 @@ static void rtw_phy_store_tx_power_by_rate(struct rtw_dev *rtwdev, rate_num > RTW_RF_PATH_MAX)) return; + s8 (*tx_pwr_by_rate_offset) = (band == PHY_BANK_2G) + ? hal->tx_pwr_by_rate_offset_2g[rfpath] + : hal->tx_pwr_by_rate_offset_5g[rfpath]; + for (i = 0; i < rate_num; i++) { offset = pwr_by_rate[i]; rate = rates[i]; - if (band == PHY_BAND_2G) - hal->tx_pwr_by_rate_offset_2g[rfpath][rate] = offset; - else if (band == PHY_BAND_5G) - hal->tx_pwr_by_rate_offset_5g[rfpath][rate] = offset; - else - continue; + tx_pwr_by_rate_offset[rate] = offset; } }
The previous implementation performs check for the band in each iteration, which is unnecessary and further more there is a else condition which will never get triggered, since a check is done to see if the band is either 2G or 5G earlier and the band either be any of those 2. We can refactor this by assigning a pointer to the appropriate power offset array based on the band before the loop and updating this. Signed-off-by: Mohammed Anees <pvmohammedanees2003@gmail.com> --- drivers/net/wireless/realtek/rtw88/phy.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)