On 2020-04-17 15:46:39 [+0800], yhchuang@xxxxxxxxxxx wrote:
> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8723d.c b/drivers/net/wireless/realtek/rtw88/rtw8723d.c
> index 94784c7f0743..b66bd969e007 100644
> --- a/drivers/net/wireless/realtek/rtw88/rtw8723d.c
> +++ b/drivers/net/wireless/realtek/rtw88/rtw8723d.c
…
> +struct iqk_backup_regs {
> + u32 adda[IQK_ADDA_REG_NUM];
> + u8 mac8[IQK_MAC8_REG_NUM];
> + u32 mac32[IQK_MAC32_REG_NUM];
> + u32 bb[IQK_BB_REG_NUM];
> +
> + u32 lte_path;
> + u32 lte_gnt;
> +
> + u8 btg_sel;
> + u32 bb_sel_btg;
> +
> + u8 igia;
> + u8 igib;
The struct has 128 bytes. Putting btg_sel after bb_sel_btg will result
in 124 bytes. How likely is it that it will grow? I'm asking because it
is allocated on stack.
> +};
> +
> +static void rtw8723d_iqk_backup_regs(struct rtw_dev *rtwdev,
> + struct iqk_backup_regs *backup)
> +{
> + int i;
> +
> + for (i = 0; i < IQK_ADDA_REG_NUM; i++)
> + backup->adda[i] = rtw_read32(rtwdev, iqk_adda_regs[i]);
> +
> + for (i = 0; i < IQK_MAC8_REG_NUM; i++)
> + backup->mac8[i] = rtw_read8(rtwdev, iqk_mac8_regs[i]);
> + for (i = 0; i < IQK_MAC32_REG_NUM; i++)
> + backup->mac32[i] = rtw_read32(rtwdev, iqk_mac32_regs[i]);
> +
> + for (i = 0; i < IQK_BB_REG_NUM; i++)
> + backup->bb[i] = rtw_read32(rtwdev, iqk_bb_regs[i]);
> +
> + backup->igia = (u8)rtw_read32_mask(rtwdev, REG_OFDM0_XAAGC1, MASKBYTE0);
> + backup->igib = (u8)rtw_read32_mask(rtwdev, REG_OFDM0_XBAGC1, MASKBYTE0);
igi[ab] is alreay u8, no need for cast.
> +
> + backup->bb_sel_btg = rtw_read32(rtwdev, REG_BB_SEL_BTG);
> +}
…
> +static u8 rtw8723d_iqk_rx_path(struct rtw_dev *rtwdev,
> + const struct rtw_8723d_iqk_cfg *iqk_cfg,
> + const struct iqk_backup_regs *backup)
> +{
> + u32 tx_x, tx_y;
> + u8 result = 0x00;
You could avoid the explicit init of `result' (maybe even use `ret' for
less key strokes and avoiding the confusion with the `result' array used
by the other functions here) and then
…
> + rtw8723d_iqk_one_shot(rtwdev, false, iqk_cfg);
> + result |= rtw8723d_iqk_check_tx_failed(rtwdev, iqk_cfg);
not or the returned value here. Since you don't collect it from multiple
functions I don't see the reason for it.
> + if (!result)
> + goto restore;
…
> + rtw8723d_iqk_one_shot(rtwdev, false, iqk_cfg);
> + result |= rtw8723d_iqk_check_rx_failed(rtwdev, iqk_cfg);
Same here.
> +restore:
> + rtw8723d_iqk_txrx_path_post(rtwdev, iqk_cfg, backup);
> +
> + return result;
> +}
> +
…
> +
> +static void rtw8723d_phy_calibration(struct rtw_dev *rtwdev)
> +{
> + struct rtw_dm_info *dm_info = &rtwdev->dm_info;
> + s32 result[IQK_ROUND_SIZE][IQK_NR];
> + struct iqk_backup_regs backup;
I don't know how deep you are in the call chain, but `result' takes 128
bytes and `backup' as well (this could be 124).
I'm not saying that this is bad, just that you keep an eye on it since
those two take 256 bytes.
> + u8 i, j;
> + u8 final_candidate = IQK_ROUND_INVALID;
> + bool good;
> +
> + rtw_dbg(rtwdev, RTW_DBG_RFK, "[IQK] Start!!!\n");
> +
> + memset(result, 0, sizeof(result));
Sebastian
