Re: [RFC 1/3 net] lorawan: Add LoRaWAN class module

Re: [RFC 1/3 net] lorawan: Add LoRaWAN class module

[Jian-Hong Pan]

Randy Dunlap <rdunlap@infradead.org> 於 2018年8月24日 週五 上午1:43寫道：
>
> Hi,
>
> On 08/23/2018 10:15 AM, Jian-Hong Pan wrote:
> > diff --git a/net/maclorawan/Kconfig b/net/maclorawan/Kconfig
> > new file mode 100644
> > index 000000000000..741992b059c6
> > --- /dev/null
> > +++ b/net/maclorawan/Kconfig
> > @@ -0,0 +1,14 @@
> > +config LORAWAN
> > +     tristate "MAC layer of LoRaWAN Network support"
> > +     select CRYPTO
> > +     select CRYPTO_CMAC
> > +     select CRYPTO_CBC
> > +     select CRYPTO_AES
> > +     ---help---
> > +       LoRaWAN defines a low data rate, low power and long range wireless
> > +       wide area networks. It was designed to organise networks of sensors,
> > +       switches and actuators, etc automation devices.  It could operate as
> > +       multiple kilometers wide.
>
> There are several things that I would change.  How about this instead?
>
>
>           LoRaWAN defines low data rate, low power and long range wireless
>           wide area networks. It was designed to organise networks of automation
>           devices, such as sensors, switches and actuators. It can operate
>           multiple kilometers wide.

That becomes better!  Thank you

> > +
> > +       Say Y here to compile LoRaWAN support into the kernel or say M to
> > +       compile it as modules.
>
>
> --
> ~Randy

Re: [RFC 2/3 net] lorawan: Add macro and definition for LoRaWAN class modlue

[Andreas Färber]

Hi Jian-Hong,

Many thanks and sorry for the delay. This patch mostly looks good and
should go in before its first uses, so I would like to queue it soon for
my hardware-MAC module drivers - but some questions below. Also a typo
in the subject, and we should probably prepend "net: " and I would
personally not mention the module here as it's a userspace API.

Am 23.08.18 um 19:15 schrieb Jian-Hong Pan:
> This patch add the macro and definition for the implementation of
> LoRaWAN protocol.

I would fix up the grammar nits in my tree then.

> 
> Signed-off-by: Jian-Hong Pan <starnight@g.ncu.edu.tw>
> ---
>  include/linux/socket.h              | 5 ++++-
>  include/uapi/linux/if_arp.h         | 1 +
>  include/uapi/linux/if_ether.h       | 1 +
>  net/core/dev.c                      | 4 ++--
>  security/selinux/hooks.c            | 4 +++-
>  security/selinux/include/classmap.h | 4 +++-
>  6 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/socket.h b/include/linux/socket.h
> index aa1e288b1659..e5c8381fd1aa 100644
> --- a/include/linux/socket.h
> +++ b/include/linux/socket.h
> @@ -209,8 +209,9 @@ struct ucred {
>  				 */
>  #define AF_XDP		44	/* XDP sockets			*/
>  #define AF_LORA		45	/* LoRa sockets			*/
> +#define AF_LORAWAN	46	/* LoRaWAN sockets			*/
>  
> -#define AF_MAX		46	/* For now.. */
> +#define AF_MAX		47	/* For now.. */
>  
>  /* Protocol families, same as address families. */
>  #define PF_UNSPEC	AF_UNSPEC
> @@ -261,6 +262,7 @@ struct ucred {
>  #define PF_SMC		AF_SMC
>  #define PF_XDP		AF_XDP
>  #define PF_LORA		AF_LORA
> +#define PF_LORAWAN	AF_LORAWAN
>  #define PF_MAX		AF_MAX
>  
>  /* Maximum queue length specifiable by listen.  */
> @@ -343,6 +345,7 @@ struct ucred {
>  #define SOL_KCM		281
>  #define SOL_TLS		282
>  #define SOL_XDP		283
> +#define SOL_LORAWAN	284
>  
>  /* IPX options */
>  #define IPX_TYPE	1
> diff --git a/include/uapi/linux/if_arp.h b/include/uapi/linux/if_arp.h
> index 1ed7cb3f2129..2376f7839355 100644
> --- a/include/uapi/linux/if_arp.h
> +++ b/include/uapi/linux/if_arp.h
> @@ -99,6 +99,7 @@
>  #define ARPHRD_6LOWPAN	825		/* IPv6 over LoWPAN             */
>  #define ARPHRD_VSOCKMON	826		/* Vsock monitor header		*/
>  #define ARPHRD_LORA	827		/* LoRa				*/
> +#define ARPHRD_LORAWAN	828		/* LoRaWAN			*/
>  
>  #define ARPHRD_VOID	  0xFFFF	/* Void type, nothing is known */
>  #define ARPHRD_NONE	  0xFFFE	/* zero header length */
> diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h
> index 45644dcf5b39..b1ac70d4a377 100644
> --- a/include/uapi/linux/if_ether.h
> +++ b/include/uapi/linux/if_ether.h
> @@ -148,6 +148,7 @@
>  					 * aggregation protocol
>  					 */
>  #define ETH_P_LORA	0x00FA		/* LoRa				*/
> +#define ETH_P_LORAWAN	0x00FB		/* LoRaWAN			*/
>  
>  /*
>   *	This is an Ethernet frame header.
> diff --git a/net/core/dev.c b/net/core/dev.c
> index f68122f0ab02..b95ce79ec5a8 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -297,7 +297,7 @@ static const unsigned short netdev_lock_type[] = {
>  	 ARPHRD_IRDA, ARPHRD_FCPP, ARPHRD_FCAL, ARPHRD_FCPL,
>  	 ARPHRD_FCFABRIC, ARPHRD_IEEE80211, ARPHRD_IEEE80211_PRISM,
>  	 ARPHRD_IEEE80211_RADIOTAP, ARPHRD_PHONET, ARPHRD_PHONET_PIPE,
> -	 ARPHRD_IEEE802154, ARPHRD_VOID, ARPHRD_NONE};
> +	 ARPHRD_IEEE802154, ARPHRD_LORAWAN, ARPHRD_VOID, ARPHRD_NONE};
>  
>  static const char *const netdev_lock_name[] = {
>  	"_xmit_NETROM", "_xmit_ETHER", "_xmit_EETHER", "_xmit_AX25",
> @@ -314,7 +314,7 @@ static const char *const netdev_lock_name[] = {
>  	"_xmit_IRDA", "_xmit_FCPP", "_xmit_FCAL", "_xmit_FCPL",
>  	"_xmit_FCFABRIC", "_xmit_IEEE80211", "_xmit_IEEE80211_PRISM",
>  	"_xmit_IEEE80211_RADIOTAP", "_xmit_PHONET", "_xmit_PHONET_PIPE",
> -	"_xmit_IEEE802154", "_xmit_VOID", "_xmit_NONE"};
> +	"_xmit_IEEE802154", "_xmit_LORAWAN", "_xmit_VOID", "_xmit_NONE"};
>  
>  static struct lock_class_key netdev_xmit_lock_key[ARRAY_SIZE(netdev_lock_type)];
>  static struct lock_class_key netdev_addr_lock_key[ARRAY_SIZE(netdev_lock_type)];

All your new constants except SOL_LORAWAN are always next to a LoRa one,
but not in these two arrays. I don't have such changes in my original
LoRa patch - am I missing such an array extension for ARPHRD_LORA then,
or what is the criteria for when to add this?

Other changes look fine to me.

Regards,
Andreas

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index aaf520a689d8..0da3a1d69cb8 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1477,7 +1477,9 @@ static inline u16 socket_type_to_security_class(int family, int type, int protoc
>  			return SECCLASS_XDP_SOCKET;
>  		case PF_LORA:
>  			return SECCLASS_LORA_SOCKET;
> -#if PF_MAX > 46
> +		case PF_LORAWAN:
> +			return SECCLASS_LORAWAN_SOCKET;
> +#if PF_MAX > 47
>  #error New address family defined, please update this function.
>  #endif
>  		}
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index 060d4bf8385e..fa0151fe6f32 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -244,9 +244,11 @@ struct security_class_mapping secclass_map[] = {
>  	  { COMMON_SOCK_PERMS, NULL } },
>  	{ "lora_socket",
>  	  { COMMON_SOCK_PERMS, NULL } },
> +	{ "lorawan_socket",
> +	  { COMMON_SOCK_PERMS, NULL } },
>  	{ NULL }
>    };
>  
> -#if PF_MAX > 46
> +#if PF_MAX > 47
>  #error New address family defined, please update secclass_map.
>  #endif
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [RFC 1/3 net] lorawan: Add LoRaWAN class module

[Andreas Färber]

Hi Jian-Hong,

[+ Afonso]

Am 23.08.18 um 19:15 schrieb Jian-Hong Pan:
> LoRaWAN defined by LoRa Alliance(TM) is the MAC layer over LoRa devices.
> 
> This patch implements part of Class A end-devices features defined in
> LoRaWAN(TM) Specification Ver. 1.0.2:
> 1. End-device receive slot timing
> 2. Only single channel and single data rate for now
> 3. Unconfirmed data up/down message types
> 4. Encryption/decryption for up/down link data messages
> 
> It also implements the the functions and maps to Datagram socket for
> LoRaWAN unconfirmed data messages.
> 
> On the other side, it defines the basic interface and operation
> functions for compatible LoRa device drivers.
> 
> Signed-off-by: Jian-Hong Pan <starnight@g.ncu.edu.tw>
> ---
>  include/linux/maclorawan/lora.h | 239 +++++++++++
>  net/maclorawan/Kconfig          |  14 +
>  net/maclorawan/Makefile         |   2 +
>  net/maclorawan/lorawan.h        | 219 ++++++++++
>  net/maclorawan/lrwsec.c         | 237 +++++++++++
>  net/maclorawan/lrwsec.h         |  57 +++
>  net/maclorawan/mac.c            | 552 +++++++++++++++++++++++++
>  net/maclorawan/main.c           | 665 ++++++++++++++++++++++++++++++
>  net/maclorawan/socket.c         | 700 ++++++++++++++++++++++++++++++++
>  9 files changed, 2685 insertions(+)
>  create mode 100644 include/linux/maclorawan/lora.h

Can we use include/linux/lora/lorawan.h for simplicity?

>  create mode 100644 net/maclorawan/Kconfig
>  create mode 100644 net/maclorawan/Makefile
>  create mode 100644 net/maclorawan/lorawan.h
>  create mode 100644 net/maclorawan/lrwsec.c
>  create mode 100644 net/maclorawan/lrwsec.h
>  create mode 100644 net/maclorawan/mac.c
>  create mode 100644 net/maclorawan/main.c
>  create mode 100644 net/maclorawan/socket.c

This patch is much too large for me to review...

It also doesn't seem to follow the structure I suggested: 802.15.4 has
two separate directories, net/ieee802154/ and net/mac802154/. Therefore
I had suggested net/maclorawan/ only for code that translates from
ETH_P_LORAWAN to ETH_P_LORA socket buffers, i.e. your soft MAC. Generic
socket code that applies also to hard MAC drivers I expected to go
either to net/lora/lorawan/ or better net/lorawan/ or some other clearly
separate location, with its own Kconfig symbol - any reason not to?
Consider which parts can be enabled/used independently of each other,
then you can put them into two (or more?) different patches.

Also please use SPDX license identifiers in your headers.
And please don't put the whole world in To, use CC.

Thanks,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [RFC 2/3 net] lorawan: Add macro and definition for LoRaWAN class modlue

[Jian-Hong Pan]

Andreas Färber <afaerber@suse.de> 於 2018年9月24日 週一 上午12:06寫道：
>
> Hi Jian-Hong,
>
> Many thanks and sorry for the delay. This patch mostly looks good and
> should go in before its first uses, so I would like to queue it soon for
> my hardware-MAC module drivers - but some questions below. Also a typo
> in the subject, and we should probably prepend "net: " and I would
> personally not mention the module here as it's a userspace API.
>
> Am 23.08.18 um 19:15 schrieb Jian-Hong Pan:
> > This patch add the macro and definition for the implementation of
> > LoRaWAN protocol.
>
> I would fix up the grammar nits in my tree then.

Oh!  The grammar things should be fixed.  Thanks
And, I am preparing V2 patch, too.

> >
> > Signed-off-by: Jian-Hong Pan <starnight@g.ncu.edu.tw>
> > ---
> >  include/linux/socket.h              | 5 ++++-
> >  include/uapi/linux/if_arp.h         | 1 +
> >  include/uapi/linux/if_ether.h       | 1 +
> >  net/core/dev.c                      | 4 ++--
> >  security/selinux/hooks.c            | 4 +++-
> >  security/selinux/include/classmap.h | 4 +++-
> >  6 files changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/linux/socket.h b/include/linux/socket.h
> > index aa1e288b1659..e5c8381fd1aa 100644
> > --- a/include/linux/socket.h
> > +++ b/include/linux/socket.h
> > @@ -209,8 +209,9 @@ struct ucred {
> >                                */
> >  #define AF_XDP               44      /* XDP sockets                  */
> >  #define AF_LORA              45      /* LoRa sockets                 */
> > +#define AF_LORAWAN   46      /* LoRaWAN sockets                      */
> >
> > -#define AF_MAX               46      /* For now.. */
> > +#define AF_MAX               47      /* For now.. */
> >
> >  /* Protocol families, same as address families. */
> >  #define PF_UNSPEC    AF_UNSPEC
> > @@ -261,6 +262,7 @@ struct ucred {
> >  #define PF_SMC               AF_SMC
> >  #define PF_XDP               AF_XDP
> >  #define PF_LORA              AF_LORA
> > +#define PF_LORAWAN   AF_LORAWAN
> >  #define PF_MAX               AF_MAX
> >
> >  /* Maximum queue length specifiable by listen.  */
> > @@ -343,6 +345,7 @@ struct ucred {
> >  #define SOL_KCM              281
> >  #define SOL_TLS              282
> >  #define SOL_XDP              283
> > +#define SOL_LORAWAN  284
> >
> >  /* IPX options */
> >  #define IPX_TYPE     1
> > diff --git a/include/uapi/linux/if_arp.h b/include/uapi/linux/if_arp.h
> > index 1ed7cb3f2129..2376f7839355 100644
> > --- a/include/uapi/linux/if_arp.h
> > +++ b/include/uapi/linux/if_arp.h
> > @@ -99,6 +99,7 @@
> >  #define ARPHRD_6LOWPAN       825             /* IPv6 over LoWPAN             */
> >  #define ARPHRD_VSOCKMON      826             /* Vsock monitor header         */
> >  #define ARPHRD_LORA  827             /* LoRa                         */
> > +#define ARPHRD_LORAWAN       828             /* LoRaWAN                      */
> >
> >  #define ARPHRD_VOID    0xFFFF        /* Void type, nothing is known */
> >  #define ARPHRD_NONE    0xFFFE        /* zero header length */
> > diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h
> > index 45644dcf5b39..b1ac70d4a377 100644
> > --- a/include/uapi/linux/if_ether.h
> > +++ b/include/uapi/linux/if_ether.h
> > @@ -148,6 +148,7 @@
> >                                        * aggregation protocol
> >                                        */
> >  #define ETH_P_LORA   0x00FA          /* LoRa                         */
> > +#define ETH_P_LORAWAN        0x00FB          /* LoRaWAN                      */
> >
> >  /*
> >   *   This is an Ethernet frame header.
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index f68122f0ab02..b95ce79ec5a8 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -297,7 +297,7 @@ static const unsigned short netdev_lock_type[] = {
> >        ARPHRD_IRDA, ARPHRD_FCPP, ARPHRD_FCAL, ARPHRD_FCPL,
> >        ARPHRD_FCFABRIC, ARPHRD_IEEE80211, ARPHRD_IEEE80211_PRISM,
> >        ARPHRD_IEEE80211_RADIOTAP, ARPHRD_PHONET, ARPHRD_PHONET_PIPE,
> > -      ARPHRD_IEEE802154, ARPHRD_VOID, ARPHRD_NONE};
> > +      ARPHRD_IEEE802154, ARPHRD_LORAWAN, ARPHRD_VOID, ARPHRD_NONE};
> >
> >  static const char *const netdev_lock_name[] = {
> >       "_xmit_NETROM", "_xmit_ETHER", "_xmit_EETHER", "_xmit_AX25",
> > @@ -314,7 +314,7 @@ static const char *const netdev_lock_name[] = {
> >       "_xmit_IRDA", "_xmit_FCPP", "_xmit_FCAL", "_xmit_FCPL",
> >       "_xmit_FCFABRIC", "_xmit_IEEE80211", "_xmit_IEEE80211_PRISM",
> >       "_xmit_IEEE80211_RADIOTAP", "_xmit_PHONET", "_xmit_PHONET_PIPE",
> > -     "_xmit_IEEE802154", "_xmit_VOID", "_xmit_NONE"};
> > +     "_xmit_IEEE802154", "_xmit_LORAWAN", "_xmit_VOID", "_xmit_NONE"};
> >
> >  static struct lock_class_key netdev_xmit_lock_key[ARRAY_SIZE(netdev_lock_type)];
> >  static struct lock_class_key netdev_addr_lock_key[ARRAY_SIZE(netdev_lock_type)];
>
> All your new constants except SOL_LORAWAN are always next to a LoRa one,
> but not in these two arrays. I don't have such changes in my original
> LoRa patch - am I missing such an array extension for ARPHRD_LORA then,
> or what is the criteria for when to add this?

That is for "ip" related commands or something else I did not discover.
For example:
$ ip link show lora0
4: lora0: <NOARP,UP,LOWER_UP> mtu 20 qdisc noqueue state UNKNOWN mode
DEFAULT group default qlen 1000
    link/[828] 01:02:03:04 brd ff:ff:ff:ff

The 828 is ARPHRD_LORAWAN.

Regards,
Jian-Hong Pan

Re: [RFC 1/3 net] lorawan: Add LoRaWAN class module

[Jian-Hong Pan]

Andreas Färber <afaerber@suse.de> 於 2018年9月24日 週一 上午12:40寫道：
>
> Hi Jian-Hong,
>
> [+ Afonso]
>
> Am 23.08.18 um 19:15 schrieb Jian-Hong Pan:
> > LoRaWAN defined by LoRa Alliance(TM) is the MAC layer over LoRa devices.
> >
> > This patch implements part of Class A end-devices features defined in
> > LoRaWAN(TM) Specification Ver. 1.0.2:
> > 1. End-device receive slot timing
> > 2. Only single channel and single data rate for now
> > 3. Unconfirmed data up/down message types
> > 4. Encryption/decryption for up/down link data messages
> >
> > It also implements the the functions and maps to Datagram socket for
> > LoRaWAN unconfirmed data messages.
> >
> > On the other side, it defines the basic interface and operation
> > functions for compatible LoRa device drivers.
> >
> > Signed-off-by: Jian-Hong Pan <starnight@g.ncu.edu.tw>
> > ---
> >  include/linux/maclorawan/lora.h | 239 +++++++++++
> >  net/maclorawan/Kconfig          |  14 +
> >  net/maclorawan/Makefile         |   2 +
> >  net/maclorawan/lorawan.h        | 219 ++++++++++
> >  net/maclorawan/lrwsec.c         | 237 +++++++++++
> >  net/maclorawan/lrwsec.h         |  57 +++
> >  net/maclorawan/mac.c            | 552 +++++++++++++++++++++++++
> >  net/maclorawan/main.c           | 665 ++++++++++++++++++++++++++++++
> >  net/maclorawan/socket.c         | 700 ++++++++++++++++++++++++++++++++
> >  9 files changed, 2685 insertions(+)
> >  create mode 100644 include/linux/maclorawan/lora.h
>
> Can we use include/linux/lora/lorawan.h for simplicity?

Technically, yes

> >  create mode 100644 net/maclorawan/Kconfig
> >  create mode 100644 net/maclorawan/Makefile
> >  create mode 100644 net/maclorawan/lorawan.h
> >  create mode 100644 net/maclorawan/lrwsec.c
> >  create mode 100644 net/maclorawan/lrwsec.h
> >  create mode 100644 net/maclorawan/mac.c
> >  create mode 100644 net/maclorawan/main.c
> >  create mode 100644 net/maclorawan/socket.c
>
> This patch is much too large for me to review...
>
> It also doesn't seem to follow the structure I suggested: 802.15.4 has
> two separate directories, net/ieee802154/ and net/mac802154/. Therefore
> I had suggested net/maclorawan/ only for code that translates from
> ETH_P_LORAWAN to ETH_P_LORA socket buffers, i.e. your soft MAC. Generic
> socket code that applies also to hard MAC drivers I expected to go
> either to net/lora/lorawan/ or better net/lorawan/ or some other clearly
> separate location, with its own Kconfig symbol - any reason not to?
> Consider which parts can be enabled/used independently of each other,
> then you can put them into two (or more?) different patches.

Maybe I misunderstood before.  Besides, the header files need to be
split for different paths or folders.
Anyway, I will try to separate it into parts in version 2 patches.

> Also please use SPDX license identifiers in your headers.
> And please don't put the whole world in To, use CC.

Regards,
Jian-Hong Pan