RE: [PATCH] realtek: Add switch variable to 'switch case not processed' messages

[David Laight]

> If you want to create enum->#ENUM structs and
> "const char *" lookup functions, please be my guest.
> 
> otherwise, hex is at least a consistent way to display
> what should be infrequent output.

If I've typed it right:

#define tags(x) x(A) x(B) x(C)
#define x(t) t,
enum {tags(x) tag_count};
#undef x
#define x(t) ##t,
static const char names[] = { tags(x) };
#undef x

David

Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages

[Jes Sorensen]

Joe Perches  writes:
> On Sat, 2016-09-24 at 14:06 -0500, Larry Finger wrote:
>> On 09/24/2016 12:32 PM, Joe Perches wrote:
> []
>> o Reindent all the switch/case blocks to a more normal
>>   kernel style (git diff -w would show no changes here)
>> That sounds like busy work to me, but if you want to do it, go ahead.
>
> It's really just to make the comparison case block reductions
> easier to verify for later steps done
>
>> > o cast, spacing and parenthesis reductions
>> >   Lots of odd and somewhat unique styles in various
>> >   drivers, looks like too many individual authors without
>> >   a style guide / code enforcer using slightly different
>> >   personalized code.  Glancing at the code, it looks to be
>> >   similar logic, just written in different styles.
>> Same comment.
>
> Same rationale
>
>> > o Logic changes like
>> >   from:
>> > if (foo) func(..., bar, ...); else func(..., baz, ...);
>> >   to:
>> > func(..., foo ? bar : baz, ...);
>> >   to make the case statement code blocks more consistent
>> >   and emit somewhat smaller object code.
>> I find if .. else constructs much easier to read than the cond ?  :  
>> form. I would reject any such patches.
>
>  I think object code reduction generally a good thing
> but then again, I'm not a maintainer here.

I missed this part, but I am with Larry here - 'foo ? bar : boo' are
just obfuscating the code and far less clear than if or switch
statements.

Jes

Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages

[Joe Perches]

On Sat, 2016-09-24 at 14:06 -0500, Larry Finger wrote:
> On 09/24/2016 12:32 PM, Joe Perches wrote:
[]
> o Reindent all the switch/case blocks to a more normal
>   kernel style (git diff -w would show no changes here)
> That sounds like busy work to me, but if you want to do it, go ahead.

It's really just to make the comparison case block reductions
easier to verify for later steps done

> > o cast, spacing and parenthesis reductions
> >   Lots of odd and somewhat unique styles in various
> >   drivers, looks like too many individual authors without
> >   a style guide / code enforcer using slightly different
> >   personalized code.  Glancing at the code, it looks to be
> >   similar logic, just written in different styles.
> Same comment.

Same rationale

> > o Logic changes like
> >   from:
> > if (foo) func(..., bar, ...); else func(..., baz, ...);
> >   to:
> > func(..., foo ? bar : baz, ...);
> >   to make the case statement code blocks more consistent
> >   and emit somewhat smaller object code.
> I find if .. else constructs much easier to read than the cond ?  :  
> form. I would reject any such patches.

 I think object code reduction generally a good thing
but then again, I'm not a maintainer here.

> > o Consolidation of equivalent function spanning drivers
> >   With the style only changes minimized, where possible
> >   make the drivers use common ops/callback functions.
> The is no question that there are similar routines in different drivers. I 
> would 
> like to place as much as possible into common routines, but I never seem to 
> find 
> the time. There are too many bugs in other things I support to consider these 
> niceties.

Consolidation generally reduces defects and improves ease of
updating.
> 

Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages

[Jes Sorensen]

Larry Finger  writes:
> On 09/24/2016 12:32 PM, Joe Perches wrote:
>> Is there any value in that or is Jes' work going to make
>> doing any or all of this unnecessary and futile?
>
> That is not yet determined. The only driver that is to be replaced at
> this point is rtl8192cu. Jes only has USB I/O for his driver. We are
> looking at adding SDIO, and once that is done, PCI should be possible.

If someone else wants to address PCI then it could happen quite soon,
but at the current schedule I don't see PCI happen in my driver for at
least a year, probably more.

If you can reduce the size of rtlwifi in the mean time that probably
isn't going to upset a lot of people.

Jes

Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages

[Larry Finger]

On 09/24/2016 12:32 PM, Joe Perches wrote:

(adding Jes Sorensen to recipients)

On Sat, 2016-09-24 at 11:35 -0500, Larry Finger wrote:

I have patches that makes HAL_DEF_WOWLAN be a no-op for the rest of the drivers,
and one that sets the enum values for that particular statement to hex values. I
also looked at the other large enums and decided that they never need the human
lookup.


Hey Larry.

There are many somewhat common realtek wireless drivers.

Not to step on your toes, but what do you think of
rationalizing the switch/case statements of all the
realtek drivers in a few steps:

o Reindent all the switch/case blocks to a more normal
  kernel style (git diff -w would show no changes here)


That sounds like busy work to me, but if you want to do it, go ahead.


o cast, spacing and parenthesis reductions
  Lots of odd and somewhat unique styles in various
  drivers, looks like too many individual authors without
  a style guide / code enforcer using slightly different
  personalized code.  Glancing at the code, it looks to be
  similar logic, just written in different styles.


Same comment.


o Logic changes like
  from:
if (foo) func(..., bar, ...); else func(..., baz, ...);
  to:
func(..., foo ? bar : baz, ...);
  to make the case statement code blocks more consistent
  and emit somewhat smaller object code.


I find if .. else constructs much easier to read than the cond ?  :  
form. I would reject any such patches.



o Consolidation of equivalent function spanning drivers
  With the style only changes minimized, where possible
  make the drivers use common ops/callback functions.


The is no question that there are similar routines in different drivers. I would 
like to place as much as possible into common routines, but I never seem to find 
the time. There are too many bugs in other things I support to consider these 
niceties.



Is there any value in that or is Jes' work going to make
doing any or all of this unnecessary and futile?


That is not yet determined. The only driver that is to be replaced at this point 
is rtl8192cu. Jes only has USB I/O for his driver. We are looking at adding 
SDIO, and once that is done, PCI should be possible.


Larry

Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages

[Joe Perches]

(adding Jes Sorensen to recipients)

On Sat, 2016-09-24 at 11:35 -0500, Larry Finger wrote:
> I have patches that makes HAL_DEF_WOWLAN be a no-op for the rest of the 
> drivers, 
> and one that sets the enum values for that particular statement to hex 
> values. I 
> also looked at the other large enums and decided that they never need the 
> human 
> lookup.

Hey Larry.

There are many somewhat common realtek wireless drivers.

Not to step on your toes, but what do you think of
rationalizing the switch/case statements of all the
realtek drivers in a few steps:

o Reindent all the switch/case blocks to a more normal
  kernel style (git diff -w would show no changes here)

o cast, spacing and parenthesis reductions
  Lots of odd and somewhat unique styles in various
  drivers, looks like too many individual authors without
  a style guide / code enforcer using slightly different
  personalized code.  Glancing at the code, it looks to be
  similar logic, just written in different styles.

o Logic changes like
  from:
if (foo) func(..., bar, ...); else func(..., baz, ...);
  to:
func(..., foo ? bar : baz, ...);
  to make the case statement code blocks more consistent
  and emit somewhat smaller object code.

o Consolidation of equivalent function spanning drivers
  With the style only changes minimized, where possible
  make the drivers use common ops/callback functions.

Is there any value in that or is Jes' work going to make
doing any or all of this unnecessary and futile?

Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages

[Larry Finger]

On 09/24/2016 11:15 AM, Joe Perches wrote:

On Sat, 2016-09-24 at 17:55 +0200, Jean Delvare wrote:

Would it make sense to explicitly set the enum values, or add them as
comments, to make such look-ups easier?


If you want to create enum->#ENUM structs and
"const char *" lookup functions, please be my guest.

otherwise, hex is at least a consistent way to display
what should be infrequent output.


Displaying those values as hex is OK. As Joe says, they will not be shown very 
often.


I have patches that makes HAL_DEF_WOWLAN be a no-op for the rest of the drivers, 
and one that sets the enum values for that particular statement to hex values. I 
also looked at the other large enums and decided that they never need the human 
lookup.


Larry

Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages

[Joe Perches]

On Sat, 2016-09-24 at 17:55 +0200, Jean Delvare wrote:
> Would it make sense to explicitly set the enum values, or add them as
> comments, to make such look-ups easier?

If you want to create enum->#ENUM structs and
"const char *" lookup functions, please be my guest.

otherwise, hex is at least a consistent way to display
what should be infrequent output.

Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages

[Jean Delvare]

Hi Joe, Larry,

On Fri, 23 Sep 2016 12:02:43 -0700, Joe Perches wrote:
> On Fri, 2016-09-23 at 13:59 -0500, Larry Finger wrote:
> > I'm not familiar with the %#x format. What does it do?
> 
> Outputs SPECIAL prefix, it's the same as "0x%x"
> 
> lib/vsprintf.c:
> #define SPECIAL   64  /* prefix hex with "0x", octal with "0" 
> */

Is hexadecimal actually the best way to display these values? I guess it
depends how they are listed in the datasheets (if there's anything like
that for these chips?)

I found it a bit difficult to look up the meaning of the value.
HAL_DEF_WOWLAN is an enum value, the number is not set and there's no
comment. I had to count the line numbers, taking blank lines into
account... I ended up pasting the whole enum to a random C file and
printing the value of HAL_DEF_WOWLAN to make sure it was 92.

Would it make sense to explicitly set the enum values, or add them as
comments, to make such look-ups easier?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages

[Larry Finger]

On 09/23/2016 01:27 PM, Joe Perches wrote:

Help along debugging by showing what switch/case variable is not
being processed in these messages.

Signed-off-by: Joe Perches 


Acked-by: Larry Finger 

Thanks,

Larry

Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages

[Joe Perches]

On Fri, 2016-09-23 at 13:59 -0500, Larry Finger wrote:
> I'm not familiar with the %#x format. What does it do?

Outputs SPECIAL prefix, it's the same as "0x%x"

lib/vsprintf.c:
#define SPECIAL 64  /* prefix hex with "0x", octal with "0" */

Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages

[Larry Finger]

On 09/23/2016 01:27 PM, Joe Perches wrote:

Help along debugging by showing what switch/case variable is not
being processed in these messages.

Signed-off-by: Joe Perches 


Joe,

You beat me to the patch. No problem as this one looks OK; however, I'm not 
familiar with the %#x format. What does it do?


Larry


---
 drivers/net/wireless/realtek/rtlwifi/core.c  |  3 ++-
 drivers/net/wireless/realtek/rtlwifi/pci.c   |  3 ++-
 drivers/net/wireless/realtek/rtlwifi/ps.c|  2 +-
 drivers/net/wireless/realtek/rtlwifi/rtl8188ee/fw.c  |  4 ++--
 drivers/net/wireless/realtek/rtlwifi/rtl8188ee/hw.c  |  9 +
 drivers/net/wireless/realtek/rtlwifi/rtl8188ee/led.c |  4 ++--
 drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c | 10 ++
 .../wireless/realtek/rtlwifi/rtl8192c/fw_common.c|  4 ++--
 .../wireless/realtek/rtlwifi/rtl8192c/phy_common.c   |  8 +---
 drivers/net/wireless/realtek/rtlwifi/rtl8192ce/hw.c  |  7 ---
 drivers/net/wireless/realtek/rtlwifi/rtl8192ce/led.c |  4 ++--
 drivers/net/wireless/realtek/rtlwifi/rtl8192ce/phy.c |  7 ++-
 drivers/net/wireless/realtek/rtlwifi/rtl8192cu/hw.c  |  4 ++--
 drivers/net/wireless/realtek/rtlwifi/rtl8192cu/led.c |  4 ++--
 drivers/net/wireless/realtek/rtlwifi/rtl8192cu/phy.c |  7 ++-
 drivers/net/wireless/realtek/rtlwifi/rtl8192de/fw.c  |  4 ++--
 drivers/net/wireless/realtek/rtlwifi/rtl8192de/hw.c  |  9 +
 drivers/net/wireless/realtek/rtlwifi/rtl8192de/led.c |  4 ++--
 drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c | 15 +++
 drivers/net/wireless/realtek/rtlwifi/rtl8192ee/fw.c  |  4 ++--
 drivers/net/wireless/realtek/rtlwifi/rtl8192ee/hw.c  |  9 +
 drivers/net/wireless/realtek/rtlwifi/rtl8192ee/led.c |  4 ++--
 drivers/net/wireless/realtek/rtlwifi/rtl8192ee/phy.c | 10 ++
 drivers/net/wireless/realtek/rtlwifi/rtl8192se/hw.c  |  9 +
 drivers/net/wireless/realtek/rtlwifi/rtl8192se/led.c |  4 ++--
 drivers/net/wireless/realtek/rtlwifi/rtl8192se/phy.c |  5 +++--
 drivers/net/wireless/realtek/rtlwifi/rtl8723ae/fw.c  |  4 ++--
 drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hw.c  |  9 +
 drivers/net/wireless/realtek/rtlwifi/rtl8723ae/led.c |  4 ++--
 drivers/net/wireless/realtek/rtlwifi/rtl8723ae/phy.c | 10 ++
 drivers/net/wireless/realtek/rtlwifi/rtl8723be/fw.c  |  4 ++--
 drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c  | 10 +-
 drivers/net/wireless/realtek/rtlwifi/rtl8723be/led.c |  4 ++--
 drivers/net/wireless/realtek/rtlwifi/rtl8723be/phy.c | 12 +++-
 drivers/net/wireless/realtek/rtlwifi/rtl8821ae/fw.c  |  4 ++--
 drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c  |  9 +
 drivers/net/wireless/realtek/rtlwifi/rtl8821ae/led.c |  4 ++--
 drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c | 20 ++--
 38 files changed, 128 insertions(+), 123 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c 
b/drivers/net/wireless/realtek/rtlwifi/core.c
index 7aee5ebb1..f95760c 100644
--- a/drivers/net/wireless/realtek/rtlwifi/core.c
+++ b/drivers/net/wireless/realtek/rtlwifi/core.c
@@ -765,7 +765,8 @@ static int rtl_op_config(struct ieee80211_hw *hw, u32 
changed)
mac->bw_40 = false;
mac->bw_80 = false;
RT_TRACE(rtlpriv, COMP_ERR, DBG_EMERG,
-"switch case not processed\n");
+"switch case %#x not 
processed\n",
+channel_type);
break;
}
}
diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c 
b/drivers/net/wireless/realtek/rtlwifi/pci.c
index d12586d..0dfa9ea 100644
--- a/drivers/net/wireless/realtek/rtlwifi/pci.c
+++ b/drivers/net/wireless/realtek/rtlwifi/pci.c
@@ -179,7 +179,8 @@ static void _rtl_pci_update_default_setting(struct 
ieee80211_hw *hw)
break;
default:
RT_TRACE(rtlpriv, COMP_ERR, DBG_EMERG,
-"switch case not processed\n");
+"switch case %#x not processed\n",
+rtlpci->const_support_pciaspm);
break;
}

diff --git a/drivers/net/wireless/realtek/rtlwifi/ps.c 
b/drivers/net/wireless/realtek/rtlwifi/ps.c
index 9a64f9b..18d979a 100644
--- a/drivers/net/wireless/realtek/rtlwifi/ps.c
+++ b/drivers/net/wireless/realtek/rtlwifi/ps.c
@@ -151,7 +151,7 @@ static bool rtl_ps_set_rf_state(struct ieee80211_hw *hw,

default:
RT_TRACE(rtlpriv, COMP_ERR, DBG_EMERG,
-"switch case not processed\n");
+"switch case %#x not processed\n", state_toset);
break;
}

diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/fw.c