[PATCH] Staging: rtl8192u: ieee80211_rx: Remove unnecessary code

[PATCH] Staging: rtl8192u: ieee80211_rx: Remove unnecessary code

[Shivani Bhardwaj]

The variable frame_authorized is declared and mentioned in the code but
is not used anywhere so, it should be removed. Also, two case statements
are merged by removing an extra break.
Semantic patch used:
@@
type T;
identifier i;
constant C;
position p != e.p;
@@

- T i@p;
  <+... when != i
- i = C;
  ...+>

Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
---
 drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
index 4b5f32d..130c852 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
@@ -899,7 +899,6 @@ int ieee80211_rx(struct ieee80211_device *ieee, struct sk_buff *skb,
 	struct net_device *wds = NULL;
 	struct sk_buff *skb2 = NULL;
 	struct net_device *wds = NULL;
-	int frame_authorized = 0;
 	int from_assoc_ap = 0;
 	void *sta = NULL;
 #endif
@@ -1106,10 +1105,7 @@ int ieee80211_rx(struct ieee80211_device *ieee, struct sk_buff *skb,
 		switch (hostap_handle_sta_rx(ieee, dev, skb, rx_stats,
 					     wds != NULL)) {
 		case AP_RX_CONTINUE_NOT_AUTHORIZED:
-			frame_authorized = 0;
-			break;
 		case AP_RX_CONTINUE:
-			frame_authorized = 1;
 			break;
 		case AP_RX_DROP:
 			goto rx_dropped;
-- 
2.1.0

Re: [Outreachy kernel] [PATCH] Staging: rtl8192u: ieee80211_rx: Remove unnecessary code

[Sudip Mukherjee]

On Wed, Oct 14, 2015 at 10:48:50PM +0530, Shivani Bhardwaj wrote:
> The variable frame_authorized is declared and mentioned in the code but
> is not used anywhere so, it should be removed. Also, two case statements
> are merged by removing an extra break.
<snip>
> @@ -1106,10 +1105,7 @@ int ieee80211_rx(struct ieee80211_device *ieee, struct sk_buff *skb,
>  		switch (hostap_handle_sta_rx(ieee, dev, skb, rx_stats,
>  					     wds != NULL)) {
>  		case AP_RX_CONTINUE_NOT_AUTHORIZED:
> -			frame_authorized = 0;
> -			break;
>  		case AP_RX_CONTINUE:
> -			frame_authorized = 1;
>  			break;

Just a doubt, it now becomes:
	case AP_RX_CONTINUE_NOT_AUTHORIZED:
	case AP_RX_CONTINUE:
		break;

If these two cases are not doing anything then what is the use of having
them here in the switch-case?

regards
sudip

Re: [Outreachy kernel] [PATCH] Staging: rtl8192u: ieee80211_rx: Remove unnecessary code

[Shivani Bhardwaj]

On Thu, Oct 15, 2015 at 12:20 PM, Sudip Mukherjee
<sudipm.mukherjee@gmail.com> wrote:
> On Wed, Oct 14, 2015 at 10:48:50PM +0530, Shivani Bhardwaj wrote:
>> The variable frame_authorized is declared and mentioned in the code but
>> is not used anywhere so, it should be removed. Also, two case statements
>> are merged by removing an extra break.
> <snip>
>> @@ -1106,10 +1105,7 @@ int ieee80211_rx(struct ieee80211_device *ieee, struct sk_buff *skb,
>>               switch (hostap_handle_sta_rx(ieee, dev, skb, rx_stats,
>>                                            wds != NULL)) {
>>               case AP_RX_CONTINUE_NOT_AUTHORIZED:
>> -                     frame_authorized = 0;
>> -                     break;
>>               case AP_RX_CONTINUE:
>> -                     frame_authorized = 1;
>>                       break;
>
> Just a doubt, it now becomes:
>         case AP_RX_CONTINUE_NOT_AUTHORIZED:
>         case AP_RX_CONTINUE:
>                 break;
>
> If these two cases are not doing anything then what is the use of having
> them here in the switch-case?
>
> regards
> sudip

Because we don't have a default case here and the variable in switch
might still get these values based on some other code.

Re: [Outreachy kernel] [PATCH] Staging: rtl8192u: ieee80211_rx: Remove unnecessary code

[Sudip Mukherjee]

On Thu, Oct 15, 2015 at 12:22:35PM +0530, Shivani Bhardwaj wrote:
> On Thu, Oct 15, 2015 at 12:20 PM, Sudip Mukherjee
> <sudipm.mukherjee@gmail.com> wrote:
> > On Wed, Oct 14, 2015 at 10:48:50PM +0530, Shivani Bhardwaj wrote:
> >> The variable frame_authorized is declared and mentioned in the code but
> >> is not used anywhere so, it should be removed. Also, two case statements
> >> are merged by removing an extra break.
> > <snip>
> >> @@ -1106,10 +1105,7 @@ int ieee80211_rx(struct ieee80211_device *ieee, struct sk_buff *skb,
> >>               switch (hostap_handle_sta_rx(ieee, dev, skb, rx_stats,
> >>                                            wds != NULL)) {
> >>               case AP_RX_CONTINUE_NOT_AUTHORIZED:
> >> -                     frame_authorized = 0;
> >> -                     break;
> >>               case AP_RX_CONTINUE:
> >> -                     frame_authorized = 1;
> >>                       break;
> >
> > Just a doubt, it now becomes:
> >         case AP_RX_CONTINUE_NOT_AUTHORIZED:
> >         case AP_RX_CONTINUE:
> >                 break;
> >
> > If these two cases are not doing anything then what is the use of having
> > them here in the switch-case?
> >
> > regards
> > sudip
> 
> Because we don't have a default case here and the variable in switch
> might still get these values based on some other code.

I think default case is to handle all cases which are not given by case
statement. What will happen if variable gets these values but these
cases are not mentioned? switch-case will see that these cases are not
mentioned and will exit the switch-case. In the situation where these
cases are mentioned, switch-case will see these cases exist, will come
to these cases and breaks out of switch-case.
Am I understanding anything wrong here?

regards
sudip

Re: [Outreachy kernel] [PATCH] Staging: rtl8192u: ieee80211_rx: Remove unnecessary code

[Shivani Bhardwaj]

On Thu, Oct 15, 2015 at 12:31 PM, Sudip Mukherjee
<sudipm.mukherjee@gmail.com> wrote:
> On Thu, Oct 15, 2015 at 12:22:35PM +0530, Shivani Bhardwaj wrote:
>> On Thu, Oct 15, 2015 at 12:20 PM, Sudip Mukherjee
>> <sudipm.mukherjee@gmail.com> wrote:
>> > On Wed, Oct 14, 2015 at 10:48:50PM +0530, Shivani Bhardwaj wrote:
>> >> The variable frame_authorized is declared and mentioned in the code but
>> >> is not used anywhere so, it should be removed. Also, two case statements
>> >> are merged by removing an extra break.
>> > <snip>
>> >> @@ -1106,10 +1105,7 @@ int ieee80211_rx(struct ieee80211_device *ieee, struct sk_buff *skb,
>> >>               switch (hostap_handle_sta_rx(ieee, dev, skb, rx_stats,
>> >>                                            wds != NULL)) {
>> >>               case AP_RX_CONTINUE_NOT_AUTHORIZED:
>> >> -                     frame_authorized = 0;
>> >> -                     break;
>> >>               case AP_RX_CONTINUE:
>> >> -                     frame_authorized = 1;
>> >>                       break;
>> >
>> > Just a doubt, it now becomes:
>> >         case AP_RX_CONTINUE_NOT_AUTHORIZED:
>> >         case AP_RX_CONTINUE:
>> >                 break;
>> >
>> > If these two cases are not doing anything then what is the use of having
>> > them here in the switch-case?
>> >
>> > regards
>> > sudip
>>
>> Because we don't have a default case here and the variable in switch
>> might still get these values based on some other code.
>
> I think default case is to handle all cases which are not given by case
> statement. What will happen if variable gets these values but these
> cases are not mentioned? switch-case will see that these cases are not
> mentioned and will exit the switch-case. In the situation where these
> cases are mentioned, switch-case will see these cases exist, will come
> to these cases and breaks out of switch-case.
> Am I understanding anything wrong here?
>
> regards
> sudip

No, you are completely right. :) So, should I remove these cases? And
if these cases are to be removed there won't be a need of switch case
at all. Or should I leave it thinking that programmer might have to
make some changes?
Thank you.

Re: [Outreachy kernel] [PATCH] Staging: rtl8192u: ieee80211_rx: Remove unnecessary code

[Sudip Mukherjee]

On Thu, Oct 15, 2015 at 12:37:42PM +0530, Shivani Bhardwaj wrote:
> On Thu, Oct 15, 2015 at 12:31 PM, Sudip Mukherjee
> <sudipm.mukherjee@gmail.com> wrote:
> > On Thu, Oct 15, 2015 at 12:22:35PM +0530, Shivani Bhardwaj wrote:
> >> On Thu, Oct 15, 2015 at 12:20 PM, Sudip Mukherjee
> >> <sudipm.mukherjee@gmail.com> wrote:
> >> > On Wed, Oct 14, 2015 at 10:48:50PM +0530, Shivani Bhardwaj wrote:
> >> >> The variable frame_authorized is declared and mentioned in the code but
> >> >> is not used anywhere so, it should be removed. Also, two case statements
> >> >> are merged by removing an extra break.
> >> > <snip>
> >> >> @@ -1106,10 +1105,7 @@ int ieee80211_rx(struct ieee80211_device *ieee, struct sk_buff *skb,
> >> >>               switch (hostap_handle_sta_rx(ieee, dev, skb, rx_stats,
> >> >>                                            wds != NULL)) {
> >> >>               case AP_RX_CONTINUE_NOT_AUTHORIZED:
> >> >> -                     frame_authorized = 0;
> >> >> -                     break;
> >> >>               case AP_RX_CONTINUE:
> >> >> -                     frame_authorized = 1;
> >> >>                       break;
> >> >
> >> > Just a doubt, it now becomes:
> >> >         case AP_RX_CONTINUE_NOT_AUTHORIZED:
> >> >         case AP_RX_CONTINUE:
> >> >                 break;
> >> >
> >> > If these two cases are not doing anything then what is the use of having
> >> > them here in the switch-case?
> >> >
> >> > regards
> >> > sudip
> >>
> >> Because we don't have a default case here and the variable in switch
> >> might still get these values based on some other code.
> >
> > I think default case is to handle all cases which are not given by case
> > statement. What will happen if variable gets these values but these
> > cases are not mentioned? switch-case will see that these cases are not
> > mentioned and will exit the switch-case. In the situation where these
> > cases are mentioned, switch-case will see these cases exist, will come
> > to these cases and breaks out of switch-case.
> > Am I understanding anything wrong here?
> >
> > regards
> > sudip
> 
> No, you are completely right. :) So, should I remove these cases? And
> if these cases are to be removed there won't be a need of switch case
> at all. Or should I leave it thinking that programmer might have to
> make some changes?

The main change required for this driver will be to use mac80211, so
that means all these codes regarding ieee80211 has to be removed one
day. So, I guess you can remove these cases. Anyways, lets wait for
Greg's comment then change if required.

regards
sudip

Re: [Outreachy kernel] [PATCH] Staging: rtl8192u: ieee80211_rx: Remove unnecessary code

[Shivani Bhardwaj]

On Thu, Oct 15, 2015 at 12:49 PM, Sudip Mukherjee
<sudipm.mukherjee@gmail.com> wrote:
> On Thu, Oct 15, 2015 at 12:37:42PM +0530, Shivani Bhardwaj wrote:
>> On Thu, Oct 15, 2015 at 12:31 PM, Sudip Mukherjee
>> <sudipm.mukherjee@gmail.com> wrote:
>> > On Thu, Oct 15, 2015 at 12:22:35PM +0530, Shivani Bhardwaj wrote:
>> >> On Thu, Oct 15, 2015 at 12:20 PM, Sudip Mukherjee
>> >> <sudipm.mukherjee@gmail.com> wrote:
>> >> > On Wed, Oct 14, 2015 at 10:48:50PM +0530, Shivani Bhardwaj wrote:
>> >> >> The variable frame_authorized is declared and mentioned in the code but
>> >> >> is not used anywhere so, it should be removed. Also, two case statements
>> >> >> are merged by removing an extra break.
>> >> > <snip>
>> >> >> @@ -1106,10 +1105,7 @@ int ieee80211_rx(struct ieee80211_device *ieee, struct sk_buff *skb,
>> >> >>               switch (hostap_handle_sta_rx(ieee, dev, skb, rx_stats,
>> >> >>                                            wds != NULL)) {
>> >> >>               case AP_RX_CONTINUE_NOT_AUTHORIZED:
>> >> >> -                     frame_authorized = 0;
>> >> >> -                     break;
>> >> >>               case AP_RX_CONTINUE:
>> >> >> -                     frame_authorized = 1;
>> >> >>                       break;
>> >> >
>> >> > Just a doubt, it now becomes:
>> >> >         case AP_RX_CONTINUE_NOT_AUTHORIZED:
>> >> >         case AP_RX_CONTINUE:
>> >> >                 break;
>> >> >
>> >> > If these two cases are not doing anything then what is the use of having
>> >> > them here in the switch-case?
>> >> >
>> >> > regards
>> >> > sudip
>> >>
>> >> Because we don't have a default case here and the variable in switch
>> >> might still get these values based on some other code.
>> >
>> > I think default case is to handle all cases which are not given by case
>> > statement. What will happen if variable gets these values but these
>> > cases are not mentioned? switch-case will see that these cases are not
>> > mentioned and will exit the switch-case. In the situation where these
>> > cases are mentioned, switch-case will see these cases exist, will come
>> > to these cases and breaks out of switch-case.
>> > Am I understanding anything wrong here?
>> >
>> > regards
>> > sudip
>>
>> No, you are completely right. :) So, should I remove these cases? And
>> if these cases are to be removed there won't be a need of switch case
>> at all. Or should I leave it thinking that programmer might have to
>> make some changes?
>
> The main change required for this driver will be to use mac80211, so
> that means all these codes regarding ieee80211 has to be removed one
> day. So, I guess you can remove these cases. Anyways, lets wait for
> Greg's comment then change if required.
>
> regards
> sudip

All right.
Thank you. :)

Re: [Outreachy kernel] [PATCH] Staging: rtl8192u: ieee80211_rx: Remove unnecessary code

[Julia Lawall]

On Thu, 15 Oct 2015, Sudip Mukherjee wrote:

> On Thu, Oct 15, 2015 at 12:22:35PM +0530, Shivani Bhardwaj wrote:
> > On Thu, Oct 15, 2015 at 12:20 PM, Sudip Mukherjee
> > <sudipm.mukherjee@gmail.com> wrote:
> > > On Wed, Oct 14, 2015 at 10:48:50PM +0530, Shivani Bhardwaj wrote:
> > >> The variable frame_authorized is declared and mentioned in the code but
> > >> is not used anywhere so, it should be removed. Also, two case statements
> > >> are merged by removing an extra break.
> > > <snip>
> > >> @@ -1106,10 +1105,7 @@ int ieee80211_rx(struct ieee80211_device *ieee, struct sk_buff *skb,
> > >>               switch (hostap_handle_sta_rx(ieee, dev, skb, rx_stats,
> > >>                                            wds != NULL)) {
> > >>               case AP_RX_CONTINUE_NOT_AUTHORIZED:
> > >> -                     frame_authorized = 0;
> > >> -                     break;
> > >>               case AP_RX_CONTINUE:
> > >> -                     frame_authorized = 1;
> > >>                       break;
> > >
> > > Just a doubt, it now becomes:
> > >         case AP_RX_CONTINUE_NOT_AUTHORIZED:
> > >         case AP_RX_CONTINUE:
> > >                 break;
> > >
> > > If these two cases are not doing anything then what is the use of having
> > > them here in the switch-case?
> > >
> > > regards
> > > sudip
> >
> > Because we don't have a default case here and the variable in switch
> > might still get these values based on some other code.
>
> I think default case is to handle all cases which are not given by case
> statement. What will happen if variable gets these values but these
> cases are not mentioned? switch-case will see that these cases are not
> mentioned and will exit the switch-case. In the situation where these
> cases are mentioned, switch-case will see these cases exist, will come
> to these cases and breaks out of switch-case.
> Am I understanding anything wrong here?

If there is no default and a case is not mentioned, then nothing should
happen on the switch.  I guess one could test this easily, if there are
any doubts.  Maybe having cases that do nothing protects against adding a
default that does something?

julia