All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH bluetooth-next 1/3] ieee802154: add ieee802154_skb_dst_pan helper
@ 2016-06-17 16:34 Alexander Aring
  2016-06-17 16:34 ` [PATCH bluetooth-next 2/3] ieee802154: add ieee802154_skb_src_pan helper Alexander Aring
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Alexander Aring @ 2016-06-17 16:34 UTC (permalink / raw)
  To: linux-wpan; +Cc: kernel, Alexander Aring

This patch adds ieee802154_skb_dst_pan function to get the pointer
address of the destination pan id at skb mac pointer.

Signed-off-by: Alexander Aring <aar@pengutronix.de>
---
 include/linux/ieee802154.h | 16 ++++++++++++++++
 include/net/mac802154.h    | 29 +++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/include/linux/ieee802154.h b/include/linux/ieee802154.h
index acedbb6..91f4665 100644
--- a/include/linux/ieee802154.h
+++ b/include/linux/ieee802154.h
@@ -31,6 +31,8 @@
 #define IEEE802154_MIN_PSDU_LEN		9
 #define IEEE802154_FCS_LEN		2
 #define IEEE802154_MAX_AUTH_TAG_LEN	16
+#define IEEE802154_FC_LEN		2
+#define IEEE802154_SEQ_LEN		1
 
 /*  General MAC frame format:
  *  2 bytes: Frame Control
@@ -221,9 +223,14 @@ enum {
 #define IEEE802154_FCTL_ACKREQ		0x0020
 #define IEEE802154_FCTL_SECEN		0x0004
 #define IEEE802154_FCTL_INTRA_PAN	0x0040
+#define IEEE802154_FCTL_DADDR		0x0c00
 
 #define IEEE802154_FTYPE_DATA		0x0001
 
+#define IEEE802154_FCTL_ADDR_NONE	0x0000
+#define IEEE802154_FCTL_DADDR_SHORT	0x0800
+#define IEEE802154_FCTL_DADDR_EXTENDED	0x0c00
+
 /*
  * ieee802154_is_data - check if type is IEEE802154_FTYPE_DATA
  * @fc: frame control bytes in little-endian byteorder
@@ -261,6 +268,15 @@ static inline bool ieee802154_is_intra_pan(__le16 fc)
 	return fc & cpu_to_le16(IEEE802154_FCTL_INTRA_PAN);
 }
 
+/*
+ * ieee802154_daddr_mode - get daddr mode from fc
+ * @fc: frame control bytes in little-endian byteorder
+ */
+static inline __le16 ieee802154_daddr_mode(__le16 fc)
+{
+	return fc & cpu_to_le16(IEEE802154_FCTL_DADDR);
+}
+
 /**
  * ieee802154_is_valid_psdu_len - check if psdu len is valid
  * available lengths:
diff --git a/include/net/mac802154.h b/include/net/mac802154.h
index e465c85..b3f7cd8 100644
--- a/include/net/mac802154.h
+++ b/include/net/mac802154.h
@@ -258,6 +258,35 @@ static inline __le16 ieee802154_get_fc_from_skb(const struct sk_buff *skb)
 }
 
 /**
+ * ieee802154_skb_dst_pan - get the pointer to destination pan field
+ * @fc: mac header frame control field
+ * @skb: skb where the destination pan pointer will be get from
+ */
+static inline unsigned char *ieee802154_skb_dst_pan(__le16 fc,
+						    const struct sk_buff *skb)
+{
+	unsigned char *dst_pan;
+
+	switch (ieee802154_daddr_mode(fc)) {
+	case cpu_to_le16(IEEE802154_FCTL_ADDR_NONE):
+		dst_pan = NULL;
+		break;
+	case cpu_to_le16(IEEE802154_FCTL_DADDR_SHORT):
+	case cpu_to_le16(IEEE802154_FCTL_DADDR_EXTENDED):
+		dst_pan = skb_mac_header(skb) +
+			  IEEE802154_FC_LEN +
+			  IEEE802154_SEQ_LEN;
+		break;
+	default:
+		WARN_ONCE(1, "invalid addr mode detected");
+		dst_pan = NULL;
+		break;
+	}
+
+	return dst_pan;
+}
+
+/**
  * ieee802154_be64_to_le64 - copies and convert be64 to le64
  * @le64_dst: le64 destination pointer
  * @be64_src: be64 source pointer
-- 
2.8.3


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH bluetooth-next 2/3] ieee802154: add ieee802154_skb_src_pan helper
  2016-06-17 16:34 [PATCH bluetooth-next 1/3] ieee802154: add ieee802154_skb_dst_pan helper Alexander Aring
@ 2016-06-17 16:34 ` Alexander Aring
  2016-06-27  9:19   ` Alexander Aring
  2016-06-17 16:34 ` [PATCH bluetooth-next 3/3] ieee802154: 6lowpan: fix intra pan id check Alexander Aring
  2016-06-22 14:46 ` [PATCH bluetooth-next 1/3] ieee802154: add ieee802154_skb_dst_pan helper Stefan Schmidt
  2 siblings, 1 reply; 7+ messages in thread
From: Alexander Aring @ 2016-06-17 16:34 UTC (permalink / raw)
  To: linux-wpan; +Cc: kernel, Alexander Aring

This patch adds ieee802154_skb_src_pan function to get the pointer
address of the source pan id at skb mac pointer.

Signed-off-by: Alexander Aring <aar@pengutronix.de>
---
 include/linux/ieee802154.h | 12 ++++++++++++
 include/net/mac802154.h    | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/include/linux/ieee802154.h b/include/linux/ieee802154.h
index 91f4665..fd14815 100644
--- a/include/linux/ieee802154.h
+++ b/include/linux/ieee802154.h
@@ -224,12 +224,15 @@ enum {
 #define IEEE802154_FCTL_SECEN		0x0004
 #define IEEE802154_FCTL_INTRA_PAN	0x0040
 #define IEEE802154_FCTL_DADDR		0x0c00
+#define IEEE802154_FCTL_SADDR		0xc000
 
 #define IEEE802154_FTYPE_DATA		0x0001
 
 #define IEEE802154_FCTL_ADDR_NONE	0x0000
 #define IEEE802154_FCTL_DADDR_SHORT	0x0800
 #define IEEE802154_FCTL_DADDR_EXTENDED	0x0c00
+#define IEEE802154_FCTL_SADDR_SHORT	0x8000
+#define IEEE802154_FCTL_SADDR_EXTENDED	0xc000
 
 /*
  * ieee802154_is_data - check if type is IEEE802154_FTYPE_DATA
@@ -277,6 +280,15 @@ static inline __le16 ieee802154_daddr_mode(__le16 fc)
 	return fc & cpu_to_le16(IEEE802154_FCTL_DADDR);
 }
 
+/*
+ * ieee802154_saddr_mode - get saddr mode from fc
+ * @fc: frame control bytes in little-endian byteorder
+ */
+static inline __le16 ieee802154_saddr_mode(__le16 fc)
+{
+	return fc & cpu_to_le16(IEEE802154_FCTL_SADDR);
+}
+
 /**
  * ieee802154_is_valid_psdu_len - check if psdu len is valid
  * available lengths:
diff --git a/include/net/mac802154.h b/include/net/mac802154.h
index b3f7cd8..deb90a1 100644
--- a/include/net/mac802154.h
+++ b/include/net/mac802154.h
@@ -287,6 +287,52 @@ static inline unsigned char *ieee802154_skb_dst_pan(__le16 fc,
 }
 
 /**
+ * ieee802154_skb_src_pan - get the pointer to source pan field
+ * @fc: mac header frame control field
+ * @skb: skb where the source pan pointer will be get from
+ */
+static inline unsigned char *ieee802154_skb_src_pan(__le16 fc,
+						    const struct sk_buff *skb)
+{
+	unsigned char *src_pan;
+
+	switch (ieee802154_saddr_mode(fc)) {
+	case cpu_to_le16(IEEE802154_FCTL_ADDR_NONE):
+		src_pan = NULL;
+		break;
+	case cpu_to_le16(IEEE802154_FCTL_SADDR_SHORT):
+	case cpu_to_le16(IEEE802154_FCTL_SADDR_EXTENDED):
+		src_pan = skb_mac_header(skb) +
+			  IEEE802154_FC_LEN +
+			  IEEE802154_SEQ_LEN;
+
+		switch (ieee802154_daddr_mode(fc)) {
+		case cpu_to_le16(IEEE802154_FCTL_ADDR_NONE):
+			break;
+		case cpu_to_le16(IEEE802154_FCTL_DADDR_SHORT):
+			if (!ieee802154_is_intra_pan(fc))
+				src_pan += IEEE802154_SHORT_ADDR_LEN;
+			break;
+		case cpu_to_le16(IEEE802154_FCTL_DADDR_EXTENDED):
+			if (!ieee802154_is_intra_pan(fc))
+				src_pan += IEEE802154_EXTENDED_ADDR_LEN;
+			break;
+		default:
+			WARN_ONCE(1, "invalid addr mode detected");
+			src_pan = NULL;
+			break;
+		}
+		break;
+	default:
+		WARN_ONCE(1, "invalid addr mode detected");
+		src_pan = NULL;
+		break;
+	}
+
+	return src_pan;
+}
+
+/**
  * ieee802154_be64_to_le64 - copies and convert be64 to le64
  * @le64_dst: le64 destination pointer
  * @be64_src: be64 source pointer
-- 
2.8.3


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH bluetooth-next 3/3] ieee802154: 6lowpan: fix intra pan id check
  2016-06-17 16:34 [PATCH bluetooth-next 1/3] ieee802154: add ieee802154_skb_dst_pan helper Alexander Aring
  2016-06-17 16:34 ` [PATCH bluetooth-next 2/3] ieee802154: add ieee802154_skb_src_pan helper Alexander Aring
@ 2016-06-17 16:34 ` Alexander Aring
  2016-06-22 14:42   ` Stefan Schmidt
  2016-06-22 14:46 ` [PATCH bluetooth-next 1/3] ieee802154: add ieee802154_skb_dst_pan helper Stefan Schmidt
  2 siblings, 1 reply; 7+ messages in thread
From: Alexander Aring @ 2016-06-17 16:34 UTC (permalink / raw)
  To: linux-wpan; +Cc: kernel, Alexander Aring

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);
+}
+
+/**
  * 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 */
-- 
2.8.3


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH bluetooth-next 3/3] ieee802154: 6lowpan: fix intra pan id check
  2016-06-17 16:34 ` [PATCH bluetooth-next 3/3] ieee802154: 6lowpan: fix intra pan id check Alexander Aring
@ 2016-06-22 14:42   ` Stefan Schmidt
  2016-06-22 20:41     ` Alexander Aring
  0 siblings, 1 reply; 7+ messages in thread
From: Stefan Schmidt @ 2016-06-22 14:42 UTC (permalink / raw)
  To: Alexander Aring, linux-wpan; +Cc: kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bluetooth-next 1/3] ieee802154: add ieee802154_skb_dst_pan helper
  2016-06-17 16:34 [PATCH bluetooth-next 1/3] ieee802154: add ieee802154_skb_dst_pan helper Alexander Aring
  2016-06-17 16:34 ` [PATCH bluetooth-next 2/3] ieee802154: add ieee802154_skb_src_pan helper Alexander Aring
  2016-06-17 16:34 ` [PATCH bluetooth-next 3/3] ieee802154: 6lowpan: fix intra pan id check Alexander Aring
@ 2016-06-22 14:46 ` Stefan Schmidt
  2 siblings, 0 replies; 7+ messages in thread
From: Stefan Schmidt @ 2016-06-22 14:46 UTC (permalink / raw)
  To: Alexander Aring, linux-wpan; +Cc: kernel

Hello.

On 17/06/16 18:34, Alexander Aring wrote:
> This patch adds ieee802154_skb_dst_pan function to get the pointer
> address of the destination pan id at skb mac pointer.
>
> Signed-off-by: Alexander Aring <aar@pengutronix.de>
> ---
>   include/linux/ieee802154.h | 16 ++++++++++++++++
>   include/net/mac802154.h    | 29 +++++++++++++++++++++++++++++
>   2 files changed, 45 insertions(+)
>
> diff --git a/include/linux/ieee802154.h b/include/linux/ieee802154.h
> index acedbb6..91f4665 100644
> --- a/include/linux/ieee802154.h
> +++ b/include/linux/ieee802154.h
> @@ -31,6 +31,8 @@
>   #define IEEE802154_MIN_PSDU_LEN		9
>   #define IEEE802154_FCS_LEN		2
>   #define IEEE802154_MAX_AUTH_TAG_LEN	16
> +#define IEEE802154_FC_LEN		2
> +#define IEEE802154_SEQ_LEN		1
>   
>   /*  General MAC frame format:
>    *  2 bytes: Frame Control
> @@ -221,9 +223,14 @@ enum {
>   #define IEEE802154_FCTL_ACKREQ		0x0020
>   #define IEEE802154_FCTL_SECEN		0x0004
>   #define IEEE802154_FCTL_INTRA_PAN	0x0040
> +#define IEEE802154_FCTL_DADDR		0x0c00
>   
>   #define IEEE802154_FTYPE_DATA		0x0001
>   
> +#define IEEE802154_FCTL_ADDR_NONE	0x0000
> +#define IEEE802154_FCTL_DADDR_SHORT	0x0800
> +#define IEEE802154_FCTL_DADDR_EXTENDED	0x0c00
> +
>   /*
>    * ieee802154_is_data - check if type is IEEE802154_FTYPE_DATA
>    * @fc: frame control bytes in little-endian byteorder
> @@ -261,6 +268,15 @@ static inline bool ieee802154_is_intra_pan(__le16 fc)
>   	return fc & cpu_to_le16(IEEE802154_FCTL_INTRA_PAN);
>   }
>   
> +/*
> + * ieee802154_daddr_mode - get daddr mode from fc
> + * @fc: frame control bytes in little-endian byteorder
> + */
> +static inline __le16 ieee802154_daddr_mode(__le16 fc)
> +{
> +	return fc & cpu_to_le16(IEEE802154_FCTL_DADDR);
> +}
> +
>   /**
>    * ieee802154_is_valid_psdu_len - check if psdu len is valid
>    * available lengths:
> diff --git a/include/net/mac802154.h b/include/net/mac802154.h
> index e465c85..b3f7cd8 100644
> --- a/include/net/mac802154.h
> +++ b/include/net/mac802154.h
> @@ -258,6 +258,35 @@ static inline __le16 ieee802154_get_fc_from_skb(const struct sk_buff *skb)
>   }
>   
>   /**
> + * ieee802154_skb_dst_pan - get the pointer to destination pan field
> + * @fc: mac header frame control field
> + * @skb: skb where the destination pan pointer will be get from
> + */
> +static inline unsigned char *ieee802154_skb_dst_pan(__le16 fc,
> +						    const struct sk_buff *skb)
> +{
> +	unsigned char *dst_pan;
> +
> +	switch (ieee802154_daddr_mode(fc)) {
> +	case cpu_to_le16(IEEE802154_FCTL_ADDR_NONE):
> +		dst_pan = NULL;
> +		break;
> +	case cpu_to_le16(IEEE802154_FCTL_DADDR_SHORT):
> +	case cpu_to_le16(IEEE802154_FCTL_DADDR_EXTENDED):
> +		dst_pan = skb_mac_header(skb) +
> +			  IEEE802154_FC_LEN +
> +			  IEEE802154_SEQ_LEN;

Same as in my other mail. Why doing pointer games here? Why not just get 
the PAN ID value and return it? That would make the function easier and 
safer to use.

> +		break;
> +	default:
> +		WARN_ONCE(1, "invalid addr mode detected");
> +		dst_pan = NULL;
> +		break;
> +	}
> +
> +	return dst_pan;
> +}
> +
> +/**
>    * ieee802154_be64_to_le64 - copies and convert be64 to le64
>    * @le64_dst: le64 destination pointer
>    * @be64_src: be64 source pointer

regards
Stefan Schmidt

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bluetooth-next 3/3] ieee802154: 6lowpan: fix intra pan id check
  2016-06-22 14:42   ` Stefan Schmidt
@ 2016-06-22 20:41     ` Alexander Aring
  0 siblings, 0 replies; 7+ messages in thread
From: Alexander Aring @ 2016-06-22 20:41 UTC (permalink / raw)
  To: Stefan Schmidt, linux-wpan; +Cc: kernel


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bluetooth-next 2/3] ieee802154: add ieee802154_skb_src_pan helper
  2016-06-17 16:34 ` [PATCH bluetooth-next 2/3] ieee802154: add ieee802154_skb_src_pan helper Alexander Aring
@ 2016-06-27  9:19   ` Alexander Aring
  0 siblings, 0 replies; 7+ messages in thread
From: Alexander Aring @ 2016-06-27  9:19 UTC (permalink / raw)
  To: linux-wpan; +Cc: kernel

Hi,

On 06/17/2016 06:34 PM, Alexander Aring wrote:
> This patch adds ieee802154_skb_src_pan function to get the pointer
> address of the source pan id at skb mac pointer.
> 
> Signed-off-by: Alexander Aring <aar@pengutronix.de>
> ---
>  include/linux/ieee802154.h | 12 ++++++++++++
>  include/net/mac802154.h    | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/include/linux/ieee802154.h b/include/linux/ieee802154.h
> index 91f4665..fd14815 100644
> --- a/include/linux/ieee802154.h
> +++ b/include/linux/ieee802154.h
> @@ -224,12 +224,15 @@ enum {
>  #define IEEE802154_FCTL_SECEN		0x0004
>  #define IEEE802154_FCTL_INTRA_PAN	0x0040
>  #define IEEE802154_FCTL_DADDR		0x0c00
> +#define IEEE802154_FCTL_SADDR		0xc000
>  
>  #define IEEE802154_FTYPE_DATA		0x0001
>  
>  #define IEEE802154_FCTL_ADDR_NONE	0x0000
>  #define IEEE802154_FCTL_DADDR_SHORT	0x0800
>  #define IEEE802154_FCTL_DADDR_EXTENDED	0x0c00
> +#define IEEE802154_FCTL_SADDR_SHORT	0x8000
> +#define IEEE802154_FCTL_SADDR_EXTENDED	0xc000
>  
>  /*
>   * ieee802154_is_data - check if type is IEEE802154_FTYPE_DATA
> @@ -277,6 +280,15 @@ static inline __le16 ieee802154_daddr_mode(__le16 fc)
>  	return fc & cpu_to_le16(IEEE802154_FCTL_DADDR);
>  }
>  
> +/*
> + * ieee802154_saddr_mode - get saddr mode from fc
> + * @fc: frame control bytes in little-endian byteorder
> + */
> +static inline __le16 ieee802154_saddr_mode(__le16 fc)
> +{
> +	return fc & cpu_to_le16(IEEE802154_FCTL_SADDR);
> +}
> +
>  /**
>   * ieee802154_is_valid_psdu_len - check if psdu len is valid
>   * available lengths:
> diff --git a/include/net/mac802154.h b/include/net/mac802154.h
> index b3f7cd8..deb90a1 100644
> --- a/include/net/mac802154.h
> +++ b/include/net/mac802154.h
> @@ -287,6 +287,52 @@ static inline unsigned char *ieee802154_skb_dst_pan(__le16 fc,
>  }
>  
>  /**
> + * ieee802154_skb_src_pan - get the pointer to source pan field
> + * @fc: mac header frame control field
> + * @skb: skb where the source pan pointer will be get from
> + */
> +static inline unsigned char *ieee802154_skb_src_pan(__le16 fc,
> +						    const struct sk_buff *skb)
> +{
> +	unsigned char *src_pan;
> +
> +	switch (ieee802154_saddr_mode(fc)) {
> +	case cpu_to_le16(IEEE802154_FCTL_ADDR_NONE):
> +		src_pan = NULL;
> +		break;
> +	case cpu_to_le16(IEEE802154_FCTL_SADDR_SHORT):
> +	case cpu_to_le16(IEEE802154_FCTL_SADDR_EXTENDED):
> +		src_pan = skb_mac_header(skb) +
> +			  IEEE802154_FC_LEN +
> +			  IEEE802154_SEQ_LEN;
> +
> +		switch (ieee802154_daddr_mode(fc)) {
> +		case cpu_to_le16(IEEE802154_FCTL_ADDR_NONE):
> +			break;
> +		case cpu_to_le16(IEEE802154_FCTL_DADDR_SHORT):

src_pan += IEEE802154_PAN_ID_LEN;

is missing for, because daddr is non NONE we need to add destination
pan_id length.

> +			if (!ieee802154_is_intra_pan(fc))
> +				src_pan += IEEE802154_SHORT_ADDR_LEN;
> +			break;
> +		case cpu_to_le16(IEEE802154_FCTL_DADDR_EXTENDED):

same here.

I will send a v2.

- Alex

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-06-27  9:19 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-17 16:34 [PATCH bluetooth-next 1/3] ieee802154: add ieee802154_skb_dst_pan helper Alexander Aring
2016-06-17 16:34 ` [PATCH bluetooth-next 2/3] ieee802154: add ieee802154_skb_src_pan helper Alexander Aring
2016-06-27  9:19   ` Alexander Aring
2016-06-17 16:34 ` [PATCH bluetooth-next 3/3] ieee802154: 6lowpan: fix intra pan id check Alexander Aring
2016-06-22 14:42   ` Stefan Schmidt
2016-06-22 20:41     ` Alexander Aring
2016-06-22 14:46 ` [PATCH bluetooth-next 1/3] ieee802154: add ieee802154_skb_dst_pan helper Stefan Schmidt

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.