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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Sun, 28 Sep 2014 17:40:52 -0500
From:	Larry Finger <Larry.Finger@...inger.net>
To:	Francois Romieu <romieu@...zoreil.com>
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

On 09/27/2014 02:49 PM, Francois Romieu wrote:
>
> 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.

I do not understand what you mean here.

>
>> +
>> +	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.

Thanks for the review.

As I explained in V1 of these patches, I am trying to merge the kernel and 
Realtek internal code repositories in the hope that Realtek will keep the two of 
them in sync. In addition, I am hoping that they will take over maintaining 
these codes in the future.

If I insist on an absolute minimum of change in the kernel code, then the churn 
of the Realtek repo is increased, and I run the risk of failing in the process 
of merging the two repos. In my estimation, that would be a bigger disaster than 
getting some code into the kernel that is not as pretty as could be. In 
addition, I am not sure how much longer I want to volunteer full time to support 
these codes.

Larry

--
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