[PATCH bluetooth-next 0/3] ieee802154: small cleanups

[PATCH bluetooth-next 0/3] ieee802154: small cleanups

[Alexander Aring]

Hi,

this patch series contains some small cleanups. THe first patch do a more
correct checking on valid psdu len, this is necessary e.g. in at86rf230
promiscous mode frame length checking to avoid an access out of mpdu buffer.
The last two patches simple remove some unnecessary temporary variable.

- Alex

Alexander Aring (3):
  ieee802154: correct ieee802154_is_valid_psdu_len
  ieee802154: cleanup ieee802154_be64_to_le64
  ieee802154: cleanup ieee802154_le64_to_be64

 include/linux/ieee802154.h | 12 ++++++++++--
 include/net/mac802154.h    |  8 ++------
 2 files changed, 12 insertions(+), 8 deletions(-)

-- 
2.2.2

[PATCH bluetooth-next 1/3] ieee802154: correct ieee802154_is_valid_psdu_len

[Alexander Aring]

This patch corrects the ieee802154_is_valid_psdu_len function that this
function also checks on reserved values 6-8 for validation the psdu
length.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 include/linux/ieee802154.h | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/linux/ieee802154.h b/include/linux/ieee802154.h
index 6e82d88..40b0ab9 100644
--- a/include/linux/ieee802154.h
+++ b/include/linux/ieee802154.h
@@ -28,7 +28,8 @@
 #include <asm/byteorder.h>
 
 #define IEEE802154_MTU			127
-#define IEEE802154_MIN_PSDU_LEN		5
+#define IEEE802154_ACK_PSDU_LEN		5
+#define IEEE802154_MIN_PSDU_LEN		9
 
 #define IEEE802154_PAN_ID_BROADCAST	0xffff
 #define IEEE802154_ADDR_SHORT_BROADCAST	0xffff
@@ -204,11 +205,18 @@ enum {
 
 /**
  * ieee802154_is_valid_psdu_len - check if psdu len is valid
+ * available lengths:
+ *	0-4	Reserved
+ *	5	MPDU (Acknowledgment)
+ *	6-8	Reserved
+ *	9-127	MPDU
+ *
  * @len: psdu len with (MHR + payload + MFR)
  */
 static inline bool ieee802154_is_valid_psdu_len(const u8 len)
 {
-	return (len >= IEEE802154_MIN_PSDU_LEN && len <= IEEE802154_MTU);
+	return (len == IEEE802154_ACK_PSDU_LEN ||
+		(len >= IEEE802154_MIN_PSDU_LEN && len <= IEEE802154_MTU));
 }
 
 /**
-- 
2.2.2

[PATCH bluetooth-next 2/3] ieee802154: cleanup ieee802154_be64_to_le64

[Alexander Aring]

This patch cleanups the ieee802154_be64_to_le64 function. This patch
removes an unnecessary temporary variable.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 include/net/mac802154.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/net/mac802154.h b/include/net/mac802154.h
index 8506478..a4dcefe 100644
--- a/include/net/mac802154.h
+++ b/include/net/mac802154.h
@@ -233,9 +233,7 @@ struct ieee802154_ops {
  */
 static inline void ieee802154_be64_to_le64(void *le64_dst, const void *be64_src)
 {
-	__le64 tmp = (__force __le64)swab64p(be64_src);
-
-	memcpy(le64_dst, &tmp, IEEE802154_EXTENDED_ADDR_LEN);
+	*((__le64 *)le64_dst) = (__force __le64)swab64p(be64_src);
 }
 
 /**
-- 
2.2.2

Re: [PATCH bluetooth-next 2/3] ieee802154: cleanup ieee802154_be64_to_le64

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1222 bytes --]

On 02/10/2015 11:14 PM, Alexander Aring wrote:
> This patch cleanups the ieee802154_be64_to_le64 function. This patch
> removes an unnecessary temporary variable.
> 
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
>  include/net/mac802154.h | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/include/net/mac802154.h b/include/net/mac802154.h
> index 8506478..a4dcefe 100644
> --- a/include/net/mac802154.h
> +++ b/include/net/mac802154.h
> @@ -233,9 +233,7 @@ struct ieee802154_ops {
>   */
>  static inline void ieee802154_be64_to_le64(void *le64_dst, const void *be64_src)
>  {
> -	__le64 tmp = (__force __le64)swab64p(be64_src);
> -
> -	memcpy(le64_dst, &tmp, IEEE802154_EXTENDED_ADDR_LEN);
> +	*((__le64 *)le64_dst) = (__force __le64)swab64p(be64_src);

I assume the compiler optimizes the memcpy, due to the constant length
argument. It the dst always 64 bit aligned?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH bluetooth-next 2/3] ieee802154: cleanup ieee802154_be64_to_le64

[Alexander Aring]

On Wed, Feb 11, 2015 at 01:17:50PM +0100, Marc Kleine-Budde wrote:
> On 02/10/2015 11:14 PM, Alexander Aring wrote:
> > This patch cleanups the ieee802154_be64_to_le64 function. This patch
> > removes an unnecessary temporary variable.
> > 
> > Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > ---
> >  include/net/mac802154.h | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/include/net/mac802154.h b/include/net/mac802154.h
> > index 8506478..a4dcefe 100644
> > --- a/include/net/mac802154.h
> > +++ b/include/net/mac802154.h
> > @@ -233,9 +233,7 @@ struct ieee802154_ops {
> >   */
> >  static inline void ieee802154_be64_to_le64(void *le64_dst, const void *be64_src)
> >  {
> > -	__le64 tmp = (__force __le64)swab64p(be64_src);
> > -
> > -	memcpy(le64_dst, &tmp, IEEE802154_EXTENDED_ADDR_LEN);
> > +	*((__le64 *)le64_dst) = (__force __le64)swab64p(be64_src);
> 
> I assume the compiler optimizes the memcpy, due to the constant length
> argument. It the dst always 64 bit aligned?

should be. I can't check this, but dst and src should always 64 bit long.
(Otherwise the programmer do something wrong).

It's just byte swapping a little endian 64 bit value to big endian 64
bit value. Because mac header is little and the most netdev core api
uses big endian. These function are some helper function to transfer
__le64 to __be64. Sometimes the __le64 and __be64 are implementated as
arrays so I do this over some void pointers, like [0].

At [0] you see the assigned MAC PIB extended addr (saved as __le64)
which is set to the netdevice dev_addr array (assumes big endian).

The swab64p is an architecture optimized byte swapping function.

Do you think that this patch will decrease the perfomance of this function?
I know the casts looks a little bit ugly but swab64p isn't made for
"endianness swapping" and assumes u64 as parameter (pointer) and result.

- Alex

[0] http://git.kernel.org/cgit/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/mac802154/iface.c#n558

Re: [PATCH bluetooth-next 2/3] ieee802154: cleanup ieee802154_be64_to_le64

[Phoebe Buckheister]

On Wed, 11 Feb 2015 13:36:03 +0100
Alexander Aring <alex.aring@gmail.com> wrote:

> On Wed, Feb 11, 2015 at 01:17:50PM +0100, Marc Kleine-Budde wrote:
> > On 02/10/2015 11:14 PM, Alexander Aring wrote:
> > > This patch cleanups the ieee802154_be64_to_le64 function. This
> > > patch removes an unnecessary temporary variable.
> > > 
> > > Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > > ---
> > >  include/net/mac802154.h | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/include/net/mac802154.h b/include/net/mac802154.h
> > > index 8506478..a4dcefe 100644
> > > --- a/include/net/mac802154.h
> > > +++ b/include/net/mac802154.h
> > > @@ -233,9 +233,7 @@ struct ieee802154_ops {
> > >   */
> > >  static inline void ieee802154_be64_to_le64(void *le64_dst, const
> > > void *be64_src) {
> > > -	__le64 tmp = (__force __le64)swab64p(be64_src);
> > > -
> > > -	memcpy(le64_dst, &tmp, IEEE802154_EXTENDED_ADDR_LEN);
> > > +	*((__le64 *)le64_dst) = (__force
> > > __le64)swab64p(be64_src);
> > 
> > I assume the compiler optimizes the memcpy, due to the constant
> > length argument. It the dst always 64 bit aligned?
> 
> should be. I can't check this, but dst and src should always 64 bit
> long. (Otherwise the programmer do something wrong).
> 
> It's just byte swapping a little endian 64 bit value to big endian 64
> bit value. Because mac header is little and the most netdev core api
> uses big endian. These function are some helper function to transfer
> __le64 to __be64. Sometimes the __le64 and __be64 are implementated as
> arrays so I do this over some void pointers, like [0].

If you convert into an array (which happens sometimes, as you point
out), you cannot assume alignment stronger than single bytes. If you do
remove the memcpy, please adjust the dst argument pointer to be
__le64* to catch these cases of possible misalignment.

> At [0] you see the assigned MAC PIB extended addr (saved as __le64)
> which is set to the netdevice dev_addr array (assumes big endian).
> 
> The swab64p is an architecture optimized byte swapping function.
> 
> Do you think that this patch will decrease the perfomance of this
> function? I know the casts looks a little bit ugly but swab64p isn't
> made for "endianness swapping" and assumes u64 as parameter (pointer)
> and result.
> 
> - Alex
> 
> [0]
> http://git.kernel.org/cgit/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/mac802154/iface.c#n558
> -- 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

Re: [PATCH bluetooth-next 2/3] ieee802154: cleanup ieee802154_be64_to_le64

[Alexander Aring]

On Wed, Feb 11, 2015 at 01:59:58PM +0100, Phoebe Buckheister wrote:
> On Wed, 11 Feb 2015 13:36:03 +0100
> Alexander Aring <alex.aring@gmail.com> wrote:
> 
> > On Wed, Feb 11, 2015 at 01:17:50PM +0100, Marc Kleine-Budde wrote:
> > > On 02/10/2015 11:14 PM, Alexander Aring wrote:
> > > > This patch cleanups the ieee802154_be64_to_le64 function. This
> > > > patch removes an unnecessary temporary variable.
> > > > 
> > > > Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > > > ---
> > > >  include/net/mac802154.h | 4 +---
> > > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > > 
> > > > diff --git a/include/net/mac802154.h b/include/net/mac802154.h
> > > > index 8506478..a4dcefe 100644
> > > > --- a/include/net/mac802154.h
> > > > +++ b/include/net/mac802154.h
> > > > @@ -233,9 +233,7 @@ struct ieee802154_ops {
> > > >   */
> > > >  static inline void ieee802154_be64_to_le64(void *le64_dst, const
> > > > void *be64_src) {
> > > > -	__le64 tmp = (__force __le64)swab64p(be64_src);
> > > > -
> > > > -	memcpy(le64_dst, &tmp, IEEE802154_EXTENDED_ADDR_LEN);
> > > > +	*((__le64 *)le64_dst) = (__force
> > > > __le64)swab64p(be64_src);
> > > 
> > > I assume the compiler optimizes the memcpy, due to the constant
> > > length argument. It the dst always 64 bit aligned?
> > 
> > should be. I can't check this, but dst and src should always 64 bit
> > long. (Otherwise the programmer do something wrong).
> > 
> > It's just byte swapping a little endian 64 bit value to big endian 64
> > bit value. Because mac header is little and the most netdev core api
> > uses big endian. These function are some helper function to transfer
> > __le64 to __be64. Sometimes the __le64 and __be64 are implementated as
> > arrays so I do this over some void pointers, like [0].
> 
> If you convert into an array (which happens sometimes, as you point
> out), you cannot assume alignment stronger than single bytes. If you do
> remove the memcpy, please adjust the dst argument pointer to be
> __le64* to catch these cases of possible misalignment.
> 

ah, I see (maybe).

Then I should use more "put_unaligned_le64" for ieee802154_be64_to_le64
and "put_unaligned_be64" for ieee802154_le64_to_be64.


- Alex

Re: [PATCH bluetooth-next 2/3] ieee802154: cleanup ieee802154_be64_to_le64

[Alexander Aring]

On Wed, Feb 11, 2015 at 02:06:41PM +0100, Alexander Aring wrote:
> On Wed, Feb 11, 2015 at 01:59:58PM +0100, Phoebe Buckheister wrote:
> > On Wed, 11 Feb 2015 13:36:03 +0100
> > Alexander Aring <alex.aring@gmail.com> wrote:
> > 
> > > On Wed, Feb 11, 2015 at 01:17:50PM +0100, Marc Kleine-Budde wrote:
> > > > On 02/10/2015 11:14 PM, Alexander Aring wrote:
> > > > > This patch cleanups the ieee802154_be64_to_le64 function. This
> > > > > patch removes an unnecessary temporary variable.
> > > > > 
> > > > > Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > > > > ---
> > > > >  include/net/mac802154.h | 4 +---
> > > > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/include/net/mac802154.h b/include/net/mac802154.h
> > > > > index 8506478..a4dcefe 100644
> > > > > --- a/include/net/mac802154.h
> > > > > +++ b/include/net/mac802154.h
> > > > > @@ -233,9 +233,7 @@ struct ieee802154_ops {
> > > > >   */
> > > > >  static inline void ieee802154_be64_to_le64(void *le64_dst, const
> > > > > void *be64_src) {
> > > > > -	__le64 tmp = (__force __le64)swab64p(be64_src);
> > > > > -
> > > > > -	memcpy(le64_dst, &tmp, IEEE802154_EXTENDED_ADDR_LEN);
> > > > > +	*((__le64 *)le64_dst) = (__force
> > > > > __le64)swab64p(be64_src);
> > > > 
> > > > I assume the compiler optimizes the memcpy, due to the constant
> > > > length argument. It the dst always 64 bit aligned?
> > > 
> > > should be. I can't check this, but dst and src should always 64 bit
> > > long. (Otherwise the programmer do something wrong).
> > > 
> > > It's just byte swapping a little endian 64 bit value to big endian 64
> > > bit value. Because mac header is little and the most netdev core api
> > > uses big endian. These function are some helper function to transfer
> > > __le64 to __be64. Sometimes the __le64 and __be64 are implementated as
> > > arrays so I do this over some void pointers, like [0].
> > 
> > If you convert into an array (which happens sometimes, as you point
> > out), you cannot assume alignment stronger than single bytes. If you do
> > remove the memcpy, please adjust the dst argument pointer to be
> > __le64* to catch these cases of possible misalignment.
> > 
> 
> ah, I see (maybe).
> 
> Then I should use more "put_unaligned_le64" for ieee802154_be64_to_le64
> and "put_unaligned_be64" for ieee802154_le64_to_be64.
> 

ehm, I mean just do put_unaligned64 in both functions. I need to run swab64p
in every endianess case. Doesn't depend if I am on a big or little endian
machine.

- Alex

[PATCH bluetooth-next 3/3] ieee802154: cleanup ieee802154_le64_to_be64

[Alexander Aring]

This patch cleanups the ieee802154_le64_to_be64 function. This patch
removes an unnecessary temporary variable.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 include/net/mac802154.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/net/mac802154.h b/include/net/mac802154.h
index a4dcefe..1362eb5 100644
--- a/include/net/mac802154.h
+++ b/include/net/mac802154.h
@@ -243,9 +243,7 @@ static inline void ieee802154_be64_to_le64(void *le64_dst, const void *be64_src)
  */
 static inline void ieee802154_le64_to_be64(void *be64_dst, const void *le64_src)
 {
-	__be64 tmp = (__force __be64)swab64p(le64_src);
-
-	memcpy(be64_dst, &tmp, IEEE802154_EXTENDED_ADDR_LEN);
+	*((__be64 *)be64_dst) = (__force __be64)swab64p(le64_src);
 }
 
 /* Basic interface to register ieee802154 hwice */
-- 
2.2.2