| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20140927194939.GA28553@electric-eye.fr.zoreil.com>
Date: Sat, 27 Sep 2014 21:49:39 +0200
From: Francois Romieu <romieu@...zoreil.com>
To: Larry Finger <Larry.Finger@...inger.net>
Cc: linville@...driver.com, linux-wireless@...r.kernel.org,
troy_tan@...lsil.com.cn, netdev@...r.kernel.org
Subject: Re: [PATCH 9/11 V2 NEXT] rtlwifi: rtl8188ee: Update driver to match
Realtek release of 06282014
Larry Finger <Larry.Finger@...inger.net> :
[...]
> diff --git a/drivers/net/wireless/rtlwifi/core.c b/drivers/net/wireless/rtlwifi/core.c
> index 40738e3..d30f416 100644
> --- a/drivers/net/wireless/rtlwifi/core.c
> +++ b/drivers/net/wireless/rtlwifi/core.c
> @@ -28,6 +28,7 @@
> #include "cam.h"
> #include "base.h"
> #include "ps.h"
> +#include "pwrseqcmd.h"
>
> #include "btcoexist/rtl_btc.h"
> #include <linux/firmware.h>
> @@ -1700,3 +1701,100 @@ const struct ieee80211_ops rtl_ops = {
> };
> EXPORT_SYMBOL_GPL(rtl_ops);
>
> +/* Description:
> + * This routine deals with the Power Configuration CMD
> + * parsing for RTL8723/RTL8188E Series IC.
> + * Assumption:
> + * We should follow specific format that was released from HW SD.
"released" is a bit misleading if it does not mean "publicly released".
> + */
> +bool rtl_hal_pwrseqcmdparsing(struct rtl_priv *rtlpriv, u8 cut_version,
> + u8 faversion, u8 interface_type,
> + struct wlan_pwr_cfg pwrcfgcmd[])
> +{
> + struct wlan_pwr_cfg cfg_cmd = {0};
> + bool polling_bit = false;
> + u32 ary_idx = 0;
u32 i;
> + u8 value = 0;
> + u32 offset = 0;
Useless init.
> + u32 polling_count = 0;
Wrong scope.
> + u32 max_polling_cnt = 5000;
Inverted xmas tree for declaration.
> +
> + do {
while (1) {
> + cfg_cmd = pwrcfgcmd[ary_idx];
> + RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> + "rtl_hal_pwrseqcmdparsing(): offset(%#x),cut_msk(%#x), famsk(%#x), interface_msk(%#x), base(%#x), cmd(%#x), msk(%#x), value(%#x)\n",
^^^ missing space
> + GET_PWR_CFG_OFFSET(cfg_cmd),
> + GET_PWR_CFG_CUT_MASK(cfg_cmd),
> + GET_PWR_CFG_FAB_MASK(cfg_cmd),
> + GET_PWR_CFG_INTF_MASK(cfg_cmd),
> + GET_PWR_CFG_BASE(cfg_cmd), GET_PWR_CFG_CMD(cfg_cmd),
> + GET_PWR_CFG_MASK(cfg_cmd), GET_PWR_CFG_VALUE(cfg_cmd));
> +
> + if ((GET_PWR_CFG_FAB_MASK(cfg_cmd)&faversion) &&
> + (GET_PWR_CFG_CUT_MASK(cfg_cmd)&cut_version) &&
^^^ missing spaces
> + (GET_PWR_CFG_INTF_MASK(cfg_cmd)&interface_type)) {
> + switch (GET_PWR_CFG_CMD(cfg_cmd)) {
> + case PWR_CMD_READ:
> + RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> + "rtl_hal_pwrseqcmdparsing(): PWR_CMD_READ\n");
It should line up after the parenthesis.
> + break;
> + case PWR_CMD_WRITE:
> + RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> + "rtl_hal_pwrseqcmdparsing(): PWR_CMD_WRITE\n");
> + offset = GET_PWR_CFG_OFFSET(cfg_cmd);
> +
> + /*Read the value from system register*/
> + value = rtl_read_byte(rtlpriv, offset);
> + value &= (~(GET_PWR_CFG_MASK(cfg_cmd)));
Excess parenthsis.
> + value |= (GET_PWR_CFG_VALUE(cfg_cmd) &
> + GET_PWR_CFG_MASK(cfg_cmd));
Excess parenthsis.
> +
> + /*Write the value back to sytem register*/
> + rtl_write_byte(rtlpriv, offset, value);
> + break;
> + case PWR_CMD_POLLING:
> + RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> + "rtl_hal_pwrseqcmdparsing(): PWR_CMD_POLLING\n");
It should line up after the parenthesis.
> + polling_bit = false;
> + offset = GET_PWR_CFG_OFFSET(cfg_cmd);
> +
> + do {
> + value = rtl_read_byte(rtlpriv, offset);
> +
> + value &= GET_PWR_CFG_MASK(cfg_cmd);
> + if (value ==
> + (GET_PWR_CFG_VALUE(cfg_cmd) &
> + GET_PWR_CFG_MASK(cfg_cmd)))
> + polling_bit = true;
> + else
> + udelay(10);
> +
> + if (polling_count++ > max_polling_cnt)
> + return false;
> + } while (!polling_bit);
Simple "for" loop in disguise.
An helper function may help.
Imnsho the whole do { more, more, more } while (1) concept is broken.
[...]
> diff --git a/drivers/net/wireless/rtlwifi/rtl8188ee/def.h b/drivers/net/wireless/rtlwifi/rtl8188ee/def.h
> index c764fff..d9ea9d0 100644
> --- a/drivers/net/wireless/rtlwifi/rtl8188ee/def.h
> +++ b/drivers/net/wireless/rtlwifi/rtl8188ee/def.h
[...]
> @@ -225,44 +218,37 @@ enum power_polocy_config {
> };
>
> enum interface_select_pci {
> - INTF_SEL1_MINICARD,
> - INTF_SEL0_PCIE,
> - INTF_SEL2_RSV,
> - INTF_SEL3_RSV,
> + INTF_SEL1_MINICARD = 0,
> + INTF_SEL0_PCIE = 1,
> + INTF_SEL2_RSV = 2,
> + INTF_SEL3_RSV = 3,
Please use tabs to line things up.
[...]
> + HAL_FW_C2H_CMD_READ_MACREG = 0,
> + HAL_FW_C2H_CMD_READ_BBREG = 1,
> + HAL_FW_C2H_CMD_READ_RFREG = 2,
> + HAL_FW_C2H_CMD_READ_EEPROM = 3,
> + HAL_FW_C2H_CMD_READ_EFUSE = 4,
See above.
[...]
> diff --git a/drivers/net/wireless/rtlwifi/rtl8188ee/dm.c b/drivers/net/wireless/rtlwifi/rtl8188ee/dm.c
> index f8daa61..2aa34d9 100644
> --- a/drivers/net/wireless/rtlwifi/rtl8188ee/dm.c
> +++ b/drivers/net/wireless/rtlwifi/rtl8188ee/dm.c
[...]
> - rtl_set_bbreg(hw, ROFDM0_XATXIQIMBAL, MASKDWORD,
> - value32);
> + rtl_set_bbreg(hw, ROFDM0_XATXIQIMBALANCE,
> + MASKDWORD, value32);
It lacks of consistency, see the *NICE* marker below.
> value32 = (ele_c & 0x000003C0) >> 6;
> - rtl_set_bbreg(hw, ROFDM0_XCTXAFE, MASKH4BITS, value32);
> + rtl_set_bbreg(hw, ROFDM0_XCTXAFE, MASKH4BITS,
> + value32);
?
> value32 = ((iqk_result_x * ele_d) >> 7) & 0x01;
> - rtl_set_bbreg(hw, ROFDM0_ECCATHRES, BIT(24), value32);
> + rtl_set_bbreg(hw, ROFDM0_ECCATHRESHOLD, BIT(24),
> + value32);
> break;
> case RF90_PATH_B:
> value32 = (ele_d << 22)|((ele_c & 0x3F)<<16) | ele_a;
> - rtl_set_bbreg(hw, ROFDM0_XBTXIQIMBAL,
> - MASKDWORD, value32);
> + rtl_set_bbreg(hw, ROFDM0_XBTXIQIMBALANCE, MASKDWORD,
> + value32);
*NICE*
[...]
> @@ -210,16 +209,20 @@ static void rtl88e_set_iqk_matrix(struct ieee80211_hw *hw,
> } else {
> switch (rfpath) {
> case RF90_PATH_A:
> - rtl_set_bbreg(hw, ROFDM0_XATXIQIMBAL, MASKDWORD,
> - ofdmswing_table[ofdm_index]);
> - rtl_set_bbreg(hw, ROFDM0_XCTXAFE, MASKH4BITS, 0x00);
> - rtl_set_bbreg(hw, ROFDM0_ECCATHRES, BIT(24), 0x00);
> + rtl_set_bbreg(hw, ROFDM0_XATXIQIMBALANCE,
> + MASKDWORD, ofdmswing_table[ofdm_index]);
> + rtl_set_bbreg(hw, ROFDM0_XCTXAFE,
> + MASKH4BITS, 0x00);
rtl_set_bbreg(hw, ROFDM0_XCTXAFE, MASKH4BITS, 0x00);
> + rtl_set_bbreg(hw, ROFDM0_ECCATHRESHOLD,
> + BIT(24), 0x00);
rtl_set_bbreg(hw, ROFDM0_ECCATHRESHOLD, BIT(24), 0x00);
> break;
> case RF90_PATH_B:
> - rtl_set_bbreg(hw, ROFDM0_XBTXIQIMBAL, MASKDWORD,
> - ofdmswing_table[ofdm_index]);
> - rtl_set_bbreg(hw, ROFDM0_XDTXAFE, MASKH4BITS, 0x00);
> - rtl_set_bbreg(hw, ROFDM0_ECCATHRES, BIT(28), 0x00);
> + rtl_set_bbreg(hw, ROFDM0_XBTXIQIMBALANCE,
> + MASKDWORD, ofdmswing_table[ofdm_index]);
> + rtl_set_bbreg(hw, ROFDM0_XDTXAFE,
> + MASKH4BITS, 0x00);
rtl_set_bbreg(hw, ROFDM0_XDTXAFE, MASKH4BITS, 0x00);
> + rtl_set_bbreg(hw, ROFDM0_ECCATHRESHOLD,
> + BIT(28), 0x00);
rtl_set_bbreg(hw, ROFDM0_ECCATHRESHOLD, BIT(28), 0x00);
[...]
> @@ -263,46 +266,75 @@ void rtl88e_dm_txpower_track_adjust(struct ieee80211_hw *hw,
> (pwr_val << 24);
> }
>
> -
> -static void rtl88e_chk_tx_track(struct ieee80211_hw *hw,
> - enum pwr_track_control_method method,
> - u8 rfpath, u8 index)
> +static void dm_tx_pwr_track_set_pwr(struct ieee80211_hw *hw,
> + enum pwr_track_control_method method,
> + u8 rfpath, u8 channel_mapped_index)
> {
> struct rtl_priv *rtlpriv = rtl_priv(hw);
> - struct rtl_phy *rtlphy = &(rtlpriv->phy);
> + struct rtl_phy *rtlphy = &rtlpriv->phy;
> struct rtl_dm *rtldm = rtl_dm(rtl_priv(hw));
> - int jj = rtldm->swing_idx_cck;
> - int i;
>
> if (method == TXAGC) {
> - if (rtldm->swing_flag_ofdm == true ||
> - rtldm->swing_flag_cck == true) {
> - u8 chan = rtlphy->current_channel;
> - rtl88e_phy_set_txpower_level(hw, chan);
> + if (rtldm->swing_flag_ofdm ||
> + rtldm->swing_flag_cck) {
Single line.
> + rtl88e_phy_set_txpower_level(hw,
> + rtlphy->current_channel);
> rtldm->swing_flag_ofdm = false;
> rtldm->swing_flag_cck = false;
> }
> } else if (method == BBSWING) {
> if (!rtldm->cck_inch14) {
> - for (i = 0; i < 8; i++)
> - rtl_write_byte(rtlpriv, 0xa22 + i,
> - cck_tbl_ch1_13[jj][i]);
> + rtl_write_byte(rtlpriv, 0xa22,
> + cck_tbl_ch1_13[rtldm->swing_idx_cck][0]);
> + rtl_write_byte(rtlpriv, 0xa23,
> + cck_tbl_ch1_13[rtldm->swing_idx_cck][1]);
> + rtl_write_byte(rtlpriv, 0xa24,
> + cck_tbl_ch1_13[rtldm->swing_idx_cck][2]);
> + rtl_write_byte(rtlpriv, 0xa25,
> + cck_tbl_ch1_13[rtldm->swing_idx_cck][3]);
> + rtl_write_byte(rtlpriv, 0xa26,
> + cck_tbl_ch1_13[rtldm->swing_idx_cck][4]);
> + rtl_write_byte(rtlpriv, 0xa27,
> + cck_tbl_ch1_13[rtldm->swing_idx_cck][5]);
> + rtl_write_byte(rtlpriv, 0xa28,
> + cck_tbl_ch1_13[rtldm->swing_idx_cck][6]);
> + rtl_write_byte(rtlpriv, 0xa29,
> + cck_tbl_ch1_13[rtldm->swing_idx_cck][7]);
?
I hope someone got the "-R" flag of patch wrong.
> } else {
> - for (i = 0; i < 8; i++)
> - rtl_write_byte(rtlpriv, 0xa22 + i,
> - cck_tbl_ch14[jj][i]);
> + rtl_write_byte(rtlpriv, 0xa22,
> + cck_tbl_ch14[rtldm->swing_idx_cck][0]);
> + rtl_write_byte(rtlpriv, 0xa23,
> + cck_tbl_ch14[rtldm->swing_idx_cck][1]);
> + rtl_write_byte(rtlpriv, 0xa24,
> + cck_tbl_ch14[rtldm->swing_idx_cck][2]);
> + rtl_write_byte(rtlpriv, 0xa25,
> + cck_tbl_ch14[rtldm->swing_idx_cck][3]);
> + rtl_write_byte(rtlpriv, 0xa26,
> + cck_tbl_ch14[rtldm->swing_idx_cck][4]);
> + rtl_write_byte(rtlpriv, 0xa27,
> + cck_tbl_ch14[rtldm->swing_idx_cck][5]);
> + rtl_write_byte(rtlpriv, 0xa28,
> + cck_tbl_ch14[rtldm->swing_idx_cck][6]);
> + rtl_write_byte(rtlpriv, 0xa29,
> + cck_tbl_ch14[rtldm->swing_idx_cck][7]);
Sic.
> }
>
> if (rfpath == RF90_PATH_A) {
> - long x = rtlphy->iqk_matrix[index].value[0][0];
> - long y = rtlphy->iqk_matrix[index].value[0][1];
> - u8 indx = rtldm->swing_idx_ofdm[rfpath];
> - rtl88e_set_iqk_matrix(hw, indx, rfpath, x, y);
> + rtl88e_set_iqk_matrix(hw, rtldm->swing_idx_ofdm[rfpath],
> + rfpath, rtlphy->iqk_matrix
> + [channel_mapped_index].
> + value[0][0],
> + rtlphy->iqk_matrix
> + [channel_mapped_index].
> + value[0][1]);
> } else if (rfpath == RF90_PATH_B) {
> - u8 indx = rtldm->swing_idx_ofdm[rfpath];
> - long x = rtlphy->iqk_matrix[indx].value[0][4];
> - long y = rtlphy->iqk_matrix[indx].value[0][5];
> - rtl88e_set_iqk_matrix(hw, indx, rfpath, x, y);
> + rtl88e_set_iqk_matrix(hw, rtldm->swing_idx_ofdm[rfpath],
> + rfpath, rtlphy->iqk_matrix
> + [channel_mapped_index].
> + value[0][4],
> + rtlphy->iqk_matrix
> + [channel_mapped_index].
> + value[0][5]);
:o(
I stop here. I don't enjoy playing the bad cop and this patch is feeding crap
into the kernel.
--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists