if/else block default coding style question

if/else block default coding style question

[Nicholas Mc Guire]

Hi !

 There are quite a few places (roughly 90) in the kernel where an 
 if/else if/else block repeats the last "case" presumably as 
 default e.g.  drivers/net/wireless/realtek/rtlwifi/rtl8192c/dm_common.c

   if ((rtlpcipriv->bt_coexist.bt_service == BT_BUSY) &&
       (rtlpcipriv->bt_coexist.bt_rssi_state &
        BT_RSSI_STATE_NORMAL_POWER)) {
           rtl_write_byte(rtlpriv, REG_GPIO_MUXCFG, 0xa0);
   } else if ((rtlpcipriv->bt_coexist.bt_service ==
               BT_OTHER_ACTION) && (rtlpriv->mac80211.mode <
               WIRELESS_MODE_N_24G) &&
               (rtlpcipriv->bt_coexist.bt_rssi_state &
               BT_RSSI_STATE_SPECIAL_LOW)) {
           rtl_write_byte(rtlpriv, REG_GPIO_MUXCFG, 0xa0);
   } else if (rtlpcipriv->bt_coexist.bt_service == BT_PAN) {
           rtl_write_byte(rtlpriv, REG_GPIO_MUXCFG, tmp1byte);
   } else {
           rtl_write_byte(rtlpriv, REG_GPIO_MUXCFG, tmp1byte);
   }

 with the last else if and else being identical.
 So the question is if this is accepted coding style (notably 
 without a coment) or if things like this should be flagged.

 I personally find this irritating as (without a comment) it is hard to say
 if this is a trivial type -> missed case, or if this is
 intended as a default behavior.

 So - before starting to generate a series of patches - should stuff like 
 this be flagged ?

thx!
hofrat

if/else block default coding style question

[Valdis.Kletnieks at vt.edu]

On Sat, 08 Oct 2016 10:40:37 -0000, Nicholas Mc Guire said:

>    } else if (rtlpcipriv->bt_coexist.bt_service == BT_PAN) {
>            rtl_write_byte(rtlpriv, REG_GPIO_MUXCFG, tmp1byte);
>    } else {
>            rtl_write_byte(rtlpriv, REG_GPIO_MUXCFG, tmp1byte);
>    }

That *does* smell like a bug.  If nothing else, the last 'else if'
can be removed.  Most likely case: somebody cut-n-pasted that last
section in and failed to change it to a proper 'default' value
and the code falls through to that one rarely enough that nobody has
noticed.

>  I personally find this irritating as (without a comment) it is hard to say
>  if this is a trivial type -> missed case, or if this is
>  intended as a default behavior.

Unfortunately, it will probably be a really rough job figuring out what
exactly was intended in each case.  Figuring it out in filesystem code
or other similar areas of the kernel wouldn't be too bad - but if it's
a hardware driver, you're going to have to ask an expert (or somebody who
has the hardware) for help.

How did you find there were 90 of them?  Did you cook up a Coccinelle script
for it?

If nothing else, cooking up a test to toss into scripts/coccinelle/misc
would probably be a Good Thing...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 484 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20161008/4a6c5495/attachment.bin 

if/else block default coding style question

[Robert P. J. Day]

On Sat, 8 Oct 2016, Valdis.Kletnieks at vt.edu wrote:

> On Sat, 08 Oct 2016 10:40:37 -0000, Nicholas Mc Guire said:
>
> >    } else if (rtlpcipriv->bt_coexist.bt_service == BT_PAN) {
> >            rtl_write_byte(rtlpriv, REG_GPIO_MUXCFG, tmp1byte);
> >    } else {
> >            rtl_write_byte(rtlpriv, REG_GPIO_MUXCFG, tmp1byte);
> >    }
>
> That *does* smell like a bug.  If nothing else, the last 'else if'
> can be removed.  Most likely case: somebody cut-n-pasted that last
> section in and failed to change it to a proper 'default' value and
> the code falls through to that one rarely enough that nobody has
> noticed.

  if that's the behaviour the developer actually wants, then yes, it's
messy. but i would be very careful just simplifying it wholesale,
since it also smacks of a typo where one copy-and-pasted to add the
default case, then forgot to tweak it to be different.

  rather than "fixing" it, i would bring it to the attention of the
maintainer, and ask him or her to resolve it.

rday

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                        http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================

if/else block default coding style question

[Nicholas Mc Guire]

On Sat, Oct 08, 2016 at 10:58:41AM -0400, Valdis.Kletnieks at vt.edu wrote:
> On Sat, 08 Oct 2016 10:40:37 -0000, Nicholas Mc Guire said:
> 
> >    } else if (rtlpcipriv->bt_coexist.bt_service == BT_PAN) {
> >            rtl_write_byte(rtlpriv, REG_GPIO_MUXCFG, tmp1byte);
> >    } else {
> >            rtl_write_byte(rtlpriv, REG_GPIO_MUXCFG, tmp1byte);
> >    }
> 
> That *does* smell like a bug.  If nothing else, the last 'else if'
> can be removed.  Most likely case: somebody cut-n-pasted that last
> section in and failed to change it to a proper 'default' value
> and the code falls through to that one rarely enough that nobody has
> noticed.
> 
> >  I personally find this irritating as (without a comment) it is hard to say
> >  if this is a trivial type -> missed case, or if this is
> >  intended as a default behavior.
> 
> Unfortunately, it will probably be a really rough job figuring out what
> exactly was intended in each case.  Figuring it out in filesystem code
> or other similar areas of the kernel wouldn't be too bad - but if it's
> a hardware driver, you're going to have to ask an expert (or somebody who
> has the hardware) for help.

Actually fs had one legitimate case where the positional side-effect was in use

<snip: fs/kernfs/file.c:kernfs_fop_open()>
       * Both paths of the branch look the same.  They're supposed to
       * look that way and give @of->mutex different static lockdep keys.
       */
      if (has_mmap)
              mutex_init(&of->mutex);
      else
              mutex_init(&of->mutex);
<snip>

but almost all of the other cases look phony

> 
> How did you find there were 90 of them?  Did you cook up a Coccinelle script
> for it?
>

simple cocci script - Im checking through the reported cases to
see if it is actually reliable. In some cases it was obvious bugs (cut&paste
type) but in others it does seem to be a "coding pattern".
 
> If nothing else, cooking up a test to toss into scripts/coccinelle/misc
> would probably be a Good Thing...

Thats actually the cause of this mail - it just becaem obvious
that some of these if==else are intentional

Now reporting these cases if they should be fixed makes sense
while if this is accepted practice then it would be reporting
false-positives, which would be bad.

thx!
hofrat

if/else block default coding style question

[Nicholas Mc Guire]

On Sat, Oct 08, 2016 at 11:10:10AM -0400, Robert P. J. Day wrote:
> On Sat, 8 Oct 2016, Valdis.Kletnieks at vt.edu wrote:
> 
> > On Sat, 08 Oct 2016 10:40:37 -0000, Nicholas Mc Guire said:
> >
> > >    } else if (rtlpcipriv->bt_coexist.bt_service == BT_PAN) {
> > >            rtl_write_byte(rtlpriv, REG_GPIO_MUXCFG, tmp1byte);
> > >    } else {
> > >            rtl_write_byte(rtlpriv, REG_GPIO_MUXCFG, tmp1byte);
> > >    }
> >
> > That *does* smell like a bug.  If nothing else, the last 'else if'
> > can be removed.  Most likely case: somebody cut-n-pasted that last
> > section in and failed to change it to a proper 'default' value and
> > the code falls through to that one rarely enough that nobody has
> > noticed.
> 
>   if that's the behaviour the developer actually wants, then yes, it's
> messy. but i would be very careful just simplifying it wholesale,
> since it also smacks of a typo where one copy-and-pasted to add the
> default case, then forgot to tweak it to be different.
> 
>   rather than "fixing" it, i would bring it to the attention of the
> maintainer, and ask him or her to resolve it.
>
sure - no point in fixing code one does not understand.

if at all I send "fixes" out as RFCs if it seems like an obvious
case - and in all other cases a notification/questions are sent
but no patch.

thx!
hofrat
 

if/else block default coding style question

[Valdis.Kletnieks at vt.edu]

On Sat, 08 Oct 2016 15:19:18 -0000, Nicholas Mc Guire said:

> Thats actually the cause of this mail - it just becaem obvious
> that some of these if==else are intentional

At the very least, the intentional ones need a comment similar to the
one found in file.c

> Now reporting these cases if they should be fixed makes sense
> while if this is accepted practice then it would be reporting
> false-positives, which would be bad.

Coccinelle can only warn about things that syntactically look suspicious.
The person running Coccinelle needs to have enough neurons to double-check
for comments explaining the odd usage, etc - and the person creating that
sort of code needs to leave some bread crumbs for people reading the code.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 484 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20161008/81c27344/attachment.bin