On 01/30/2012 07:50 AM, Devendra.Naga wrote:
> From: "Devendra.Naga"<[email protected]>
>
> The regaddr is compared against different rates in each case and the powerlevels are set
> accordingly. as this can match for the first if and we still check for other powerlevels too,
> use else if ratherthan using if statements
> Signed-off-by: Devendra.Naga<[email protected]>
If this is correct, and I'm not saying it is or isn't, would it not be
clearer to turn this whole section into a switch statement? The cases
that requiring fiddling with the bitmask would need to be dealt with in
the default: case.
Why was it written as a series of if statements to begin with: can (or
could) any of these if statements change the value of regaddr such that
later ifs evaluate differently? From reading only the patch the answer
seems to be "no", so that looks good.
Or what about turning this into a data-driven section? It looks like
values of regaddr are being translated into offsets into
rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt]. Can this be done with a lookup
table?
> ---
> drivers/net/wireless/rtlwifi/rtl8192c/phy_common.c | 47 +++++++------------
> 1 files changed, 17 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/net/wireless/rtlwifi/rtl8192c/phy_common.c b/drivers/net/wireless/rtlwifi/rtl8192c/phy_common.c
> index 22e998d..cb11e5a 100644
> --- a/drivers/net/wireless/rtlwifi/rtl8192c/phy_common.c
> +++ b/drivers/net/wireless/rtlwifi/rtl8192c/phy_common.c
> @@ -260,106 +260,93 @@ void _rtl92c_store_pwrIndex_diffrate_offset(struct ieee80211_hw *hw,
> "MCSTxPowerLevelOriginalOffset[%d][0] = 0x%x\n",
> rtlphy->pwrgroup_cnt,
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][0]);
> - }
> - if (regaddr == RTXAGC_A_RATE54_24) {
> + } else if (regaddr == RTXAGC_A_RATE54_24) {
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][1] = data;
> RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> "MCSTxPowerLevelOriginalOffset[%d][1] = 0x%x\n",
> rtlphy->pwrgroup_cnt,
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][1]);
> - }
> - if (regaddr == RTXAGC_A_CCK1_MCS32) {
> + } else if (regaddr == RTXAGC_A_CCK1_MCS32) {
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][6] = data;
> RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> "MCSTxPowerLevelOriginalOffset[%d][6] = 0x%x\n",
> rtlphy->pwrgroup_cnt,
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][6]);
> - }
> - if (regaddr == RTXAGC_B_CCK11_A_CCK2_11&& bitmask == 0xffffff00) {
> + } else if (regaddr == RTXAGC_B_CCK11_A_CCK2_11&&
> + bitmask == 0xffffff00) {
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][7] = data;
> RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> "MCSTxPowerLevelOriginalOffset[%d][7] = 0x%x\n",
> rtlphy->pwrgroup_cnt,
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][7]);
> - }
> - if (regaddr == RTXAGC_A_MCS03_MCS00) {
> + } else if (regaddr == RTXAGC_A_MCS03_MCS00) {
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][2] = data;
> RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> "MCSTxPowerLevelOriginalOffset[%d][2] = 0x%x\n",
> rtlphy->pwrgroup_cnt,
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][2]);
> - }
> - if (regaddr == RTXAGC_A_MCS07_MCS04) {
> + } else if (regaddr == RTXAGC_A_MCS07_MCS04) {
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][3] = data;
> RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> "MCSTxPowerLevelOriginalOffset[%d][3] = 0x%x\n",
> rtlphy->pwrgroup_cnt,
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][3]);
> - }
> - if (regaddr == RTXAGC_A_MCS11_MCS08) {
> + } else if (regaddr == RTXAGC_A_MCS11_MCS08) {
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][4] = data;
> RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> "MCSTxPowerLevelOriginalOffset[%d][4] = 0x%x\n",
> rtlphy->pwrgroup_cnt,
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][4]);
> - }
> - if (regaddr == RTXAGC_A_MCS15_MCS12) {
> + } else if (regaddr == RTXAGC_A_MCS15_MCS12) {
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][5] = data;
> RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> "MCSTxPowerLevelOriginalOffset[%d][5] = 0x%x\n",
> rtlphy->pwrgroup_cnt,
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][5]);
> - }
> - if (regaddr == RTXAGC_B_RATE18_06) {
> + } else if (regaddr == RTXAGC_B_RATE18_06) {
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][8] = data;
> RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> "MCSTxPowerLevelOriginalOffset[%d][8] = 0x%x\n",
> rtlphy->pwrgroup_cnt,
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][8]);
> - }
> - if (regaddr == RTXAGC_B_RATE54_24) {
> + } else if (regaddr == RTXAGC_B_RATE54_24) {
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][9] = data;
> RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> "MCSTxPowerLevelOriginalOffset[%d][9] = 0x%x\n",
> rtlphy->pwrgroup_cnt,
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][9]);
> - }
> - if (regaddr == RTXAGC_B_CCK1_55_MCS32) {
> + } else if (regaddr == RTXAGC_B_CCK1_55_MCS32) {
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][14] = data;
> RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> "MCSTxPowerLevelOriginalOffset[%d][14] = 0x%x\n",
> rtlphy->pwrgroup_cnt,
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][14]);
> - }
> - if (regaddr == RTXAGC_B_CCK11_A_CCK2_11&& bitmask == 0x000000ff) {
> + } else if (regaddr == RTXAGC_B_CCK11_A_CCK2_11&&
> + bitmask == 0x000000ff) {
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][15] = data;
> RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> "MCSTxPowerLevelOriginalOffset[%d][15] = 0x%x\n",
> rtlphy->pwrgroup_cnt,
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][15]);
> - }
> - if (regaddr == RTXAGC_B_MCS03_MCS00) {
> + } else if (regaddr == RTXAGC_B_MCS03_MCS00) {
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][10] = data;
> RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> "MCSTxPowerLevelOriginalOffset[%d][10] = 0x%x\n",
> rtlphy->pwrgroup_cnt,
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][10]);
> - }
> - if (regaddr == RTXAGC_B_MCS07_MCS04) {
> + } else if (regaddr == RTXAGC_B_MCS07_MCS04) {
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][11] = data;
> RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> "MCSTxPowerLevelOriginalOffset[%d][11] = 0x%x\n",
> rtlphy->pwrgroup_cnt,
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][11]);
> - }
> - if (regaddr == RTXAGC_B_MCS11_MCS08) {
> + } else if (regaddr == RTXAGC_B_MCS11_MCS08) {
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][12] = data;
> RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> "MCSTxPowerLevelOriginalOffset[%d][12] = 0x%x\n",
> rtlphy->pwrgroup_cnt,
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][12]);
> - }
> - if (regaddr == RTXAGC_B_MCS15_MCS12) {
> + } else if (regaddr == RTXAGC_B_MCS15_MCS12) {
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][13] = data;
> RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> "MCSTxPowerLevelOriginalOffset[%d][13] = 0x%x\n",
On 02/05/2012 02:12 AM, Jan Ceuleers wrote:
> On 01/30/2012 07:50 AM, Devendra.Naga wrote:
>> From: "Devendra.Naga"<[email protected]>
>>
>> The regaddr is compared against different rates in each case and the
>> powerlevels are set
>> accordingly. as this can match for the first if and we still check for other
>> powerlevels too,
>> use else if ratherthan using if statements
>> Signed-off-by: Devendra.Naga<[email protected]>
> If this is correct, and I'm not saying it is or isn't, would it not be clearer
> to turn this whole section into a switch statement? The cases that requiring
> fiddling with the bitmask would need to be dealt with in the default: case.
Yes, it is correct. It might be clearer if it were written as a switch
statement, but that would be a lot more intrusive. Besides, have you ever looked
at the way gcc compiles a switch? I have done some reverse engineering, and at
least some implementations generate code that looks like if .. else if .. ..
else. I have not looked at the generated code, but I'm quite certain that the
optimizer in gcc is good enough to know that once it gets a match between
regaddr and one of the RTXAGC constants, there is no need to test the rest, and
that the change to "else if" is mostly cosmetic, and will not greatly affect the
execution.
> Why was it written as a series of if statements to begin with: can (or could)
> any of these if statements change the value of regaddr such that later ifs
> evaluate differently? From reading only the patch the answer seems to be "no",
> so that looks good.
I cannot answer the question of why it was written this way - you would have to
ask the original authors. As to the possibility of the code changing regaddr,
you either need to look more carefully at the code, or you need to restudy how C
works.
> Or what about turning this into a data-driven section? It looks like values of
> regaddr are being translated into offsets into
> rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt]. Can this be done with a lookup table?
I would state it a little differently - the values of regaddr are used to
determine where in the double-indexed array rtlphy->MCS_TXPWR[][] that the
quantity "data" should be placed. There are always multiple ways to do anything.
For the kernel, a major rule is to make minimal changes that are easy to verify.
The patch in question meets that condition. If this code were in the hot path,
it might be worth the effort required to do a complete rewrite with the
associated testing, but in my mind, that effort would be wasted. Something that
would be easy to do, easy to verify, and would reduce the size of the generated
code would be
int index;
if (regaddr == RTXAGC_A_RATE18_06) {
index = 0;
else if (if (regaddr == RTXAGC_A_RATE54_24) {
index = 1;
.....
}
rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][index] = data;
RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
"MCSTxPowerLevelOriginalOffset[%d][%d] = 0x%x\n",
rtlphy->pwrgroup_cnt, index,
rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][index]);
...
Larry
> Yes, it is correct. It might be clearer if it were written as
> a switch statement, but that would be a lot more intrusive.
> Besides, have you ever looked at the way gcc compiles a switch?
How gcc compiles a switch statement depends (amongst other
things) on whether the labels form a 'dense set' or not.
If they are dense it will usually generate a jump table.
If they are not dense it will compare against the 'middle'
entry first and generate a tree of conditionals.
An 'else if' chain is a linear search.
If the performance of the code matters in can be worth
an explicit test for the common case outside the switch.
Alternatively an 'if else' chain with every conditional
marked 'unlikely' should generate exactly one mispredicted
branch (if you can force static branch prediction).
There are further constraints that apply to some cpus.
Mostly it isn't worth worrying about - although a lot of
code can be significantly sped up by avoiding/containing
register spills and mis-predicted branches.
The code is question seemed to be checking bits, not
an ordinal - so changing to a case might be problematic.
(Could more than one bit bet set????)
David