| Message ID | 5299D306.7070701@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. >
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);