[PATCH wpan-next v2 0/2] ieee802154: Internal moves

[PATCH wpan-next v2 0/2] ieee802154: Internal moves

[Miquel Raynal]

I am sending just a couple of patches from David that can easily be
reviewed outside of any other big patch series:
* reorder a bit the Kconfig entries
* move the ieee802154_addr structure and give it a kdoc before using it
  in the scan procedure.

Changes since v1:
* There were four patches merged into two as advised by Stefan.

David Girault (2):
  net: ieee802154: Move the IEEE 802.15.4 Kconfig main entries
  net: ieee802154: Move the address structure earlier and provide a kdoc

 include/net/cfg802154.h | 28 +++++++++++++++++++---------
 net/Kconfig             |  3 +--
 net/ieee802154/Kconfig  |  1 +
 3 files changed, 21 insertions(+), 11 deletions(-)

-- 
2.27.0

[PATCH wpan-next v2 1/2] net: ieee802154: Move the IEEE 802.15.4 Kconfig main entries

[Miquel Raynal]

From: David Girault <david.girault@qorvo.com>

It makes certainly more sense to have all the low-range wireless
protocols such as Bluetooth, IEEE 802.11 (WiFi) and IEEE 802.15.4
together, so let's move the main IEEE 802.15.4 stack Kconfig entry at a
better location.

As the softMAC layer has no meaning outside of the IEEE 802.15.4 stack
and cannot be used without it, also move the mac802154 menu inside
ieee802154/.

Signed-off-by: David Girault <david.girault@qorvo.com>
[miquel.raynal@bootlin.com: Isolate this change from a bigger commit and
rewrite the commit message.]
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/Kconfig            | 3 +--
 net/ieee802154/Kconfig | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/Kconfig b/net/Kconfig
index 8a1f9d0287de..a5e31078fd14 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -228,8 +228,6 @@ source "net/x25/Kconfig"
 source "net/lapb/Kconfig"
 source "net/phonet/Kconfig"
 source "net/6lowpan/Kconfig"
-source "net/ieee802154/Kconfig"
-source "net/mac802154/Kconfig"
 source "net/sched/Kconfig"
 source "net/dcb/Kconfig"
 source "net/dns_resolver/Kconfig"
@@ -380,6 +378,7 @@ source "net/mac80211/Kconfig"
 
 endif # WIRELESS
 
+source "net/ieee802154/Kconfig"
 source "net/rfkill/Kconfig"
 source "net/9p/Kconfig"
 source "net/caif/Kconfig"
diff --git a/net/ieee802154/Kconfig b/net/ieee802154/Kconfig
index 31aed75fe62d..7e4b1d49d445 100644
--- a/net/ieee802154/Kconfig
+++ b/net/ieee802154/Kconfig
@@ -36,6 +36,7 @@ config IEEE802154_SOCKET
 	  for 802.15.4 dataframes. Also RAW socket interface to build MAC
 	  header from userspace.
 
+source "net/mac802154/Kconfig"
 source "net/ieee802154/6lowpan/Kconfig"
 
 endif
-- 
2.27.0

[PATCH wpan-next v2 2/2] net: ieee802154: Move the address structure earlier and provide a kdoc

[Miquel Raynal]

From: David Girault <david.girault@qorvo.com>

Move the address structure earlier in the cfg802154.h header in order to
use it in subsequent additions. Give this structure a header to better
explain its content.

Signed-off-by: David Girault <david.girault@qorvo.com>
[miquel.raynal@bootlin.com: Isolate this change from a bigger commit and
                            reword the comment]
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/net/cfg802154.h | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
index 4491e2724ff2..0b8b1812cea1 100644
--- a/include/net/cfg802154.h
+++ b/include/net/cfg802154.h
@@ -29,6 +29,25 @@ struct ieee802154_llsec_key_id;
 struct ieee802154_llsec_key;
 #endif /* CONFIG_IEEE802154_NL802154_EXPERIMENTAL */
 
+/**
+ * struct ieee802154_addr - IEEE802.15.4 device address
+ * @mode: Address mode from frame header. Can be one of:
+ *        - @IEEE802154_ADDR_NONE
+ *        - @IEEE802154_ADDR_SHORT
+ *        - @IEEE802154_ADDR_LONG
+ * @pan_id: The PAN ID this address belongs to
+ * @short_addr: address if @mode is @IEEE802154_ADDR_SHORT
+ * @extended_addr: address if @mode is @IEEE802154_ADDR_LONG
+ */
+struct ieee802154_addr {
+	u8 mode;
+	__le16 pan_id;
+	union {
+		__le16 short_addr;
+		__le64 extended_addr;
+	};
+};
+
 struct cfg802154_ops {
 	struct net_device * (*add_virtual_intf_deprecated)(struct wpan_phy *wpan_phy,
 							   const char *name,
@@ -277,15 +296,6 @@ static inline void wpan_phy_net_set(struct wpan_phy *wpan_phy, struct net *net)
 	write_pnet(&wpan_phy->_net, net);
 }
 
-struct ieee802154_addr {
-	u8 mode;
-	__le16 pan_id;
-	union {
-		__le16 short_addr;
-		__le64 extended_addr;
-	};
-};
-
 struct ieee802154_llsec_key_id {
 	u8 mode;
 	u8 id;
-- 
2.27.0

Re: [PATCH wpan-next v2 1/2] net: ieee802154: Move the IEEE 802.15.4 Kconfig main entries

[Alexander Aring]

Hi,

On Fri, Jan 28, 2022 at 6:20 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> From: David Girault <david.girault@qorvo.com>
>
> It makes certainly more sense to have all the low-range wireless
> protocols such as Bluetooth, IEEE 802.11 (WiFi) and IEEE 802.15.4
> together, so let's move the main IEEE 802.15.4 stack Kconfig entry at a
> better location.
>
> As the softMAC layer has no meaning outside of the IEEE 802.15.4 stack
> and cannot be used without it, also move the mac802154 menu inside
> ieee802154/.
>
> Signed-off-by: David Girault <david.girault@qorvo.com>
> [miquel.raynal@bootlin.com: Isolate this change from a bigger commit and
> rewrite the commit message.]
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  net/Kconfig            | 3 +--
>  net/ieee802154/Kconfig | 1 +
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/Kconfig b/net/Kconfig
> index 8a1f9d0287de..a5e31078fd14 100644
> --- a/net/Kconfig
> +++ b/net/Kconfig
> @@ -228,8 +228,6 @@ source "net/x25/Kconfig"
>  source "net/lapb/Kconfig"
>  source "net/phonet/Kconfig"
>  source "net/6lowpan/Kconfig"
> -source "net/ieee802154/Kconfig"
> -source "net/mac802154/Kconfig"
>  source "net/sched/Kconfig"
>  source "net/dcb/Kconfig"
>  source "net/dns_resolver/Kconfig"
> @@ -380,6 +378,7 @@ source "net/mac80211/Kconfig"
>
>  endif # WIRELESS
>
> +source "net/ieee802154/Kconfig"

I would argue here that IEEE 802.15.4 is no "network option". However
I was talking once about moving it, but people don't like to move
things there around.
In my opinion there is no formal place to "have all the low-range
wireless such as Bluetooth, IEEE 802.11 (WiFi) and IEEE 802.15.4
together". If you bring all subsystems together and put them into an
own menuentry this would look different.

You forgot to move mac802154 as well here, even though it's changed in
the following patch.

If nobody else complains about moving Kconfig entries here around it
looks okay for me.

- Alex

Re: [PATCH wpan-next v2 1/2] net: ieee802154: Move the IEEE 802.15.4 Kconfig main entries

[Alexander Aring]

Hi,

I will do this review again because I messed up with other series.

On Fri, Jan 28, 2022 at 6:20 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> From: David Girault <david.girault@qorvo.com>
>
> It makes certainly more sense to have all the low-range wireless
> protocols such as Bluetooth, IEEE 802.11 (WiFi) and IEEE 802.15.4
> together, so let's move the main IEEE 802.15.4 stack Kconfig entry at a
> better location.
>
> As the softMAC layer has no meaning outside of the IEEE 802.15.4 stack
> and cannot be used without it, also move the mac802154 menu inside
> ieee802154/.
>

That's why there is a "depends on".

> Signed-off-by: David Girault <david.girault@qorvo.com>
> [miquel.raynal@bootlin.com: Isolate this change from a bigger commit and
> rewrite the commit message.]
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  net/Kconfig            | 3 +--
>  net/ieee802154/Kconfig | 1 +
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/Kconfig b/net/Kconfig
> index 8a1f9d0287de..a5e31078fd14 100644
> --- a/net/Kconfig
> +++ b/net/Kconfig
> @@ -228,8 +228,6 @@ source "net/x25/Kconfig"
>  source "net/lapb/Kconfig"
>  source "net/phonet/Kconfig"
>  source "net/6lowpan/Kconfig"
> -source "net/ieee802154/Kconfig"

I would argue here that IEEE 802.15.4 is no "network option". However
I was talking once about moving it, but people don't like to move
things there around.
In my opinion there is no formal place to "have all the low-range
wireless such as Bluetooth, IEEE 802.11 (WiFi) and IEEE 802.15.4
together". If you bring all subsystems together and put them into an
own menuentry this would look different.

If nobody else complains about moving Kconfig entries here around it
looks okay for me.

> -source "net/mac802154/Kconfig"
>  source "net/sched/Kconfig"
>  source "net/dcb/Kconfig"
>  source "net/dns_resolver/Kconfig"
> @@ -380,6 +378,7 @@ source "net/mac80211/Kconfig"
>
>  endif # WIRELESS
>
> +source "net/ieee802154/Kconfig"
>  source "net/rfkill/Kconfig"
>  source "net/9p/Kconfig"
>  source "net/caif/Kconfig"
> diff --git a/net/ieee802154/Kconfig b/net/ieee802154/Kconfig
> index 31aed75fe62d..7e4b1d49d445 100644
> --- a/net/ieee802154/Kconfig
> +++ b/net/ieee802154/Kconfig
> @@ -36,6 +36,7 @@ config IEEE802154_SOCKET
>           for 802.15.4 dataframes. Also RAW socket interface to build MAC
>           header from userspace.
>
> +source "net/mac802154/Kconfig"

The next person in a year will probably argue "but wireless do source
of wireless/mac80211 in net/Kconfig... so this is wrong".
To avoid this issue maybe we should take out the menuentry here and do
whatever wireless is doing without questioning it?

- Alex

Re: [PATCH wpan-next v2 2/2] net: ieee802154: Move the address structure earlier and provide a kdoc

[Alexander Aring]

Hi,

On Fri, Jan 28, 2022 at 6:20 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> From: David Girault <david.girault@qorvo.com>
>
> Move the address structure earlier in the cfg802154.h header in order to
> use it in subsequent additions. Give this structure a header to better
> explain its content.
>
> Signed-off-by: David Girault <david.girault@qorvo.com>
> [miquel.raynal@bootlin.com: Isolate this change from a bigger commit and
>                             reword the comment]
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  include/net/cfg802154.h | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> index 4491e2724ff2..0b8b1812cea1 100644
> --- a/include/net/cfg802154.h
> +++ b/include/net/cfg802154.h
> @@ -29,6 +29,25 @@ struct ieee802154_llsec_key_id;
>  struct ieee802154_llsec_key;
>  #endif /* CONFIG_IEEE802154_NL802154_EXPERIMENTAL */
>
> +/**
> + * struct ieee802154_addr - IEEE802.15.4 device address
> + * @mode: Address mode from frame header. Can be one of:
> + *        - @IEEE802154_ADDR_NONE
> + *        - @IEEE802154_ADDR_SHORT
> + *        - @IEEE802154_ADDR_LONG
> + * @pan_id: The PAN ID this address belongs to
> + * @short_addr: address if @mode is @IEEE802154_ADDR_SHORT
> + * @extended_addr: address if @mode is @IEEE802154_ADDR_LONG
> + */
> +struct ieee802154_addr {
> +       u8 mode;
> +       __le16 pan_id;
> +       union {
> +               __le16 short_addr;
> +               __le64 extended_addr;
> +       };
> +};
> +
>  struct cfg802154_ops {
>         struct net_device * (*add_virtual_intf_deprecated)(struct wpan_phy *wpan_phy,
>                                                            const char *name,
> @@ -277,15 +296,6 @@ static inline void wpan_phy_net_set(struct wpan_phy *wpan_phy, struct net *net)
>         write_pnet(&wpan_phy->_net, net);
>  }
>
> -struct ieee802154_addr {
> -       u8 mode;
> -       __le16 pan_id;
> -       union {
> -               __le16 short_addr;
> -               __le64 extended_addr;
> -       };
> -};
> -

I don't see the sense of moving this around? Is there a compilation
warning/error?

- Alex

Re: [PATCH wpan-next v2 2/2] net: ieee802154: Move the address structure earlier and provide a kdoc

[Miquel Raynal]

Hi Alexander,

alex.aring@gmail.com wrote on Sun, 30 Jan 2022 16:09:00 -0500:

> Hi,
> 
> On Fri, Jan 28, 2022 at 6:20 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > From: David Girault <david.girault@qorvo.com>
> >
> > Move the address structure earlier in the cfg802154.h header in order to
> > use it in subsequent additions. Give this structure a header to better
> > explain its content.
> >
> > Signed-off-by: David Girault <david.girault@qorvo.com>
> > [miquel.raynal@bootlin.com: Isolate this change from a bigger commit and
> >                             reword the comment]
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  include/net/cfg802154.h | 28 +++++++++++++++++++---------
> >  1 file changed, 19 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> > index 4491e2724ff2..0b8b1812cea1 100644
> > --- a/include/net/cfg802154.h
> > +++ b/include/net/cfg802154.h
> > @@ -29,6 +29,25 @@ struct ieee802154_llsec_key_id;
> >  struct ieee802154_llsec_key;
> >  #endif /* CONFIG_IEEE802154_NL802154_EXPERIMENTAL */
> >
> > +/**
> > + * struct ieee802154_addr - IEEE802.15.4 device address
> > + * @mode: Address mode from frame header. Can be one of:
> > + *        - @IEEE802154_ADDR_NONE
> > + *        - @IEEE802154_ADDR_SHORT
> > + *        - @IEEE802154_ADDR_LONG
> > + * @pan_id: The PAN ID this address belongs to
> > + * @short_addr: address if @mode is @IEEE802154_ADDR_SHORT
> > + * @extended_addr: address if @mode is @IEEE802154_ADDR_LONG
> > + */
> > +struct ieee802154_addr {
> > +       u8 mode;
> > +       __le16 pan_id;
> > +       union {
> > +               __le16 short_addr;
> > +               __le64 extended_addr;
> > +       };
> > +};
> > +
> >  struct cfg802154_ops {
> >         struct net_device * (*add_virtual_intf_deprecated)(struct wpan_phy *wpan_phy,
> >                                                            const char *name,
> > @@ -277,15 +296,6 @@ static inline void wpan_phy_net_set(struct wpan_phy *wpan_phy, struct net *net)
> >         write_pnet(&wpan_phy->_net, net);
> >  }
> >
> > -struct ieee802154_addr {
> > -       u8 mode;
> > -       __le16 pan_id;
> > -       union {
> > -               __le16 short_addr;
> > -               __le64 extended_addr;
> > -       };
> > -};
> > -  
> 
> I don't see the sense of moving this around? Is there a compilation
> warning/error?

Not yet but we will need to move this structure around soon. This
commit is like a 'preparation' step for the changes coming later.

I can move this later if you prefer.

Thanks,
Miquèl

Re: [PATCH wpan-next v2 1/2] net: ieee802154: Move the IEEE 802.15.4 Kconfig main entries

[Miquel Raynal]

Hi Alexander,

alex.aring@gmail.com wrote on Sun, 30 Jan 2022 16:07:53 -0500:

> Hi,
> 
> I will do this review again because I messed up with other series.
> 
> On Fri, Jan 28, 2022 at 6:20 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > From: David Girault <david.girault@qorvo.com>
> >
> > It makes certainly more sense to have all the low-range wireless
> > protocols such as Bluetooth, IEEE 802.11 (WiFi) and IEEE 802.15.4
> > together, so let's move the main IEEE 802.15.4 stack Kconfig entry at a
> > better location.
> >
> > As the softMAC layer has no meaning outside of the IEEE 802.15.4 stack
> > and cannot be used without it, also move the mac802154 menu inside
> > ieee802154/.
> >  
> 
> That's why there is a "depends on".
> 
> > Signed-off-by: David Girault <david.girault@qorvo.com>
> > [miquel.raynal@bootlin.com: Isolate this change from a bigger commit and
> > rewrite the commit message.]
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  net/Kconfig            | 3 +--
> >  net/ieee802154/Kconfig | 1 +
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/Kconfig b/net/Kconfig
> > index 8a1f9d0287de..a5e31078fd14 100644
> > --- a/net/Kconfig
> > +++ b/net/Kconfig
> > @@ -228,8 +228,6 @@ source "net/x25/Kconfig"
> >  source "net/lapb/Kconfig"
> >  source "net/phonet/Kconfig"
> >  source "net/6lowpan/Kconfig"
> > -source "net/ieee802154/Kconfig"  
> 
> I would argue here that IEEE 802.15.4 is no "network option". However
> I was talking once about moving it, but people don't like to move
> things there around.
> In my opinion there is no formal place to "have all the low-range
> wireless such as Bluetooth, IEEE 802.11 (WiFi) and IEEE 802.15.4
> together". If you bring all subsystems together and put them into an
> own menuentry this would look different.
> 
> If nobody else complains about moving Kconfig entries here around it
> looks okay for me.
> 
> > -source "net/mac802154/Kconfig"
> >  source "net/sched/Kconfig"
> >  source "net/dcb/Kconfig"
> >  source "net/dns_resolver/Kconfig"
> > @@ -380,6 +378,7 @@ source "net/mac80211/Kconfig"
> >
> >  endif # WIRELESS
> >
> > +source "net/ieee802154/Kconfig"
> >  source "net/rfkill/Kconfig"
> >  source "net/9p/Kconfig"
> >  source "net/caif/Kconfig"
> > diff --git a/net/ieee802154/Kconfig b/net/ieee802154/Kconfig
> > index 31aed75fe62d..7e4b1d49d445 100644
> > --- a/net/ieee802154/Kconfig
> > +++ b/net/ieee802154/Kconfig
> > @@ -36,6 +36,7 @@ config IEEE802154_SOCKET
> >           for 802.15.4 dataframes. Also RAW socket interface to build MAC
> >           header from userspace.
> >
> > +source "net/mac802154/Kconfig"  
> 
> The next person in a year will probably argue "but wireless do source
> of wireless/mac80211 in net/Kconfig... so this is wrong".
> To avoid this issue maybe we should take out the menuentry here and do
> whatever wireless is doing without questioning it?

Without discussing the cleanliness of the wireless subsystem, I don't
feel bad proposing alternatives :)

I'm fine adapting to your preferred solution either way, so could you
clarify what should I do:
- Drop that commit entirely.
- Move things into their own submenu (we can discuss the naming,
  "Low range wireless Networks" might be a good start).
- Keep it like it is.

Thanks,
Miquèl

Re: [PATCH wpan-next v2 2/2] net: ieee802154: Move the address structure earlier and provide a kdoc

[Miquel Raynal]

Hi Miquel,

miquel.raynal@bootlin.com wrote on Mon, 31 Jan 2022 11:46:45 +0100:

> Hi Alexander,
> 
> alex.aring@gmail.com wrote on Sun, 30 Jan 2022 16:09:00 -0500:
> 
> > Hi,
> > 
> > On Fri, Jan 28, 2022 at 6:20 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> > >
> > > From: David Girault <david.girault@qorvo.com>
> > >
> > > Move the address structure earlier in the cfg802154.h header in order to
> > > use it in subsequent additions. Give this structure a header to better
> > > explain its content.
> > >
> > > Signed-off-by: David Girault <david.girault@qorvo.com>
> > > [miquel.raynal@bootlin.com: Isolate this change from a bigger commit and
> > >                             reword the comment]
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  include/net/cfg802154.h | 28 +++++++++++++++++++---------
> > >  1 file changed, 19 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> > > index 4491e2724ff2..0b8b1812cea1 100644
> > > --- a/include/net/cfg802154.h
> > > +++ b/include/net/cfg802154.h
> > > @@ -29,6 +29,25 @@ struct ieee802154_llsec_key_id;
> > >  struct ieee802154_llsec_key;
> > >  #endif /* CONFIG_IEEE802154_NL802154_EXPERIMENTAL */
> > >
> > > +/**
> > > + * struct ieee802154_addr - IEEE802.15.4 device address
> > > + * @mode: Address mode from frame header. Can be one of:
> > > + *        - @IEEE802154_ADDR_NONE
> > > + *        - @IEEE802154_ADDR_SHORT
> > > + *        - @IEEE802154_ADDR_LONG
> > > + * @pan_id: The PAN ID this address belongs to
> > > + * @short_addr: address if @mode is @IEEE802154_ADDR_SHORT
> > > + * @extended_addr: address if @mode is @IEEE802154_ADDR_LONG
> > > + */
> > > +struct ieee802154_addr {
> > > +       u8 mode;
> > > +       __le16 pan_id;
> > > +       union {
> > > +               __le16 short_addr;
> > > +               __le64 extended_addr;
> > > +       };
> > > +};
> > > +
> > >  struct cfg802154_ops {
> > >         struct net_device * (*add_virtual_intf_deprecated)(struct wpan_phy *wpan_phy,
> > >                                                            const char *name,
> > > @@ -277,15 +296,6 @@ static inline void wpan_phy_net_set(struct wpan_phy *wpan_phy, struct net *net)
> > >         write_pnet(&wpan_phy->_net, net);
> > >  }
> > >
> > > -struct ieee802154_addr {
> > > -       u8 mode;
> > > -       __le16 pan_id;
> > > -       union {
> > > -               __le16 short_addr;
> > > -               __le64 extended_addr;
> > > -       };
> > > -};
> > > -    
> > 
> > I don't see the sense of moving this around? Is there a compilation
> > warning/error?  
> 
> Not yet but we will need to move this structure around soon. This
> commit is like a 'preparation' step for the changes coming later.
> 
> I can move this later if you prefer.

Actually there is not actual need for moving this structure anymore.
The number of changes applied on top of the original series have turned
that move unnecessary. I still believe however that structures should,
as far as possible, be defined at the top of headers files, instead of
be defined right before where they will be immediately used when
introduced. I'll cancel the move but I'll keep the addition of the kdoc
which I think is useful.

Thanks,
Miquèl

Re: [PATCH wpan-next v2 2/2] net: ieee802154: Move the address structure earlier and provide a kdoc

[Alexander Aring]

Hi,

On Mon, Jan 31, 2022 at 9:09 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Miquel,
>
> miquel.raynal@bootlin.com wrote on Mon, 31 Jan 2022 11:46:45 +0100:
>
> > Hi Alexander,
> >
> > alex.aring@gmail.com wrote on Sun, 30 Jan 2022 16:09:00 -0500:
> >
> > > Hi,
> > >
> > > On Fri, Jan 28, 2022 at 6:20 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > >
> > > > From: David Girault <david.girault@qorvo.com>
> > > >
> > > > Move the address structure earlier in the cfg802154.h header in order to
> > > > use it in subsequent additions. Give this structure a header to better
> > > > explain its content.
> > > >
> > > > Signed-off-by: David Girault <david.girault@qorvo.com>
> > > > [miquel.raynal@bootlin.com: Isolate this change from a bigger commit and
> > > >                             reword the comment]
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  include/net/cfg802154.h | 28 +++++++++++++++++++---------
> > > >  1 file changed, 19 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> > > > index 4491e2724ff2..0b8b1812cea1 100644
> > > > --- a/include/net/cfg802154.h
> > > > +++ b/include/net/cfg802154.h
> > > > @@ -29,6 +29,25 @@ struct ieee802154_llsec_key_id;
> > > >  struct ieee802154_llsec_key;
> > > >  #endif /* CONFIG_IEEE802154_NL802154_EXPERIMENTAL */
> > > >
> > > > +/**
> > > > + * struct ieee802154_addr - IEEE802.15.4 device address
> > > > + * @mode: Address mode from frame header. Can be one of:
> > > > + *        - @IEEE802154_ADDR_NONE
> > > > + *        - @IEEE802154_ADDR_SHORT
> > > > + *        - @IEEE802154_ADDR_LONG
> > > > + * @pan_id: The PAN ID this address belongs to
> > > > + * @short_addr: address if @mode is @IEEE802154_ADDR_SHORT
> > > > + * @extended_addr: address if @mode is @IEEE802154_ADDR_LONG
> > > > + */
> > > > +struct ieee802154_addr {
> > > > +       u8 mode;
> > > > +       __le16 pan_id;
> > > > +       union {
> > > > +               __le16 short_addr;
> > > > +               __le64 extended_addr;
> > > > +       };
> > > > +};
> > > > +
> > > >  struct cfg802154_ops {
> > > >         struct net_device * (*add_virtual_intf_deprecated)(struct wpan_phy *wpan_phy,
> > > >                                                            const char *name,
> > > > @@ -277,15 +296,6 @@ static inline void wpan_phy_net_set(struct wpan_phy *wpan_phy, struct net *net)
> > > >         write_pnet(&wpan_phy->_net, net);
> > > >  }
> > > >
> > > > -struct ieee802154_addr {
> > > > -       u8 mode;
> > > > -       __le16 pan_id;
> > > > -       union {
> > > > -               __le16 short_addr;
> > > > -               __le64 extended_addr;
> > > > -       };
> > > > -};
> > > > -
> > >
> > > I don't see the sense of moving this around? Is there a compilation
> > > warning/error?
> >
> > Not yet but we will need to move this structure around soon. This
> > commit is like a 'preparation' step for the changes coming later.
> >
> > I can move this later if you prefer.
>
> Actually there is not actual need for moving this structure anymore.
> The number of changes applied on top of the original series have turned
> that move unnecessary. I still believe however that structures should,
> as far as possible, be defined at the top of headers files, instead of
> be defined right before where they will be immediately used when
> introduced. I'll cancel the move but I'll keep the addition of the kdoc
> which I think is useful.
>
ok.

- Alex

Re: [PATCH wpan-next v2 1/2] net: ieee802154: Move the IEEE 802.15.4 Kconfig main entries

[Alexander Aring]

Hi,

On Mon, Jan 31, 2022 at 8:46 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> alex.aring@gmail.com wrote on Sun, 30 Jan 2022 16:07:53 -0500:
>
> > Hi,
> >
> > I will do this review again because I messed up with other series.
> >
> > On Fri, Jan 28, 2022 at 6:20 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > From: David Girault <david.girault@qorvo.com>
> > >
> > > It makes certainly more sense to have all the low-range wireless
> > > protocols such as Bluetooth, IEEE 802.11 (WiFi) and IEEE 802.15.4
> > > together, so let's move the main IEEE 802.15.4 stack Kconfig entry at a
> > > better location.
> > >
> > > As the softMAC layer has no meaning outside of the IEEE 802.15.4 stack
> > > and cannot be used without it, also move the mac802154 menu inside
> > > ieee802154/.
> > >
> >
> > That's why there is a "depends on".
> >
> > > Signed-off-by: David Girault <david.girault@qorvo.com>
> > > [miquel.raynal@bootlin.com: Isolate this change from a bigger commit and
> > > rewrite the commit message.]
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  net/Kconfig            | 3 +--
> > >  net/ieee802154/Kconfig | 1 +
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/net/Kconfig b/net/Kconfig
> > > index 8a1f9d0287de..a5e31078fd14 100644
> > > --- a/net/Kconfig
> > > +++ b/net/Kconfig
> > > @@ -228,8 +228,6 @@ source "net/x25/Kconfig"
> > >  source "net/lapb/Kconfig"
> > >  source "net/phonet/Kconfig"
> > >  source "net/6lowpan/Kconfig"
> > > -source "net/ieee802154/Kconfig"
> >
> > I would argue here that IEEE 802.15.4 is no "network option". However
> > I was talking once about moving it, but people don't like to move
> > things there around.
> > In my opinion there is no formal place to "have all the low-range
> > wireless such as Bluetooth, IEEE 802.11 (WiFi) and IEEE 802.15.4
> > together". If you bring all subsystems together and put them into an
> > own menuentry this would look different.
> >
> > If nobody else complains about moving Kconfig entries here around it
> > looks okay for me.
> >
> > > -source "net/mac802154/Kconfig"
> > >  source "net/sched/Kconfig"
> > >  source "net/dcb/Kconfig"
> > >  source "net/dns_resolver/Kconfig"
> > > @@ -380,6 +378,7 @@ source "net/mac80211/Kconfig"
> > >
> > >  endif # WIRELESS
> > >
> > > +source "net/ieee802154/Kconfig"
> > >  source "net/rfkill/Kconfig"
> > >  source "net/9p/Kconfig"
> > >  source "net/caif/Kconfig"
> > > diff --git a/net/ieee802154/Kconfig b/net/ieee802154/Kconfig
> > > index 31aed75fe62d..7e4b1d49d445 100644
> > > --- a/net/ieee802154/Kconfig
> > > +++ b/net/ieee802154/Kconfig
> > > @@ -36,6 +36,7 @@ config IEEE802154_SOCKET
> > >           for 802.15.4 dataframes. Also RAW socket interface to build MAC
> > >           header from userspace.
> > >
> > > +source "net/mac802154/Kconfig"
> >
> > The next person in a year will probably argue "but wireless do source
> > of wireless/mac80211 in net/Kconfig... so this is wrong".
> > To avoid this issue maybe we should take out the menuentry here and do
> > whatever wireless is doing without questioning it?
>
> Without discussing the cleanliness of the wireless subsystem, I don't
> feel bad proposing alternatives :)
>
> I'm fine adapting to your preferred solution either way, so could you
> clarify what should I do:
> - Drop that commit entirely.
> - Move things into their own submenu (we can discuss the naming,
>   "Low range wireless Networks" might be a good start).
> - Keep it like it is.

I think we should move things around to end in a situation like
wireless has it in net/Kconfig. This will avoid other movements
whoever declares what's wrong and right of handling Kconfig entries.

Sure you can do follow up patches which introduce "Low range wireless
Networks" and ask for acks for doing such change.

- A;ex