[PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

In next-20131122 tree, if "sdata->vif.type != NL80211_IFTYPE_AP",
'chanctx_conf' will be not initialized, so need check it. Related
warning (with allmodconfig under hexagon):

    CC [M]  net/mac80211/tx.o
  net/mac80211/tx.c: In function 'ieee80211_subif_start_xmit':
  net/mac80211/tx.c:1827:27: warning: 'chanctx_conf' may be used uninitialized in this function [-Wuninitialized]


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 net/mac80211/tx.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index c558b24..f3245d6 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1814,8 +1814,9 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
 			break;
 		/* fall through */
 	case NL80211_IFTYPE_AP:
-		if (sdata->vif.type == NL80211_IFTYPE_AP)
-			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
+		if (sdata->vif.type != NL80211_IFTYPE_AP)
+			goto fail_rcu;
+		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
 		if (!chanctx_conf)
 			goto fail_rcu;
 		fc |= cpu_to_le16(IEEE80211_FCTL_FROMDS);
-- 
1.7.7.6

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Johannes Berg]

> +++ b/net/mac80211/tx.c
> @@ -1814,8 +1814,9 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>  			break;
>  		/* fall through */
>  	case NL80211_IFTYPE_AP:
> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> +		if (sdata->vif.type != NL80211_IFTYPE_AP)
> +			goto fail_rcu;
> +		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);

This change is completely wrong.

johannes

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

On 11/29/2013 11:38 PM, Johannes Berg wrote:
> 
>> +++ b/net/mac80211/tx.c
>> @@ -1814,8 +1814,9 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>>  			break;
>>  		/* fall through */
>>  	case NL80211_IFTYPE_AP:
>> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
>> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>> +		if (sdata->vif.type != NL80211_IFTYPE_AP)
>> +			goto fail_rcu;
>> +		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> 
> This change is completely wrong.
> 

Oh, it is.

Hmm... for me, this work flow still can be implemented with a little
clearer way (at least it will avoid related warning):

-------------------------diff begin------------------------------

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index c558b24..7076128 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1810,14 +1810,14 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
 		if (!chanctx_conf)
 			goto fail_rcu;
 		band = chanctx_conf->def.chan->band;
-		if (sta)
-			break;
-		/* fall through */
+		if (!sta)
+			goto try_next;
+		break;
 	case NL80211_IFTYPE_AP:
-		if (sdata->vif.type == NL80211_IFTYPE_AP)
-			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
+		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
 		if (!chanctx_conf)
 			goto fail_rcu;
+try_next:
 		fc |= cpu_to_le16(IEEE80211_FCTL_FROMDS);
 		/* DA BSSID SA */
 		memcpy(hdr.addr1, skb->data, ETH_ALEN);



-------------------------diff end--------------------------------


Thanks.
-- 
Chen Gang

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Johannes Berg]

On Sat, 2013-11-30 at 19:59 +0800, Chen Gang wrote:
> On 11/29/2013 11:38 PM, Johannes Berg wrote:
> > 
> >> +++ b/net/mac80211/tx.c
> >> @@ -1814,8 +1814,9 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
> >>  			break;
> >>  		/* fall through */
> >>  	case NL80211_IFTYPE_AP:
> >> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
> >> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> >> +		if (sdata->vif.type != NL80211_IFTYPE_AP)
> >> +			goto fail_rcu;
> >> +		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> > 
> > This change is completely wrong.
> > 
> 
> Oh, it is.
> 
> Hmm... for me, this work flow still can be implemented with a little
> clearer way (at least it will avoid related warning):
> 
> -------------------------diff begin------------------------------
> 
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index c558b24..7076128 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -1810,14 +1810,14 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>  		if (!chanctx_conf)
>  			goto fail_rcu;
>  		band = chanctx_conf->def.chan->band;
> -		if (sta)
> -			break;
> -		/* fall through */
> +		if (!sta)
> +			goto try_next;
> +		break;
>  	case NL80211_IFTYPE_AP:
> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> +		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>  		if (!chanctx_conf)
>  			goto fail_rcu;
> +try_next:

I don't think that's better than the (fairly obvious) fall-through, and
has a pretty odd goto. Also, depending on the compiler, it still knows
the previous case label and doesn't warn.

johannes

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

On 11/30/2013 08:53 PM, Johannes Berg wrote:
> On Sat, 2013-11-30 at 19:59 +0800, Chen Gang wrote:
>> On 11/29/2013 11:38 PM, Johannes Berg wrote:
>>>
>>>> +++ b/net/mac80211/tx.c
>>>> @@ -1814,8 +1814,9 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>>>>  			break;
>>>>  		/* fall through */
>>>>  	case NL80211_IFTYPE_AP:
>>>> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
>>>> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>>>> +		if (sdata->vif.type != NL80211_IFTYPE_AP)
>>>> +			goto fail_rcu;
>>>> +		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>>>
>>> This change is completely wrong.
>>>
>>
>> Oh, it is.
>>
>> Hmm... for me, this work flow still can be implemented with a little
>> clearer way (at least it will avoid related warning):
>>
>> -------------------------diff begin------------------------------
>>
>> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
>> index c558b24..7076128 100644
>> --- a/net/mac80211/tx.c
>> +++ b/net/mac80211/tx.c
>> @@ -1810,14 +1810,14 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>>  		if (!chanctx_conf)
>>  			goto fail_rcu;
>>  		band = chanctx_conf->def.chan->band;
>> -		if (sta)
>> -			break;
>> -		/* fall through */
>> +		if (!sta)
>> +			goto try_next;
>> +		break;
>>  	case NL80211_IFTYPE_AP:
>> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
>> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>> +		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>>  		if (!chanctx_conf)
>>  			goto fail_rcu;
>> +try_next:
> 
> I don't think that's better than the (fairly obvious) fall-through, and
> has a pretty odd goto. Also, depending on the compiler, it still knows
> the previous case label and doesn't warn.
> 

Yeah, fall-through is obvious. But check 'A' again just near by "case A"
seems a little strange, and some of compilers (or some of versions) are
really not quit smart enough to know it is not a warning.

Hmm... for me, if the code (implementation) can express real logical
work flow as much as directly and simply, the code (implementation) is
clear enough (don't mind whether use 'goto' or not).


And originally, at first, I am really not quite careful enough, and sent
an incorrect patch after noticed the related compiler's warning. :-)


Thanks.
-- 
Chen Gang

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

On 11/30/2013 09:50 PM, Chen Gang wrote:
> On 11/30/2013 08:53 PM, Johannes Berg wrote:
>> On Sat, 2013-11-30 at 19:59 +0800, Chen Gang wrote:
>>> On 11/29/2013 11:38 PM, Johannes Berg wrote:
>>>>
>>>>> +++ b/net/mac80211/tx.c
>>>>> @@ -1814,8 +1814,9 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>>>>>  			break;
>>>>>  		/* fall through */
>>>>>  	case NL80211_IFTYPE_AP:
>>>>> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
>>>>> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>>>>> +		if (sdata->vif.type != NL80211_IFTYPE_AP)
>>>>> +			goto fail_rcu;
>>>>> +		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>>>>
>>>> This change is completely wrong.
>>>>
>>>
>>> Oh, it is.
>>>
>>> Hmm... for me, this work flow still can be implemented with a little
>>> clearer way (at least it will avoid related warning):
>>>
>>> -------------------------diff begin------------------------------
>>>
>>> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
>>> index c558b24..7076128 100644
>>> --- a/net/mac80211/tx.c
>>> +++ b/net/mac80211/tx.c
>>> @@ -1810,14 +1810,14 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>>>  		if (!chanctx_conf)
>>>  			goto fail_rcu;
>>>  		band = chanctx_conf->def.chan->band;
>>> -		if (sta)
>>> -			break;
>>> -		/* fall through */
>>> +		if (!sta)
>>> +			goto try_next;
>>> +		break;
>>>  	case NL80211_IFTYPE_AP:
>>> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
>>> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>>> +		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>>>  		if (!chanctx_conf)
>>>  			goto fail_rcu;
>>> +try_next:
>>
>> I don't think that's better than the (fairly obvious) fall-through, and
>> has a pretty odd goto. Also, depending on the compiler, it still knows
>> the previous case label and doesn't warn.
>>
> 
> Yeah, fall-through is obvious. But check 'A' again just near by "case A"
> seems a little strange, and some of compilers (or some of versions) are
> really not quit smart enough to know it is not a warning.
> 

Sorry, the paragraph above may lead misunderstanding, I repeated again:

 - fall-through is obvious (although I did not notice it, originally).

 - Check 'A' again just near by "case A" seems a little strange.

 - Some compilers aren't quit smart enough to know 'chanctx_conf' is OK.


Thanks.

> Hmm... for me, if the code (implementation) can express real logical
> work flow as much as directly and simply, the code (implementation) is
> clear enough (don't mind whether use 'goto' or not).
> 
> 
> And originally, at first, I am really not quite careful enough, and sent
> an incorrect patch after noticed the related compiler's warning. :-)
> 
> 
> Thanks.
> 


-- 
Chen Gang

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Johannes Berg]

On Sat, 2013-11-30 at 22:02 +0800, Chen Gang wrote:

> >>>  	case NL80211_IFTYPE_AP:
> >>> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
> >>> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> >>> +		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> >>>  		if (!chanctx_conf)
> >>>  			goto fail_rcu;
> >>> +try_next:
> >>
> >> I don't think that's better than the (fairly obvious) fall-through, and
> >> has a pretty odd goto. Also, depending on the compiler, it still knows
> >> the previous case label and doesn't warn.
> >>
> > 
> > Yeah, fall-through is obvious. But check 'A' again just near by "case A"
> > seems a little strange, and some of compilers (or some of versions) are
> > really not quit smart enough to know it is not a warning.
> > 
> 
> Sorry, the paragraph above may lead misunderstanding, I repeated again:
> 
>  - fall-through is obvious (although I did not notice it, originally).
> 
>  - Check 'A' again just near by "case A" seems a little strange.
> 
>  - Some compilers aren't quit smart enough to know 'chanctx_conf' is OK.

I know. If you have any good ideas of how to make it more obvious to the
compiler, I'm all ears, I just don't like any of the solutions offered
so far (and you aren't the first to do so either) :-)

FWIW, I find the label to be odd because if you're familiar with the
code then AP/AP_VLAN *should* be identical except for two special things
that are now linearly & neatly handled in the code (the first being the
4-addr station, the second the chanctx assignment which always has to be
done regardless of 4-addr). IMHO the == check after case should be
enough to make a human reader take a closer look. I understand that you
didn't and that's OK since you were just trying to squelch compile
warnings, but I don't see that this one warrants much attention.

johannes

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Joe Perches]

On Sat, 2013-11-30 at 21:08 +0100, Johannes Berg wrote:
> On Sat, 2013-11-30 at 22:02 +0800, Chen Gang wrote:
> 
> > >>>  	case NL80211_IFTYPE_AP:
> > >>> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
> > >>> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> > >>> +		chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> > >>>  		if (!chanctx_conf)
> > >>>  			goto fail_rcu;
> > >>> +try_next:
> > >>
> > >> I don't think that's better than the (fairly obvious) fall-through, and
> > >> has a pretty odd goto. Also, depending on the compiler, it still knows
> > >> the previous case label and doesn't warn.
> > >>
> > > 
> > > Yeah, fall-through is obvious. But check 'A' again just near by "case A"
> > > seems a little strange, and some of compilers (or some of versions) are
> > > really not quit smart enough to know it is not a warning.
> > > 
> > 
> > Sorry, the paragraph above may lead misunderstanding, I repeated again:
> > 
> >  - fall-through is obvious (although I did not notice it, originally).
> > 
> >  - Check 'A' again just near by "case A" seems a little strange.
> > 
> >  - Some compilers aren't quit smart enough to know 'chanctx_conf' is OK.
> 
> I know. If you have any good ideas of how to make it more obvious to the
> compiler, I'm all ears, I just don't like any of the solutions offered
> so far (and you aren't the first to do so either) :-)
> 
> FWIW, I find the label to be odd because if you're familiar with the
> code then AP/AP_VLAN *should* be identical except for two special things
> that are now linearly & neatly handled in the code (the first being the
> 4-addr station, the second the chanctx assignment which always has to be
> done regardless of 4-addr). IMHO the == check after case should be
> enough to make a human reader take a closer look. I understand that you
> didn't and that's OK since you were just trying to squelch compile
> warnings, but I don't see that this one warrants much attention.

The label/test could be moved to save a couple of lines
of duplicated code.

Maybe:

 net/mac80211/tx.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 70b5a05..b2160f4 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1777,18 +1777,16 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
 		}
 		ap_sdata = container_of(sdata->bss, struct ieee80211_sub_if_data,
 					u.ap);
-		chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf);
-		if (!chanctx_conf)
-			goto fail_rcu;
-		band = chanctx_conf->def.chan->band;
-		if (sta)
-			break;
 		/* fall through */
 	case NL80211_IFTYPE_AP:
-		if (sdata->vif.type == NL80211_IFTYPE_AP)
-			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
+		chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf);
 		if (!chanctx_conf)
 			goto fail_rcu;
+		if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) {
+			band = chanctx_conf->def.chan->band;
+			if (sta)
+				break;
+		}
 		fc |= cpu_to_le16(IEEE80211_FCTL_FROMDS);
 		/* DA BSSID SA */
 		memcpy(hdr.addr1, skb->data, ETH_ALEN);

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

On 12/01/2013 04:39 AM, Joe Perches wrote:
> On Sat, 2013-11-30 at 21:08 +0100, Johannes Berg wrote:
>> On Sat, 2013-11-30 at 22:02 +0800, Chen Gang wrote:
>>>  - fall-through is obvious (although I did not notice it, originally).
>>>
>>>  - Check 'A' again just near by "case A" seems a little strange.
>>>
>>>  - Some compilers aren't quit smart enough to know 'chanctx_conf' is OK.
>>
>> I know. If you have any good ideas of how to make it more obvious to the
>> compiler, I'm all ears, I just don't like any of the solutions offered
>> so far (and you aren't the first to do so either) :-)
>>

OK, thanks.

>> FWIW, I find the label to be odd because if you're familiar with the
>> code then AP/AP_VLAN *should* be identical except for two special things
>> that are now linearly & neatly handled in the code (the first being the
>> 4-addr station, the second the chanctx assignment which always has to be
>> done regardless of 4-addr). IMHO the == check after case should be
>> enough to make a human reader take a closer look. I understand that you
>> didn't and that's OK since you were just trying to squelch compile
>> warnings, but I don't see that this one warrants much attention.
> 
> The label/test could be moved to save a couple of lines
> of duplicated code.
> 

Hmm... for me, it must be an implementation which can satisfy all of us.

If ieee80211_subif_start_xmit() is not performance sensitive (I guess
so), we can use some short static functions instead of some code blocks
within ieee80211_subif_start_xmit().

 - ieee80211_subif_start_xmit() is a long function (600+ lines).

 - use short static function can share some code.

 - if code can be shared, the work flow can be more clearer too (don't
   need fall-through or goto).


And I guess, the implementation below can cause panic when
"sdata->vif.type == NL80211_IFTYPE_AP_VLAN".  :-(

> Maybe:
> 
>  net/mac80211/tx.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 70b5a05..b2160f4 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -1777,18 +1777,16 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>  		}
>  		ap_sdata = container_of(sdata->bss, struct ieee80211_sub_if_data,
>  					u.ap);
> -		chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf);
> -		if (!chanctx_conf)
> -			goto fail_rcu;
> -		band = chanctx_conf->def.chan->band;
> -		if (sta)
> -			break;
>  		/* fall through */
>  	case NL80211_IFTYPE_AP:
> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> +		chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf);
>  		if (!chanctx_conf)
>  			goto fail_rcu;
> +		if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) {
> +			band = chanctx_conf->def.chan->band;
> +			if (sta)
> +				break;
> +		}
>  		fc |= cpu_to_le16(IEEE80211_FCTL_FROMDS);
>  		/* DA BSSID SA */
>  		memcpy(hdr.addr1, skb->data, ETH_ALEN);
> 
> 


-- 
Chen Gang

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

On 12/01/2013 07:48 AM, Chen Gang wrote:
> On 12/01/2013 04:39 AM, Joe Perches wrote:
>> On Sat, 2013-11-30 at 21:08 +0100, Johannes Berg wrote:
>>> On Sat, 2013-11-30 at 22:02 +0800, Chen Gang wrote:
>>>>  - fall-through is obvious (although I did not notice it, originally).
>>>>
>>>>  - Check 'A' again just near by "case A" seems a little strange.
>>>>
>>>>  - Some compilers aren't quit smart enough to know 'chanctx_conf' is OK.
>>>
>>> I know. If you have any good ideas of how to make it more obvious to the
>>> compiler, I'm all ears, I just don't like any of the solutions offered
>>> so far (and you aren't the first to do so either) :-)
>>>
> 
> OK, thanks.
> 
>>> FWIW, I find the label to be odd because if you're familiar with the
>>> code then AP/AP_VLAN *should* be identical except for two special things
>>> that are now linearly & neatly handled in the code (the first being the
>>> 4-addr station, the second the chanctx assignment which always has to be
>>> done regardless of 4-addr). IMHO the == check after case should be
>>> enough to make a human reader take a closer look. I understand that you
>>> didn't and that's OK since you were just trying to squelch compile
>>> warnings, but I don't see that this one warrants much attention.
>>
>> The label/test could be moved to save a couple of lines
>> of duplicated code.
>>
> 
> Hmm... for me, it must be an implementation which can satisfy all of us.
> 
> If ieee80211_subif_start_xmit() is not performance sensitive (I guess
> so), we can use some short static functions instead of some code blocks
> within ieee80211_subif_start_xmit().
> 
>  - ieee80211_subif_start_xmit() is a long function (600+ lines).
> 

Oh, one typo issue (need use 400+ instead of 600+), it is a long
function (although not quite long).

>  - use short static function can share some code.
> 
>  - if code can be shared, the work flow can be more clearer too (don't
>    need fall-through or goto).
> 
> 
> And I guess, the implementation below can cause panic when
> "sdata->vif.type == NL80211_IFTYPE_AP_VLAN".  :-(
> 
>> Maybe:
>>
>>  net/mac80211/tx.c | 14 ++++++--------
>>  1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
>> index 70b5a05..b2160f4 100644
>> --- a/net/mac80211/tx.c
>> +++ b/net/mac80211/tx.c
>> @@ -1777,18 +1777,16 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>>  		}
>>  		ap_sdata = container_of(sdata->bss, struct ieee80211_sub_if_data,
>>  					u.ap);
>> -		chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf);
>> -		if (!chanctx_conf)
>> -			goto fail_rcu;
>> -		band = chanctx_conf->def.chan->band;
>> -		if (sta)
>> -			break;
>>  		/* fall through */
>>  	case NL80211_IFTYPE_AP:
>> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
>> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>> +		chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf);
>>  		if (!chanctx_conf)
>>  			goto fail_rcu;
>> +		if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) {
>> +			band = chanctx_conf->def.chan->band;
>> +			if (sta)
>> +				break;
>> +		}
>>  		fc |= cpu_to_le16(IEEE80211_FCTL_FROMDS);
>>  		/* DA BSSID SA */
>>  		memcpy(hdr.addr1, skb->data, ETH_ALEN);
>>
>>
> 
> 


-- 
Chen Gang

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Johannes Berg]

On Sun, 2013-12-01 at 07:48 +0800, Chen Gang wrote:

> If ieee80211_subif_start_xmit() is not performance sensitive (I guess
> so), we can use some short static functions instead of some code blocks
> within ieee80211_subif_start_xmit().
> 
>  - ieee80211_subif_start_xmit() is a long function (600+ lines).
> 
>  - use short static function can share some code.
> 
>  - if code can be shared, the work flow can be more clearer too (don't
>    need fall-through or goto).

Frankly, I'm getting tired of discussing this. Please don't try to
rewrite this code until you've understood it. You suggesting that
"start_xmit()" isn't a performance sensitive function makes me realize
you haven't even tried.

johannes

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

On 12/01/2013 05:37 PM, Johannes Berg wrote:
> On Sun, 2013-12-01 at 07:48 +0800, Chen Gang wrote:
> 
>> If ieee80211_subif_start_xmit() is not performance sensitive (I guess
>> so), we can use some short static functions instead of some code blocks
>> within ieee80211_subif_start_xmit().
>>
>>  - ieee80211_subif_start_xmit() is a long function (600+ lines).
>>
>>  - use short static function can share some code.
>>
>>  - if code can be shared, the work flow can be more clearer too (don't
>>    need fall-through or goto).
> 
> Frankly, I'm getting tired of discussing this. Please don't try to
> rewrite this code until you've understood it. You suggesting that
> "start_xmit()" isn't a performance sensitive function makes me realize
> you haven't even tried.
> 

OK, thank you for "you are getting tired ...". Please help try when you
have time.  :-)

No I didn't try -- so use 'if' and 'guess' for discussing.

Hmm... if it is performance sensitive:

 - use static function instead of some code block and try to share them.
   it adds additional instructions, but can shrink binary code size.
   if the performance is acceptable, it is the best way to me.

 - else (1st way not acceptable), use macro instead of static function.
   it expends binary code size, but can save some instructions.
   if the performance is acceptable, it is the acceptable way to me.

 - If neither of static function nor macro are acceptable,
   I still prefer to use 'goto' instead of 'fall-through'.
   that can let all compilers feel well, and can not feel any strange.

     normally, when prev case uses fall-through,
     it need be sure of next case need not notice about it (prev case).
     or it will make a little strange for readers when read next case.


Welcome any suggestions or completions.

Thanks.
-- 
Chen Gang

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Johannes Berg]

On Sat, 2013-11-30 at 12:39 -0800, Joe Perches wrote:

> +++ b/net/mac80211/tx.c
> @@ -1777,18 +1777,16 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>  		}
>  		ap_sdata = container_of(sdata->bss, struct ieee80211_sub_if_data,
>  					u.ap);
> -		chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf);
> -		if (!chanctx_conf)
> -			goto fail_rcu;
> -		band = chanctx_conf->def.chan->band;
> -		if (sta)
> -			break;
>  		/* fall through */
>  	case NL80211_IFTYPE_AP:
> -		if (sdata->vif.type == NL80211_IFTYPE_AP)
> -			chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> +		chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf);

Good try, but no, now ap_sdata isn't even assigned. :)

johannes

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Joe Perches]

On Sun, 2013-12-01 at 10:35 +0100, Johannes Berg wrote:
> Good try, but no, now ap_sdata isn't even assigned. :)

Right.  Oh well.  There's no improving this without
significant rewrite.  Even then, there may not be much
overall improvement.

btw: Chen, I think fall-throughs are fine as long as
they are commented appropriately.

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

On 12/02/2013 06:38 AM, Joe Perches wrote:
> On Sun, 2013-12-01 at 10:35 +0100, Johannes Berg wrote:
>> Good try, but no, now ap_sdata isn't even assigned. :)
> 
> Right.  Oh well.  There's no improving this without
> significant rewrite.  Even then, there may not be much
> overall improvement.
> 
> btw: Chen, I think fall-throughs are fine as long as
> they are commented appropriately.
> 

Hmm... for me, when use fall-through, need 2 things:

 - need a comment.

 - next case need not notice about prev case which fall-through.


If we can not fit the 2 things above, either can not use sub functions
or macros for it because of performance reason, 'goto' is better than
'fall-through'.

For me, in most cases, when a function becomes a long function, it is
always better to use sub-fuctions or macros instead of some blocks (but
it is really not urgent :-) ).


Thanks.
-- 
Chen Gang

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Johannes Berg]

On Sun, 2013-12-01 at 14:38 -0800, Joe Perches wrote:
> On Sun, 2013-12-01 at 10:35 +0100, Johannes Berg wrote:
> > Good try, but no, now ap_sdata isn't even assigned. :)
> 
> Right.  Oh well.  There's no improving this without
> significant rewrite.  Even then, there may not be much
> overall improvement.

I could see an improvement if we were to actually make changes to assign
the pointer based on the iftype, when that changes, so that each
function only has to handle the right type and we don't need the switch
statement at all...

But that's tricky to get right and I'm between vacations and holidays
and all that ...

johannes

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

On 12/02/2013 10:48 PM, Johannes Berg wrote:
> On Sun, 2013-12-01 at 14:38 -0800, Joe Perches wrote:
>> On Sun, 2013-12-01 at 10:35 +0100, Johannes Berg wrote:
>>> Good try, but no, now ap_sdata isn't even assigned. :)
>>
>> Right.  Oh well.  There's no improving this without
>> significant rewrite.  Even then, there may not be much
>> overall improvement.
> 
> I could see an improvement if we were to actually make changes to assign
> the pointer based on the iftype, when that changes, so that each
> function only has to handle the right type and we don't need the switch
> statement at all...
> 

OK, thanks, it is one of good ideas to me (which I can not find).  :-)


> But that's tricky to get right and I'm between vacations and holidays
> and all that ...
> 

It is really not urgent, and for keeping quality, it is necessary to
spend suitable time resource (e.g 1 hour or more) to  make, review and
test this kind of patch carefully by oneself.

So could you please help improve it when you have suitable related time
resources?


Thanks.
-- 
Chen Gang

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Johannes Berg]

On Wed, 2013-12-04 at 10:12 +0800, Chen Gang wrote:

> It is really not urgent, and for keeping quality, it is necessary to
> spend suitable time resource (e.g 1 hour or more) to  make, review and
> test this kind of patch carefully by oneself.
> 
> So could you please help improve it when you have suitable related time
> resources?

No. Who are you to suggest what I need to work on?

johannes

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

On 12/04/2013 04:04 PM, Johannes Berg wrote:
> On Wed, 2013-12-04 at 10:12 +0800, Chen Gang wrote:
> 
>> It is really not urgent, and for keeping quality, it is necessary to
>> spend suitable time resource (e.g 1 hour or more) to  make, review and
>> test this kind of patch carefully by oneself.
>>
>> So could you please help improve it when you have suitable related time
>> resources?
> 
> No. Who are you to suggest what I need to work on?
> 
> johannes
> 

I am one of volunteers for Open Source, and I don't care about who I am.

Since this thread is started by me, I have duty to try my best to let it
finish (but may fail).

According to our original discussion, it seems we agree that I am not
the suitable member to finish it, so I suggest you or another members to
try.


Thanks.
-- 
Chen Gang

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Johannes Berg]

On Wed, 2013-12-04 at 16:41 +0800, Chen Gang wrote:

> According to our original discussion, it seems we agree that I am not
> the suitable member to finish it, so I suggest you or another members to
> try.

There's nothing to finish here. The code is fine. The compiler is wrong,
but we haven't found a way to shut up the compiler without breaking the
code. Please let's just let it rest.

johannes

Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

[Chen Gang]

On 12/04/2013 04:49 PM, Johannes Berg wrote:
> On Wed, 2013-12-04 at 16:41 +0800, Chen Gang wrote:
> 
>> According to our original discussion, it seems we agree that I am not
>> the suitable member to finish it, so I suggest you or another members to
>> try.
> 
> There's nothing to finish here. The code is fine. The compiler is wrong,
> but we haven't found a way to shut up the compiler without breaking the
> code. Please let's just let it rest.
> 

For me, I still stick to the code can be improved, although it is not
urgent.

But just like you said, we can just stop discussing -- every members can
save their own opinions.

And I am not the related maintainer, so if no additional suggestions,
discussions or completions, I will/should stop here.


Thanks.
-- 
Chen Gang