* [PATCH wpan-next] mac802154: Allow the creation of coordinator interfaces @ 2022-10-18 18:36 Miquel Raynal 2022-10-18 23:57 ` Alexander Aring 0 siblings, 1 reply; 8+ messages in thread From: Miquel Raynal @ 2022-10-18 18:36 UTC (permalink / raw) To: Alexander Aring, Stefan Schmidt, linux-wpan Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet, netdev, David Girault, Romuald Despres, Frederic Blain, Nicolas Schodet, Guilhem Imberton, Thomas Petazzoni, Miquel Raynal 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 support will be improved later, in particular regarding the filtering. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- net/mac802154/iface.c | 14 ++++++++------ net/mac802154/main.c | 2 ++ net/mac802154/rx.c | 11 +++++++---- 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c index d9b50884d34e..682249f3369b 100644 --- a/net/mac802154/iface.c +++ b/net/mac802154/iface.c @@ -262,13 +262,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. @@ -565,6 +565,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); @@ -624,6 +625,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/main.c b/net/mac802154/main.c index 40fab08df24b..d03ecb747afc 100644 --- a/net/mac802154/main.c +++ b/net/mac802154/main.c @@ -219,6 +219,8 @@ int ieee802154_register_hw(struct ieee802154_hw *hw) if (hw->flags & IEEE802154_HW_PROMISCUOUS) local->phy->supported.iftypes |= BIT(NL802154_IFTYPE_MONITOR); + else + local->phy->supported.iftypes &= ~BIT(NL802154_IFTYPE_COORD); rc = wpan_phy_register(local->phy); if (rc < 0) diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c index 2ae23a2f4a09..aca348d7834b 100644 --- a/net/mac802154/rx.c +++ b/net/mac802154/rx.c @@ -208,6 +208,7 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local, int ret; struct ieee802154_sub_if_data *sdata; struct ieee802154_hdr hdr; + struct sk_buff *skb2; ret = ieee802154_parse_frame_start(skb, &hdr); if (ret) { @@ -217,7 +218,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; if (!ieee802154_sdata_running(sdata)) @@ -230,9 +231,11 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local, sdata->required_filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) continue; - ieee802154_subif_frame(sdata, skb, &hdr); - skb = NULL; - break; + skb2 = skb_clone(skb, GFP_ATOMIC); + if (skb2) { + skb2->dev = sdata->dev; + ieee802154_subif_frame(sdata, skb2, &hdr); + } } kfree_skb(skb); -- 2.34.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH wpan-next] mac802154: Allow the creation of coordinator interfaces 2022-10-18 18:36 [PATCH wpan-next] mac802154: Allow the creation of coordinator interfaces Miquel Raynal @ 2022-10-18 23:57 ` Alexander Aring 2022-10-19 9:52 ` Miquel Raynal 0 siblings, 1 reply; 8+ messages in thread From: Alexander Aring @ 2022-10-18 23:57 UTC (permalink / raw) To: Miquel Raynal Cc: Alexander Aring, Stefan Schmidt, linux-wpan, David S. Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet, netdev, David Girault, Romuald Despres, Frederic Blain, Nicolas Schodet, Guilhem Imberton, Thomas Petazzoni Hi, On Tue, Oct 18, 2022 at 2:36 PM 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 support will be improved later, in particular regarding > the filtering. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > net/mac802154/iface.c | 14 ++++++++------ > net/mac802154/main.c | 2 ++ > net/mac802154/rx.c | 11 +++++++---- > 3 files changed, 17 insertions(+), 10 deletions(-) > > diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c > index d9b50884d34e..682249f3369b 100644 > --- a/net/mac802154/iface.c > +++ b/net/mac802154/iface.c > @@ -262,13 +262,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. > @@ -565,6 +565,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); > @@ -624,6 +625,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/main.c b/net/mac802154/main.c > index 40fab08df24b..d03ecb747afc 100644 > --- a/net/mac802154/main.c > +++ b/net/mac802154/main.c > @@ -219,6 +219,8 @@ int ieee802154_register_hw(struct ieee802154_hw *hw) > > if (hw->flags & IEEE802154_HW_PROMISCUOUS) > local->phy->supported.iftypes |= BIT(NL802154_IFTYPE_MONITOR); > + else > + local->phy->supported.iftypes &= ~BIT(NL802154_IFTYPE_COORD); > So this means if somebody in the driver sets iftype COORD is supported but then IEEE802154_HW_PROMISCUOUS is not supported it will not support COORD? Why is IEEE802154_HW_PROMISCUOUS required for COORD iftype? I thought IEEE802154_HW_PROMISCUOUS is required to do a scan? > rc = wpan_phy_register(local->phy); > if (rc < 0) > diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c > index 2ae23a2f4a09..aca348d7834b 100644 > --- a/net/mac802154/rx.c > +++ b/net/mac802154/rx.c > @@ -208,6 +208,7 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local, > int ret; > struct ieee802154_sub_if_data *sdata; > struct ieee802154_hdr hdr; > + struct sk_buff *skb2; > > ret = ieee802154_parse_frame_start(skb, &hdr); > if (ret) { > @@ -217,7 +218,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 guess this will work but I would like to see no logic about if it's not MONITOR it's NODE or COORD, because introducing others requires updating those again... however I think it's fine. I would like to see a different receive path for coord_rx() and node_rx(), but yea currently I guess they are mostly the same... in future I think they are required as PACKTE_HOST, etc. will be changed regarding pan coordinator or just coordinator (so far I understood). > > if (!ieee802154_sdata_running(sdata)) > @@ -230,9 +231,11 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local, > sdata->required_filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) > continue; > > - ieee802154_subif_frame(sdata, skb, &hdr); > - skb = NULL; > - break; > + skb2 = skb_clone(skb, GFP_ATOMIC); > + if (skb2) { > + skb2->dev = sdata->dev; > + ieee802154_subif_frame(sdata, skb2, &hdr); > + } > } > > kfree_skb(skb); If we do the clone above this kfree_skb() should be move to ieee802154_rx() right after __ieee802154_rx_handle_packet(). This patch also changes that we deliver one skb to multiple interfaces if there are more than one and I was not aware that we currently do that. :/ - Alex ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH wpan-next] mac802154: Allow the creation of coordinator interfaces 2022-10-18 23:57 ` Alexander Aring @ 2022-10-19 9:52 ` Miquel Raynal 2022-10-23 23:13 ` Alexander Aring 0 siblings, 1 reply; 8+ messages in thread From: Miquel Raynal @ 2022-10-19 9:52 UTC (permalink / raw) To: Alexander Aring Cc: Alexander Aring, Stefan Schmidt, linux-wpan, David S. Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet, netdev, David Girault, Romuald Despres, Frederic Blain, Nicolas Schodet, Guilhem Imberton, Thomas Petazzoni Hi Alexander, aahringo@redhat.com wrote on Tue, 18 Oct 2022 19:57:19 -0400: > Hi, > > On Tue, Oct 18, 2022 at 2:36 PM 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 support will be improved later, in particular regarding > > the filtering. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > net/mac802154/iface.c | 14 ++++++++------ > > net/mac802154/main.c | 2 ++ > > net/mac802154/rx.c | 11 +++++++---- > > 3 files changed, 17 insertions(+), 10 deletions(-) > > > > diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c > > index d9b50884d34e..682249f3369b 100644 > > --- a/net/mac802154/iface.c > > +++ b/net/mac802154/iface.c > > @@ -262,13 +262,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. > > @@ -565,6 +565,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); > > @@ -624,6 +625,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/main.c b/net/mac802154/main.c > > index 40fab08df24b..d03ecb747afc 100644 > > --- a/net/mac802154/main.c > > +++ b/net/mac802154/main.c > > @@ -219,6 +219,8 @@ int ieee802154_register_hw(struct ieee802154_hw *hw) > > > > if (hw->flags & IEEE802154_HW_PROMISCUOUS) > > local->phy->supported.iftypes |= BIT(NL802154_IFTYPE_MONITOR); > > + else > > + local->phy->supported.iftypes &= ~BIT(NL802154_IFTYPE_COORD); > > > > So this means if somebody in the driver sets iftype COORD is supported > but then IEEE802154_HW_PROMISCUOUS is not supported it will not > support COORD? > > Why is IEEE802154_HW_PROMISCUOUS required for COORD iftype? I thought > IEEE802154_HW_PROMISCUOUS is required to do a scan? You are totally right this is inconsistent, I'll drop the else block entirely. The fact that HW_PROMISCUOUS is supported when starting a scan is handled by the -EOPNOTSUPP return code from drv_set_promiscuous_mode() called by drv_start() in mac802154_trigger_scan(). However I need your input on the following topic: in my branch I have somewhere a patch adding IFTYPE_COORD to the list of phy->supported.iftypes in each individual driver. But right now, if we drop the promiscuous constraint as you suggest, I don't see anymore the need for setting this as a per-driver value. Should we make the possibility to create IFTYPE_COORD interfaces the default instead, something like this? --- a/net/mac802154/main.c +++ b/net/mac802154/main.c @@ -118,7 +118,7 @@ ieee802154_alloc_hw(size_t priv_data_len, const struct ieee802154_ops *ops) phy->supported.lbt = NL802154_SUPPORTED_BOOL_FALSE; /* always supported */ - phy->supported.iftypes = BIT(NL802154_IFTYPE_NODE); + phy->supported.iftypes = BIT(NL802154_IFTYPE_NODE) | BIT(NL802154_IFTYPE_COORD); > > rc = wpan_phy_register(local->phy); > > if (rc < 0) > > diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c > > index 2ae23a2f4a09..aca348d7834b 100644 > > --- a/net/mac802154/rx.c > > +++ b/net/mac802154/rx.c > > @@ -208,6 +208,7 @@ __ieee802154_rx_handle_packet(struct > > ieee802154_local *local, int ret; > > struct ieee802154_sub_if_data *sdata; > > struct ieee802154_hdr hdr; > > + struct sk_buff *skb2; > > > > ret = ieee802154_parse_frame_start(skb, &hdr); > > if (ret) { > > @@ -217,7 +218,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 guess this will work but I would like to see no logic about if it's > not MONITOR it's NODE or COORD, because introducing others requires > updating those again... however I think it's fine. Actually I don't get why we would not want this logic, it seem very relevant to me. Do you want a helper instead and hide the condition inside? Or something else? Or is it just fine for now? > I would like to see > a different receive path for coord_rx() and node_rx(), but yea > currently I guess they are mostly the same... in future I think they > are required as PACKTE_HOST, etc. will be changed regarding pan > coordinator or just coordinator (so far I understood). I agree it is tempting, but yeah, there is really very little changes between the two, for me splitting the rx path would just darken the code without bringing much... About the way we handle the PAN coordinator role I have a couple of questions: - shall we consider only the PAN coordinator to be able to accept associations or is any coordinator in the PAN able to do it? (this is not clear to me) - If a coordinator receives a packet with no destination it should expect it to be targeted at the PAN controller. Should we consider relaying the packet? - Is relaying a hardware feature or should we do it in software? > > if (!ieee802154_sdata_running(sdata)) > > @@ -230,9 +231,11 @@ __ieee802154_rx_handle_packet(struct > > ieee802154_local *local, sdata->required_filtering == > > IEEE802154_FILTERING_4_FRAME_FIELDS) continue; > > > > - ieee802154_subif_frame(sdata, skb, &hdr); > > - skb = NULL; > > - break; > > + skb2 = skb_clone(skb, GFP_ATOMIC); > > + if (skb2) { > > + skb2->dev = sdata->dev; > > + ieee802154_subif_frame(sdata, skb2, &hdr); > > + } > > } > > > > kfree_skb(skb); > > If we do the clone above this kfree_skb() should be move to > ieee802154_rx() right after __ieee802154_rx_handle_packet(). Ok! > This patch also changes that we deliver one skb to multiple interfaces if > there are more than one and I was not aware that we currently do that. > :/ Just as a side note: we do that already if we have several MONITOR interfaces opened on the same PHY, it is possible to have them all open. Regarding the situation where we would have NODE + MONITOR or COORD + MONITOR, while the interface creation would work, both could not be open at the same time because the following happens: mac802154_wpan_open() { ieee802154_check_concurrent_iface() { ieee802154_check_mac_settings() { /* prevent the two interface types from being * open at the same time because the filtering * needs are not compatible. */ } } } Then, because you asked me to anticipate if we ever want to support more than one NODE or COORD interface at the same time, or at least not to do anything that would lead to a step back on this regard, I decided I would provide all the infrastructure to gracefully handle this situation in the Rx path, even though right now it still cannot happen because when opening an interface, ieee802154_check_concurrent_iface() will also prevent two NODE / COORD interfaces to be opened at the same time. TL;DR * MONITOR + MONITOR = already supported and working * (NODE + MONITOR) || (COORD + MONITOR) = iface creation allowed, but cannot be opened at the same time because of the filtering level incompatibility on a single PHY * (NODE + NODE) || (COORD + COORD) || (NODE + COORD) = iface creation allowed, but cannot be opened at the same time because only one PHY available on the device So for me we are safe and future proof. Thanks, Miquèl ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH wpan-next] mac802154: Allow the creation of coordinator interfaces 2022-10-19 9:52 ` Miquel Raynal @ 2022-10-23 23:13 ` Alexander Aring 2022-10-23 23:26 ` Alexander Aring 2022-10-24 12:16 ` Miquel Raynal 0 siblings, 2 replies; 8+ messages in thread From: Alexander Aring @ 2022-10-23 23:13 UTC (permalink / raw) To: Miquel Raynal Cc: Alexander Aring, Stefan Schmidt, linux-wpan, David S. Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet, netdev, David Girault, Romuald Despres, Frederic Blain, Nicolas Schodet, Guilhem Imberton, Thomas Petazzoni Hi, On Wed, Oct 19, 2022 at 5:52 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > aahringo@redhat.com wrote on Tue, 18 Oct 2022 19:57:19 -0400: > > > Hi, > > > > On Tue, Oct 18, 2022 at 2:36 PM 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 support will be improved later, in particular regarding > > > the filtering. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > net/mac802154/iface.c | 14 ++++++++------ > > > net/mac802154/main.c | 2 ++ > > > net/mac802154/rx.c | 11 +++++++---- > > > 3 files changed, 17 insertions(+), 10 deletions(-) > > > > > > diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c > > > index d9b50884d34e..682249f3369b 100644 > > > --- a/net/mac802154/iface.c > > > +++ b/net/mac802154/iface.c > > > @@ -262,13 +262,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. > > > @@ -565,6 +565,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); > > > @@ -624,6 +625,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/main.c b/net/mac802154/main.c > > > index 40fab08df24b..d03ecb747afc 100644 > > > --- a/net/mac802154/main.c > > > +++ b/net/mac802154/main.c > > > @@ -219,6 +219,8 @@ int ieee802154_register_hw(struct ieee802154_hw *hw) > > > > > > if (hw->flags & IEEE802154_HW_PROMISCUOUS) > > > local->phy->supported.iftypes |= BIT(NL802154_IFTYPE_MONITOR); > > > + else > > > + local->phy->supported.iftypes &= ~BIT(NL802154_IFTYPE_COORD); > > > > > > > So this means if somebody in the driver sets iftype COORD is supported > > but then IEEE802154_HW_PROMISCUOUS is not supported it will not > > support COORD? > > > > Why is IEEE802154_HW_PROMISCUOUS required for COORD iftype? I thought > > IEEE802154_HW_PROMISCUOUS is required to do a scan? > > You are totally right this is inconsistent, I'll drop the else block > entirely. The fact that HW_PROMISCUOUS is supported when starting a > scan is handled by the -EOPNOTSUPP return code from > drv_set_promiscuous_mode() called by drv_start() in > mac802154_trigger_scan(). > > However I need your input on the following topic: in my branch I > have somewhere a patch adding IFTYPE_COORD to the list of > phy->supported.iftypes in each individual driver. But right now, if we > drop the promiscuous constraint as you suggest, I don't see anymore the > need for setting this as a per-driver value. > > Should we make the possibility to create IFTYPE_COORD interfaces the > default instead, something like this? > > --- a/net/mac802154/main.c > +++ b/net/mac802154/main.c > @@ -118,7 +118,7 @@ ieee802154_alloc_hw(size_t priv_data_len, const struct ieee802154_ops *ops) > phy->supported.lbt = NL802154_SUPPORTED_BOOL_FALSE; > > /* always supported */ > - phy->supported.iftypes = BIT(NL802154_IFTYPE_NODE); > + phy->supported.iftypes = BIT(NL802154_IFTYPE_NODE) | BIT(NL802154_IFTYPE_COORD); > okay. > > > rc = wpan_phy_register(local->phy); > > > if (rc < 0) > > > diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c > > > index 2ae23a2f4a09..aca348d7834b 100644 > > > --- a/net/mac802154/rx.c > > > +++ b/net/mac802154/rx.c > > > @@ -208,6 +208,7 @@ __ieee802154_rx_handle_packet(struct > > > ieee802154_local *local, int ret; > > > struct ieee802154_sub_if_data *sdata; > > > struct ieee802154_hdr hdr; > > > + struct sk_buff *skb2; > > > > > > ret = ieee802154_parse_frame_start(skb, &hdr); > > > if (ret) { > > > @@ -217,7 +218,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 guess this will work but I would like to see no logic about if it's > > not MONITOR it's NODE or COORD, because introducing others requires > > updating those again... however I think it's fine. > > Actually I don't get why we would not want this logic, it seem very > relevant to me. Do you want a helper instead and hide the condition > inside? Or something else? Or is it just fine for now? > > > I would like to see > > a different receive path for coord_rx() and node_rx(), but yea > > currently I guess they are mostly the same... in future I think they > > are required as PACKTE_HOST, etc. will be changed regarding pan > > coordinator or just coordinator (so far I understood). > yes, I think so too. > I agree it is tempting, but yeah, there is really very little changes > between the two, for me splitting the rx path would just darken the > code without bringing much... > ok. > About the way we handle the PAN coordinator role I have a couple of > questions: > - shall we consider only the PAN coordinator to be able to accept > associations or is any coordinator in the PAN able to do it? (this is > not clear to me) Me either, it sounds for me that coordinators are "leaves" and pan coordinators are not. It is like in IPv6 level it is a host or router. > - If a coordinator receives a packet with no destination it should > expect it to be targeted at the PAN controller. Should we consider > relaying the packet? I guess it depends what the standard says here? > - Is relaying a hardware feature or should we do it in software? > I think for SoftMAC it is only the address filter which needs to be changed. The rest is in software. So far what I can see here. Question is what we are using here in the Linux kernel to provide such functionality... e.g. see: include/net/dst.h > > > if (!ieee802154_sdata_running(sdata)) > > > @@ -230,9 +231,11 @@ __ieee802154_rx_handle_packet(struct > > > ieee802154_local *local, sdata->required_filtering == > > > IEEE802154_FILTERING_4_FRAME_FIELDS) continue; > > > > > > - ieee802154_subif_frame(sdata, skb, &hdr); > > > - skb = NULL; > > > - break; > > > + skb2 = skb_clone(skb, GFP_ATOMIC); > > > + if (skb2) { > > > + skb2->dev = sdata->dev; > > > + ieee802154_subif_frame(sdata, skb2, &hdr); > > > + } > > > } > > > > > > kfree_skb(skb); > > > > If we do the clone above this kfree_skb() should be move to > > ieee802154_rx() right after __ieee802154_rx_handle_packet(). > > Ok! > > > This patch also changes that we deliver one skb to multiple interfaces if > > there are more than one and I was not aware that we currently do that. > > :/ > > Just as a side note: we do that already if we have several MONITOR > interfaces opened on the same PHY, it is possible to have them all open. > yes, I used that feature with hwsim once. > Regarding the situation where we would have NODE + MONITOR or COORD + > MONITOR, while the interface creation would work, both could not be > open at the same time because the following happens: > mac802154_wpan_open() { > ieee802154_check_concurrent_iface() { > ieee802154_check_mac_settings() { > /* prevent the two interface types from being > * open at the same time because the filtering > * needs are not compatible. */ > } > } > } > > Then, because you asked me to anticipate if we ever want to support more > than one NODE or COORD interface at the same time, or at least not to > do anything that would lead to a step back on this regard, I decided I > would provide all the infrastructure to gracefully handle this > situation in the Rx path, even though right now it still cannot happen > because when opening an interface, ieee802154_check_concurrent_iface() > will also prevent two NODE / COORD interfaces to be opened at the same > time. yes, but you are assuming the actual hardware here. A hardware with multiple address filters can indeed support other interfaces at the same time. I can also name one, hwsim and a real one - atusb. > > TL;DR > * MONITOR + MONITOR > = already supported and working > * (NODE + MONITOR) || (COORD + MONITOR) > = iface creation allowed, but cannot be opened at the same time > because of the filtering level incompatibility on a single PHY > * (NODE + NODE) || (COORD + COORD) || (NODE + COORD) > = iface creation allowed, but cannot be opened at the same time > because only one PHY available on the device > > So for me we are safe and future proof. > Yes, this is currently kind of difficult to handle, but I see that we check such default filtering type thing per phy which depends on what kind of interface is currently running there? Something like that... - Alex ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH wpan-next] mac802154: Allow the creation of coordinator interfaces 2022-10-23 23:13 ` Alexander Aring @ 2022-10-23 23:26 ` Alexander Aring 2022-10-23 23:27 ` Alexander Aring 2022-10-24 12:16 ` Miquel Raynal 1 sibling, 1 reply; 8+ messages in thread From: Alexander Aring @ 2022-10-23 23:26 UTC (permalink / raw) To: Miquel Raynal Cc: Alexander Aring, Stefan Schmidt, linux-wpan, David S. Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet, netdev, David Girault, Romuald Despres, Frederic Blain, Nicolas Schodet, Guilhem Imberton, Thomas Petazzoni Hi, On Sun, Oct 23, 2022 at 7:13 PM Alexander Aring <aahringo@redhat.com> wrote: > > Hi, > > On Wed, Oct 19, 2022 at 5:52 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Alexander, > > > > aahringo@redhat.com wrote on Tue, 18 Oct 2022 19:57:19 -0400: > > > > > Hi, > > > > > > On Tue, Oct 18, 2022 at 2:36 PM 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 support will be improved later, in particular regarding > > > > the filtering. > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > --- > > > > net/mac802154/iface.c | 14 ++++++++------ > > > > net/mac802154/main.c | 2 ++ > > > > net/mac802154/rx.c | 11 +++++++---- > > > > 3 files changed, 17 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c > > > > index d9b50884d34e..682249f3369b 100644 > > > > --- a/net/mac802154/iface.c > > > > +++ b/net/mac802154/iface.c > > > > @@ -262,13 +262,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. > > > > @@ -565,6 +565,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); > > > > @@ -624,6 +625,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/main.c b/net/mac802154/main.c > > > > index 40fab08df24b..d03ecb747afc 100644 > > > > --- a/net/mac802154/main.c > > > > +++ b/net/mac802154/main.c > > > > @@ -219,6 +219,8 @@ int ieee802154_register_hw(struct ieee802154_hw *hw) > > > > > > > > if (hw->flags & IEEE802154_HW_PROMISCUOUS) > > > > local->phy->supported.iftypes |= BIT(NL802154_IFTYPE_MONITOR); > > > > + else > > > > + local->phy->supported.iftypes &= ~BIT(NL802154_IFTYPE_COORD); > > > > > > > > > > So this means if somebody in the driver sets iftype COORD is supported > > > but then IEEE802154_HW_PROMISCUOUS is not supported it will not > > > support COORD? > > > > > > Why is IEEE802154_HW_PROMISCUOUS required for COORD iftype? I thought > > > IEEE802154_HW_PROMISCUOUS is required to do a scan? > > > > You are totally right this is inconsistent, I'll drop the else block > > entirely. The fact that HW_PROMISCUOUS is supported when starting a > > scan is handled by the -EOPNOTSUPP return code from > > drv_set_promiscuous_mode() called by drv_start() in > > mac802154_trigger_scan(). > > > > However I need your input on the following topic: in my branch I > > have somewhere a patch adding IFTYPE_COORD to the list of > > phy->supported.iftypes in each individual driver. But right now, if we > > drop the promiscuous constraint as you suggest, I don't see anymore the > > need for setting this as a per-driver value. > > > > Should we make the possibility to create IFTYPE_COORD interfaces the > > default instead, something like this? > > > > --- a/net/mac802154/main.c > > +++ b/net/mac802154/main.c > > @@ -118,7 +118,7 @@ ieee802154_alloc_hw(size_t priv_data_len, const struct ieee802154_ops *ops) > > phy->supported.lbt = NL802154_SUPPORTED_BOOL_FALSE; > > > > /* always supported */ > > - phy->supported.iftypes = BIT(NL802154_IFTYPE_NODE); > > + phy->supported.iftypes = BIT(NL802154_IFTYPE_NODE) | BIT(NL802154_IFTYPE_COORD); > > > > okay. > > > > > rc = wpan_phy_register(local->phy); > > > > if (rc < 0) > > > > diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c > > > > index 2ae23a2f4a09..aca348d7834b 100644 > > > > --- a/net/mac802154/rx.c > > > > +++ b/net/mac802154/rx.c > > > > @@ -208,6 +208,7 @@ __ieee802154_rx_handle_packet(struct > > > > ieee802154_local *local, int ret; > > > > struct ieee802154_sub_if_data *sdata; > > > > struct ieee802154_hdr hdr; > > > > + struct sk_buff *skb2; > > > > > > > > ret = ieee802154_parse_frame_start(skb, &hdr); > > > > if (ret) { > > > > @@ -217,7 +218,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 guess this will work but I would like to see no logic about if it's > > > not MONITOR it's NODE or COORD, because introducing others requires > > > updating those again... however I think it's fine. > > > > Actually I don't get why we would not want this logic, it seem very > > relevant to me. Do you want a helper instead and hide the condition > > inside? Or something else? Or is it just fine for now? > > > > > I would like to see > > > a different receive path for coord_rx() and node_rx(), but yea > > > currently I guess they are mostly the same... in future I think they > > > are required as PACKTE_HOST, etc. will be changed regarding pan > > > coordinator or just coordinator (so far I understood). > > > > yes, I think so too. > > > I agree it is tempting, but yeah, there is really very little changes > > between the two, for me splitting the rx path would just darken the > > code without bringing much... > > > > ok. > > > About the way we handle the PAN coordinator role I have a couple of > > questions: > > - shall we consider only the PAN coordinator to be able to accept > > associations or is any coordinator in the PAN able to do it? (this is > > not clear to me) > > Me either, it sounds for me that coordinators are "leaves" and pan > coordinators are not. It is like in IPv6 level it is a host or router. > In this case pan coordinators are able to accept associations only but others can send associations. We should talk about how the difference is here between a node interface and a coordinator interface. For me a node interface is a "mesh" 802.15.4 interface. Coordinators/Pan Coordinators build up star topologies, or not? What I think about is maybe coord interfaces are just pan coordinators. Node interfaces act at the beginning as a fully mesh interface, but soon as you associate it with a pan coordinator it will become what 802.15.4 says a coordinator. I think we don't need any kind of iftype indicator for that. I am not sure if there is really a difference between node and coordinator, but pan coordinator should be a different interface. And a coordinator is a node type but only when it's associated with a pan coordinator. Somehow it needs to fit into the current infrastructure. - Alex ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH wpan-next] mac802154: Allow the creation of coordinator interfaces 2022-10-23 23:26 ` Alexander Aring @ 2022-10-23 23:27 ` Alexander Aring 0 siblings, 0 replies; 8+ messages in thread From: Alexander Aring @ 2022-10-23 23:27 UTC (permalink / raw) To: Miquel Raynal Cc: Alexander Aring, Stefan Schmidt, linux-wpan, David S. Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet, netdev, David Girault, Romuald Despres, Frederic Blain, Nicolas Schodet, Guilhem Imberton, Thomas Petazzoni Hi, On Sun, Oct 23, 2022 at 7:26 PM Alexander Aring <aahringo@redhat.com> wrote: > > Hi, > > On Sun, Oct 23, 2022 at 7:13 PM Alexander Aring <aahringo@redhat.com> wrote: > > > > Hi, > > > > On Wed, Oct 19, 2022 at 5:52 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > Hi Alexander, > > > > > > aahringo@redhat.com wrote on Tue, 18 Oct 2022 19:57:19 -0400: > > > > > > > Hi, > > > > > > > > On Tue, Oct 18, 2022 at 2:36 PM 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 support will be improved later, in particular regarding > > > > > the filtering. > > > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > > --- > > > > > net/mac802154/iface.c | 14 ++++++++------ > > > > > net/mac802154/main.c | 2 ++ > > > > > net/mac802154/rx.c | 11 +++++++---- > > > > > 3 files changed, 17 insertions(+), 10 deletions(-) > > > > > > > > > > diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c > > > > > index d9b50884d34e..682249f3369b 100644 > > > > > --- a/net/mac802154/iface.c > > > > > +++ b/net/mac802154/iface.c > > > > > @@ -262,13 +262,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. > > > > > @@ -565,6 +565,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); > > > > > @@ -624,6 +625,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/main.c b/net/mac802154/main.c > > > > > index 40fab08df24b..d03ecb747afc 100644 > > > > > --- a/net/mac802154/main.c > > > > > +++ b/net/mac802154/main.c > > > > > @@ -219,6 +219,8 @@ int ieee802154_register_hw(struct ieee802154_hw *hw) > > > > > > > > > > if (hw->flags & IEEE802154_HW_PROMISCUOUS) > > > > > local->phy->supported.iftypes |= BIT(NL802154_IFTYPE_MONITOR); > > > > > + else > > > > > + local->phy->supported.iftypes &= ~BIT(NL802154_IFTYPE_COORD); > > > > > > > > > > > > > So this means if somebody in the driver sets iftype COORD is supported > > > > but then IEEE802154_HW_PROMISCUOUS is not supported it will not > > > > support COORD? > > > > > > > > Why is IEEE802154_HW_PROMISCUOUS required for COORD iftype? I thought > > > > IEEE802154_HW_PROMISCUOUS is required to do a scan? > > > > > > You are totally right this is inconsistent, I'll drop the else block > > > entirely. The fact that HW_PROMISCUOUS is supported when starting a > > > scan is handled by the -EOPNOTSUPP return code from > > > drv_set_promiscuous_mode() called by drv_start() in > > > mac802154_trigger_scan(). > > > > > > However I need your input on the following topic: in my branch I > > > have somewhere a patch adding IFTYPE_COORD to the list of > > > phy->supported.iftypes in each individual driver. But right now, if we > > > drop the promiscuous constraint as you suggest, I don't see anymore the > > > need for setting this as a per-driver value. > > > > > > Should we make the possibility to create IFTYPE_COORD interfaces the > > > default instead, something like this? > > > > > > --- a/net/mac802154/main.c > > > +++ b/net/mac802154/main.c > > > @@ -118,7 +118,7 @@ ieee802154_alloc_hw(size_t priv_data_len, const struct ieee802154_ops *ops) > > > phy->supported.lbt = NL802154_SUPPORTED_BOOL_FALSE; > > > > > > /* always supported */ > > > - phy->supported.iftypes = BIT(NL802154_IFTYPE_NODE); > > > + phy->supported.iftypes = BIT(NL802154_IFTYPE_NODE) | BIT(NL802154_IFTYPE_COORD); > > > > > > > okay. > > > > > > > rc = wpan_phy_register(local->phy); > > > > > if (rc < 0) > > > > > diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c > > > > > index 2ae23a2f4a09..aca348d7834b 100644 > > > > > --- a/net/mac802154/rx.c > > > > > +++ b/net/mac802154/rx.c > > > > > @@ -208,6 +208,7 @@ __ieee802154_rx_handle_packet(struct > > > > > ieee802154_local *local, int ret; > > > > > struct ieee802154_sub_if_data *sdata; > > > > > struct ieee802154_hdr hdr; > > > > > + struct sk_buff *skb2; > > > > > > > > > > ret = ieee802154_parse_frame_start(skb, &hdr); > > > > > if (ret) { > > > > > @@ -217,7 +218,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 guess this will work but I would like to see no logic about if it's > > > > not MONITOR it's NODE or COORD, because introducing others requires > > > > updating those again... however I think it's fine. > > > > > > Actually I don't get why we would not want this logic, it seem very > > > relevant to me. Do you want a helper instead and hide the condition > > > inside? Or something else? Or is it just fine for now? > > > > > > > I would like to see > > > > a different receive path for coord_rx() and node_rx(), but yea > > > > currently I guess they are mostly the same... in future I think they > > > > are required as PACKTE_HOST, etc. will be changed regarding pan > > > > coordinator or just coordinator (so far I understood). > > > > > > > yes, I think so too. > > > > > I agree it is tempting, but yeah, there is really very little changes > > > between the two, for me splitting the rx path would just darken the > > > code without bringing much... > > > > > > > ok. > > > > > About the way we handle the PAN coordinator role I have a couple of > > > questions: > > > - shall we consider only the PAN coordinator to be able to accept > > > associations or is any coordinator in the PAN able to do it? (this is > > > not clear to me) > > > > Me either, it sounds for me that coordinators are "leaves" and pan > > coordinators are not. It is like in IPv6 level it is a host or router. > > > > In this case pan coordinators are able to accept associations only but > others can send associations. > > We should talk about how the difference is here between a node > interface and a coordinator interface. For me a node interface is a > "mesh" 802.15.4 interface. Coordinators/Pan Coordinators build up star > topologies, or not? What I think about is maybe coord interfaces are > just pan coordinators. Node interfaces act at the beginning as a fully *are just node interfaces. Sorry. - Alex ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH wpan-next] mac802154: Allow the creation of coordinator interfaces 2022-10-23 23:13 ` Alexander Aring 2022-10-23 23:26 ` Alexander Aring @ 2022-10-24 12:16 ` Miquel Raynal 2022-10-31 1:23 ` Alexander Aring 1 sibling, 1 reply; 8+ messages in thread From: Miquel Raynal @ 2022-10-24 12:16 UTC (permalink / raw) To: Alexander Aring Cc: Alexander Aring, Stefan Schmidt, linux-wpan, David S. Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet, netdev, David Girault, Romuald Despres, Frederic Blain, Nicolas Schodet, Guilhem Imberton, Thomas Petazzoni Hi Alexander, > > About the way we handle the PAN coordinator role I have a couple of > > questions: > > - shall we consider only the PAN coordinator to be able to accept > > associations or is any coordinator in the PAN able to do it? (this is > > not clear to me) > > Me either, it sounds for me that coordinators are "leaves" and pan > coordinators are not. It is like in IPv6 level it is a host or router. I went through the spec once again and I actually (re)discovered Annexe E "Time-slot relaying based link extension (TRLE)" which indeed seems to tell us that relaying is an extension, so otherwise coordinators are "leaves" as you say. > > - If a coordinator receives a packet with no destination it should > > expect it to be targeted at the PAN controller. Should we consider > > relaying the packet? > > I guess it depends what the standard says here? While we don't implement TRLE (and this is a project on its own) I guess we should not perform any relaying. > > - Is relaying a hardware feature or should we do it in software? > > > > I think for SoftMAC it is only the address filter which needs to be > changed. The rest is in software. So far what I can see here. If we need to change the address filters then I guess the hardware is broken, it would not be usable. The hardware must have a "PAN controller" bit to know whether or not the packet must be dropped or not when there is no destination field. > Question is what we are using here in the Linux kernel to provide such > functionality... > > e.g. see: > > include/net/dst.h > > > Regarding the situation where we would have NODE + MONITOR or COORD + > > MONITOR, while the interface creation would work, both could not be > > open at the same time because the following happens: > > mac802154_wpan_open() { > > ieee802154_check_concurrent_iface() { > > ieee802154_check_mac_settings() { > > /* prevent the two interface types from being > > * open at the same time because the filtering > > * needs are not compatible. */ > > } > > } > > } > > > > Then, because you asked me to anticipate if we ever want to support more > > than one NODE or COORD interface at the same time, or at least not to > > do anything that would lead to a step back on this regard, I decided I > > would provide all the infrastructure to gracefully handle this > > situation in the Rx path, even though right now it still cannot happen > > because when opening an interface, ieee802154_check_concurrent_iface() > > will also prevent two NODE / COORD interfaces to be opened at the same > > time. > > yes, but you are assuming the actual hardware here. A hardware with > multiple address filters can indeed support other interfaces at the > same time. I can also name one, hwsim and a real one - atusb. I have this use case in mind, I know the support for it may be brought at some point, and I think my proposal is future proof on this aspect. Isn't it? Thanks, Miquèl ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH wpan-next] mac802154: Allow the creation of coordinator interfaces 2022-10-24 12:16 ` Miquel Raynal @ 2022-10-31 1:23 ` Alexander Aring 0 siblings, 0 replies; 8+ messages in thread From: Alexander Aring @ 2022-10-31 1:23 UTC (permalink / raw) To: Miquel Raynal Cc: Alexander Aring, Stefan Schmidt, linux-wpan, David S. Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet, netdev, David Girault, Romuald Despres, Frederic Blain, Nicolas Schodet, Guilhem Imberton, Thomas Petazzoni Hi, On Mon, Oct 24, 2022 at 8:16 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > > > About the way we handle the PAN coordinator role I have a couple of > > > questions: > > > - shall we consider only the PAN coordinator to be able to accept > > > associations or is any coordinator in the PAN able to do it? (this is > > > not clear to me) > > > > Me either, it sounds for me that coordinators are "leaves" and pan > > coordinators are not. It is like in IPv6 level it is a host or router. > > I went through the spec once again and I actually (re)discovered > Annexe E "Time-slot relaying based link extension (TRLE)" which indeed > seems to tell us that relaying is an extension, so otherwise > coordinators are "leaves" as you say. > okay. > > > - If a coordinator receives a packet with no destination it should > > > expect it to be targeted at the PAN controller. Should we consider > > > relaying the packet? > > > > I guess it depends what the standard says here? > > While we don't implement TRLE (and this is a project on its own) I > guess we should not perform any relaying. > Yes, it's a project on its own. > > > - Is relaying a hardware feature or should we do it in software? > > > > > > > I think for SoftMAC it is only the address filter which needs to be > > changed. The rest is in software. So far what I can see here. > > If we need to change the address filters then I guess the hardware is > broken, it would not be usable. The hardware must have a "PAN > controller" bit to know whether or not the packet must be dropped or > not when there is no destination field. > yes, I was more concerned here that in those "non leaves" the address filter is set so as it's required to do such handling. Sure hardware which needs to support it to change it as e.g. ack vs no ack needs to be reported back - same thing. > > Question is what we are using here in the Linux kernel to provide such > > functionality... > > > > e.g. see: > > > > include/net/dst.h > > > > > Regarding the situation where we would have NODE + MONITOR or COORD + > > > MONITOR, while the interface creation would work, both could not be > > > open at the same time because the following happens: > > > mac802154_wpan_open() { > > > ieee802154_check_concurrent_iface() { > > > ieee802154_check_mac_settings() { > > > /* prevent the two interface types from being > > > * open at the same time because the filtering > > > * needs are not compatible. */ > > > } > > > } > > > } > > > > > > Then, because you asked me to anticipate if we ever want to support more > > > than one NODE or COORD interface at the same time, or at least not to > > > do anything that would lead to a step back on this regard, I decided I > > > would provide all the infrastructure to gracefully handle this > > > situation in the Rx path, even though right now it still cannot happen > > > because when opening an interface, ieee802154_check_concurrent_iface() > > > will also prevent two NODE / COORD interfaces to be opened at the same > > > time. > > > > yes, but you are assuming the actual hardware here. A hardware with > > multiple address filters can indeed support other interfaces at the > > same time. I can also name one, hwsim and a real one - atusb. > > I have this use case in mind, I know the support for it may be > brought at some point, and I think my proposal is future proof on this > aspect. Isn't it? It is. - Alex ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-10-31 1:24 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-10-18 18:36 [PATCH wpan-next] mac802154: Allow the creation of coordinator interfaces Miquel Raynal 2022-10-18 23:57 ` Alexander Aring 2022-10-19 9:52 ` Miquel Raynal 2022-10-23 23:13 ` Alexander Aring 2022-10-23 23:26 ` Alexander Aring 2022-10-23 23:27 ` Alexander Aring 2022-10-24 12:16 ` Miquel Raynal 2022-10-31 1:23 ` Alexander Aring
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).