* re: rtlwifi: rtl8723be: Add new driver
@ 2015-10-19 15:10 Dan Carpenter
2015-10-19 18:26 ` Jakub Sitnicki
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2015-10-19 15:10 UTC (permalink / raw)
To: Larry.Finger; +Cc: linux-wireless
Hello Larry Finger,
The patch a619d1abe20c: "rtlwifi: rtl8723be: Add new driver" from Feb
28, 2014, leads to the following static checker warning:
drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c:1802 _rtl8723be_read_power_value_fromprom()
warn: why is the last element skipped?
drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c
1770 if (autoload_fail) {
1771 RT_TRACE(rtlpriv, COMP_INIT, DBG_LOUD,
1772 "auto load fail : Use Default value!\n");
1773 for (path = 0; path < MAX_RF_PATH; path++) {
1774 /* 2.4G default value */
1775 for (group = 0 ; group < MAX_CHNL_GROUP_24G; group++) {
^^^^^^^^^^^^^^^^^^^^^^^^^^
Here we go up to the end.
1776 pw2g->index_cck_base[path][group] = 0x2D;
1777 pw2g->index_bw40_base[path][group] = 0x2D;
1778 }
1779 for (cnt = 0; cnt < MAX_TX_COUNT; cnt++) {
1780 if (cnt == 0) {
1781 pw2g->bw20_diff[path][0] = 0x02;
1782 pw2g->ofdm_diff[path][0] = 0x04;
1783 } else {
1784 pw2g->bw20_diff[path][cnt] = 0xFE;
1785 pw2g->bw40_diff[path][cnt] = 0xFE;
1786 pw2g->cck_diff[path][cnt] = 0xFE;
1787 pw2g->ofdm_diff[path][cnt] = 0xFE;
1788 }
1789 }
1790 }
1791 return;
1792 }
1793
1794 for (path = 0; path < MAX_RF_PATH; path++) {
1795 /*2.4G default value*/
1796 for (group = 0; group < MAX_CHNL_GROUP_24G; group++) {
^^^^^^^^^^^^^^^^^^^^^^^^^^
Here we go to the end.
1797 pw2g->index_cck_base[path][group] = hwinfo[addr++];
1798 if (pw2g->index_cck_base[path][group] == 0xFF)
1799 pw2g->index_cck_base[path][group] = 0x2D;
1800
1801 }
1802 for (group = 0; group < MAX_CHNL_GROUP_24G - 1; group++) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Here we skip the last element. So far as I can see this looks like a
mistake but I don't know the code well.
1803 pw2g->index_bw40_base[path][group] = hwinfo[addr++];
1804 if (pw2g->index_bw40_base[path][group] == 0xFF)
1805 pw2g->index_bw40_base[path][group] = 0x2D;
1806 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: rtlwifi: rtl8723be: Add new driver
2015-10-19 15:10 rtlwifi: rtl8723be: Add new driver Dan Carpenter
@ 2015-10-19 18:26 ` Jakub Sitnicki
0 siblings, 0 replies; 4+ messages in thread
From: Jakub Sitnicki @ 2015-10-19 18:26 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Larry.Finger, linux-wireless
Hi Dan,
On Mon, Oct 19, 2015 at 05:10 PM CEST, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> The patch a619d1abe20c: "rtlwifi: rtl8723be: Add new driver" from Feb
> 28, 2014, leads to the following static checker warning:
>
> drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c:1802 _rtl8723be_read_power_value_fromprom()
> warn: why is the last element skipped?
>
> drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c
[snip]
> 1794 for (path = 0; path < MAX_RF_PATH; path++) {
> 1795 /*2.4G default value*/
> 1796 for (group = 0; group < MAX_CHNL_GROUP_24G; group++) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^
> Here we go to the end.
>
> 1797 pw2g->index_cck_base[path][group] = hwinfo[addr++];
> 1798 if (pw2g->index_cck_base[path][group] == 0xFF)
> 1799 pw2g->index_cck_base[path][group] = 0x2D;
> 1800
> 1801 }
> 1802 for (group = 0; group < MAX_CHNL_GROUP_24G - 1; group++) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Here we skip the last element. So far as I can see this looks like a
> mistake but I don't know the code well.
>
> 1803 pw2g->index_bw40_base[path][group] = hwinfo[addr++];
> 1804 if (pw2g->index_bw40_base[path][group] == 0xFF)
> 1805 pw2g->index_bw40_base[path][group] = 0x2D;
> 1806 }
I have seen the same thing in rtlwifi/rtl8188ee and staging/rtl8188eu
drivers (see read_power_value_fromprom() and
Hal_ReadPowerValueFromPROM_8188E(), respectively), and I've asked myself
exactly the same question before so I'll share my thoughts.
Please take it all with a grain of salt, I'm new at this.
The rtl8188e[eu] drivers divide 2.4 GHz channels into six sets
(MAX_CHNL_GROUP_24G == 6) as far as the TX power goes (see
_rtl88e_get_chnl_group(), Hal_GetChnlGroup88E()):
group 0: 1-2
group 1: 3-5
group 2: 6-8
group 3: 9-11
group 4: 12-13
group 5: 14
As you see the last group is special, it's only channel 14, which
applies only to 802.11b in Japan, AFAIK. The loop in question
initializes the index_bw40_base[] array, which name suggests that it is
related to 40 MHz channel bandwidth - 802.11n only then. That would
explain the `MAX_CHNL_GROUP_24G - 1' expression.
This is the case for rtl8188e[eu]. However, rtl8723be driver divides
2.4 GHz channels into just three groups - 0: 1-3, 1: 4-9, and 2: 10-14
(_rtl8723be_get_chnl_group()). In that regard it is similar to
rtlwifi/rtl8192cu and staging/rtl8723au (rtl92c_get_chnl_group(),
Hal_GetChnlGroup()).
That would mean that the index_bw40_base[3..5] range in this
one-size-fits all array is never accessed by the rtl8723be driver.
That's all I know, beyond that point I tell myself that it's a driver
for a chip with no datasheet so questions without answers are part of
the deal ;-)
Cheers,
Jakub
^ permalink raw reply [flat|nested] 4+ messages in thread
* re: rtlwifi: rtl8723be: Add new driver
@ 2014-03-06 21:54 Dan Carpenter
2014-03-09 6:00 ` Larry Finger
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2014-03-06 21:54 UTC (permalink / raw)
To: Larry.Finger; +Cc: linux-wireless
Hi Larry,
Sorry to bother you about this, because I know we see this same bug
every time we add another Realtek driver and you must be as sick of it
as I am... :/
The patch a619d1abe20c: "rtlwifi: rtl8723be: Add new driver" from Feb
28, 2014, leads to the following static checker warning:
drivers/net/wireless/rtlwifi/rtl8723be/phy.c:667 _rtl8723be_store_tx_power_by_rate()
error: buffer overflow 'rtlphy->tx_power_by_rate_offset[band]' 4 <= 5
drivers/net/wireless/rtlwifi/rtl8723be/phy.c
646 static void _rtl8723be_store_tx_power_by_rate(struct ieee80211_hw *hw,
647 u32 band, u32 rfpath,
648 u32 txnum, u32 regaddr,
649 u32 bitmask, u32 data)
650 {
651 struct rtl_priv *rtlpriv = rtl_priv(hw);
652 struct rtl_phy *rtlphy = &(rtlpriv->phy);
653 u8 rate_section = _rtl8723be_get_rate_section_index(regaddr);
654
655 if (band != BAND_ON_2_4G && band != BAND_ON_5G)
656 RT_TRACE(rtlpriv, COMP_POWER, PHY_TXPWR,
657 "Invalid Band %d\n", band);
658
659 if (rfpath > MAX_RF_PATH)
^^^^^^^^^^^
This should be >= TX_PWR_BY_RATE_NUM_RF. We should return on error
instead of printing an error and then corrupting memory.
I don't know what to do here to make these bugs go away...
660 RT_TRACE(rtlpriv, COMP_POWER, PHY_TXPWR,
661 "Invalid RfPath %d\n", rfpath);
662
663 if (txnum > MAX_RF_PATH)
664 RT_TRACE(rtlpriv, COMP_POWER, PHY_TXPWR,
665 "Invalid TxNum %d\n", txnum);
666
667 rtlphy->tx_power_by_rate_offset[band][rfpath][txnum][rate_section] =
668 data;
669 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: rtlwifi: rtl8723be: Add new driver
2014-03-06 21:54 Dan Carpenter
@ 2014-03-09 6:00 ` Larry Finger
0 siblings, 0 replies; 4+ messages in thread
From: Larry Finger @ 2014-03-09 6:00 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-wireless
On 03/06/2014 03:54 PM, Dan Carpenter wrote:
> Hi Larry,
>
> Sorry to bother you about this, because I know we see this same bug
> every time we add another Realtek driver and you must be as sick of it
> as I am... :/
>
> The patch a619d1abe20c: "rtlwifi: rtl8723be: Add new driver" from Feb
> 28, 2014, leads to the following static checker warning:
>
> drivers/net/wireless/rtlwifi/rtl8723be/phy.c:667 _rtl8723be_store_tx_power_by_rate()
> error: buffer overflow 'rtlphy->tx_power_by_rate_offset[band]' 4 <= 5
>
> drivers/net/wireless/rtlwifi/rtl8723be/phy.c
> 646 static void _rtl8723be_store_tx_power_by_rate(struct ieee80211_hw *hw,
> 647 u32 band, u32 rfpath,
> 648 u32 txnum, u32 regaddr,
> 649 u32 bitmask, u32 data)
> 650 {
> 651 struct rtl_priv *rtlpriv = rtl_priv(hw);
> 652 struct rtl_phy *rtlphy = &(rtlpriv->phy);
> 653 u8 rate_section = _rtl8723be_get_rate_section_index(regaddr);
> 654
> 655 if (band != BAND_ON_2_4G && band != BAND_ON_5G)
> 656 RT_TRACE(rtlpriv, COMP_POWER, PHY_TXPWR,
> 657 "Invalid Band %d\n", band);
> 658
> 659 if (rfpath > MAX_RF_PATH)
> ^^^^^^^^^^^
> This should be >= TX_PWR_BY_RATE_NUM_RF. We should return on error
> instead of printing an error and then corrupting memory.
>
> I don't know what to do here to make these bugs go away...
>
> 660 RT_TRACE(rtlpriv, COMP_POWER, PHY_TXPWR,
> 661 "Invalid RfPath %d\n", rfpath);
> 662
> 663 if (txnum > MAX_RF_PATH)
> 664 RT_TRACE(rtlpriv, COMP_POWER, PHY_TXPWR,
> 665 "Invalid TxNum %d\n", txnum);
> 666
> 667 rtlphy->tx_power_by_rate_offset[band][rfpath][txnum][rate_section] =
> 668 data;
> 669 }
Thanks for pointing to these problems.
BTW, what static checker found the problem? I ran the latest Smatch and it did
not find it.
Larry
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-10-19 18:26 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-10-19 15:10 rtlwifi: rtl8723be: Add new driver Dan Carpenter
2015-10-19 18:26 ` Jakub Sitnicki
-- strict thread matches above, loose matches on Subject: below --
2014-03-06 21:54 Dan Carpenter
2014-03-09 6:00 ` Larry Finger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).