Skip to contentSkip to conversation

rtw89: add Realtek 802.11ax driver

bug report Dan Carpenter 2 participants 2 messages Nov 8, 2021 – Nov 9, 2021

lore
Share & tools
lore
#0 lore raw

Hello Ping-Ke Shih,

The patch e3ec7017f6a2: "rtw89: add Realtek 802.11ax driver" from Oct 11, 2021, leads to the following Smatch static checker warning:

	drivers/net/wireless/realtek/rtw89/mac.c:586 hfc_ch_ctrl()
	warn: array off by one? 'cfg[ch]'

drivers/net/wireless/realtek/rtw89/mac.c

    568 static int hfc_ch_ctrl(struct rtw89_dev *rtwdev, u8 ch)
    569 {
    570         struct rtw89_hfc_param *param = &rtwdev->mac.hfc_param;
    571         const struct rtw89_hfc_ch_cfg *cfg = param->ch_cfg;
    572         int ret = 0;
    573         u32 val = 0;
    574 
    575         ret = rtw89_mac_check_mac_en(rtwdev, RTW89_MAC_0, RTW89_DMAC_SEL);
    576         if (ret)
    577                 return ret;
    578 
    579         ret = hfc_ch_cfg_chk(rtwdev, ch);
    580         if (ret)
    581                 return ret;
    582 
    583         if (ch > RTW89_DMA_B1HI)
                    ^^^^^^^^^^^^^^^^^^^

    584                 return -EINVAL;
    585 
--> 586         val = u32_encode_bits(cfg[ch].min, B_AX_MIN_PG_MASK) |
    587               u32_encode_bits(cfg[ch].max, B_AX_MAX_PG_MASK) |
    588               (cfg[ch].grp ? B_AX_GRP : 0);

This is an unpublished Smatch warning which uses an "assume every > comparison should be >= wrong until proven otherwise" approach. In this case, Smatch can't tell the size of the cfg[] array.

Manually it looks like cfg can only be rtw8852a_hfc_chcfg_pcie[] which has RTW89_DMA_CH_NUM (13) elements. Is there a reason why we don't use the last element? Also it's puzzling that the last element is FWCMDQ instead of H2C. So maybe that's the reason?

    589         rtw89_write32(rtwdev, R_AX_ACH0_PAGE_CTRL + ch * 4, val);
    590 
    591         return 0;
    592 }

regards,
dan carpenter

#1 lore raw
Quoted 41 lines

-----Original Message-----
From: Dan Carpenter <[email protected]>
Sent: Monday, November 8, 2021 10:47 PM
To: Pkshih <[email protected]>
Cc: [email protected]
Subject: [bug report] rtw89: add Realtek 802.11ax driver

Hello Ping-Ke Shih,

The patch e3ec7017f6a2: "rtw89: add Realtek 802.11ax driver" from Oct 11, 2021, leads to the following Smatch static checker warning:

	drivers/net/wireless/realtek/rtw89/mac.c:586 hfc_ch_ctrl()
	warn: array off by one? 'cfg[ch]'

drivers/net/wireless/realtek/rtw89/mac.c

    568 static int hfc_ch_ctrl(struct rtw89_dev *rtwdev, u8 ch)
    569 {
    570         struct rtw89_hfc_param *param = &rtwdev->mac.hfc_param;
    571         const struct rtw89_hfc_ch_cfg *cfg = param->ch_cfg;
    572         int ret = 0;
    573         u32 val = 0;
    574
    575         ret = rtw89_mac_check_mac_en(rtwdev, RTW89_MAC_0, RTW89_DMAC_SEL);
    576         if (ret)
    577                 return ret;
    578
    579         ret = hfc_ch_cfg_chk(rtwdev, ch);
    580         if (ret)
    581                 return ret;
    582
    583         if (ch > RTW89_DMA_B1HI)
                    ^^^^^^^^^^^^^^^^^^^

    584                 return -EINVAL;
    585
--> 586         val = u32_encode_bits(cfg[ch].min, B_AX_MIN_PG_MASK) |
    587               u32_encode_bits(cfg[ch].max, B_AX_MAX_PG_MASK) |
    588               (cfg[ch].grp ? B_AX_GRP : 0);
Quoted 3 lines

This is an unpublished Smatch warning which uses an "assume every > comparison should be >= wrong until proven otherwise" approach. In this case, Smatch can't tell the size of the cfg[] array.

I'm not clear how to fix this warning. Any suggestion?

The caller of this function is

	for (ch = RTW89_DMA_ACH0; ch < RTW89_DMA_H2C; ch++) {
		ret = hfc_ch_ctrl(rtwdev, ch);
		...
	}

So, 'ch' is between 0 and 11. Maybe, this checking can be removed?

Quoted 4 lines

Manually it looks like cfg can only be rtw8852a_hfc_chcfg_pcie[] which has RTW89_DMA_CH_NUM (13) elements. Is there a reason why we don't use the last element?

We don't use the last element, because channel 0 to 11 are the regular TX channels that will transmit packets to air. The channel 12 (H2C) is a special channel we send firmware commands from driver, so it doesn't need additional configurations. More, the list of R_AX_ACH0_PAGE_CTRL series don't have one for channel 12 neither.

Quoted 2 lines

Also it's puzzling that the last element is FWCMDQ instead of H2C. So maybe that's the reason?

FWCMDQ is a queue or channel that is used to send H2C from driver to firmware. So, FWCMD and H2C can be seen as synonyms, as well as FWCMDQ and H2CQ.

Quoted 6 lines
    589         rtw89_write32(rtwdev, R_AX_ACH0_PAGE_CTRL + ch * 4, val);
    590
    591         return 0;
    592 }

--
Ping-Ke