From: Alexander Aring <aahringo@redhat.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Alexander Aring <alex.aring@gmail.com>,
Stefan Schmidt <stefan@datenfreihafen.org>,
linux-wpan - ML <linux-wpan@vger.kernel.org>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
Eric Dumazet <edumazet@google.com>,
Network Development <netdev@vger.kernel.org>,
David Girault <david.girault@qorvo.com>,
Romuald Despres <romuald.despres@qorvo.com>,
Frederic Blain <frederic.blain@qorvo.com>,
Nicolas Schodet <nico@ni.fr.eu.org>,
Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH wpan-next 01/20] net: mac802154: Allow the creation of coordinator interfaces
Date: Wed, 24 Aug 2022 17:43:11 -0400 [thread overview]
Message-ID: <CAK-6q+gqX8w+WEgSk2J9FOdrFJPvqJOsgmaY4wOu=siRszBujA@mail.gmail.com> (raw)
In-Reply-To: <20220824093547.16f05d15@xps-13>
On Wed, Aug 24, 2022 at 3:35 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@redhat.com wrote on Tue, 23 Aug 2022 17:44:52 -0400:
>
> > Hi,
> >
> > On Tue, Aug 23, 2022 at 12:29 PM Miquel Raynal
> > <miquel.raynal@bootlin.com> wrote:
> > >
> > > Hi Alexander,
> > >
> > > aahringo@redhat.com wrote on Tue, 23 Aug 2022 08:33:30 -0400:
> > >
> > > > Hi,
> > > >
> > > > On Fri, Aug 19, 2022 at 1:11 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > >
> > > > > Hi Alexander,
> > > > >
> > > > > aahringo@redhat.com wrote on Tue, 5 Jul 2022 21:51:02 -0400:
> > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Fri, Jul 1, 2022 at 10:36 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > > > >
> > > > > > > As a first strep in introducing proper PAN management and association,
> > > > > > > we need to be able to create coordinator interfaces which might act as
> > > > > > > coordinator or PAN coordinator.
> > > > > > >
> > > > > > > Hence, let's add the minimum support to allow the creation of these
> > > > > > > interfaces. This might be restrained and improved later.
> > > > > > >
> > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > > > ---
> > > > > > > net/mac802154/iface.c | 14 ++++++++------
> > > > > > > net/mac802154/rx.c | 2 +-
> > > > > > > 2 files changed, 9 insertions(+), 7 deletions(-)
> > > > > > >
> > > > > > > diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
> > > > > > > index 500ed1b81250..7ac0c5685d3f 100644
> > > > > > > --- a/net/mac802154/iface.c
> > > > > > > +++ b/net/mac802154/iface.c
> > > > > > > @@ -273,13 +273,13 @@ ieee802154_check_concurrent_iface(struct ieee802154_sub_if_data *sdata,
> > > > > > > if (nsdata != sdata && ieee802154_sdata_running(nsdata)) {
> > > > > > > int ret;
> > > > > > >
> > > > > > > - /* TODO currently we don't support multiple node types
> > > > > > > - * we need to run skb_clone at rx path. Check if there
> > > > > > > - * exist really an use case if we need to support
> > > > > > > - * multiple node types at the same time.
> > > > > > > + /* TODO currently we don't support multiple node/coord
> > > > > > > + * types we need to run skb_clone at rx path. Check if
> > > > > > > + * there exist really an use case if we need to support
> > > > > > > + * multiple node/coord types at the same time.
> > > > > > > */
> > > > > > > - if (wpan_dev->iftype == NL802154_IFTYPE_NODE &&
> > > > > > > - nsdata->wpan_dev.iftype == NL802154_IFTYPE_NODE)
> > > > > > > + if (wpan_dev->iftype != NL802154_IFTYPE_MONITOR &&
> > > > > > > + nsdata->wpan_dev.iftype != NL802154_IFTYPE_MONITOR)
> > > > > > > return -EBUSY;
> > > > > > >
> > > > > > > /* check all phy mac sublayer settings are the same.
> > > > > > > @@ -577,6 +577,7 @@ ieee802154_setup_sdata(struct ieee802154_sub_if_data *sdata,
> > > > > > > wpan_dev->short_addr = cpu_to_le16(IEEE802154_ADDR_BROADCAST);
> > > > > > >
> > > > > > > switch (type) {
> > > > > > > + case NL802154_IFTYPE_COORD:
> > > > > > > case NL802154_IFTYPE_NODE:
> > > > > > > ieee802154_be64_to_le64(&wpan_dev->extended_addr,
> > > > > > > sdata->dev->dev_addr);
> > > > > > > @@ -636,6 +637,7 @@ ieee802154_if_add(struct ieee802154_local *local, const char *name,
> > > > > > > ieee802154_le64_to_be64(ndev->perm_addr,
> > > > > > > &local->hw.phy->perm_extended_addr);
> > > > > > > switch (type) {
> > > > > > > + case NL802154_IFTYPE_COORD:
> > > > > > > case NL802154_IFTYPE_NODE:
> > > > > > > ndev->type = ARPHRD_IEEE802154;
> > > > > > > if (ieee802154_is_valid_extended_unicast_addr(extended_addr)) {
> > > > > > > diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> > > > > > > index b8ce84618a55..39459d8d787a 100644
> > > > > > > --- a/net/mac802154/rx.c
> > > > > > > +++ b/net/mac802154/rx.c
> > > > > > > @@ -203,7 +203,7 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local,
> > > > > > > }
> > > > > > >
> > > > > > > list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> > > > > > > - if (sdata->wpan_dev.iftype != NL802154_IFTYPE_NODE)
> > > > > > > + if (sdata->wpan_dev.iftype == NL802154_IFTYPE_MONITOR)
> > > > > > > continue;
> > > > > >
> > > > > > I probably get why you are doing that, but first the overall design is
> > > > > > working differently - means you should add an additional receive path
> > > > > > for the special interface type.
> > > > > >
> > > > > > Also we "discovered" before that the receive path of node vs
> > > > > > coordinator is different... Where is the different handling here? I
> > > > > > don't see it, I see that NODE and COORD are the same now (because that
> > > > > > is _currently_ everything else than monitor). This change is not
> > > > > > enough and does "something" to handle in some way coordinator receive
> > > > > > path but there are things missing.
> > > > > >
> > > > > > 1. Changing the address filters that it signals the transceiver it's
> > > > > > acting as coordinator
> > > > > > 2. We _should_ also have additional handling for whatever the
> > > > > > additional handling what address filters are doing in mac802154
> > > > > > _because_ there is hardware which doesn't have address filtering e.g.
> > > > > > hwsim which depend that this is working in software like other
> > > > > > transceiver hardware address filters.
> > > > > >
> > > > > > For the 2. one, I don't know if we do that even for NODE right or we
> > > > > > just have the bare minimal support there... I don't assume that
> > > > > > everything is working correctly here but what I want to see is a
> > > > > > separate receive path for coordinators that people can send patches to
> > > > > > fix it.
> > > > >
> > > > > Yes, we do very little differently between the two modes, that's why I
> > > > > took the easy way: just changing the condition. I really don't see what
> > > > > I can currently add here, but I am fine changing the style to easily
> > > > > show people where to add filters for such or such interface, but right
> > > > > now both path will look very "identical", do we agree on that?
> > > >
> > > > mostly yes, but there exists a difference and we should at least check
> > > > if the node receive path violates the coordinator receive path and
> > > > vice versa.
> > > > Put it in a receive_path() function and then coord_receive_path(),
> > > > node_receive_path() that calls the receive_path() and do the
> > > > additional filtering for coordinators, etc.
> > > >
> > > > There should be a part in the standard about "third level filter rule
> > > > if it's a coordinator".
> > > > btw: this is because the address filter on the transceiver needs to
> > > > have the "i am a coordinator" boolean set which is missing in this
> > > > series. However it depends on the transceiver filtering level and the
> > > > mac802154 receive path if we actually need to run such filtering or
> > > > not.
> > >
> > > I must be missing some information because I can't find any places
> > > where what you suggest is described in the spec.
> > >
> > > I agree there are multiple filtering level so let's go through them one
> > > by one (6.7.2 Reception and rejection):
> > > - first level: is the checksum (FCS) valid?
> > > yes -> goto second level
> > > no -> drop
> > > - second level: are we in promiscuous mode?
> > > yes -> forward to upper layers
> > > no -> goto second level (bis)
> > > - second level (bis): are we scanning?
> > > yes -> goto scan filtering
> > > no -> goto third level
> > > - scan filtering: is it a beacon?
> > > yes -> process the beacon
> > > no -> drop
> > > - third level: is the frame valid? (type, source, destination, pan id,
> > > etc)
> > > yes -> forward to upper layers
> > > no -> drop
> > >
> > > But none of them, as you said, is dependent on the interface type.
> > > There is no mention of a specific filtering operation to do in all
> > > those cases when running in COORD mode. So I still don't get what
> > > should be included in either node_receive_path() which should be
> > > different than in coord_receive_path() for now.
> > >
> > > There are, however, two situations where the interface type has its
> > > importance:
> > > - Enhanced beacon requests with Enhanced beacon filter IE, which asks
> > > the receiving device to process/drop the request upon certain
> > > conditions (minimum LQI and/or randomness), as detailed in
> > > 7.4.4.6 Enhanced Beacon Filter IE. But, as mentioned in
> > > 7.5.9 Enhanced Beacon Request command: "The Enhanced Beacon Request
> > > command is optional for an FFD and an RFD", so this series was only
> > > targeting basic beaconing for now.
> > > - In relaying mode, the destination address must not be validated
> > > because the message needs to be re-emitted. Indeed, a receiver in
> > > relaying mode may not be the recipient. This is also optional and out
> > > of the scope of this series.
> > >
> > > Right now I have the below diff, which clarifies the two path, without
> > > too much changes in the current code because I don't really see why it
> > > would be necessary. Unless you convince me otherwise or read the spec
> > > differently than I do :) What do you think?
> > >
> >
> > "Reception and rejection"
> >
> > third-level filtering regarding "destination address" and if the
> > device is "PAN coordinator".
> > This is, in my opinion, what the coordinator boolean tells the
> > transceiver to do on hardware when doing address filter there. You can
> > also read that up in datasheets of transceivers as atf86rf233, search
> > for I_AM_COORD.
>
> Oh right, I now see what you mean!
>
> > Whereas they use the word "PAN coordinator" not "coordinator", if they
> > really make a difference there at this point..., if so then the kernel
> > must know if the coordinator is a pan coordinator or coordinator
> > because we need to set the address filter in kernel.
>
> Yes we need to make a difference, you can have several coordinators but
> a single PAN coordinator in a PAN. I think we can assume that the PAN
> coordinator is the coordinator with no parent (association-wise). With
> the addition of the association series, I can handle that, so I will
> create the two path as you advise, add a comment about this additional
> filter rule that we don't yet support, and finally after the
> association series add another commit to make this filtering rule real.
>
> >
> > > Thanks,
> > > Miquèl
> > >
> > > ---
> > >
> > > --- a/net/mac802154/rx.c
> > > +++ b/net/mac802154/rx.c
> > > @@ -194,6 +194,7 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local,
> > > int ret;
> > > struct ieee802154_sub_if_data *sdata;
> > > struct ieee802154_hdr hdr;
> > > + bool iface_found = false;
> > >
> > > ret = ieee802154_parse_frame_start(skb, &hdr);
> > > if (ret) {
> > > @@ -203,18 +204,31 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local,
> > > }
> > >
> > > list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> > > - if (sdata->wpan_dev.iftype != NL802154_IFTYPE_NODE)
> > > + if (sdata->wpan_dev.iftype == NL802154_IFTYPE_MONITOR)
> > > continue;
> > >
> > > if (!ieee802154_sdata_running(sdata))
> > > continue;
> > >
> > > + iface_found = true;
> > > + break;
> > > + }
> > > +
> > > + if (!iface_found) {
> > > + kfree_skb(skb);
> > > + return;
> > > + }
> > > +
> > > + /* TBD: Additional filtering is possible on NODEs and/or COORDINATORs */
> > > + switch (sdata->wpan_dev.iftype) {
> > > + case NL802154_IFTYPE_COORD:
> > > + case NL802154_IFTYPE_NODE:
> > > ieee802154_subif_frame(sdata, skb, &hdr);
> > > - skb = NULL;
> > > + break;
> > > + default:
> > > + kfree_skb(skb);
> > > break;
> > > }
> >
> > Why do you remove the whole interface looping above and make it only
> > run for one ?first found? ?
>
> To reduce the indentation level.
>
> > That code changes this behaviour and I do
> > not know why.
>
> The precedent code did:
> for_each_iface() {
> if (not a node)
> continue;
> if (not running)
> continue;
>
> subif_frame();
> break;
> }
>
> That final break also elected only the first running node iface.
> Otherwise it would mean that we allow the same skb to be consumed
> twice, which is wrong IMHO?
no? Why is that wrong? There is a real use-case to have multiple
interfaces on one phy (or to do it in near future, I said that
multiple times). This patch does a step backwards to this.
- Alex
next prev parent reply other threads:[~2022-08-24 21:43 UTC|newest]
Thread overview: 84+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-01 14:30 [PATCH wpan-next 00/20] net: ieee802154: Support scanning/beaconing Miquel Raynal
2022-07-01 14:30 ` [PATCH wpan-next 01/20] net: mac802154: Allow the creation of coordinator interfaces Miquel Raynal
2022-07-06 1:51 ` Alexander Aring
2022-08-19 17:11 ` Miquel Raynal
2022-08-23 12:33 ` Alexander Aring
2022-08-23 16:29 ` Miquel Raynal
2022-08-23 21:44 ` Alexander Aring
2022-08-24 7:35 ` Miquel Raynal
2022-08-24 21:43 ` Alexander Aring [this message]
2022-08-25 8:40 ` Miquel Raynal
2022-08-26 0:51 ` Alexander Aring
2022-08-26 1:35 ` Alexander Aring
2022-08-26 8:08 ` Miquel Raynal
2022-08-29 2:31 ` Alexander Aring
2022-08-29 8:05 ` Miquel Raynal
2022-08-26 7:30 ` Miquel Raynal
2022-08-24 10:20 ` Miquel Raynal
2022-08-24 12:43 ` Alexander Aring
2022-08-24 13:26 ` Miquel Raynal
2022-08-24 21:53 ` Alexander Aring
2022-08-25 1:02 ` Alexander Aring
2022-08-25 8:46 ` Miquel Raynal
2022-08-25 12:58 ` Miquel Raynal
2022-08-26 1:05 ` Alexander Aring
2022-08-26 7:54 ` Miquel Raynal
2022-08-29 2:52 ` Alexander Aring
2022-08-29 8:02 ` Miquel Raynal
2022-08-30 2:23 ` Alexander Aring
2022-08-31 15:39 ` Miquel Raynal
2022-09-01 0:09 ` Miquel Raynal
2022-09-01 13:09 ` Miquel Raynal
2022-09-02 2:38 ` Alexander Aring
2022-09-03 0:08 ` Miquel Raynal
2022-09-03 14:20 ` Alexander Aring
2022-09-03 14:31 ` Alexander Aring
2022-09-03 16:05 ` Miquel Raynal
2022-09-03 18:21 ` Alexander Aring
2022-09-03 18:29 ` Alexander Aring
2022-09-03 19:07 ` Alexander Aring
2022-09-03 19:10 ` Alexander Aring
2022-09-03 19:40 ` Alexander Aring
2022-09-05 3:16 ` Miquel Raynal
2022-09-05 22:35 ` Alexander Aring
2022-09-02 2:23 ` Alexander Aring
2022-09-02 2:39 ` Alexander Aring
2022-09-02 2:09 ` Alexander Aring
2022-07-01 14:30 ` [PATCH wpan-next 02/20] net: ieee802154: Advertize coordinators discovery Miquel Raynal
2022-07-01 14:30 ` [PATCH wpan-next 03/20] net: ieee802154: Handle " Miquel Raynal
2022-07-01 14:30 ` [PATCH wpan-next 04/20] net: ieee802154: Trace the registration of new PANs Miquel Raynal
2022-07-01 14:30 ` [PATCH wpan-next 05/20] net: ieee802154: Define frame types Miquel Raynal
2022-07-11 2:06 ` Alexander Aring
2022-08-19 17:13 ` Miquel Raynal
2022-07-01 14:30 ` [PATCH wpan-next 06/20] net: ieee802154: Add support for user scanning requests Miquel Raynal
2022-07-01 14:30 ` [PATCH wpan-next 07/20] net: ieee802154: Define a beacon frame header Miquel Raynal
2022-07-01 14:30 ` [PATCH wpan-next 08/20] net: mac802154: Prepare forcing specific symbol duration Miquel Raynal
2022-07-01 14:30 ` [PATCH wpan-next 09/20] net: mac802154: Introduce a global device lock Miquel Raynal
2022-07-04 1:12 ` Alexander Aring
2022-08-19 17:06 ` Miquel Raynal
2022-07-01 14:30 ` [PATCH wpan-next 10/20] net: mac802154: Handle passive scanning Miquel Raynal
2022-07-15 3:33 ` Alexander Aring
2022-07-15 3:42 ` Alexander Aring
2022-08-19 17:22 ` Miquel Raynal
2022-08-01 23:42 ` Alexander Aring
2022-08-01 23:54 ` Alexander Aring
2022-07-01 14:30 ` [PATCH wpan-next 11/20] net: ieee802154: Add support for user beaconing requests Miquel Raynal
2022-07-01 14:30 ` [PATCH wpan-next 12/20] net: mac802154: Handle basic beaconing Miquel Raynal
2022-07-01 14:30 ` [PATCH wpan-next 13/20] net: ieee802154: Add support for user active scan requests Miquel Raynal
2022-07-01 14:30 ` [PATCH wpan-next 14/20] net: mac802154: Handle active scanning Miquel Raynal
2022-07-01 14:30 ` [PATCH wpan-next 15/20] net: ieee802154: Add support for allowing to answer BEACON_REQ Miquel Raynal
2022-07-01 14:30 ` [PATCH wpan-next 16/20] net: mac802154: Handle received BEACON_REQ Miquel Raynal
2022-07-01 14:30 ` [PATCH wpan-next 17/20] net: ieee802154: Handle limited devices with only datagram support Miquel Raynal
2022-07-15 3:16 ` Alexander Aring
2022-08-19 17:13 ` Miquel Raynal
2022-08-23 12:43 ` Alexander Aring
2022-07-01 14:30 ` [PATCH wpan-next 18/20] ieee802154: ca8210: Flag the driver as being limited Miquel Raynal
2022-07-01 14:30 ` [PATCH wpan-next 19/20] ieee802154: hwsim: Do not check the rtnl Miquel Raynal
2022-07-06 1:23 ` Alexander Aring
2022-08-01 23:58 ` Alexander Aring
2022-08-19 17:09 ` Miquel Raynal
2022-08-25 22:41 ` Miquel Raynal
2022-07-01 14:30 ` [PATCH wpan-next 20/20] ieee802154: hwsim: Allow devices to be coordinators Miquel Raynal
2022-07-11 2:01 ` Alexander Aring
2022-08-19 17:12 ` Miquel Raynal
2022-07-04 1:17 ` [PATCH wpan-next 00/20] net: ieee802154: Support scanning/beaconing Alexander Aring
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAK-6q+gqX8w+WEgSk2J9FOdrFJPvqJOsgmaY4wOu=siRszBujA@mail.gmail.com' \
--to=aahringo@redhat.com \
--cc=alex.aring@gmail.com \
--cc=davem@davemloft.net \
--cc=david.girault@qorvo.com \
--cc=edumazet@google.com \
--cc=frederic.blain@qorvo.com \
--cc=kuba@kernel.org \
--cc=linux-wpan@vger.kernel.org \
--cc=miquel.raynal@bootlin.com \
--cc=netdev@vger.kernel.org \
--cc=nico@ni.fr.eu.org \
--cc=pabeni@redhat.com \
--cc=romuald.despres@qorvo.com \
--cc=stefan@datenfreihafen.org \
--cc=thomas.petazzoni@bootlin.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).