| Message ID | 20160617163450.3229-3-aar@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hello. On 17/06/16 18:34, Alexander Aring wrote: > The RIOT-OS stack does send intra-pan frames but don't set the intra pan > flag inside the mac header. It seems this is valid frame addressing but > inefficient. Anyway this patch adds a new function for intra pan > addressing, doesn't matter if intra pan flag or source and destination > are the same. The newly introduction function will be used to check on > intra pan addressing for 6lowpan. > > Signed-off-by: Alexander Aring <aar@pengutronix.de> > --- > include/linux/ieee802154.h | 1 + > include/net/mac802154.h | 19 +++++++++++++++++++ > net/ieee802154/6lowpan/rx.c | 2 +- > 3 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/include/linux/ieee802154.h b/include/linux/ieee802154.h > index fd14815..ddb8901 100644 > --- a/include/linux/ieee802154.h > +++ b/include/linux/ieee802154.h > @@ -50,6 +50,7 @@ > > #define IEEE802154_EXTENDED_ADDR_LEN 8 > #define IEEE802154_SHORT_ADDR_LEN 2 > +#define IEEE802154_PAN_ID_LEN 2 > > #define IEEE802154_LIFS_PERIOD 40 > #define IEEE802154_SIFS_PERIOD 12 > diff --git a/include/net/mac802154.h b/include/net/mac802154.h > index deb90a1..df34187 100644 > --- a/include/net/mac802154.h > +++ b/include/net/mac802154.h > @@ -333,6 +333,25 @@ static inline unsigned char *ieee802154_skb_src_pan(__le16 fc, > } > > /** > + * ieee802154_skb_is_intra_pan_addressing - checks whenever the mac addressing > + * is an intra pan communication > + * @fc: mac header frame control field > + * @skb: skb where the source and destination pan should be get from > + */ > +static inline bool ieee802154_skb_is_intra_pan_addressing(__le16 fc, > + const struct sk_buff *skb) > +{ > + unsigned char *dst_pan = ieee802154_skb_dst_pan(fc, skb), > + *src_pan = ieee802154_skb_src_pan(fc, skb); > + > + /* if one is NULL is no intra pan addressing */ > + if (!dst_pan || !src_pan) > + return false; > + > + return !memcmp(dst_pan, src_pan, IEEE802154_PAN_ID_LEN); > +} This seems like a complicated way to compare two PAN IDs which are basicall le16 types. Why would you could the detour here expressing them as char *? In my opinion the ieee802154_skb_{src,dst}_pan() should just return the PAN ID as le16 value. That is what I would expect. > + > +/** > * ieee802154_be64_to_le64 - copies and convert be64 to le64 > * @le64_dst: le64 destination pointer > * @be64_src: be64 source pointer > diff --git a/net/ieee802154/6lowpan/rx.c b/net/ieee802154/6lowpan/rx.c > index ef185dd..649e7d45 100644 > --- a/net/ieee802154/6lowpan/rx.c > +++ b/net/ieee802154/6lowpan/rx.c > @@ -262,7 +262,7 @@ static inline bool lowpan_rx_h_check(struct sk_buff *skb) > > /* check on ieee802154 conform 6LoWPAN header */ > if (!ieee802154_is_data(fc) || > - !ieee802154_is_intra_pan(fc)) > + !ieee802154_skb_is_intra_pan_addressing(fc, skb)) > return false; > > /* check if we can dereference the dispatch */ regards Stefan Schmidt -- To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 06/22/2016 04:42 PM, Stefan Schmidt wrote: > Hello. > > On 17/06/16 18:34, Alexander Aring wrote: >> The RIOT-OS stack does send intra-pan frames but don't set the intra pan >> flag inside the mac header. It seems this is valid frame addressing but >> inefficient. Anyway this patch adds a new function for intra pan >> addressing, doesn't matter if intra pan flag or source and destination >> are the same. The newly introduction function will be used to check on >> intra pan addressing for 6lowpan. >> >> Signed-off-by: Alexander Aring <aar@pengutronix.de> >> --- >> include/linux/ieee802154.h | 1 + >> include/net/mac802154.h | 19 +++++++++++++++++++ >> net/ieee802154/6lowpan/rx.c | 2 +- >> 3 files changed, 21 insertions(+), 1 deletion(-) >> >> diff --git a/include/linux/ieee802154.h b/include/linux/ieee802154.h >> index fd14815..ddb8901 100644 >> --- a/include/linux/ieee802154.h >> +++ b/include/linux/ieee802154.h >> @@ -50,6 +50,7 @@ >> #define IEEE802154_EXTENDED_ADDR_LEN 8 >> #define IEEE802154_SHORT_ADDR_LEN 2 >> +#define IEEE802154_PAN_ID_LEN 2 >> #define IEEE802154_LIFS_PERIOD 40 >> #define IEEE802154_SIFS_PERIOD 12 >> diff --git a/include/net/mac802154.h b/include/net/mac802154.h >> index deb90a1..df34187 100644 >> --- a/include/net/mac802154.h >> +++ b/include/net/mac802154.h >> @@ -333,6 +333,25 @@ static inline unsigned char *ieee802154_skb_src_pan(__le16 fc, >> } >> /** >> + * ieee802154_skb_is_intra_pan_addressing - checks whenever the mac addressing >> + * is an intra pan communication >> + * @fc: mac header frame control field >> + * @skb: skb where the source and destination pan should be get from >> + */ >> +static inline bool ieee802154_skb_is_intra_pan_addressing(__le16 fc, >> + const struct sk_buff *skb) >> +{ >> + unsigned char *dst_pan = ieee802154_skb_dst_pan(fc, skb), >> + *src_pan = ieee802154_skb_src_pan(fc, skb); >> + >> + /* if one is NULL is no intra pan addressing */ >> + if (!dst_pan || !src_pan) >> + return false; >> + >> + return !memcmp(dst_pan, src_pan, IEEE802154_PAN_ID_LEN); >> +} > > This seems like a complicated way to compare two PAN IDs which are basicall le16 types. Why would you could the detour here expressing them as char *? > I thought many times about how do deal with that for my current use-case which is comparing src_pan and dst_pan. These function delivers the pointer on mac_header where the src and dst pan id is. To compare the src and dst pan id, we don't need to put them on stack dealing with unaligned memory access. Also in case of intra pan flag, then we have "dst_pan == src_pan" and I think some memcmp implementations doesn't compare data if dst, src pointers are the same. This is also what 802.15.4 says for the intra pan communication. Sometimes I would like to simple compare directly on mac_header, e.g. comparing destination address with current wpan_dev MIB addressing values without putting these attributes on the stack. (I think this is also one reason why we save mostly everything in byteorder like mac header). Also dealing with pointers let us handle the none case by returning NULL. This doesn't work every time, for different use-case we can introduce a higher-level function which use the "getting the right pointer by frame control field" functions, see below. > In my opinion the ieee802154_skb_{src,dst}_pan() should just return the PAN ID as le16 value. That is what I would expect. For this we can and should add some function without keyword "_skb_" which maybe call the lower functionality "ieee802154_skb_{src,dst}_pan" and do some mempcy on __le16 type and return it, but you _maybe_ need to check if addr mode for saddr/daddr isn't none. I say maybe here, because there exists some tweaks in 802.15.4 frame format to decide if it's valid frame format and I think for dataframes one (src or dst addr mode, I don't remember) can't be none, such frames should be filtered out before deliver to packet layer, so we can sure some things after checking on dataframes. btw: I don't believe that we do that, such filtering and also don't trust the hardware filters here, the hardware filters may filter such frames or not. We don't know it so we check twice. :-) --- I hope these arguments are enough to get this approach into kernel. - Alex -- To unsubscribe from this list: send the line "unsubscribe linux-wpan" 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/include/linux/ieee802154.h b/include/linux/ieee802154.h index fd14815..ddb8901 100644 --- a/include/linux/ieee802154.h +++ b/include/linux/ieee802154.h @@ -50,6 +50,7 @@ #define IEEE802154_EXTENDED_ADDR_LEN 8 #define IEEE802154_SHORT_ADDR_LEN 2 +#define IEEE802154_PAN_ID_LEN 2 #define IEEE802154_LIFS_PERIOD 40 #define IEEE802154_SIFS_PERIOD 12 diff --git a/include/net/mac802154.h b/include/net/mac802154.h index deb90a1..df34187 100644 --- a/include/net/mac802154.h +++ b/include/net/mac802154.h @@ -333,6 +333,25 @@ static inline unsigned char *ieee802154_skb_src_pan(__le16 fc, } /** + * ieee802154_skb_is_intra_pan_addressing - checks whenever the mac addressing + * is an intra pan communication + * @fc: mac header frame control field + * @skb: skb where the source and destination pan should be get from + */ +static inline bool ieee802154_skb_is_intra_pan_addressing(__le16 fc, + const struct sk_buff *skb) +{ + unsigned char *dst_pan = ieee802154_skb_dst_pan(fc, skb), + *src_pan = ieee802154_skb_src_pan(fc, skb); + + /* if one is NULL is no intra pan addressing */ + if (!dst_pan || !src_pan) + return false; + + return !memcmp(dst_pan, src_pan, IEEE802154_PAN_ID_LEN); +} + +/** * ieee802154_be64_to_le64 - copies and convert be64 to le64 * @le64_dst: le64 destination pointer * @be64_src: be64 source pointer diff --git a/net/ieee802154/6lowpan/rx.c b/net/ieee802154/6lowpan/rx.c index ef185dd..649e7d45 100644 --- a/net/ieee802154/6lowpan/rx.c +++ b/net/ieee802154/6lowpan/rx.c @@ -262,7 +262,7 @@ static inline bool lowpan_rx_h_check(struct sk_buff *skb) /* check on ieee802154 conform 6LoWPAN header */ if (!ieee802154_is_data(fc) || - !ieee802154_is_intra_pan(fc)) + !ieee802154_skb_is_intra_pan_addressing(fc, skb)) return false; /* check if we can dereference the dispatch */
The RIOT-OS stack does send intra-pan frames but don't set the intra pan flag inside the mac header. It seems this is valid frame addressing but inefficient. Anyway this patch adds a new function for intra pan addressing, doesn't matter if intra pan flag or source and destination are the same. The newly introduction function will be used to check on intra pan addressing for 6lowpan. Signed-off-by: Alexander Aring <aar@pengutronix.de> --- include/linux/ieee802154.h | 1 + include/net/mac802154.h | 19 +++++++++++++++++++ net/ieee802154/6lowpan/rx.c | 2 +- 3 files changed, 21 insertions(+), 1 deletion(-)