Re: [PATCH bluetooth-next 3/3] ieee802154: 6lowpan: fix intra pan id check

[Stefan Schmidt <stefan@osg.samsung.com>]

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