| Message ID | 20220701143052.1267509-2-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ieee802154: Support scanning/beaconing | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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. Thanks. - Alex
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? Thanks, Miquèl
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. - Alex
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? 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; } - - kfree_skb(skb); } static void
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. 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. > 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? ? That code changes this behaviour and I do not know why. Move the switch case into the loop and do a different receive path if coord and node and do whatever differs from filtering and then call ieee802154_subif_frame(). - Alex
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? > Move the switch case into the loop and do a different > receive path if coord and node and do whatever differs from filtering > and then call ieee802154_subif_frame(). > > - Alex > Thanks, Miquèl
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 Actually right now the second level is not enforced, and all the filtering levels are a bit fuzzy and spread everywhere in rx.c. I'm gonna see if I can at least clarify all of that and only make coord-dependent the right section because right now a ieee802154_coord_rx() path in ieee802154_rx_handle_packet() does not really make sense given that the level 3 filtering rules are mostly enforced in ieee802154_subif_frame(). Thanks, Miquèl
Hi, On Wed, Aug 24, 2022 at 6:21 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: ... > > Actually right now the second level is not enforced, and all the > filtering levels are a bit fuzzy and spread everywhere in rx.c. > > I'm gonna see if I can at least clarify all of that and only make > coord-dependent the right section because right now a > ieee802154_coord_rx() path in ieee802154_rx_handle_packet() does not > really make sense given that the level 3 filtering rules are mostly > enforced in ieee802154_subif_frame(). One thing I mentioned before is that we probably like to have a parameter for rx path to give mac802154 a hint on which filtering level it was received. We don't have that, I currently see that this is a parameter for hwsim receiving it on promiscuous level only and all others do third level filtering. We need that now, because the promiscuous mode was only used for sniffing which goes directly into the rx path for monitors. With scan we mix things up here and in my opinion require such a parameter and do filtering if necessary. - Alex
Hi Alexander, aahringo@redhat.com wrote on Wed, 24 Aug 2022 08:43:20 -0400: > Hi, > > On Wed, Aug 24, 2022 at 6:21 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > ... > > > > Actually right now the second level is not enforced, and all the > > filtering levels are a bit fuzzy and spread everywhere in rx.c. > > > > I'm gonna see if I can at least clarify all of that and only make > > coord-dependent the right section because right now a > > ieee802154_coord_rx() path in ieee802154_rx_handle_packet() does not > > really make sense given that the level 3 filtering rules are mostly > > enforced in ieee802154_subif_frame(). > > One thing I mentioned before is that we probably like to have a > parameter for rx path to give mac802154 a hint on which filtering > level it was received. We don't have that, I currently see that this > is a parameter for hwsim receiving it on promiscuous level only and > all others do third level filtering. > We need that now, because the promiscuous mode was only used for > sniffing which goes directly into the rx path for monitors. With scan > we mix things up here and in my opinion require such a parameter and > do filtering if necessary. I am currently trying to implement a slightly different approach. The core does not know hwsim is always in promiscuous mode, but it does know that it does not check FCS. So the core checks it. This is level 1 achieved. Then in level 2 we want to know if the core asked the transceiver to enter promiscuous mode, which, if it did, should not imply more filtering. If the device is working in promiscuous mode but this was not asked explicitly by the core, we don't really care, software filtering will apply anyway. I am reworking the rx path to clarify what is being done and when, because I found this part very obscure right now. In the end I don't think we need additional rx info from the drivers. Hopefully my proposal will clarify why this is (IMHO) not needed. Thanks, Miquèl
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
Hi, On Wed, Aug 24, 2022 at 9:27 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > aahringo@redhat.com wrote on Wed, 24 Aug 2022 08:43:20 -0400: > > > Hi, > > > > On Wed, Aug 24, 2022 at 6:21 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > ... > > > > > > Actually right now the second level is not enforced, and all the > > > filtering levels are a bit fuzzy and spread everywhere in rx.c. > > > > > > I'm gonna see if I can at least clarify all of that and only make > > > coord-dependent the right section because right now a > > > ieee802154_coord_rx() path in ieee802154_rx_handle_packet() does not > > > really make sense given that the level 3 filtering rules are mostly > > > enforced in ieee802154_subif_frame(). > > > > One thing I mentioned before is that we probably like to have a > > parameter for rx path to give mac802154 a hint on which filtering > > level it was received. We don't have that, I currently see that this > > is a parameter for hwsim receiving it on promiscuous level only and > > all others do third level filtering. > > We need that now, because the promiscuous mode was only used for > > sniffing which goes directly into the rx path for monitors. With scan > > we mix things up here and in my opinion require such a parameter and > > do filtering if necessary. > > I am currently trying to implement a slightly different approach. The > core does not know hwsim is always in promiscuous mode, but it does > know that it does not check FCS. So the core checks it. This is > level 1 achieved. Then in level 2 we want to know if the core asked > the transceiver to enter promiscuous mode, which, if it did, should > not imply more filtering. If the device is working in promiscuous > mode but this was not asked explicitly by the core, we don't really > care, software filtering will apply anyway. > I doubt that I will be happy with this solution, this all sounds like "for the specific current behaviour that we support 2 filtering levels it will work", just do a parameter on which 802.15.4 filtering level it was received and the rx path will check what kind of filter is required and which not. As driver ops start() callback you should say which filtering level the receive mode should start with. > I am reworking the rx path to clarify what is being done and when, > because I found this part very obscure right now. In the end I don't > think we need additional rx info from the drivers. Hopefully my > proposal will clarify why this is (IMHO) not needed. > Never looked much in 802.15.4 receive path as it just worked but I said that there might be things to clean up when filtering things on hardware and when on software and I have the feeling we are doing things twice. Sometimes it is also necessary to set some skb fields e.g. PACKET_HOST, etc. and I think this is what the most important part of it is there. However, there are probably some tune ups if we know we are in third leveling filtering... - Alex
Hi, On Wed, Aug 24, 2022 at 5:53 PM Alexander Aring <aahringo@redhat.com> wrote: > > Hi, > > On Wed, Aug 24, 2022 at 9:27 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Alexander, > > > > aahringo@redhat.com wrote on Wed, 24 Aug 2022 08:43:20 -0400: > > > > > Hi, > > > > > > On Wed, Aug 24, 2022 at 6:21 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > ... > > > > > > > > Actually right now the second level is not enforced, and all the > > > > filtering levels are a bit fuzzy and spread everywhere in rx.c. > > > > > > > > I'm gonna see if I can at least clarify all of that and only make > > > > coord-dependent the right section because right now a > > > > ieee802154_coord_rx() path in ieee802154_rx_handle_packet() does not > > > > really make sense given that the level 3 filtering rules are mostly > > > > enforced in ieee802154_subif_frame(). > > > > > > One thing I mentioned before is that we probably like to have a > > > parameter for rx path to give mac802154 a hint on which filtering > > > level it was received. We don't have that, I currently see that this > > > is a parameter for hwsim receiving it on promiscuous level only and > > > all others do third level filtering. > > > We need that now, because the promiscuous mode was only used for > > > sniffing which goes directly into the rx path for monitors. With scan > > > we mix things up here and in my opinion require such a parameter and > > > do filtering if necessary. > > > > I am currently trying to implement a slightly different approach. The > > core does not know hwsim is always in promiscuous mode, but it does > > know that it does not check FCS. So the core checks it. This is > > level 1 achieved. Then in level 2 we want to know if the core asked > > the transceiver to enter promiscuous mode, which, if it did, should > > not imply more filtering. If the device is working in promiscuous > > mode but this was not asked explicitly by the core, we don't really > > care, software filtering will apply anyway. > > > > I doubt that I will be happy with this solution, this all sounds like > "for the specific current behaviour that we support 2 filtering levels > it will work", just do a parameter on which 802.15.4 filtering level > it was received and the rx path will check what kind of filter is I think a per phy field is enough here because the receive path should be synchronized with changing filtering level on hardware. No need for per receive path parameter. "If the device is working in promiscuous mode but this was not asked explicitly by the core, we don't really care, software filtering will apply anyway." I don't understand this sentence, we should not filter on things which the hardware is doing for us. I mean okay I'm fine to handle it now just to check twice, but in the future there might be more "we don't need to filter this because we know the hardware is doing it" patches. - Alex
Hi Alexander, aahringo@redhat.com wrote on Wed, 24 Aug 2022 17:43:11 -0400: > 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. So we need to duplicate the skb because it automatically gets freed in the "forward to upper layer" path. Am I right? I'm fine doing so if this is the way to go, but I am interested if you can give me a real use case where having NODE+COORDINATOR on the same PHY is useful? Thanks, Miquèl
Hi Alexander, aahringo@redhat.com wrote on Wed, 24 Aug 2022 21:02:29 -0400: > Hi, > > On Wed, Aug 24, 2022 at 5:53 PM Alexander Aring <aahringo@redhat.com> wrote: > > > > Hi, > > > > On Wed, Aug 24, 2022 at 9:27 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > Hi Alexander, > > > > > > aahringo@redhat.com wrote on Wed, 24 Aug 2022 08:43:20 -0400: > > > > > > > Hi, > > > > > > > > On Wed, Aug 24, 2022 at 6:21 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > ... > > > > > > > > > > Actually right now the second level is not enforced, and all the > > > > > filtering levels are a bit fuzzy and spread everywhere in rx.c. > > > > > > > > > > I'm gonna see if I can at least clarify all of that and only make > > > > > coord-dependent the right section because right now a > > > > > ieee802154_coord_rx() path in ieee802154_rx_handle_packet() does not > > > > > really make sense given that the level 3 filtering rules are mostly > > > > > enforced in ieee802154_subif_frame(). > > > > > > > > One thing I mentioned before is that we probably like to have a > > > > parameter for rx path to give mac802154 a hint on which filtering > > > > level it was received. We don't have that, I currently see that this > > > > is a parameter for hwsim receiving it on promiscuous level only and > > > > all others do third level filtering. > > > > We need that now, because the promiscuous mode was only used for > > > > sniffing which goes directly into the rx path for monitors. With scan > > > > we mix things up here and in my opinion require such a parameter and > > > > do filtering if necessary. > > > > > > I am currently trying to implement a slightly different approach. The > > > core does not know hwsim is always in promiscuous mode, but it does > > > know that it does not check FCS. So the core checks it. This is > > > level 1 achieved. Then in level 2 we want to know if the core asked > > > the transceiver to enter promiscuous mode, which, if it did, should > > > not imply more filtering. If the device is working in promiscuous > > > mode but this was not asked explicitly by the core, we don't really > > > care, software filtering will apply anyway. > > > > > > > I doubt that I will be happy with this solution, this all sounds like > > "for the specific current behaviour that we support 2 filtering levels > > it will work", just do a parameter on which 802.15.4 filtering level > > it was received and the rx path will check what kind of filter is > > I think a per phy field is enough here because the receive path should > be synchronized with changing filtering level on hardware. No need for > per receive path parameter. Ok, I prefer the per-PHY field rather than the per-received-skb info. I will add a parameter in the start field set to LEVEL3, drivers are free to change this (like hwsim) if they can't. I will add also the major filtering rules in the rx path but we will actually use them only if the hw filtering level is lower than what is requested, as you said. > > "If the device is working in promiscuous mode but this was not asked > explicitly by the core, we don't really care, software filtering will > apply anyway." > I don't understand this sentence, we should not filter on things which > the hardware is doing for us. I mean okay I'm fine to handle it now > just to check twice, but in the future there might be more "we don't > need to filter this because we know the hardware is doing it" patches. > > - Alex > Thanks, Miquèl
Hi Alexander, aahringo@redhat.com wrote on Wed, 24 Aug 2022 17:53:45 -0400: > Hi, > > On Wed, Aug 24, 2022 at 9:27 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Alexander, > > > > aahringo@redhat.com wrote on Wed, 24 Aug 2022 08:43:20 -0400: > > > > > Hi, > > > > > > On Wed, Aug 24, 2022 at 6:21 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > ... > > > > > > > > Actually right now the second level is not enforced, and all the > > > > filtering levels are a bit fuzzy and spread everywhere in rx.c. > > > > > > > > I'm gonna see if I can at least clarify all of that and only make > > > > coord-dependent the right section because right now a > > > > ieee802154_coord_rx() path in ieee802154_rx_handle_packet() does not > > > > really make sense given that the level 3 filtering rules are mostly > > > > enforced in ieee802154_subif_frame(). > > > > > > One thing I mentioned before is that we probably like to have a > > > parameter for rx path to give mac802154 a hint on which filtering > > > level it was received. We don't have that, I currently see that this > > > is a parameter for hwsim receiving it on promiscuous level only and > > > all others do third level filtering. > > > We need that now, because the promiscuous mode was only used for > > > sniffing which goes directly into the rx path for monitors. With scan > > > we mix things up here and in my opinion require such a parameter and > > > do filtering if necessary. > > > > I am currently trying to implement a slightly different approach. The > > core does not know hwsim is always in promiscuous mode, but it does > > know that it does not check FCS. So the core checks it. This is > > level 1 achieved. Then in level 2 we want to know if the core asked > > the transceiver to enter promiscuous mode, which, if it did, should > > not imply more filtering. If the device is working in promiscuous > > mode but this was not asked explicitly by the core, we don't really > > care, software filtering will apply anyway. > > > > I doubt that I will be happy with this solution, this all sounds like > "for the specific current behaviour that we support 2 filtering levels > it will work", just do a parameter on which 802.15.4 filtering level > it was received and the rx path will check what kind of filter is > required and which not. > As driver ops start() callback you should say which filtering level > the receive mode should start with. > > > I am reworking the rx path to clarify what is being done and when, > > because I found this part very obscure right now. In the end I don't > > think we need additional rx info from the drivers. Hopefully my > > proposal will clarify why this is (IMHO) not needed. > > > > Never looked much in 802.15.4 receive path as it just worked but I > said that there might be things to clean up when filtering things on > hardware and when on software and I have the feeling we are doing > things twice. Sometimes it is also necessary to set some skb fields > e.g. PACKET_HOST, etc. and I think this is what the most important > part of it is there. However, there are probably some tune ups if we > know we are in third leveling filtering... Ok, I've done the following. - Adding a PHY parameter which reflects the actual filtering level of the transceiver, the default level is 4 (standard situation, you're receiving data) but of course if the PHY does not support this state (like hwsim) it should overwrite this value by setting the actual filtering level (none, in the hwsim case) so that the core knows what it receives. - I've replaced the specific "do not check the FCS" flag only used by hwsim by this filtering level, which gives all the information we need. - I've added a real promiscuous filtering mode which truly does not care about the content of the frame but only checks the FCS if not already done by the xceiver. - I've also implemented in software filtering level 4 for most regular data packets. Without changing the default PHY level mentioned in the first item above, this additional filtering will be skipped which ensures we keep the same behavior of most driver. In the case of hwsim however, these filters will become active if the MAC is not in promiscuous mode or in scan mode, which is actually what people should be expecting. Hopefully all this fits what you had in mind. I have one item left on my current todo list: improving a bit the userspace tool with a "monitor" command. Otherwise the remaining things to do are to discuss the locking design which might need to be changed to avoid lockdep issues and keep the rtnl locked eg. during a channel change. I still don't know how to do that, so it's likely that the right next version will not include any change in this area unless something pops up. Thanks, Miquèl
Hi, On Thu, Aug 25, 2022 at 4:41 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > aahringo@redhat.com wrote on Wed, 24 Aug 2022 17:43:11 -0400: > > > 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. > > So we need to duplicate the skb because it automatically gets freed in > the "forward to upper layer" path. Am I right? I'm fine doing so if What is the definition of "duplicate the skb" here. > this is the way to go, but I am interested if you can give me a real > use case where having NODE+COORDINATOR on the same PHY is useful? > Testing. - Alex
Hi, On Thu, Aug 25, 2022 at 8:58 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > aahringo@redhat.com wrote on Wed, 24 Aug 2022 17:53:45 -0400: > > > Hi, > > > > On Wed, Aug 24, 2022 at 9:27 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > Hi Alexander, > > > > > > aahringo@redhat.com wrote on Wed, 24 Aug 2022 08:43:20 -0400: > > > > > > > Hi, > > > > > > > > On Wed, Aug 24, 2022 at 6:21 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > ... > > > > > > > > > > Actually right now the second level is not enforced, and all the > > > > > filtering levels are a bit fuzzy and spread everywhere in rx.c. > > > > > > > > > > I'm gonna see if I can at least clarify all of that and only make > > > > > coord-dependent the right section because right now a > > > > > ieee802154_coord_rx() path in ieee802154_rx_handle_packet() does not > > > > > really make sense given that the level 3 filtering rules are mostly > > > > > enforced in ieee802154_subif_frame(). > > > > > > > > One thing I mentioned before is that we probably like to have a > > > > parameter for rx path to give mac802154 a hint on which filtering > > > > level it was received. We don't have that, I currently see that this > > > > is a parameter for hwsim receiving it on promiscuous level only and > > > > all others do third level filtering. > > > > We need that now, because the promiscuous mode was only used for > > > > sniffing which goes directly into the rx path for monitors. With scan > > > > we mix things up here and in my opinion require such a parameter and > > > > do filtering if necessary. > > > > > > I am currently trying to implement a slightly different approach. The > > > core does not know hwsim is always in promiscuous mode, but it does > > > know that it does not check FCS. So the core checks it. This is > > > level 1 achieved. Then in level 2 we want to know if the core asked > > > the transceiver to enter promiscuous mode, which, if it did, should > > > not imply more filtering. If the device is working in promiscuous > > > mode but this was not asked explicitly by the core, we don't really > > > care, software filtering will apply anyway. > > > > > > > I doubt that I will be happy with this solution, this all sounds like > > "for the specific current behaviour that we support 2 filtering levels > > it will work", just do a parameter on which 802.15.4 filtering level > > it was received and the rx path will check what kind of filter is > > required and which not. > > As driver ops start() callback you should say which filtering level > > the receive mode should start with. > > > > > I am reworking the rx path to clarify what is being done and when, > > > because I found this part very obscure right now. In the end I don't > > > think we need additional rx info from the drivers. Hopefully my > > > proposal will clarify why this is (IMHO) not needed. > > > > > > > Never looked much in 802.15.4 receive path as it just worked but I > > said that there might be things to clean up when filtering things on > > hardware and when on software and I have the feeling we are doing > > things twice. Sometimes it is also necessary to set some skb fields > > e.g. PACKET_HOST, etc. and I think this is what the most important > > part of it is there. However, there are probably some tune ups if we > > know we are in third leveling filtering... > > Ok, I've done the following. > > - Adding a PHY parameter which reflects the actual filtering level of > the transceiver, the default level is 4 (standard situation, you're 3? > receiving data) but of course if the PHY does not support this state > (like hwsim) it should overwrite this value by setting the actual > filtering level (none, in the hwsim case) so that the core knows what > it receives. > ok. > - I've replaced the specific "do not check the FCS" flag only used by > hwsim by this filtering level, which gives all the information we > need. > ok. > - I've added a real promiscuous filtering mode which truly does not > care about the content of the frame but only checks the FCS if not > already done by the xceiver. > not sure what a "real promiscuous filtering here is" people have different understanding about it, but 802.15.4 has a definition for it. You should consider that having monitors, frames with bad fcs should not be filtered out by hardware. There it comes back what I said before, the filtering level should be a parameter for start() driver ops. > - I've also implemented in software filtering level 4 for most regular 3? > data packets. Without changing the default PHY level mentioned in the > first item above, this additional filtering will be skipped which > ensures we keep the same behavior of most driver. In the case of hwsim > however, these filters will become active if the MAC is not in > promiscuous mode or in scan mode, which is actually what people > should be expecting. > To give feedback to that I need to see code. And please don't send the whole feature stuff again, just this specific part of it. Thanks. > Hopefully all this fits what you had in mind. > > I have one item left on my current todo list: improving a bit the > userspace tool with a "monitor" command. > > Otherwise the remaining things to do are to discuss the locking design > which might need to be changed to avoid lockdep issues and keep the > rtnl locked eg. during a channel change. I still don't know how to do > that, so it's likely that the right next version will not include any > change in this area unless something pops up. I try to look at that on the weekend. - Alex
Hi, On Thu, Aug 25, 2022 at 8:51 PM Alexander Aring <aahringo@redhat.com> wrote: > > Hi, > > On Thu, Aug 25, 2022 at 4:41 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Alexander, > > > > aahringo@redhat.com wrote on Wed, 24 Aug 2022 17:43:11 -0400: > > > > > 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. > > > > So we need to duplicate the skb because it automatically gets freed in > > the "forward to upper layer" path. Am I right? I'm fine doing so if > > What is the definition of "duplicate the skb" here. > > > this is the way to go, but I am interested if you can give me a real > > use case where having NODE+COORDINATOR on the same PHY is useful? > > > > Testing. I need to say that I really used multiple monitors at the same time on one phy only and I did that with hwsim to run multiple user space stacks. It was working and I was happy and didn't need to do a lot of phy creations in hwsim. Most hardware can probably not run multiple nodes and coordinators at the same time ?yet?, _but_ there is a candidate which can do that and this is atusb. On atusb we have a co-processor that can deal with multiple address filters. People already asked to do something like a node which can operate on two pans as I remember, that would be a candidate for such a feature. I really don't want to move step backwards here and delete this thing which probably can be useful later. I don't know how wireless history dealt with it and how complicated it was to bring such a feature in to e.g. run multiple access points on one phy. I also see it in ethernet with macvlan, which is a similar feature. We don't need to support it, make it so that on an ifup it returns -EBUSY if something doesn't fit together as it currently is. We can later add support for it after playing around with hwsim a little bit more. We should at least take care that I can still run my multiple monitors at the same time (which is currently allowed). - Alex
Hi Alexander, aahringo@redhat.com wrote on Thu, 25 Aug 2022 20:51:49 -0400: > Hi, > > On Thu, Aug 25, 2022 at 4:41 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Alexander, > > > > aahringo@redhat.com wrote on Wed, 24 Aug 2022 17:43:11 -0400: > > > > > 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. > > > > So we need to duplicate the skb because it automatically gets freed in > > the "forward to upper layer" path. Am I right? I'm fine doing so if > > What is the definition of "duplicate the skb" here. skb2 = skb_clone(skb, GFP_ATOMIC); if (skb2) { skb2->dev = sdata->dev; ieee802154_deliver_skb(skb2); ... } This is exactly what the ieee80254_monitors_rx() function does, it loops over all the monitor interfaces and each time there is one running, copies the skb and consumes it by forwarding it to the upper layers. I've done the same in the other path to keep the ability to use more than one "visible" interface on the same PHY (by visible I mean: not a monitor). > > this is the way to go, but I am interested if you can give me a real > > use case where having NODE+COORDINATOR on the same PHY is useful? > > > > Testing. Ok :-) Thanks, Miquèl
Hi Alexander, aahringo@redhat.com wrote on Thu, 25 Aug 2022 21:05:09 -0400: > Hi, > > On Thu, Aug 25, 2022 at 8:58 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Alexander, > > > > aahringo@redhat.com wrote on Wed, 24 Aug 2022 17:53:45 -0400: > > > > > Hi, > > > > > > On Wed, Aug 24, 2022 at 9:27 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > Hi Alexander, > > > > > > > > aahringo@redhat.com wrote on Wed, 24 Aug 2022 08:43:20 -0400: > > > > > > > > > Hi, > > > > > > > > > > On Wed, Aug 24, 2022 at 6:21 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > ... > > > > > > > > > > > > Actually right now the second level is not enforced, and all the > > > > > > filtering levels are a bit fuzzy and spread everywhere in rx.c. > > > > > > > > > > > > I'm gonna see if I can at least clarify all of that and only make > > > > > > coord-dependent the right section because right now a > > > > > > ieee802154_coord_rx() path in ieee802154_rx_handle_packet() does not > > > > > > really make sense given that the level 3 filtering rules are mostly > > > > > > enforced in ieee802154_subif_frame(). > > > > > > > > > > One thing I mentioned before is that we probably like to have a > > > > > parameter for rx path to give mac802154 a hint on which filtering > > > > > level it was received. We don't have that, I currently see that this > > > > > is a parameter for hwsim receiving it on promiscuous level only and > > > > > all others do third level filtering. > > > > > We need that now, because the promiscuous mode was only used for > > > > > sniffing which goes directly into the rx path for monitors. With scan > > > > > we mix things up here and in my opinion require such a parameter and > > > > > do filtering if necessary. > > > > > > > > I am currently trying to implement a slightly different approach. The > > > > core does not know hwsim is always in promiscuous mode, but it does > > > > know that it does not check FCS. So the core checks it. This is > > > > level 1 achieved. Then in level 2 we want to know if the core asked > > > > the transceiver to enter promiscuous mode, which, if it did, should > > > > not imply more filtering. If the device is working in promiscuous > > > > mode but this was not asked explicitly by the core, we don't really > > > > care, software filtering will apply anyway. > > > > > > > > > > I doubt that I will be happy with this solution, this all sounds like > > > "for the specific current behaviour that we support 2 filtering levels > > > it will work", just do a parameter on which 802.15.4 filtering level > > > it was received and the rx path will check what kind of filter is > > > required and which not. > > > As driver ops start() callback you should say which filtering level > > > the receive mode should start with. > > > > > > > I am reworking the rx path to clarify what is being done and when, > > > > because I found this part very obscure right now. In the end I don't > > > > think we need additional rx info from the drivers. Hopefully my > > > > proposal will clarify why this is (IMHO) not needed. > > > > > > > > > > Never looked much in 802.15.4 receive path as it just worked but I > > > said that there might be things to clean up when filtering things on > > > hardware and when on software and I have the feeling we are doing > > > things twice. Sometimes it is also necessary to set some skb fields > > > e.g. PACKET_HOST, etc. and I think this is what the most important > > > part of it is there. However, there are probably some tune ups if we > > > know we are in third leveling filtering... > > > > Ok, I've done the following. > > > > - Adding a PHY parameter which reflects the actual filtering level of > > the transceiver, the default level is 4 (standard situation, you're > > 3? Honestly there are only two filtering levels in the normal path and one additional for scanning situations. But the spec mentions 4, so I figured we should use the same naming to avoid confusing people on what "level 3 means, if it's level 3 because level 1 and 2 are identical at PHY level, or level 3 which is the scan filtering as mentioned in the spec?". I used this enum to clarify the amount of filtering that is involved, hopefully it is clear enough. I remember we talked about this already but an unrelated thread, and was not capable of finding it anymore O:-). /** enum ieee802154_filtering_level - Filtering levels applicable to a PHY * @IEEE802154_FILTERING_NONE: No filtering at all, what is received is * forwarded to the softMAC * @IEEE802154_FILTERING_1_FCS: First filtering level, frames with an invalid * FCS should be dropped * @IEEE802154_FILTERING_2_PROMISCUOUS: Second filtering level, promiscuous * mode, identical in terms of filtering to the first level at the PHY * level, but no ACK should be transmitted automatically and at the MAC * level the frame should be forwarded to the upper layer directly * @IEEE802154_FILTERING_3_SCAN: Third filtering level, enforced during scans, * which only forwards beacons * @IEEE802154_FILTERING_4_FRAME_FIELDS: Fourth filtering level actually * enforcing the validity of the content of the frame with various checks */ enum ieee802154_filtering_level { IEEE802154_FILTERING_NONE, IEEE802154_FILTERING_1_FCS, IEEE802154_FILTERING_2_PROMISCUOUS, IEEE802154_FILTERING_3_SCAN, IEEE802154_FILTERING_4_FRAME_FIELDS, }; > > > receiving data) but of course if the PHY does not support this state > > (like hwsim) it should overwrite this value by setting the actual > > filtering level (none, in the hwsim case) so that the core knows what > > it receives. > > > > ok. > > > - I've replaced the specific "do not check the FCS" flag only used by > > hwsim by this filtering level, which gives all the information we > > need. > > > > ok. > > > - I've added a real promiscuous filtering mode which truly does not > > care about the content of the frame but only checks the FCS if not > > already done by the xceiver. > > > > not sure what a "real promiscuous filtering here is" people have > different understanding about it, but 802.15.4 has a definition for > it. Promiscuous, by the 802154 spec means: the FCS is good so the content of the received packet must means something, just forward it and let upper layers handle it. Until now there was no real promiscuous mode in the mac NODE rx path. Only monitors would get all the frames (including the ones with a wrong FCS), which is fine because it's a bit out of the spec, so I'm fine with this idea. But otherwise in the NODE/COORD rx path, the FCS should be checked even in promiscuous mode to correctly match the spec. Until now, ieee802154_parse_frame_start() was always called in these path and this would validate the frame headers. I've added a more precise promiscuous mode in the rx patch which skips any additional checks. What happens however is that, if the transceiver disables FCS checks in promiscuous mode, then FCS is not checked at all and this is invalid. With my current implementation, the devices which do not check the FCS might be easily "fixed" by changing their PHY filtering level to "FILTERING_NONE" in the promiscuous callback. > You should consider that having monitors, frames with bad fcs > should not be filtered out by hardware. There it comes back what I > said before, the filtering level should be a parameter for start() > driver ops. > > > - I've also implemented in software filtering level 4 for most > > regular > > 3? > > > data packets. Without changing the default PHY level mentioned in > > the first item above, this additional filtering will be skipped > > which ensures we keep the same behavior of most driver. In the case > > of hwsim however, these filters will become active if the MAC is > > not in promiscuous mode or in scan mode, which is actually what > > people should be expecting. > > > > To give feedback to that I need to see code. And please don't send the > whole feature stuff again, just this specific part of it. Thanks. The entire filtering feature is split: there are the basis introduced before the scan, and then after the whole scan+association thing I've introduced additional filtering levels. > > Hopefully all this fits what you had in mind. > > > > I have one item left on my current todo list: improving a bit the > > userspace tool with a "monitor" command. > > > > Otherwise the remaining things to do are to discuss the locking > > design which might need to be changed to avoid lockdep issues and > > keep the rtnl locked eg. during a channel change. I still don't > > know how to do that, so it's likely that the right next version > > will not include any change in this area unless something pops up. > > I try to look at that on the weekend. I've had an idea yesterday night which seem to work, I think I can drop the two patches which you disliked regarding discarding the rtnl in the tx path and in hwsim:change_channel(). Thanks, Miquèl
Hi Alexander, aahringo@redhat.com wrote on Thu, 25 Aug 2022 21:35:05 -0400: > Hi, > > On Thu, Aug 25, 2022 at 8:51 PM Alexander Aring <aahringo@redhat.com> wrote: > > > > Hi, > > > > On Thu, Aug 25, 2022 at 4:41 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > Hi Alexander, > > > > > > aahringo@redhat.com wrote on Wed, 24 Aug 2022 17:43:11 -0400: > > > > > > > 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. > > > > > > So we need to duplicate the skb because it automatically gets freed in > > > the "forward to upper layer" path. Am I right? I'm fine doing so if > > > > What is the definition of "duplicate the skb" here. > > > > > this is the way to go, but I am interested if you can give me a real > > > use case where having NODE+COORDINATOR on the same PHY is useful? > > > > > > > Testing. > > I need to say that I really used multiple monitors at the same time on > one phy only and I did that with hwsim to run multiple user space > stacks. It was working and I was happy and didn't need to do a lot of > phy creations in hwsim. Indeed, looking at the code, you could use as many MONITOR interfaces you needed, but only a single NODE. I've changed that to use as many NODE and COORD that we wish. > Most hardware can probably not run multiple > nodes and coordinators at the same time ?yet?, _but_ there is a > candidate which can do that and this is atusb. On atusb we have a > co-processor that can deal with multiple address filters. People > already asked to do something like a node which can operate on two > pans as I remember, that would be a candidate for such a feature. Oh nice! Yes this makes sense. > I > really don't want to move step backwards here and delete this thing > which probably can be useful later. I don't know how wireless history > dealt with it and how complicated it was to bring such a feature in to > e.g. run multiple access points on one phy. I also see it in ethernet > with macvlan, which is a similar feature. > > We don't need to support it, make it so that on an ifup it returns > -EBUSY if something doesn't fit together as it currently is. We can > later add support for it after playing around with hwsim a little bit > more. We should at least take care that I can still run my multiple > monitors at the same time (which is currently allowed). > > - Alex > Thanks, Miquèl
Hi, On Fri, Aug 26, 2022 at 4:08 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: ... > > > > I need to say that I really used multiple monitors at the same time on > > one phy only and I did that with hwsim to run multiple user space > > stacks. It was working and I was happy and didn't need to do a lot of > > phy creations in hwsim. > > Indeed, looking at the code, you could use as many MONITOR interfaces > you needed, but only a single NODE. I've changed that to use as many > NODE and COORD that we wish. > Be careful there, there is a reason why we don't allow this and this has to do with support of multiple address filters... but here it depends also what you mean with "use". I need to admit, you can achieve the same behaviour of multiple user space stacks and one monitor, _but_ you can easily filter the traffic if you do it per interface... - Alex
Hi, On Fri, Aug 26, 2022 at 3:54 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > aahringo@redhat.com wrote on Thu, 25 Aug 2022 21:05:09 -0400: > > > Hi, > > > > On Thu, Aug 25, 2022 at 8:58 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > Hi Alexander, > > > > > > aahringo@redhat.com wrote on Wed, 24 Aug 2022 17:53:45 -0400: > > > > > > > Hi, > > > > > > > > On Wed, Aug 24, 2022 at 9:27 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > > > Hi Alexander, > > > > > > > > > > aahringo@redhat.com wrote on Wed, 24 Aug 2022 08:43:20 -0400: > > > > > > > > > > > Hi, > > > > > > > > > > > > On Wed, Aug 24, 2022 at 6:21 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > ... > > > > > > > > > > > > > > Actually right now the second level is not enforced, and all the > > > > > > > filtering levels are a bit fuzzy and spread everywhere in rx.c. > > > > > > > > > > > > > > I'm gonna see if I can at least clarify all of that and only make > > > > > > > coord-dependent the right section because right now a > > > > > > > ieee802154_coord_rx() path in ieee802154_rx_handle_packet() does not > > > > > > > really make sense given that the level 3 filtering rules are mostly > > > > > > > enforced in ieee802154_subif_frame(). > > > > > > > > > > > > One thing I mentioned before is that we probably like to have a > > > > > > parameter for rx path to give mac802154 a hint on which filtering > > > > > > level it was received. We don't have that, I currently see that this > > > > > > is a parameter for hwsim receiving it on promiscuous level only and > > > > > > all others do third level filtering. > > > > > > We need that now, because the promiscuous mode was only used for > > > > > > sniffing which goes directly into the rx path for monitors. With scan > > > > > > we mix things up here and in my opinion require such a parameter and > > > > > > do filtering if necessary. > > > > > > > > > > I am currently trying to implement a slightly different approach. The > > > > > core does not know hwsim is always in promiscuous mode, but it does > > > > > know that it does not check FCS. So the core checks it. This is > > > > > level 1 achieved. Then in level 2 we want to know if the core asked > > > > > the transceiver to enter promiscuous mode, which, if it did, should > > > > > not imply more filtering. If the device is working in promiscuous > > > > > mode but this was not asked explicitly by the core, we don't really > > > > > care, software filtering will apply anyway. > > > > > > > > > > > > > I doubt that I will be happy with this solution, this all sounds like > > > > "for the specific current behaviour that we support 2 filtering levels > > > > it will work", just do a parameter on which 802.15.4 filtering level > > > > it was received and the rx path will check what kind of filter is > > > > required and which not. > > > > As driver ops start() callback you should say which filtering level > > > > the receive mode should start with. > > > > > > > > > I am reworking the rx path to clarify what is being done and when, > > > > > because I found this part very obscure right now. In the end I don't > > > > > think we need additional rx info from the drivers. Hopefully my > > > > > proposal will clarify why this is (IMHO) not needed. > > > > > > > > > > > > > Never looked much in 802.15.4 receive path as it just worked but I > > > > said that there might be things to clean up when filtering things on > > > > hardware and when on software and I have the feeling we are doing > > > > things twice. Sometimes it is also necessary to set some skb fields > > > > e.g. PACKET_HOST, etc. and I think this is what the most important > > > > part of it is there. However, there are probably some tune ups if we > > > > know we are in third leveling filtering... > > > > > > Ok, I've done the following. > > > > > > - Adding a PHY parameter which reflects the actual filtering level of > > > the transceiver, the default level is 4 (standard situation, you're > > > > 3? > > Honestly there are only two filtering levels in the normal path and one > additional for scanning situations. But the spec mentions 4, so I > figured we should use the same naming to avoid confusing people on what > "level 3 means, if it's level 3 because level 1 and 2 are identical at > PHY level, or level 3 which is the scan filtering as mentioned in the > spec?". > > I used this enum to clarify the amount of filtering that is involved, > hopefully it is clear enough. I remember we talked about this already > but an unrelated thread, and was not capable of finding it anymore O:-). > > /** enum ieee802154_filtering_level - Filtering levels applicable to a PHY > * @IEEE802154_FILTERING_NONE: No filtering at all, what is received is > * forwarded to the softMAC > * @IEEE802154_FILTERING_1_FCS: First filtering level, frames with an invalid > * FCS should be dropped > * @IEEE802154_FILTERING_2_PROMISCUOUS: Second filtering level, promiscuous > * mode, identical in terms of filtering to the first level at the PHY > * level, but no ACK should be transmitted automatically and at the MAC > * level the frame should be forwarded to the upper layer directly > * @IEEE802154_FILTERING_3_SCAN: Third filtering level, enforced during scans, > * which only forwards beacons > * @IEEE802154_FILTERING_4_FRAME_FIELDS: Fourth filtering level actually > * enforcing the validity of the content of the frame with various checks > */ > enum ieee802154_filtering_level { > IEEE802154_FILTERING_NONE, > IEEE802154_FILTERING_1_FCS, > IEEE802154_FILTERING_2_PROMISCUOUS, > IEEE802154_FILTERING_3_SCAN, > IEEE802154_FILTERING_4_FRAME_FIELDS, > }; > I am fine to drop all this level number naming at all and we do our own filtering definition here, additionally to the mandatory ones. E.g. The SCAN filter can also be implemented in e.g. atusb by using other filter modes which are based on 802.15.4 modes (or level whatever). I am currently thinking about if we might need to change something here in the default handling of the monitor interface, it should use 802.15.4 compatible modes (and this is what we should expect is always being supported). IEEE802154_FILTERING_NONE is not a 802.15.4 filtering mode and is considered to be optional. So the default behaviour of the monitor should be IEEE802154_FILTERING_FCS with a possibility to have a switch to change to IEEE802154_FILTERING_NONE mode if it's supported by the hardware. You should also add a note on the filter level/modes which are mandatory (means given by the spec) and put their level inside there? > > > > > receiving data) but of course if the PHY does not support this state > > > (like hwsim) it should overwrite this value by setting the actual > > > filtering level (none, in the hwsim case) so that the core knows what > > > it receives. > > > > > > > ok. > > > > > - I've replaced the specific "do not check the FCS" flag only used by > > > hwsim by this filtering level, which gives all the information we > > > need. > > > > > > > ok. > > > > > - I've added a real promiscuous filtering mode which truly does not > > > care about the content of the frame but only checks the FCS if not > > > already done by the xceiver. > > > > > > > not sure what a "real promiscuous filtering here is" people have > > different understanding about it, but 802.15.4 has a definition for > > it. > > Promiscuous, by the 802154 spec means: the FCS is good so the content of > the received packet must means something, just forward it and let upper > layers handle it. > > Until now there was no real promiscuous mode in the mac NODE rx path. > Only monitors would get all the frames (including the ones with a wrong > FCS), which is fine because it's a bit out of the spec, so I'm fine > with this idea. But otherwise in the NODE/COORD rx path, the FCS should > be checked even in promiscuous mode to correctly match the spec. > If we parse the frame, the FCS should always be checked. The frame should _never_ be parsed before it hits the monitor receive path. The wording "real promiscuous mode" is in my opinion still debatable, however that's not the point here. > Until now, ieee802154_parse_frame_start() was always called in these > path and this would validate the frame headers. I've added a more > precise promiscuous mode in the rx patch which skips any additional > checks. What happens however is that, if the transceiver disables FCS > checks in promiscuous mode, then FCS is not checked at all and this is > invalid. With my current implementation, the devices which do not check > the FCS might be easily "fixed" by changing their PHY filtering level > to "FILTERING_NONE" in the promiscuous callback. > > > You should consider that having monitors, frames with bad fcs > > should not be filtered out by hardware. There it comes back what I > > said before, the filtering level should be a parameter for start() > > driver ops. > > > > > - I've also implemented in software filtering level 4 for most > > > regular > > > > 3? > > > > > data packets. Without changing the default PHY level mentioned in > > > the first item above, this additional filtering will be skipped > > > which ensures we keep the same behavior of most driver. In the case > > > of hwsim however, these filters will become active if the MAC is > > > not in promiscuous mode or in scan mode, which is actually what > > > people should be expecting. > > > > > > > To give feedback to that I need to see code. And please don't send the > > whole feature stuff again, just this specific part of it. Thanks. > > The entire filtering feature is split: there are the basis introduced > before the scan, and then after the whole scan+association thing I've > introduced additional filtering levels. > No idea what that means. > > > Hopefully all this fits what you had in mind. > > > > > > I have one item left on my current todo list: improving a bit the > > > userspace tool with a "monitor" command. > > > > > > Otherwise the remaining things to do are to discuss the locking > > > design which might need to be changed to avoid lockdep issues and > > > keep the rtnl locked eg. during a channel change. I still don't > > > know how to do that, so it's likely that the right next version > > > will not include any change in this area unless something pops up. > > > > I try to look at that on the weekend. > > I've had an idea yesterday night which seem to work, I think I can drop > the two patches which you disliked regarding discarding the rtnl in the > tx path and in hwsim:change_channel(). I would like to bring the filtering level question at first upstream. If you don't mind. - Alex
Hi Alexander, aahringo@redhat.com wrote on Sun, 28 Aug 2022 22:52:14 -0400: > Hi, > > On Fri, Aug 26, 2022 at 3:54 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Alexander, > > > > aahringo@redhat.com wrote on Thu, 25 Aug 2022 21:05:09 -0400: > > > > > Hi, > > > > > > On Thu, Aug 25, 2022 at 8:58 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > Hi Alexander, > > > > > > > > aahringo@redhat.com wrote on Wed, 24 Aug 2022 17:53:45 -0400: > > > > > > > > > Hi, > > > > > > > > > > On Wed, Aug 24, 2022 at 9:27 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > > > > > Hi Alexander, > > > > > > > > > > > > aahringo@redhat.com wrote on Wed, 24 Aug 2022 08:43:20 -0400: > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > On Wed, Aug 24, 2022 at 6:21 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > ... > > > > > > > > > > > > > > > > Actually right now the second level is not enforced, and all the > > > > > > > > filtering levels are a bit fuzzy and spread everywhere in rx.c. > > > > > > > > > > > > > > > > I'm gonna see if I can at least clarify all of that and only make > > > > > > > > coord-dependent the right section because right now a > > > > > > > > ieee802154_coord_rx() path in ieee802154_rx_handle_packet() does not > > > > > > > > really make sense given that the level 3 filtering rules are mostly > > > > > > > > enforced in ieee802154_subif_frame(). > > > > > > > > > > > > > > One thing I mentioned before is that we probably like to have a > > > > > > > parameter for rx path to give mac802154 a hint on which filtering > > > > > > > level it was received. We don't have that, I currently see that this > > > > > > > is a parameter for hwsim receiving it on promiscuous level only and > > > > > > > all others do third level filtering. > > > > > > > We need that now, because the promiscuous mode was only used for > > > > > > > sniffing which goes directly into the rx path for monitors. With scan > > > > > > > we mix things up here and in my opinion require such a parameter and > > > > > > > do filtering if necessary. > > > > > > > > > > > > I am currently trying to implement a slightly different approach. The > > > > > > core does not know hwsim is always in promiscuous mode, but it does > > > > > > know that it does not check FCS. So the core checks it. This is > > > > > > level 1 achieved. Then in level 2 we want to know if the core asked > > > > > > the transceiver to enter promiscuous mode, which, if it did, should > > > > > > not imply more filtering. If the device is working in promiscuous > > > > > > mode but this was not asked explicitly by the core, we don't really > > > > > > care, software filtering will apply anyway. > > > > > > > > > > > > > > > > I doubt that I will be happy with this solution, this all sounds like > > > > > "for the specific current behaviour that we support 2 filtering levels > > > > > it will work", just do a parameter on which 802.15.4 filtering level > > > > > it was received and the rx path will check what kind of filter is > > > > > required and which not. > > > > > As driver ops start() callback you should say which filtering level > > > > > the receive mode should start with. > > > > > > > > > > > I am reworking the rx path to clarify what is being done and when, > > > > > > because I found this part very obscure right now. In the end I don't > > > > > > think we need additional rx info from the drivers. Hopefully my > > > > > > proposal will clarify why this is (IMHO) not needed. > > > > > > > > > > > > > > > > Never looked much in 802.15.4 receive path as it just worked but I > > > > > said that there might be things to clean up when filtering things on > > > > > hardware and when on software and I have the feeling we are doing > > > > > things twice. Sometimes it is also necessary to set some skb fields > > > > > e.g. PACKET_HOST, etc. and I think this is what the most important > > > > > part of it is there. However, there are probably some tune ups if we > > > > > know we are in third leveling filtering... > > > > > > > > Ok, I've done the following. > > > > > > > > - Adding a PHY parameter which reflects the actual filtering level of > > > > the transceiver, the default level is 4 (standard situation, you're > > > > > > 3? > > > > Honestly there are only two filtering levels in the normal path and one > > additional for scanning situations. But the spec mentions 4, so I > > figured we should use the same naming to avoid confusing people on what > > "level 3 means, if it's level 3 because level 1 and 2 are identical at > > PHY level, or level 3 which is the scan filtering as mentioned in the > > spec?". > > > > I used this enum to clarify the amount of filtering that is involved, > > hopefully it is clear enough. I remember we talked about this already > > but an unrelated thread, and was not capable of finding it anymore O:-). > > > > /** enum ieee802154_filtering_level - Filtering levels applicable to a PHY > > * @IEEE802154_FILTERING_NONE: No filtering at all, what is received is > > * forwarded to the softMAC > > * @IEEE802154_FILTERING_1_FCS: First filtering level, frames with an invalid > > * FCS should be dropped > > * @IEEE802154_FILTERING_2_PROMISCUOUS: Second filtering level, promiscuous > > * mode, identical in terms of filtering to the first level at the PHY > > * level, but no ACK should be transmitted automatically and at the MAC > > * level the frame should be forwarded to the upper layer directly > > * @IEEE802154_FILTERING_3_SCAN: Third filtering level, enforced during scans, > > * which only forwards beacons > > * @IEEE802154_FILTERING_4_FRAME_FIELDS: Fourth filtering level actually > > * enforcing the validity of the content of the frame with various checks > > */ > > enum ieee802154_filtering_level { > > IEEE802154_FILTERING_NONE, > > IEEE802154_FILTERING_1_FCS, > > IEEE802154_FILTERING_2_PROMISCUOUS, > > IEEE802154_FILTERING_3_SCAN, > > IEEE802154_FILTERING_4_FRAME_FIELDS, > > }; > > > > I am fine to drop all this level number naming at all and we do our > own filtering definition here, additionally to the mandatory ones. I can add intermediate filtering levels but I don't know what they are, you need to give me an exhaustive list of what you have in mind? > E.g. The SCAN filter can also be implemented in e.g. atusb by using > other filter modes which are based on 802.15.4 modes (or level > whatever). That is actually what I've proposed. The core requests a level of filtering among the official ones, the PHY driver when it gets the request does what is possible and adjusts the final filtering level to what it achieved. The core will then have to handle the missing checks. > I am currently thinking about if we might need to change something > here in the default handling of the monitor interface, it should use > 802.15.4 compatible modes (and this is what we should expect is always > being supported). IEEE802154_FILTERING_NONE is not a 802.15.4 > filtering mode and is considered to be optional. So the default > behaviour of the monitor should be IEEE802154_FILTERING_FCS with a > possibility to have a switch to change to IEEE802154_FILTERING_NONE > mode if it's supported by the hardware. That's what I've done, besides that the default filtering level for a monitor interface is PROMISCUOUS and not FCS. In practice, the filtering regarding the incoming frames will be exactly the same between FCS, PROMISCUOUS and SCAN, but in the PROMISCUOUS case we ask the PHY not to send ACKS (which is not the case of the FCS filtering level, acks can be automatically sent by the PHY, we don't want that). > You should also add a note on the filter level/modes which are > mandatory (means given by the spec) and put their level inside there? > > > > > > > > receiving data) but of course if the PHY does not support > > > > this state (like hwsim) it should overwrite this value by > > > > setting the actual filtering level (none, in the hwsim case) so > > > > that the core knows what it receives. > > > > > > > > > > ok. > > > > > > > - I've replaced the specific "do not check the FCS" flag only > > > > used by hwsim by this filtering level, which gives all the > > > > information we need. > > > > > > > > > > ok. > > > > > > > - I've added a real promiscuous filtering mode which truly does > > > > not care about the content of the frame but only checks the FCS > > > > if not already done by the xceiver. > > > > > > > > > > not sure what a "real promiscuous filtering here is" people have > > > different understanding about it, but 802.15.4 has a definition > > > for it. > > > > Promiscuous, by the 802154 spec means: the FCS is good so the > > content of the received packet must means something, just forward > > it and let upper layers handle it. > > > > Until now there was no real promiscuous mode in the mac NODE rx > > path. Only monitors would get all the frames (including the ones > > with a wrong FCS), which is fine because it's a bit out of the > > spec, so I'm fine with this idea. But otherwise in the NODE/COORD > > rx path, the FCS should be checked even in promiscuous mode to > > correctly match the spec. > > If we parse the frame, the FCS should always be checked. The frame > should _never_ be parsed before it hits the monitor receive path. Yes. > > The wording "real promiscuous mode" is in my opinion still debatable, > however that's not the point here. I meant "like it is described in the spec". > > Until now, ieee802154_parse_frame_start() was always called in these > > path and this would validate the frame headers. I've added a more > > precise promiscuous mode in the rx patch which skips any additional > > checks. What happens however is that, if the transceiver disables > > FCS checks in promiscuous mode, then FCS is not checked at all and > > this is invalid. With my current implementation, the devices which > > do not check the FCS might be easily "fixed" by changing their PHY > > filtering level to "FILTERING_NONE" in the promiscuous callback. > > > > > You should consider that having monitors, frames with bad fcs > > > should not be filtered out by hardware. There it comes back what I > > > said before, the filtering level should be a parameter for start() > > > driver ops. > > > > > > > - I've also implemented in software filtering level 4 for most > > > > regular > > > > > > 3? > > > > > > > data packets. Without changing the default PHY level > > > > mentioned in the first item above, this additional filtering > > > > will be skipped which ensures we keep the same behavior of most > > > > driver. In the case of hwsim however, these filters will become > > > > active if the MAC is not in promiscuous mode or in scan mode, > > > > which is actually what people should be expecting. > > > > > > > > > > To give feedback to that I need to see code. And please don't > > > send the whole feature stuff again, just this specific part of > > > it. Thanks. > > > > The entire filtering feature is split: there are the basis > > introduced before the scan, and then after the whole > > scan+association thing I've introduced additional filtering levels. > > > > No idea what that means. In my series, the first patches address the missing bits about filtering. Then there are patches about the scan. And finally there are two patches improving the filtering, which are not actually needed right now for the scan to work. > > > > Hopefully all this fits what you had in mind. > > > > > > > > I have one item left on my current todo list: improving a bit > > > > the userspace tool with a "monitor" command. > > > > > > > > Otherwise the remaining things to do are to discuss the locking > > > > design which might need to be changed to avoid lockdep issues > > > > and keep the rtnl locked eg. during a channel change. I still > > > > don't know how to do that, so it's likely that the right next > > > > version will not include any change in this area unless > > > > something pops up. > > > > > > I try to look at that on the weekend. > > > > I've had an idea yesterday night which seem to work, I think I can > > drop the two patches which you disliked regarding discarding the > > rtnl in the tx path and in hwsim:change_channel(). > > I would like to bring the filtering level question at first upstream. > If you don't mind. The additional filtering which I've written has nothing to do with the scan, and more importantly it uses many new enums/helpers which are added along the way (by the scan series and the association series). Hence moving this filtering earlier in the series is a real pain and would anyway not bring anything really useful at this stage. All the important filtering changes are already available in the v2 series sent last week. Not mentioning, I would really like to move forward on the scan series as it's been in the air for quite some time. Thanks, Miquèl
Hi Alexander, aahringo@redhat.com wrote on Sun, 28 Aug 2022 22:31:19 -0400: > Hi, > > On Fri, Aug 26, 2022 at 4:08 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > ... > > > > > > I need to say that I really used multiple monitors at the same time on > > > one phy only and I did that with hwsim to run multiple user space > > > stacks. It was working and I was happy and didn't need to do a lot of > > > phy creations in hwsim. > > > > Indeed, looking at the code, you could use as many MONITOR interfaces > > you needed, but only a single NODE. I've changed that to use as many > > NODE and COORD that we wish. > > > > Be careful there, there is a reason why we don't allow this and this > has to do with support of multiple address filters... but here it > depends also what you mean with "use". I've updated this part, you're right I was not careful enough on the per-iface filtering part, I think it's fine now. Basically, compared to the v2 sent last week, I've stopped considering that if one interface was in promiscuous mode, all of them should and did a per-PHY check, so that otherwise we can apply the necessary filtering if needed. > I need to admit, you can achieve the same behaviour of multiple user > space stacks and one monitor, _but_ you can easily filter the traffic > if you do it per interface... Thanks, Miquèl
Hi, On Mon, Aug 29, 2022 at 4:02 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > aahringo@redhat.com wrote on Sun, 28 Aug 2022 22:52:14 -0400: > > > Hi, > > > > On Fri, Aug 26, 2022 at 3:54 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > Hi Alexander, > > > > > > aahringo@redhat.com wrote on Thu, 25 Aug 2022 21:05:09 -0400: > > > > > > > Hi, > > > > > > > > On Thu, Aug 25, 2022 at 8:58 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > > > Hi Alexander, > > > > > > > > > > aahringo@redhat.com wrote on Wed, 24 Aug 2022 17:53:45 -0400: > > > > > > > > > > > Hi, > > > > > > > > > > > > On Wed, Aug 24, 2022 at 9:27 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > > > > > > > Hi Alexander, > > > > > > > > > > > > > > aahringo@redhat.com wrote on Wed, 24 Aug 2022 08:43:20 -0400: > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > On Wed, Aug 24, 2022 at 6:21 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > ... > > > > > > > > > > > > > > > > > > Actually right now the second level is not enforced, and all the > > > > > > > > > filtering levels are a bit fuzzy and spread everywhere in rx.c. > > > > > > > > > > > > > > > > > > I'm gonna see if I can at least clarify all of that and only make > > > > > > > > > coord-dependent the right section because right now a > > > > > > > > > ieee802154_coord_rx() path in ieee802154_rx_handle_packet() does not > > > > > > > > > really make sense given that the level 3 filtering rules are mostly > > > > > > > > > enforced in ieee802154_subif_frame(). > > > > > > > > > > > > > > > > One thing I mentioned before is that we probably like to have a > > > > > > > > parameter for rx path to give mac802154 a hint on which filtering > > > > > > > > level it was received. We don't have that, I currently see that this > > > > > > > > is a parameter for hwsim receiving it on promiscuous level only and > > > > > > > > all others do third level filtering. > > > > > > > > We need that now, because the promiscuous mode was only used for > > > > > > > > sniffing which goes directly into the rx path for monitors. With scan > > > > > > > > we mix things up here and in my opinion require such a parameter and > > > > > > > > do filtering if necessary. > > > > > > > > > > > > > > I am currently trying to implement a slightly different approach. The > > > > > > > core does not know hwsim is always in promiscuous mode, but it does > > > > > > > know that it does not check FCS. So the core checks it. This is > > > > > > > level 1 achieved. Then in level 2 we want to know if the core asked > > > > > > > the transceiver to enter promiscuous mode, which, if it did, should > > > > > > > not imply more filtering. If the device is working in promiscuous > > > > > > > mode but this was not asked explicitly by the core, we don't really > > > > > > > care, software filtering will apply anyway. > > > > > > > > > > > > > > > > > > > I doubt that I will be happy with this solution, this all sounds like > > > > > > "for the specific current behaviour that we support 2 filtering levels > > > > > > it will work", just do a parameter on which 802.15.4 filtering level > > > > > > it was received and the rx path will check what kind of filter is > > > > > > required and which not. > > > > > > As driver ops start() callback you should say which filtering level > > > > > > the receive mode should start with. > > > > > > > > > > > > > I am reworking the rx path to clarify what is being done and when, > > > > > > > because I found this part very obscure right now. In the end I don't > > > > > > > think we need additional rx info from the drivers. Hopefully my > > > > > > > proposal will clarify why this is (IMHO) not needed. > > > > > > > > > > > > > > > > > > > Never looked much in 802.15.4 receive path as it just worked but I > > > > > > said that there might be things to clean up when filtering things on > > > > > > hardware and when on software and I have the feeling we are doing > > > > > > things twice. Sometimes it is also necessary to set some skb fields > > > > > > e.g. PACKET_HOST, etc. and I think this is what the most important > > > > > > part of it is there. However, there are probably some tune ups if we > > > > > > know we are in third leveling filtering... > > > > > > > > > > Ok, I've done the following. > > > > > > > > > > - Adding a PHY parameter which reflects the actual filtering level of > > > > > the transceiver, the default level is 4 (standard situation, you're > > > > > > > > 3? > > > > > > Honestly there are only two filtering levels in the normal path and one > > > additional for scanning situations. But the spec mentions 4, so I > > > figured we should use the same naming to avoid confusing people on what > > > "level 3 means, if it's level 3 because level 1 and 2 are identical at > > > PHY level, or level 3 which is the scan filtering as mentioned in the > > > spec?". > > > > > > I used this enum to clarify the amount of filtering that is involved, > > > hopefully it is clear enough. I remember we talked about this already > > > but an unrelated thread, and was not capable of finding it anymore O:-). > > > > > > /** enum ieee802154_filtering_level - Filtering levels applicable to a PHY > > > * @IEEE802154_FILTERING_NONE: No filtering at all, what is received is > > > * forwarded to the softMAC > > > * @IEEE802154_FILTERING_1_FCS: First filtering level, frames with an invalid > > > * FCS should be dropped > > > * @IEEE802154_FILTERING_2_PROMISCUOUS: Second filtering level, promiscuous > > > * mode, identical in terms of filtering to the first level at the PHY > > > * level, but no ACK should be transmitted automatically and at the MAC > > > * level the frame should be forwarded to the upper layer directly > > > * @IEEE802154_FILTERING_3_SCAN: Third filtering level, enforced during scans, > > > * which only forwards beacons > > > * @IEEE802154_FILTERING_4_FRAME_FIELDS: Fourth filtering level actually > > > * enforcing the validity of the content of the frame with various checks > > > */ > > > enum ieee802154_filtering_level { > > > IEEE802154_FILTERING_NONE, > > > IEEE802154_FILTERING_1_FCS, > > > IEEE802154_FILTERING_2_PROMISCUOUS, > > > IEEE802154_FILTERING_3_SCAN, > > > IEEE802154_FILTERING_4_FRAME_FIELDS, > > > }; > > > > > > > I am fine to drop all this level number naming at all and we do our > > own filtering definition here, additionally to the mandatory ones. > > I can add intermediate filtering levels but I don't know what they are, > you need to give me an exhaustive list of what you have in mind? > see below. > > E.g. The SCAN filter can also be implemented in e.g. atusb by using > > other filter modes which are based on 802.15.4 modes (or level > > whatever). > > That is actually what I've proposed. The core requests a level of > filtering among the official ones, the PHY driver when it gets the > request does what is possible and adjusts the final filtering level to > what it achieved. The core will then have to handle the missing checks. > We talked about the same thing months ago. I am fine with that, but see below. > > I am currently thinking about if we might need to change something > > here in the default handling of the monitor interface, it should use > > 802.15.4 compatible modes (and this is what we should expect is always > > being supported). IEEE802154_FILTERING_NONE is not a 802.15.4 > > filtering mode and is considered to be optional. So the default > > behaviour of the monitor should be IEEE802154_FILTERING_FCS with a > > possibility to have a switch to change to IEEE802154_FILTERING_NONE > > mode if it's supported by the hardware. > > That's what I've done, besides that the default filtering level for a > monitor interface is PROMISCUOUS and not FCS. In practice, the Who says what the default filtering is for a monitor interface? > filtering regarding the incoming frames will be exactly the same > between FCS, PROMISCUOUS and SCAN, but in the PROMISCUOUS case we ask > the PHY not to send ACKS (which is not the case of the FCS filtering > level, acks can be automatically sent by the PHY, we don't want that). > Usually it's when we don't have a working address filter, then there is no auto ackknowledge feature activated (I think this can be counted as general rule for now). This is currently the case with the set_promiscuousmode() driver ops. > > You should also add a note on the filter level/modes which are > > mandatory (means given by the spec) and put their level inside there? > > > > > > > > > > > receiving data) but of course if the PHY does not support > > > > > this state (like hwsim) it should overwrite this value by > > > > > setting the actual filtering level (none, in the hwsim case) so > > > > > that the core knows what it receives. > > > > > > > > > > > > > ok. > > > > > > > > > - I've replaced the specific "do not check the FCS" flag only > > > > > used by hwsim by this filtering level, which gives all the > > > > > information we need. > > > > > > > > > > > > > ok. > > > > > > > > > - I've added a real promiscuous filtering mode which truly does > > > > > not care about the content of the frame but only checks the FCS > > > > > if not already done by the xceiver. > > > > > > > > > > > > > not sure what a "real promiscuous filtering here is" people have > > > > different understanding about it, but 802.15.4 has a definition > > > > for it. > > > > > > Promiscuous, by the 802154 spec means: the FCS is good so the > > > content of the received packet must means something, just forward > > > it and let upper layers handle it. > > > > > > Until now there was no real promiscuous mode in the mac NODE rx > > > path. Only monitors would get all the frames (including the ones > > > with a wrong FCS), which is fine because it's a bit out of the > > > spec, so I'm fine with this idea. But otherwise in the NODE/COORD > > > rx path, the FCS should be checked even in promiscuous mode to > > > correctly match the spec. > > > > If we parse the frame, the FCS should always be checked. The frame > > should _never_ be parsed before it hits the monitor receive path. > > Yes. > > > > > The wording "real promiscuous mode" is in my opinion still debatable, > > however that's not the point here. > > I meant "like it is described in the spec". > > > > Until now, ieee802154_parse_frame_start() was always called in these > > > path and this would validate the frame headers. I've added a more > > > precise promiscuous mode in the rx patch which skips any additional > > > checks. What happens however is that, if the transceiver disables > > > FCS checks in promiscuous mode, then FCS is not checked at all and > > > this is invalid. With my current implementation, the devices which > > > do not check the FCS might be easily "fixed" by changing their PHY > > > filtering level to "FILTERING_NONE" in the promiscuous callback. > > > > > > > You should consider that having monitors, frames with bad fcs > > > > should not be filtered out by hardware. There it comes back what I > > > > said before, the filtering level should be a parameter for start() > > > > driver ops. > > > > > > > > > - I've also implemented in software filtering level 4 for most > > > > > regular > > > > > > > > 3? > > > > > > > > > data packets. Without changing the default PHY level > > > > > mentioned in the first item above, this additional filtering > > > > > will be skipped which ensures we keep the same behavior of most > > > > > driver. In the case of hwsim however, these filters will become > > > > > active if the MAC is not in promiscuous mode or in scan mode, > > > > > which is actually what people should be expecting. > > > > > > > > > > > > > To give feedback to that I need to see code. And please don't > > > > send the whole feature stuff again, just this specific part of > > > > it. Thanks. > > > > > > The entire filtering feature is split: there are the basis > > > introduced before the scan, and then after the whole > > > scan+association thing I've introduced additional filtering levels. > > > > > > > No idea what that means. > > In my series, the first patches address the missing bits about > filtering. Then there are patches about the scan. And finally there are > two patches improving the filtering, which are not actually needed > right now for the scan to work. > ok. > > > > > Hopefully all this fits what you had in mind. > > > > > > > > > > I have one item left on my current todo list: improving a bit > > > > > the userspace tool with a "monitor" command. > > > > > > > > > > Otherwise the remaining things to do are to discuss the locking > > > > > design which might need to be changed to avoid lockdep issues > > > > > and keep the rtnl locked eg. during a channel change. I still > > > > > don't know how to do that, so it's likely that the right next > > > > > version will not include any change in this area unless > > > > > something pops up. > > > > > > > > I try to look at that on the weekend. > > > > > > I've had an idea yesterday night which seem to work, I think I can > > > drop the two patches which you disliked regarding discarding the > > > rtnl in the tx path and in hwsim:change_channel(). > > > > I would like to bring the filtering level question at first upstream. > > If you don't mind. > > The additional filtering which I've written has nothing to do with the > scan, and more importantly it uses many new enums/helpers which are > added along the way (by the scan series and the association series). > Hence moving this filtering earlier in the series is a real pain and > would anyway not bring anything really useful at this stage. All the > important filtering changes are already available in the v2 series sent > last week. > > Not mentioning, I would really like to move forward on the scan series > as it's been in the air for quite some time. > I did not see the v2 until now. Sorry for that. However I think there are missing bits here at the receive handling side. Which are: 1. Do a stop_tx(), stop_rx(), start_rx(filtering_level) to go into other filtering modes while ifup. I don't want to see all filtering modes here, just what we currently support with NONE (then with FCS check on software if necessary), ?THIRD/FOURTH? LEVEL filtering and that's it. What I don't want to see is runtime changes of phy flags. To tell the receive path what to filter and what's not. 2. set the pan coordinator bit for hw address filter. And there is a TODO about setting pkt_type in mac802154 receive path which we should take a look into. This bit should be addressed for coordinator support even if there is the question about coordinator vs pan coordinator, then the kernel needs a bit as coordinator iface type parameter to know if it's a pan coordinator and not coordinator. I think it makes total sense to split this work in transmit handling, where we had no support at all to send something besides the usual data path, and receive handling, where we have no way to change the filtering level besides interface type and ifup time of an interface. We are currently trying to make a receive path working in a way that "the other ideas flying around which are good" can be introduced in future. If this is done, then take care about how to add the rest of it. I will look into v2 the next few days. - Alex
Hi Alexander & Stefan,
aahringo@redhat.com wrote on Mon, 29 Aug 2022 22:23:09 -0400:
I am currently testing my code with the ATUSB devices, the association
works, so it's a good news! However I am struggling to get the
association working for a simple reason: the crafted ACKs are
transmitted (the ATUSB in monitor mode sees it) but I get absolutely
nothing on the receiver side.
The logic is:
coord0 coord1
association req ->
<- ack
<- association response
ack ->
The first ack is sent by coord1 but coord0 never sees anything. In
practice coord0 has sent an association request and received a single
one-byte packet in return which I guess is the firmware saying "okay, Tx
has been performed". Shall I interpret this byte differently? Does it
mean that the ack has also been received?
I could not find a documentation of the firmware interface, I went
through the wiki but I did not find something clear about what to
expect or "what the driver should do". But perhaps this will ring a
bell on your side?
[...]
> I did not see the v2 until now. Sorry for that.
Ah! Ok, no problem :)
>
> However I think there are missing bits here at the receive handling
> side. Which are:
>
> 1. Do a stop_tx(), stop_rx(), start_rx(filtering_level) to go into
> other filtering modes while ifup.
Who is supposed to change the filtering level?
For now there is only the promiscuous mode being applied and the user
has no knowledge about it, it's just something internal.
Changing how the promiscuous mode is applied (using a filtering level
instead of a "promiscuous on" boolean) would impact all the drivers
and for now we don't really need it.
> I don't want to see all filtering modes here, just what we currently
> support with NONE (then with FCS check on software if necessary),
> ?THIRD/FOURTH? LEVEL filtering and that's it. What I don't want to see
> is runtime changes of phy flags. To tell the receive path what to
> filter and what's not.
Runtime changes on a dedicated "filtering" PHY flag is what I've used
and it works okay for this situation, why don't you want that? It
avoids the need for (yet) another rework of the API with the drivers,
no?
> 2. set the pan coordinator bit for hw address filter. And there is a
> TODO about setting pkt_type in mac802154 receive path which we should
> take a look into. This bit should be addressed for coordinator support
> even if there is the question about coordinator vs pan coordinator,
> then the kernel needs a bit as coordinator iface type parameter to
> know if it's a pan coordinator and not coordinator.
This is not really something that we can "set". Either the device
had performed an association and it is a child device: it is not the
PAN coordinator, or it initiated the PAN and it is the PAN coordinator.
There are commands to change that later on but those are not supported.
The "PAN coordinator" information is being added in the association
series (which comes after the scan). I have handled the pkt_type you are
mentioning.
> I think it makes total sense to split this work in transmit handling,
> where we had no support at all to send something besides the usual
> data path, and receive handling, where we have no way to change the
> filtering level besides interface type and ifup time of an interface.
> We are currently trying to make a receive path working in a way that
> "the other ideas flying around which are good" can be introduced in
> future.
> If this is done, then take care about how to add the rest of it.
>
> I will look into v2 the next few days.
>
> - Alex
>
Thanks,
Miquèl
Hello again, miquel.raynal@bootlin.com wrote on Wed, 31 Aug 2022 17:39:03 +0200: > Hi Alexander & Stefan, > > aahringo@redhat.com wrote on Mon, 29 Aug 2022 22:23:09 -0400: > > I am currently testing my code with the ATUSB devices, the association > works, so it's a good news! However I am struggling to get the > association working for a simple reason: the crafted ACKs are > transmitted (the ATUSB in monitor mode sees it) but I get absolutely > nothing on the receiver side. > > The logic is: > > coord0 coord1 > association req -> > <- ack > <- association response > ack -> > > The first ack is sent by coord1 but coord0 never sees anything. In > practice coord0 has sent an association request and received a single > one-byte packet in return which I guess is the firmware saying "okay, Tx > has been performed". Shall I interpret this byte differently? Does it > mean that the ack has also been received? I think I now have a clearer understanding on how the devices behave. I turned the devices into promiscuous mode and could observe that some frames were considered wrong. Indeed, it looks like the PHYs add the FCS themselves, while the spec says that the FCS should be provided to the PHY. Anyway, I dropped the FCS calculations from the different MLME frames forged and it helped a lot. I also kind of "discovered" the concept of hardware address filtering on atusb which makes me realize that maybe we were not talking about the same "filtering" until now. Associations and disassociations now work properly, I'm glad I fixed "everything". I still need to figure out if using the promiscuous mode everywhere is really useful or not (maybe the hardware filters were disabled in this mode and it made it work). However, using the promiscuous mode was the only way I had to receive acknowledgements, otherwise they were filtered out by the hardware (the monitor was showing that the ack frames were actually being sent). Finally, changing the channel was also a piece of the puzzle, because I think some of my smart light bulbs tried to say hello and it kind of disturbed me :) > I could not find a documentation of the firmware interface, I went > through the wiki but I did not find something clear about what to > expect or "what the driver should do". But perhaps this will ring a > bell on your side? > > [...] > > > I did not see the v2 until now. Sorry for that. > > Ah! Ok, no problem :) > > > > > However I think there are missing bits here at the receive handling > > side. Which are: > > > > 1. Do a stop_tx(), stop_rx(), start_rx(filtering_level) to go into > > other filtering modes while ifup. > > Who is supposed to change the filtering level? > > For now there is only the promiscuous mode being applied and the user > has no knowledge about it, it's just something internal. > > Changing how the promiscuous mode is applied (using a filtering level > instead of a "promiscuous on" boolean) would impact all the drivers > and for now we don't really need it. > > > I don't want to see all filtering modes here, just what we currently > > support with NONE (then with FCS check on software if necessary), > > ?THIRD/FOURTH? LEVEL filtering and that's it. What I don't want to see > > is runtime changes of phy flags. To tell the receive path what to > > filter and what's not. > > Runtime changes on a dedicated "filtering" PHY flag is what I've used > and it works okay for this situation, why don't you want that? It > avoids the need for (yet) another rework of the API with the drivers, > no? > > > 2. set the pan coordinator bit for hw address filter. And there is a > > TODO about setting pkt_type in mac802154 receive path which we should > > take a look into. This bit should be addressed for coordinator support > > even if there is the question about coordinator vs pan coordinator, > > then the kernel needs a bit as coordinator iface type parameter to > > know if it's a pan coordinator and not coordinator. > > This is not really something that we can "set". Either the device > had performed an association and it is a child device: it is not the > PAN coordinator, or it initiated the PAN and it is the PAN coordinator. > There are commands to change that later on but those are not supported. > > The "PAN coordinator" information is being added in the association > series (which comes after the scan). I have handled the pkt_type you are > mentioning. > > > I think it makes total sense to split this work in transmit handling, > > where we had no support at all to send something besides the usual > > data path, and receive handling, where we have no way to change the > > filtering level besides interface type and ifup time of an interface. > > We are currently trying to make a receive path working in a way that > > "the other ideas flying around which are good" can be introduced in > > future. > > If this is done, then take care about how to add the rest of it. > > > > I will look into v2 the next few days. If possible, I would really like to understand what you expect in terms of filtering. Maybe as well a short snippet of code showing what kind of interface you have in mind. Are we talking about a rework of the promiscuous callback? Are we talking about the hardware filters? What are the inputs and outputs for these callbacks? What do we expect from the drivers in terms of advertising? I will be glad to make the relevant changes once I understand what is needed because on this topic I have a clear lack of experience, so I will try to judge what is reachable based on your inputs. Thanks, Miquèl
Hello, miquel.raynal@bootlin.com wrote on Thu, 1 Sep 2022 02:09:18 +0200: > Hello again, > > miquel.raynal@bootlin.com wrote on Wed, 31 Aug 2022 17:39:03 +0200: > > > Hi Alexander & Stefan, > > > > aahringo@redhat.com wrote on Mon, 29 Aug 2022 22:23:09 -0400: > > > > I am currently testing my code with the ATUSB devices, the association > > works, so it's a good news! However I am struggling to get the > > association working for a simple reason: the crafted ACKs are > > transmitted (the ATUSB in monitor mode sees it) but I get absolutely > > nothing on the receiver side. > > > > The logic is: > > > > coord0 coord1 > > association req -> > > <- ack > > <- association response > > ack -> > > > > The first ack is sent by coord1 but coord0 never sees anything. In > > practice coord0 has sent an association request and received a single > > one-byte packet in return which I guess is the firmware saying "okay, Tx > > has been performed". Shall I interpret this byte differently? Does it > > mean that the ack has also been received? > > I think I now have a clearer understanding on how the devices behave. > > I turned the devices into promiscuous mode and could observe that some > frames were considered wrong. Indeed, it looks like the PHYs add the > FCS themselves, while the spec says that the FCS should be provided to > the PHY. Anyway, I dropped the FCS calculations from the different MLME > frames forged and it helped a lot. > > I also kind of "discovered" the concept of hardware address filtering > on atusb which makes me realize that maybe we were not talking about > the same "filtering" until now. > > Associations and disassociations now work properly, I'm glad I fixed > "everything". I still need to figure out if using the promiscuous mode > everywhere is really useful or not (maybe the hardware filters were > disabled in this mode and it made it work). However, using the > promiscuous mode was the only way I had to receive acknowledgements, > otherwise they were filtered out by the hardware (the monitor was > showing that the ack frames were actually being sent). > > Finally, changing the channel was also a piece of the puzzle, because I > think some of my smart light bulbs tried to say hello and it kind of > disturbed me :) I tried to scan my ATUSB devices from a Zephyr based Arduino Nano BLE but for now it does not work, the ATUSB devices receive the scan requests from Zephyr and send their beacons, the ATUSB monitor shows the beacons on Wireshark but the ieee80154_recv() callback is never triggered on Zephyr side. I am new to this OS so if you have any idea or debugging tips, I would be glad to hear them. > > I could not find a documentation of the firmware interface, I went > > through the wiki but I did not find something clear about what to > > expect or "what the driver should do". But perhaps this will ring a > > bell on your side? > > > > [...] > > > > > I did not see the v2 until now. Sorry for that. > > > > Ah! Ok, no problem :) > > > > > > > > However I think there are missing bits here at the receive handling > > > side. Which are: > > > > > > 1. Do a stop_tx(), stop_rx(), start_rx(filtering_level) to go into > > > other filtering modes while ifup. > > > > Who is supposed to change the filtering level? > > > > For now there is only the promiscuous mode being applied and the user > > has no knowledge about it, it's just something internal. > > > > Changing how the promiscuous mode is applied (using a filtering level > > instead of a "promiscuous on" boolean) would impact all the drivers > > and for now we don't really need it. > > > > > I don't want to see all filtering modes here, just what we currently > > > support with NONE (then with FCS check on software if necessary), > > > ?THIRD/FOURTH? LEVEL filtering and that's it. What I don't want to see > > > is runtime changes of phy flags. To tell the receive path what to > > > filter and what's not. > > > > Runtime changes on a dedicated "filtering" PHY flag is what I've used > > and it works okay for this situation, why don't you want that? It > > avoids the need for (yet) another rework of the API with the drivers, > > no? > > > > > 2. set the pan coordinator bit for hw address filter. And there is a > > > TODO about setting pkt_type in mac802154 receive path which we should > > > take a look into. This bit should be addressed for coordinator support > > > even if there is the question about coordinator vs pan coordinator, > > > then the kernel needs a bit as coordinator iface type parameter to > > > know if it's a pan coordinator and not coordinator. > > > > This is not really something that we can "set". Either the device > > had performed an association and it is a child device: it is not the > > PAN coordinator, or it initiated the PAN and it is the PAN coordinator. > > There are commands to change that later on but those are not supported. > > > > The "PAN coordinator" information is being added in the association > > series (which comes after the scan). I have handled the pkt_type you are > > mentioning. > > > > > I think it makes total sense to split this work in transmit handling, > > > where we had no support at all to send something besides the usual > > > data path, and receive handling, where we have no way to change the > > > filtering level besides interface type and ifup time of an interface. > > > We are currently trying to make a receive path working in a way that > > > "the other ideas flying around which are good" can be introduced in > > > future. > > > If this is done, then take care about how to add the rest of it. > > > > > > I will look into v2 the next few days. > > If possible, I would really like to understand what you expect in terms > of filtering. Maybe as well a short snippet of code showing what kind > of interface you have in mind. Are we talking about a rework of the > promiscuous callback? Are we talking about the hardware filters? What > are the inputs and outputs for these callbacks? What do we expect from > the drivers in terms of advertising? I will be glad to make the > relevant changes once I understand what is needed because on this topic > I have a clear lack of experience, so I will try to judge what is > reachable based on your inputs. Also, can you please clarify when are we talking about software and when about hardware filters. Thanks, Miquèl
Hi, On Wed, Aug 31, 2022 at 11:39 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander & Stefan, > > aahringo@redhat.com wrote on Mon, 29 Aug 2022 22:23:09 -0400: > > I am currently testing my code with the ATUSB devices, the association > works, so it's a good news! However I am struggling to get the > association working for a simple reason: the crafted ACKs are > transmitted (the ATUSB in monitor mode sees it) but I get absolutely What is a crafted ACK here? > nothing on the receiver side. > > The logic is: > > coord0 coord1 > association req -> > <- ack > <- association response > ack -> > > The first ack is sent by coord1 but coord0 never sees anything. In > practice coord0 has sent an association request and received a single > one-byte packet in return which I guess is the firmware saying "okay, Tx > has been performed". Shall I interpret this byte differently? Does it > mean that the ack has also been received? > > I could not find a documentation of the firmware interface, I went > through the wiki but I did not find something clear about what to > expect or "what the driver should do". But perhaps this will ring a > bell on your side? > > [...] > > > I did not see the v2 until now. Sorry for that. > > Ah! Ok, no problem :) > > > > > However I think there are missing bits here at the receive handling > > side. Which are: > > > > 1. Do a stop_tx(), stop_rx(), start_rx(filtering_level) to go into > > other filtering modes while ifup. > > Who is supposed to change the filtering level? > depending on what mac802154 is doing, for scan it's required to switch the filter level to promiscuous? > For now there is only the promiscuous mode being applied and the user > has no knowledge about it, it's just something internal. > Okay, sounds good. > Changing how the promiscuous mode is applied (using a filtering level > instead of a "promiscuous on" boolean) would impact all the drivers > and for now we don't really need it. > no, it does not. Okay, you can hide the promiscuous mode driver callback from start()... but yes the goal would be to remove the promiscuous mode op in future. > > I don't want to see all filtering modes here, just what we currently > > support with NONE (then with FCS check on software if necessary), > > ?THIRD/FOURTH? LEVEL filtering and that's it. What I don't want to see > > is runtime changes of phy flags. To tell the receive path what to > > filter and what's not. > > Runtime changes on a dedicated "filtering" PHY flag is what I've used > and it works okay for this situation, why don't you want that? It > avoids the need for (yet) another rework of the API with the drivers, > no? > I am not sure what exactly here is "dedicated "filtering" PHY flag" if it's the hwflags it was never made to be changed during runtime. I also don't know what "yet another rework of the API" means here, there is a current behaviour which we can assume and only hwsim is a little bit out of range which should overwrite the "default". > > 2. set the pan coordinator bit for hw address filter. And there is a > > TODO about setting pkt_type in mac802154 receive path which we should > > take a look into. This bit should be addressed for coordinator support > > even if there is the question about coordinator vs pan coordinator, > > then the kernel needs a bit as coordinator iface type parameter to > > know if it's a pan coordinator and not coordinator. > > This is not really something that we can "set". Either the device > had performed an association and it is a child device: it is not the > PAN coordinator, or it initiated the PAN and it is the PAN coordinator. > There are commands to change that later on but those are not supported. > > The "PAN coordinator" information is being added in the association > series (which comes after the scan). I have handled the pkt_type you are > mentioning. > okay. - Alex
Hi, On Wed, Aug 31, 2022 at 8:09 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hello again, > > miquel.raynal@bootlin.com wrote on Wed, 31 Aug 2022 17:39:03 +0200: > > > Hi Alexander & Stefan, > > > > aahringo@redhat.com wrote on Mon, 29 Aug 2022 22:23:09 -0400: > > > > I am currently testing my code with the ATUSB devices, the association > > works, so it's a good news! However I am struggling to get the > > association working for a simple reason: the crafted ACKs are > > transmitted (the ATUSB in monitor mode sees it) but I get absolutely > > nothing on the receiver side. > > > > The logic is: > > > > coord0 coord1 > > association req -> > > <- ack > > <- association response > > ack -> > > > > The first ack is sent by coord1 but coord0 never sees anything. In > > practice coord0 has sent an association request and received a single > > one-byte packet in return which I guess is the firmware saying "okay, Tx > > has been performed". Shall I interpret this byte differently? Does it > > mean that the ack has also been received? > > I think I now have a clearer understanding on how the devices behave. > > I turned the devices into promiscuous mode and could observe that some > frames were considered wrong. Indeed, it looks like the PHYs add the > FCS themselves, while the spec says that the FCS should be provided to > the PHY. Anyway, I dropped the FCS calculations from the different MLME > frames forged and it helped a lot. > This is currently the case because monitor interfaces and AF_PACKET will not have the FCS in the payload. As you already figured out you can't refer 802.15.4 promiscuous mode to mac802154 promiscuous mode, it was a historically growing term as people wanted to have a sniffer device and used a promiscuous term from a datasheet (my guess). Vendors has a different meaning of promiscuous mode as the one from 802.15.4. IFF_PROMISC should be mapped to non-filtered mode which is more equal to a sniffer device. However we need to find solutions which fulfill everybody. > I also kind of "discovered" the concept of hardware address filtering > on atusb which makes me realize that maybe we were not talking about > the same "filtering" until now. > > Associations and disassociations now work properly, I'm glad I fixed > "everything". I still need to figure out if using the promiscuous mode > everywhere is really useful or not (maybe the hardware filters were > disabled in this mode and it made it work). However, using the > promiscuous mode was the only way I had to receive acknowledgements, > otherwise they were filtered out by the hardware (the monitor was > showing that the ack frames were actually being sent). > This is correct, the most hardware will turn off automatic ackknowledge handling if address filtering is off (I am sure I said that before). We cannot handle acks on mac802154 if they are time critical. > Finally, changing the channel was also a piece of the puzzle, because I > think some of my smart light bulbs tried to say hello and it kind of > disturbed me :) > > > I could not find a documentation of the firmware interface, I went > > through the wiki but I did not find something clear about what to > > expect or "what the driver should do". But perhaps this will ring a > > bell on your side? > > > > [...] > > > > > I did not see the v2 until now. Sorry for that. > > > > Ah! Ok, no problem :) > > > > > > > > However I think there are missing bits here at the receive handling > > > side. Which are: > > > > > > 1. Do a stop_tx(), stop_rx(), start_rx(filtering_level) to go into > > > other filtering modes while ifup. > > > > Who is supposed to change the filtering level? > > > > For now there is only the promiscuous mode being applied and the user > > has no knowledge about it, it's just something internal. > > > > Changing how the promiscuous mode is applied (using a filtering level > > instead of a "promiscuous on" boolean) would impact all the drivers > > and for now we don't really need it. > > > > > I don't want to see all filtering modes here, just what we currently > > > support with NONE (then with FCS check on software if necessary), > > > ?THIRD/FOURTH? LEVEL filtering and that's it. What I don't want to see > > > is runtime changes of phy flags. To tell the receive path what to > > > filter and what's not. > > > > Runtime changes on a dedicated "filtering" PHY flag is what I've used > > and it works okay for this situation, why don't you want that? It > > avoids the need for (yet) another rework of the API with the drivers, > > no? > > > > > 2. set the pan coordinator bit for hw address filter. And there is a > > > TODO about setting pkt_type in mac802154 receive path which we should > > > take a look into. This bit should be addressed for coordinator support > > > even if there is the question about coordinator vs pan coordinator, > > > then the kernel needs a bit as coordinator iface type parameter to > > > know if it's a pan coordinator and not coordinator. > > > > This is not really something that we can "set". Either the device > > had performed an association and it is a child device: it is not the > > PAN coordinator, or it initiated the PAN and it is the PAN coordinator. > > There are commands to change that later on but those are not supported. > > > > The "PAN coordinator" information is being added in the association > > series (which comes after the scan). I have handled the pkt_type you are > > mentioning. > > > > > I think it makes total sense to split this work in transmit handling, > > > where we had no support at all to send something besides the usual > > > data path, and receive handling, where we have no way to change the > > > filtering level besides interface type and ifup time of an interface. > > > We are currently trying to make a receive path working in a way that > > > "the other ideas flying around which are good" can be introduced in > > > future. > > > If this is done, then take care about how to add the rest of it. > > > > > > I will look into v2 the next few days. > > If possible, I would really like to understand what you expect in terms > of filtering. Maybe as well a short snippet of code showing what kind > of interface you have in mind. Are we talking about a rework of the > promiscuous callback? Are we talking about the hardware filters? What I try to do that over the weekend. Monday is a holiday here. > are the inputs and outputs for these callbacks? What do we expect from > the drivers in terms of advertising? I will be glad to make the > relevant changes once I understand what is needed because on this topic > I have a clear lack of experience, so I will try to judge what is > reachable based on your inputs. ok. Thanks. - Alex
Hi, On Thu, Sep 1, 2022 at 9:09 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hello, > > miquel.raynal@bootlin.com wrote on Thu, 1 Sep 2022 02:09:18 +0200: > > > Hello again, > > > > miquel.raynal@bootlin.com wrote on Wed, 31 Aug 2022 17:39:03 +0200: > > > > > Hi Alexander & Stefan, > > > > > > aahringo@redhat.com wrote on Mon, 29 Aug 2022 22:23:09 -0400: > > > > > > I am currently testing my code with the ATUSB devices, the association > > > works, so it's a good news! However I am struggling to get the > > > association working for a simple reason: the crafted ACKs are > > > transmitted (the ATUSB in monitor mode sees it) but I get absolutely > > > nothing on the receiver side. > > > > > > The logic is: > > > > > > coord0 coord1 > > > association req -> > > > <- ack > > > <- association response > > > ack -> > > > > > > The first ack is sent by coord1 but coord0 never sees anything. In > > > practice coord0 has sent an association request and received a single > > > one-byte packet in return which I guess is the firmware saying "okay, Tx > > > has been performed". Shall I interpret this byte differently? Does it > > > mean that the ack has also been received? > > > > I think I now have a clearer understanding on how the devices behave. > > > > I turned the devices into promiscuous mode and could observe that some > > frames were considered wrong. Indeed, it looks like the PHYs add the > > FCS themselves, while the spec says that the FCS should be provided to > > the PHY. Anyway, I dropped the FCS calculations from the different MLME > > frames forged and it helped a lot. > > > > I also kind of "discovered" the concept of hardware address filtering > > on atusb which makes me realize that maybe we were not talking about > > the same "filtering" until now. > > > > Associations and disassociations now work properly, I'm glad I fixed > > "everything". I still need to figure out if using the promiscuous mode > > everywhere is really useful or not (maybe the hardware filters were > > disabled in this mode and it made it work). However, using the > > promiscuous mode was the only way I had to receive acknowledgements, > > otherwise they were filtered out by the hardware (the monitor was > > showing that the ack frames were actually being sent). > > > > Finally, changing the channel was also a piece of the puzzle, because I > > think some of my smart light bulbs tried to say hello and it kind of > > disturbed me :) > > I tried to scan my ATUSB devices from a Zephyr based Arduino Nano > BLE but for now it does not work, the ATUSB devices receive the scan > requests from Zephyr and send their beacons, the ATUSB monitor shows > the beacons on Wireshark but the ieee80154_recv() callback is never > triggered on Zephyr side. I am new to this OS so if you have any idea > or debugging tips, I would be glad to hear them. > > > > I could not find a documentation of the firmware interface, I went > > > through the wiki but I did not find something clear about what to > > > expect or "what the driver should do". But perhaps this will ring a > > > bell on your side? > > > > > > [...] > > > > > > > I did not see the v2 until now. Sorry for that. > > > > > > Ah! Ok, no problem :) > > > > > > > > > > > However I think there are missing bits here at the receive handling > > > > side. Which are: > > > > > > > > 1. Do a stop_tx(), stop_rx(), start_rx(filtering_level) to go into > > > > other filtering modes while ifup. > > > > > > Who is supposed to change the filtering level? > > > > > > For now there is only the promiscuous mode being applied and the user > > > has no knowledge about it, it's just something internal. > > > > > > Changing how the promiscuous mode is applied (using a filtering level > > > instead of a "promiscuous on" boolean) would impact all the drivers > > > and for now we don't really need it. > > > > > > > I don't want to see all filtering modes here, just what we currently > > > > support with NONE (then with FCS check on software if necessary), > > > > ?THIRD/FOURTH? LEVEL filtering and that's it. What I don't want to see > > > > is runtime changes of phy flags. To tell the receive path what to > > > > filter and what's not. > > > > > > Runtime changes on a dedicated "filtering" PHY flag is what I've used > > > and it works okay for this situation, why don't you want that? It > > > avoids the need for (yet) another rework of the API with the drivers, > > > no? > > > > > > > 2. set the pan coordinator bit for hw address filter. And there is a > > > > TODO about setting pkt_type in mac802154 receive path which we should > > > > take a look into. This bit should be addressed for coordinator support > > > > even if there is the question about coordinator vs pan coordinator, > > > > then the kernel needs a bit as coordinator iface type parameter to > > > > know if it's a pan coordinator and not coordinator. > > > > > > This is not really something that we can "set". Either the device > > > had performed an association and it is a child device: it is not the > > > PAN coordinator, or it initiated the PAN and it is the PAN coordinator. > > > There are commands to change that later on but those are not supported. > > > > > > The "PAN coordinator" information is being added in the association > > > series (which comes after the scan). I have handled the pkt_type you are > > > mentioning. > > > > > > > I think it makes total sense to split this work in transmit handling, > > > > where we had no support at all to send something besides the usual > > > > data path, and receive handling, where we have no way to change the > > > > filtering level besides interface type and ifup time of an interface. > > > > We are currently trying to make a receive path working in a way that > > > > "the other ideas flying around which are good" can be introduced in > > > > future. > > > > If this is done, then take care about how to add the rest of it. > > > > > > > > I will look into v2 the next few days. > > > > If possible, I would really like to understand what you expect in terms > > of filtering. Maybe as well a short snippet of code showing what kind > > of interface you have in mind. Are we talking about a rework of the > > promiscuous callback? Are we talking about the hardware filters? What > > are the inputs and outputs for these callbacks? What do we expect from > > the drivers in terms of advertising? I will be glad to make the > > relevant changes once I understand what is needed because on this topic > > I have a clear lack of experience, so I will try to judge what is > > reachable based on your inputs. > I am sorry, I never looked into Zephyr for reasons... Do they not have something like /proc/interrupts look if you see a counter for your 802.15.4 transceiver? > Also, can you please clarify when are we talking about software and > when about hardware filters. > Hardware filter is currently e.g. promiscuous mode on or off setting. Software filtering is depending which receive path the frame is going and which hardware filter is present which then acts like actually with hardware filtering. I am not sure if this answers this question? - Alex
Hi, On Thu, Sep 1, 2022 at 10:23 PM Alexander Aring <aahringo@redhat.com> wrote: > > Hi, > > On Wed, Aug 31, 2022 at 8:09 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hello again, > > > > miquel.raynal@bootlin.com wrote on Wed, 31 Aug 2022 17:39:03 +0200: > > > > > Hi Alexander & Stefan, > > > > > > aahringo@redhat.com wrote on Mon, 29 Aug 2022 22:23:09 -0400: > > > > > > I am currently testing my code with the ATUSB devices, the association > > > works, so it's a good news! However I am struggling to get the > > > association working for a simple reason: the crafted ACKs are > > > transmitted (the ATUSB in monitor mode sees it) but I get absolutely > > > nothing on the receiver side. > > > > > > The logic is: > > > > > > coord0 coord1 > > > association req -> > > > <- ack > > > <- association response > > > ack -> > > > > > > The first ack is sent by coord1 but coord0 never sees anything. In > > > practice coord0 has sent an association request and received a single > > > one-byte packet in return which I guess is the firmware saying "okay, Tx > > > has been performed". Shall I interpret this byte differently? Does it > > > mean that the ack has also been received? > > > > I think I now have a clearer understanding on how the devices behave. > > > > I turned the devices into promiscuous mode and could observe that some > > frames were considered wrong. Indeed, it looks like the PHYs add the > > FCS themselves, while the spec says that the FCS should be provided to > > the PHY. Anyway, I dropped the FCS calculations from the different MLME > > frames forged and it helped a lot. > > > > This is currently the case because monitor interfaces and AF_PACKET > will not have the FCS in the payload. As you already figured out you > can't refer 802.15.4 promiscuous mode to mac802154 promiscuous mode, > it was a historically growing term as people wanted to have a sniffer > device and used a promiscuous term from a datasheet (my guess). > Vendors has a different meaning of promiscuous mode as the one from > 802.15.4. IFF_PROMISC should be mapped to non-filtered mode which is > more equal to a sniffer device. However we need to find solutions > which fulfill everybody. > > > I also kind of "discovered" the concept of hardware address filtering > > on atusb which makes me realize that maybe we were not talking about > > the same "filtering" until now. > > > > Associations and disassociations now work properly, I'm glad I fixed > > "everything". I still need to figure out if using the promiscuous mode > > everywhere is really useful or not (maybe the hardware filters were > > disabled in this mode and it made it work). However, using the > > promiscuous mode was the only way I had to receive acknowledgements, > > otherwise they were filtered out by the hardware (the monitor was > > showing that the ack frames were actually being sent). > > > > This is correct, the most hardware will turn off automatic > ackknowledge handling if address filtering is off (I am sure I said > that before). We cannot handle acks on mac802154 if they are time > critical. > If this is required we should discuss this topic... - Alex
Hi Alexander, aahringo@redhat.com wrote on Thu, 1 Sep 2022 22:38:12 -0400: > Hi, > > On Thu, Sep 1, 2022 at 9:09 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hello, > > > > miquel.raynal@bootlin.com wrote on Thu, 1 Sep 2022 02:09:18 +0200: > > > > > Hello again, > > > > > > miquel.raynal@bootlin.com wrote on Wed, 31 Aug 2022 17:39:03 +0200: > > > > > > > Hi Alexander & Stefan, > > > > > > > > aahringo@redhat.com wrote on Mon, 29 Aug 2022 22:23:09 -0400: > > > > > > > > I am currently testing my code with the ATUSB devices, the association > > > > works, so it's a good news! However I am struggling to get the > > > > association working for a simple reason: the crafted ACKs are > > > > transmitted (the ATUSB in monitor mode sees it) but I get absolutely > > > > nothing on the receiver side. > > > > > > > > The logic is: > > > > > > > > coord0 coord1 > > > > association req -> > > > > <- ack > > > > <- association response > > > > ack -> > > > > > > > > The first ack is sent by coord1 but coord0 never sees anything. In > > > > practice coord0 has sent an association request and received a single > > > > one-byte packet in return which I guess is the firmware saying "okay, Tx > > > > has been performed". Shall I interpret this byte differently? Does it > > > > mean that the ack has also been received? > > > > > > I think I now have a clearer understanding on how the devices behave. > > > > > > I turned the devices into promiscuous mode and could observe that some > > > frames were considered wrong. Indeed, it looks like the PHYs add the > > > FCS themselves, while the spec says that the FCS should be provided to > > > the PHY. Anyway, I dropped the FCS calculations from the different MLME > > > frames forged and it helped a lot. > > > > > > I also kind of "discovered" the concept of hardware address filtering > > > on atusb which makes me realize that maybe we were not talking about > > > the same "filtering" until now. > > > > > > Associations and disassociations now work properly, I'm glad I fixed > > > "everything". I still need to figure out if using the promiscuous mode > > > everywhere is really useful or not (maybe the hardware filters were > > > disabled in this mode and it made it work). However, using the > > > promiscuous mode was the only way I had to receive acknowledgements, > > > otherwise they were filtered out by the hardware (the monitor was > > > showing that the ack frames were actually being sent). > > > > > > Finally, changing the channel was also a piece of the puzzle, because I > > > think some of my smart light bulbs tried to say hello and it kind of > > > disturbed me :) > > > > I tried to scan my ATUSB devices from a Zephyr based Arduino Nano > > BLE but for now it does not work, the ATUSB devices receive the scan > > requests from Zephyr and send their beacons, the ATUSB monitor shows > > the beacons on Wireshark but the ieee80154_recv() callback is never > > triggered on Zephyr side. I am new to this OS so if you have any idea > > or debugging tips, I would be glad to hear them. > > > > > > I could not find a documentation of the firmware interface, I went > > > > through the wiki but I did not find something clear about what to > > > > expect or "what the driver should do". But perhaps this will ring a > > > > bell on your side? > > > > > > > > [...] > > > > > > > > > I did not see the v2 until now. Sorry for that. > > > > > > > > Ah! Ok, no problem :) > > > > > > > > > > > > > > However I think there are missing bits here at the receive handling > > > > > side. Which are: > > > > > > > > > > 1. Do a stop_tx(), stop_rx(), start_rx(filtering_level) to go into > > > > > other filtering modes while ifup. > > > > > > > > Who is supposed to change the filtering level? > > > > > > > > For now there is only the promiscuous mode being applied and the user > > > > has no knowledge about it, it's just something internal. > > > > > > > > Changing how the promiscuous mode is applied (using a filtering level > > > > instead of a "promiscuous on" boolean) would impact all the drivers > > > > and for now we don't really need it. > > > > > > > > > I don't want to see all filtering modes here, just what we currently > > > > > support with NONE (then with FCS check on software if necessary), > > > > > ?THIRD/FOURTH? LEVEL filtering and that's it. What I don't want to see > > > > > is runtime changes of phy flags. To tell the receive path what to > > > > > filter and what's not. > > > > > > > > Runtime changes on a dedicated "filtering" PHY flag is what I've used > > > > and it works okay for this situation, why don't you want that? It > > > > avoids the need for (yet) another rework of the API with the drivers, > > > > no? > > > > > > > > > 2. set the pan coordinator bit for hw address filter. And there is a > > > > > TODO about setting pkt_type in mac802154 receive path which we should > > > > > take a look into. This bit should be addressed for coordinator support > > > > > even if there is the question about coordinator vs pan coordinator, > > > > > then the kernel needs a bit as coordinator iface type parameter to > > > > > know if it's a pan coordinator and not coordinator. > > > > > > > > This is not really something that we can "set". Either the device > > > > had performed an association and it is a child device: it is not the > > > > PAN coordinator, or it initiated the PAN and it is the PAN coordinator. > > > > There are commands to change that later on but those are not supported. > > > > > > > > The "PAN coordinator" information is being added in the association > > > > series (which comes after the scan). I have handled the pkt_type you are > > > > mentioning. > > > > > > > > > I think it makes total sense to split this work in transmit handling, > > > > > where we had no support at all to send something besides the usual > > > > > data path, and receive handling, where we have no way to change the > > > > > filtering level besides interface type and ifup time of an interface. > > > > > We are currently trying to make a receive path working in a way that > > > > > "the other ideas flying around which are good" can be introduced in > > > > > future. > > > > > If this is done, then take care about how to add the rest of it. > > > > > > > > > > I will look into v2 the next few days. > > > > > > If possible, I would really like to understand what you expect in terms > > > of filtering. Maybe as well a short snippet of code showing what kind > > > of interface you have in mind. Are we talking about a rework of the > > > promiscuous callback? Are we talking about the hardware filters? What > > > are the inputs and outputs for these callbacks? What do we expect from > > > the drivers in terms of advertising? I will be glad to make the > > > relevant changes once I understand what is needed because on this topic > > > I have a clear lack of experience, so I will try to judge what is > > > reachable based on your inputs. > > > > I am sorry, I never looked into Zephyr for reasons... Do they not have > something like /proc/interrupts look if you see a counter for your > 802.15.4 transceiver? > > > Also, can you please clarify when are we talking about software and > > when about hardware filters. > > > > Hardware filter is currently e.g. promiscuous mode on or off setting. > Software filtering is depending which receive path the frame is going > and which hardware filter is present which then acts like actually > with hardware filtering. > I am not sure if this answers this question? I think my understand gets clearer now that I've digged into Zephyr's ieee802154 layer and in the at86rf230 datasheet. I will answer the previous e-mail but just for not I wanted to add that I managed to get Zephyr working, I had to mess around in the code a little bit and actually I discovered a net command which is necessary to use in order to turn the iface up, whatever. So I was playing with the atusb devices and I _think_ I've found a firmware bug or a hardware bug which is going to be problematic. In iface.c, when creating the interface, if you set the hardware filters (set_panid/short/ext_addr()) there is no way you will be able to get a fully transparent promiscuous mode. I am not saying that the whole promiscuous mode does not work anymore, I don't really know. What I was interested in were the acks, and getting them is a real pain. At least, enabling the promiscuous mode after setting the hw filters will lead to the acks being dropped immediately while if the promiscuous mode is enabled first (like on monitor interfaces) the acks are correctly forwarded by the PHY. While looking at the history of the drivers, I realized that the TX_ARET mode was not supported by the firmware in 2015 (that's what you say in a commit) I have seen no further updates about it so I guess it's still not available. I don't see any other way to know if a frame's ack has been received or not reliably. Do you think I can just ignore the acks during an association in mac802154? Another idea how to get them? The Atmel datasheet states the following, which is not encouraging: If (Destination Addressing Mode = 0 OR 1) AND (Source Addressing Mode = 0) no IRQ_5 (AMI) is generated, refer to Section 8.1.2.2 “Frame Control Field (FCF)” on page 80. This effectively causes all acknowledgement frames not to be announced, which otherwise always pass the fil- ter, regardless of whether they are intended for this device or not. Thanks, Miquèl
Hi, On Fri, Sep 2, 2022 at 8:08 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: ... > > > > I am sorry, I never looked into Zephyr for reasons... Do they not have > > something like /proc/interrupts look if you see a counter for your > > 802.15.4 transceiver? > > > > > Also, can you please clarify when are we talking about software and > > > when about hardware filters. > > > > > > > Hardware filter is currently e.g. promiscuous mode on or off setting. > > Software filtering is depending which receive path the frame is going > > and which hardware filter is present which then acts like actually > > with hardware filtering. > > I am not sure if this answers this question? > > I think my understand gets clearer now that I've digged into Zephyr's > ieee802154 layer and in the at86rf230 datasheet. > okay, I think for zephyr questions you are here on the wrong mailinglist. > I will answer the previous e-mail but just for not I wanted to add that > I managed to get Zephyr working, I had to mess around in the code a > little bit and actually I discovered a net command which is necessary > to use in order to turn the iface up, whatever. > aha. > So I was playing with the atusb devices and I _think_ I've found a > firmware bug or a hardware bug which is going to be problematic. In the firmware is open source, I think it's fine to send patches here (I did it as well once for do a quick hack to port it to rzusb) the atusb is "mostly" at the point that they can do open hardware from the qi-hardware organization. > iface.c, when creating the interface, if you set the hardware filters > (set_panid/short/ext_addr()) there is no way you will be able to get a > fully transparent promiscuous mode. I am not saying that the whole What is a transparent promiscuous mode? > promiscuous mode does not work anymore, I don't really know. What I was > interested in were the acks, and getting them is a real pain. At least, > enabling the promiscuous mode after setting the hw filters will lead to > the acks being dropped immediately while if the promiscuous mode is > enabled first (like on monitor interfaces) the acks are correctly > forwarded by the PHY. If we would not disable AACK handling (means we receive a frame with ack requested bit set we send a ack back) we would ack every frame it receives (speaking on at86rf233). > > While looking at the history of the drivers, I realized that the > TX_ARET mode was not supported by the firmware in 2015 (that's what you There exists ARET and AACK, both are mac mechanisms which must be offloaded on the hardware. Note that those only do "something" if the ack request bit in the frame is set. ARET will retransmit if no ack is received after some while, etc. mostly coupled with CSMA/CA handling. We cannot guarantee such timings on the Linux layer. btw: mac80211 can also not handle acks on the software layer, it must be offloaded. AACK will send a back if a frame with ack request bit was received. > say in a commit) I have seen no further updates about it so I guess > it's still not available. I don't see any other way to know if a > frame's ack has been received or not reliably. You implemented it for the at86rf230 driver (the spi one which is what also atusb uses). You implemented the ctx->trac = IEEE802154_NO_ACK; which signals the upper layer that if the ack request bit is set, that there was no ack. But yea, there is a missing feature for atusb yet which requires firmware changes as well. Btw: I can imagine that hwsim "fakes" such offload behaviours. > > Do you think I can just ignore the acks during an association in > mac802154? No, even we should WARN_ON ack frames in states we don't expect them because they must be offloaded on hardware. I am not sure if I am following what is wrong with the trac register and NO_ACK, this is the information if we got an ack or not. Do you need to turn off address filters while "an association"? Another idea how to get them? The Atmel datasheet states the > following, which is not encouraging: > > If (Destination Addressing Mode = 0 OR 1) AND (Source > Addressing Mode = 0) no IRQ_5 (AMI) is generated, refer to > Section 8.1.2.2 “Frame Control Field (FCF)” on page 80. This > effectively causes all acknowledgement frames not to be > announced, which otherwise always pass the fil- ter, regardless > of whether they are intended for this device or not. I hope the answers above are helpful because I don't know how this can be useful here. - Alex
Hi, On Sat, Sep 3, 2022 at 10:20 AM Alexander Aring <aahringo@redhat.com> wrote: > > Hi, > > On Fri, Sep 2, 2022 at 8:08 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > ... > > > > > > I am sorry, I never looked into Zephyr for reasons... Do they not have > > > something like /proc/interrupts look if you see a counter for your > > > 802.15.4 transceiver? > > > > > > > Also, can you please clarify when are we talking about software and > > > > when about hardware filters. > > > > > > > > > > Hardware filter is currently e.g. promiscuous mode on or off setting. > > > Software filtering is depending which receive path the frame is going > > > and which hardware filter is present which then acts like actually > > > with hardware filtering. > > > I am not sure if this answers this question? > > > > I think my understand gets clearer now that I've digged into Zephyr's > > ieee802154 layer and in the at86rf230 datasheet. > > > > okay, I think for zephyr questions you are here on the wrong mailinglist. > > > I will answer the previous e-mail but just for not I wanted to add that > > I managed to get Zephyr working, I had to mess around in the code a > > little bit and actually I discovered a net command which is necessary > > to use in order to turn the iface up, whatever. > > > > aha. > > > So I was playing with the atusb devices and I _think_ I've found a > > firmware bug or a hardware bug which is going to be problematic. In > > the firmware is open source, I think it's fine to send patches here (I > did it as well once for do a quick hack to port it to rzusb) the atusb > is "mostly" at the point that they can do open hardware from the > qi-hardware organization. > > > iface.c, when creating the interface, if you set the hardware filters > > (set_panid/short/ext_addr()) there is no way you will be able to get a > > fully transparent promiscuous mode. I am not saying that the whole > > What is a transparent promiscuous mode? > > > promiscuous mode does not work anymore, I don't really know. What I was > > interested in were the acks, and getting them is a real pain. At least, > > enabling the promiscuous mode after setting the hw filters will lead to > > the acks being dropped immediately while if the promiscuous mode is > > enabled first (like on monitor interfaces) the acks are correctly > > forwarded by the PHY. > > If we would not disable AACK handling (means we receive a frame with > ack requested bit set we send a ack back) we would ack every frame it > receives (speaking on at86rf233). > > > > > While looking at the history of the drivers, I realized that the > > TX_ARET mode was not supported by the firmware in 2015 (that's what you > > There exists ARET and AACK, both are mac mechanisms which must be > offloaded on the hardware. Note that those only do "something" if the > ack request bit in the frame is set. > > ARET will retransmit if no ack is received after some while, etc. > mostly coupled with CSMA/CA handling. We cannot guarantee such timings > on the Linux layer. btw: mac80211 can also not handle acks on the > software layer, it must be offloaded. > > AACK will send a back if a frame with ack request bit was received. will send an ack back* - Alex
Hi Alexander, aahringo@redhat.com wrote on Sat, 3 Sep 2022 10:20:24 -0400: > Hi, > > On Fri, Sep 2, 2022 at 8:08 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > ... > > > > > > I am sorry, I never looked into Zephyr for reasons... Do they not have > > > something like /proc/interrupts look if you see a counter for your > > > 802.15.4 transceiver? > > > > > > > Also, can you please clarify when are we talking about software and > > > > when about hardware filters. > > > > > > > > > > Hardware filter is currently e.g. promiscuous mode on or off setting. > > > Software filtering is depending which receive path the frame is going > > > and which hardware filter is present which then acts like actually > > > with hardware filtering. > > > I am not sure if this answers this question? > > > > I think my understand gets clearer now that I've digged into Zephyr's > > ieee802154 layer and in the at86rf230 datasheet. > > > > okay, I think for zephyr questions you are here on the wrong mailinglist. > > > I will answer the previous e-mail but just for not I wanted to add that > > I managed to get Zephyr working, I had to mess around in the code a > > little bit and actually I discovered a net command which is necessary > > to use in order to turn the iface up, whatever. > > > > aha. > > > So I was playing with the atusb devices and I _think_ I've found a > > firmware bug or a hardware bug which is going to be problematic. In > > the firmware is open source, I think it's fine to send patches here (I > did it as well once for do a quick hack to port it to rzusb) the atusb > is "mostly" at the point that they can do open hardware from the > qi-hardware organization. > > > iface.c, when creating the interface, if you set the hardware filters > > (set_panid/short/ext_addr()) there is no way you will be able to get a > > fully transparent promiscuous mode. I am not saying that the whole > > What is a transparent promiscuous mode? I observe something very weird: A/ If at start up time we set promisc_mode(true) and then we set the hw address filters, all the frames are forwarded to the MAC. B/ If at start up time we set the hw address filters and then set promisc_mode(true), there is some filtering happening (like the Acks which are dropped by the PHY. I need to investigate this further because I don't get why in case B we don't have the same behavior than in case A. > > promiscuous mode does not work anymore, I don't really know. What I was > > interested in were the acks, and getting them is a real pain. At least, > > enabling the promiscuous mode after setting the hw filters will lead to > > the acks being dropped immediately while if the promiscuous mode is > > enabled first (like on monitor interfaces) the acks are correctly > > forwarded by the PHY. > > If we would not disable AACK handling (means we receive a frame with > ack requested bit set we send a ack back) we would ack every frame it > receives (speaking on at86rf233). Yes, but when sending MAC frames I would like to: - be in promiscuous mode in Rx (tx paused) in order for the MAC to be aware of the acks being received (unless there is another way to do that, see below) - still ack the received frames automatically Unless we decide that we must only ack the expected frames manually? > > While looking at the history of the drivers, I realized that the > > TX_ARET mode was not supported by the firmware in 2015 (that's what you > > There exists ARET and AACK, both are mac mechanisms which must be > offloaded on the hardware. Note that those only do "something" if the > ack request bit in the frame is set. Absolutely (for the record, that's also an issue I had with Zephyr, I had to use the shell to explicitly ask the AR bit to be set in the outgoing frames, even though this in most MAC frames this is not a user choice, it's expected by the spec). > ARET will retransmit if no ack is received after some while, etc. > mostly coupled with CSMA/CA handling. We cannot guarantee such timings > on the Linux layer. btw: mac80211 can also not handle acks on the > software layer, it must be offloaded. On the Tx side, when sending eg. an association request or an association response, I must expect and wait for an ack. This is what I am struggling to do. How can I know that a frame which I just transmitted has been acked? Bonus points, how can I do that in such a way that it will work with other devices? (hints below) > AACK will send a back if a frame with ack request bit was received. > > > say in a commit) I have seen no further updates about it so I guess > > it's still not available. I don't see any other way to know if a > > frame's ack has been received or not reliably. > > You implemented it for the at86rf230 driver (the spi one which is what > also atusb uses). You implemented the > > ctx->trac = IEEE802154_NO_ACK; > > which signals the upper layer that if the ack request bit is set, that > there was no ack. > > But yea, there is a missing feature for atusb yet which requires > firmware changes as well. :'( Let's say I don't have the time to update the firmware ;). I also assume that other transceivers (or even the drivers) might be limited on this regard as well. How should I handle those "I should wait for the ack to be received" situation while trying to associate? The tricky case is the device receiving the ASSOC_REQ: - the request is received - an ack must be sent (this is okay in most cases I guess) - the device must send an association response (also ok) - and wait for the response to be acked... * either I use the promisc mode when sending the response (because of possible race conditions) and I expect the ack to be forwarded to the MAC -> This does not work on atusb, enabling promiscuous mode after the init does not turn the PHY into promiscuous mode as expected (discussed above) * or I don't turn the PHY in promiscuous mode and I expect it to return a clear status about if the ACK was received -> But this seem to be unsupported with the current ATUSB firmware, I fear other devices could have similar limitations * or I just assume the acks are received blindly -> Not sure this is robust enough? What is your "less worse" choice? > Btw: I can imagine that hwsim "fakes" such > offload behaviours. My current implementation actually did handle all the acks (waiting for them and sending them) from the MAC. I'm currently migrating the ack sending part to the hw. For the reception, that's the big question. > > Do you think I can just ignore the acks during an association in > > mac802154? > > No, even we should WARN_ON ack frames in states we don't expect them > because they must be offloaded on hardware. > > I am not sure if I am following what is wrong with the trac register > and NO_ACK, this is the information if we got an ack or not. Do you > need to turn off address filters while "an association"? If we have access to the TRAC register, I believe we no longer need to turn off address filters. > Another idea how to get them? The Atmel datasheet states the > > following, which is not encouraging: > > > > If (Destination Addressing Mode = 0 OR 1) AND (Source > > Addressing Mode = 0) no IRQ_5 (AMI) is generated, refer to > > Section 8.1.2.2 “Frame Control Field (FCF)” on page 80. This > > effectively causes all acknowledgement frames not to be > > announced, which otherwise always pass the fil- ter, regardless > > of whether they are intended for this device or not. > > I hope the answers above are helpful because I don't know how this can > be useful here. > > - Alex > Thanks, Miquèl
Hi, On Sat, Sep 3, 2022 at 12:06 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > aahringo@redhat.com wrote on Sat, 3 Sep 2022 10:20:24 -0400: > > > Hi, > > > > On Fri, Sep 2, 2022 at 8:08 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > ... > > > > > > > > I am sorry, I never looked into Zephyr for reasons... Do they not have > > > > something like /proc/interrupts look if you see a counter for your > > > > 802.15.4 transceiver? > > > > > > > > > Also, can you please clarify when are we talking about software and > > > > > when about hardware filters. > > > > > > > > > > > > > Hardware filter is currently e.g. promiscuous mode on or off setting. > > > > Software filtering is depending which receive path the frame is going > > > > and which hardware filter is present which then acts like actually > > > > with hardware filtering. > > > > I am not sure if this answers this question? > > > > > > I think my understand gets clearer now that I've digged into Zephyr's > > > ieee802154 layer and in the at86rf230 datasheet. > > > > > > > okay, I think for zephyr questions you are here on the wrong mailinglist. > > > > > I will answer the previous e-mail but just for not I wanted to add that > > > I managed to get Zephyr working, I had to mess around in the code a > > > little bit and actually I discovered a net command which is necessary > > > to use in order to turn the iface up, whatever. > > > > > > > aha. > > > > > So I was playing with the atusb devices and I _think_ I've found a > > > firmware bug or a hardware bug which is going to be problematic. In > > > > the firmware is open source, I think it's fine to send patches here (I > > did it as well once for do a quick hack to port it to rzusb) the atusb > > is "mostly" at the point that they can do open hardware from the > > qi-hardware organization. > > > > > iface.c, when creating the interface, if you set the hardware filters > > > (set_panid/short/ext_addr()) there is no way you will be able to get a > > > fully transparent promiscuous mode. I am not saying that the whole > > > > What is a transparent promiscuous mode? > > I observe something very weird: > > A/ If at start up time we set promisc_mode(true) and then we set the hw > address filters, all the frames are forwarded to the MAC. > > B/ If at start up time we set the hw address filters and then set > promisc_mode(true), there is some filtering happening (like the Acks > which are dropped by the PHY. > > I need to investigate this further because I don't get why in case B we > don't have the same behavior than in case A. > Looking in the datasheet I see only set address filters -> then setting promiscuous mode is specified? Not the other way around... > > > promiscuous mode does not work anymore, I don't really know. What I was > > > interested in were the acks, and getting them is a real pain. At least, > > > enabling the promiscuous mode after setting the hw filters will lead to > > > the acks being dropped immediately while if the promiscuous mode is > > > enabled first (like on monitor interfaces) the acks are correctly > > > forwarded by the PHY. > > > > If we would not disable AACK handling (means we receive a frame with > > ack requested bit set we send a ack back) we would ack every frame it > > receives (speaking on at86rf233). > > Yes, but when sending MAC frames I would like to: > - be in promiscuous mode in Rx (tx paused) in order for the MAC to be > aware of the acks being received (unless there is another way to do > that, see below) > - still ack the received frames automatically > > Unless we decide that we must only ack the expected frames manually? > We can't handle ack frames on mac802154 in my opinion. Or what does manually mean? Is the ack frame required as a mac command operation or as a response to a transmitted frame because the other side will do retransmissions if they don't see an ack back? The second case is not possible to implement on mac802154, it must be offloaded. > > > While looking at the history of the drivers, I realized that the > > > TX_ARET mode was not supported by the firmware in 2015 (that's what you > > > > There exists ARET and AACK, both are mac mechanisms which must be > > offloaded on the hardware. Note that those only do "something" if the > > ack request bit in the frame is set. > > Absolutely (for the record, that's also an issue I had with Zephyr, I > had to use the shell to explicitly ask the AR bit to be set in the > outgoing frames, even though this in most MAC frames this is not a user > choice, it's expected by the spec). > fyi: we have also a default_ackreq behaviour if we set the ack frame on all data frames or not. However it's currently set to not set the ackreq bit because most hardware outside can't handle it (even if required by the spec). If you have a requirement to set ack request bit then do it, if there is hardware outside which cannot handle it, it's their problem. However the dataframes which are sent via user space socket depending on the use case if they want to set it or not but if they set it you need to know your network. > > ARET will retransmit if no ack is received after some while, etc. > > mostly coupled with CSMA/CA handling. We cannot guarantee such timings > > on the Linux layer. btw: mac80211 can also not handle acks on the > > software layer, it must be offloaded. > > On the Tx side, when sending eg. an association request or an > association response, I must expect and wait for an ack. This is > what I am struggling to do. How can I know that a frame which I just > transmitted has been acked? Bonus points, how can I do that in such a > way that it will work with other devices? (hints below) > You can't do this in mac802154 if there is a timing critical requirement here. Is there a timing critical requirement here? > > AACK will send a back if a frame with ack request bit was received. > > > > > say in a commit) I have seen no further updates about it so I guess > > > it's still not available. I don't see any other way to know if a > > > frame's ack has been received or not reliably. > > > > You implemented it for the at86rf230 driver (the spi one which is what > > also atusb uses). You implemented the > > > > ctx->trac = IEEE802154_NO_ACK; > > > > which signals the upper layer that if the ack request bit is set, that > > there was no ack. > > > > But yea, there is a missing feature for atusb yet which requires > > firmware changes as well. > > :'( > > Let's say I don't have the time to update the firmware ;). I also assume > that other transceivers (or even the drivers) might be limited on this > regard as well. How should I handle those "I should wait for the ack to > be received" situation while trying to associate? > If other transceivers cannot handle giving us feedback if ack was received or not and we have the mandatory requirement to know it, it is poor hardware/driver. As I said if the spec requires to check on an ack or not we need to get his information, if the hardware/driver can't deliver this... then just assume an ACK was received as it returns TX_SUCCESS (or whatever the return value was). I said before that some hardware will act weird if they don't support it. > The tricky case is the device receiving the ASSOC_REQ: > - the request is received > - an ack must be sent (this is okay in most cases I guess) > - the device must send an association response (also ok) > - and wait for the response to be acked... > * either I use the promisc mode when sending the response > (because of possible race conditions) and I expect the ack to > be forwarded to the MAC > -> This does not work on atusb, enabling promiscuous > mode after the init does not turn the PHY into > promiscuous mode as expected (discussed above) > * or I don't turn the PHY in promiscuous mode and I expect it > to return a clear status about if the ACK was received > -> But this seem to be unsupported with the current > ATUSB firmware, I fear other devices could have similar > limitations > * or I just assume the acks are received blindly > -> Not sure this is robust enough? > Assume you always get an ack back until somebody implements this feature in their driver (It's already implemented so as they return TX_SUCCESS). We cannot do much more I think... it is not robust but then somebody needs to update the driver/firmware. It's more weird if the otherside does not support AACK, because ARET will send them 3 times (by default) the same frame. That's why we have the policy to not set the ackreq bit if it's not required. > What is your "less worse" choice? > > > Btw: I can imagine that hwsim "fakes" such > > offload behaviours. > > My current implementation actually did handle all the acks (waiting for > them and sending them) from the MAC. I'm currently migrating the ack > sending part to the hw. For the reception, that's the big question. > In my opinion we should never deal with ack frames on mac802154 level, neither on hwsim, this is an offloaded functionality. What I have in mind is to fake a "TX_NO_ACK" return value as a probability parameter to return it sometimes. E.g. as netem and drop rate, etc. Then we could do some testing with it. > > > Do you think I can just ignore the acks during an association in > > > mac802154? > > > > No, even we should WARN_ON ack frames in states we don't expect them > > because they must be offloaded on hardware. > > > > I am not sure if I am following what is wrong with the trac register > > and NO_ACK, this is the information if we got an ack or not. Do you > > need to turn off address filters while "an association"? > > If we have access to the TRAC register, I believe we no longer need to > turn off address filters. > That sounds good. - Alex
Hi, On Sat, Sep 3, 2022 at 2:21 PM Alexander Aring <aahringo@redhat.com> wrote: ... > > Assume you always get an ack back until somebody implements this > feature in their driver (It's already implemented so as they return > TX_SUCCESS). We cannot do much more I think... it is not robust but > then somebody needs to update the driver/firmware. > > It's more weird if the otherside does not support AACK, because ARET > will send them 3 times (by default) the same frame. That's why we have > the policy to not set the ackreq bit if it's not required. > > > What is your "less worse" choice? > > > > > Btw: I can imagine that hwsim "fakes" such > > > offload behaviours. > > > > My current implementation actually did handle all the acks (waiting for > > them and sending them) from the MAC. I'm currently migrating the ack > > sending part to the hw. For the reception, that's the big question. > > > > In my opinion we should never deal with ack frames on mac802154 level, > neither on hwsim, this is an offloaded functionality. What I have in * except of course in cases of monitors, but monitors really aren't part of the network and they are sniffers... A monitor can indeed send frames out, which could cause a lot of trouble in the network if you want to. It's a playground thing to do experiments... Here we never analyze any payload received and forward it directly to the user, it's also a kind of kernel bypass. I know people run some user space stacks with it (including myself) but they get problems until it gets into e.g. ackknowledge handling. - Alex
Hi, On Sat, Sep 3, 2022 at 12:06 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: ... > > On the Tx side, when sending eg. an association request or an > association response, I must expect and wait for an ack. This is > what I am struggling to do. How can I know that a frame which I just > transmitted has been acked? Bonus points, how can I do that in such a > way that it will work with other devices? (hints below) > > > AACK will send a back if a frame with ack request bit was received. > > > > > say in a commit) I have seen no further updates about it so I guess > > > it's still not available. I don't see any other way to know if a > > > frame's ack has been received or not reliably. > > > > You implemented it for the at86rf230 driver (the spi one which is what > > also atusb uses). You implemented the > > > > ctx->trac = IEEE802154_NO_ACK; > > > > which signals the upper layer that if the ack request bit is set, that > > there was no ack. > > > > But yea, there is a missing feature for atusb yet which requires > > firmware changes as well. > > :'( There is a sequence handling in tx done on atusb firmware and I think it should be pretty easy to add a byte for trac status. diff --git a/atusb/fw/mac.c b/atusb/fw/mac.c index 835002c..156bd95 100644 --- a/atusb/fw/mac.c +++ b/atusb/fw/mac.c @@ -116,7 +116,7 @@ static void receive_frame(void) static bool handle_irq(void) { - uint8_t irq; + uint8_t irq, data[2]; irq = reg_read(REG_IRQ_STATUS); if (!(irq & IRQ_TRX_END)) @@ -124,7 +124,15 @@ static bool handle_irq(void) if (txing) { if (eps[1].state == EP_IDLE) { - usb_send(&eps[1], &this_seq, 1, tx_ack_done, NULL); + data[0] = tx_ack_done; + + spi_begin(); + spi_io(REG_TRX_STATE); + + data[1] = spi_recv(); + spi_end(); + + usb_send(&eps[1], &this_seq, sizeof(data), data, NULL); } else { queued_tx_ack = 1; queued_seq = this_seq; Did not test it, but maybe something in this direction? cc Werner Almesberger here, he did most of the atusb hardware and firmware. - Alex
On Sat, Sep 3, 2022 at 3:07 PM Alexander Aring <aahringo@redhat.com> wrote: > > Hi, > > On Sat, Sep 3, 2022 at 12:06 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > ... > > > > On the Tx side, when sending eg. an association request or an > > association response, I must expect and wait for an ack. This is > > what I am struggling to do. How can I know that a frame which I just > > transmitted has been acked? Bonus points, how can I do that in such a > > way that it will work with other devices? (hints below) > > > > > AACK will send a back if a frame with ack request bit was received. > > > > > > > say in a commit) I have seen no further updates about it so I guess > > > > it's still not available. I don't see any other way to know if a > > > > frame's ack has been received or not reliably. > > > > > > You implemented it for the at86rf230 driver (the spi one which is what > > > also atusb uses). You implemented the > > > > > > ctx->trac = IEEE802154_NO_ACK; > > > > > > which signals the upper layer that if the ack request bit is set, that > > > there was no ack. > > > > > > But yea, there is a missing feature for atusb yet which requires > > > firmware changes as well. > > > > :'( > > There is a sequence handling in tx done on atusb firmware and I think > it should be pretty easy to add a byte for trac status. > > diff --git a/atusb/fw/mac.c b/atusb/fw/mac.c > index 835002c..156bd95 100644 > --- a/atusb/fw/mac.c > +++ b/atusb/fw/mac.c > @@ -116,7 +116,7 @@ static void receive_frame(void) > > static bool handle_irq(void) > { > - uint8_t irq; > + uint8_t irq, data[2]; > > irq = reg_read(REG_IRQ_STATUS); > if (!(irq & IRQ_TRX_END)) > @@ -124,7 +124,15 @@ static bool handle_irq(void) > > if (txing) { > if (eps[1].state == EP_IDLE) { > - usb_send(&eps[1], &this_seq, 1, tx_ack_done, NULL); > + data[0] = tx_ack_done; > + > + spi_begin(); > + spi_io(REG_TRX_STATE); > + > + data[1] = spi_recv(); > + spi_end(); data[1] = reg_read(REG_TRX_STATE) as seen above for REG_IRQ_STATUS would be better here... - Alex
Hi, On Sat, Sep 3, 2022 at 3:10 PM Alexander Aring <aahringo@redhat.com> wrote: > > On Sat, Sep 3, 2022 at 3:07 PM Alexander Aring <aahringo@redhat.com> wrote: > > > > Hi, > > > > On Sat, Sep 3, 2022 at 12:06 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > ... > > > > > > On the Tx side, when sending eg. an association request or an > > > association response, I must expect and wait for an ack. This is > > > what I am struggling to do. How can I know that a frame which I just > > > transmitted has been acked? Bonus points, how can I do that in such a > > > way that it will work with other devices? (hints below) > > > > > > > AACK will send a back if a frame with ack request bit was received. > > > > > > > > > say in a commit) I have seen no further updates about it so I guess > > > > > it's still not available. I don't see any other way to know if a > > > > > frame's ack has been received or not reliably. > > > > > > > > You implemented it for the at86rf230 driver (the spi one which is what > > > > also atusb uses). You implemented the > > > > > > > > ctx->trac = IEEE802154_NO_ACK; > > > > > > > > which signals the upper layer that if the ack request bit is set, that > > > > there was no ack. > > > > > > > > But yea, there is a missing feature for atusb yet which requires > > > > firmware changes as well. > > > > > > :'( > > > > There is a sequence handling in tx done on atusb firmware and I think > > it should be pretty easy to add a byte for trac status. > > > > diff --git a/atusb/fw/mac.c b/atusb/fw/mac.c > > index 835002c..156bd95 100644 > > --- a/atusb/fw/mac.c > > +++ b/atusb/fw/mac.c > > @@ -116,7 +116,7 @@ static void receive_frame(void) > > > > static bool handle_irq(void) > > { > > - uint8_t irq; > > + uint8_t irq, data[2]; > > > > irq = reg_read(REG_IRQ_STATUS); > > if (!(irq & IRQ_TRX_END)) > > @@ -124,7 +124,15 @@ static bool handle_irq(void) > > > > if (txing) { > > if (eps[1].state == EP_IDLE) { > > - usb_send(&eps[1], &this_seq, 1, tx_ack_done, NULL); > > + data[0] = tx_ack_done; > > + > > + spi_begin(); > > + spi_io(REG_TRX_STATE); > > + > > + data[1] = spi_recv(); > > + spi_end(); > > data[1] = reg_read(REG_TRX_STATE) as seen above for REG_IRQ_STATUS > would be better here... > after digging the code more, there is another queue case which we should handle, also correct using buffer parameter instead of the callback parameter which was stupid... However I think the direction is clear. Sorry for the spam. diff --git a/atusb/fw/mac.c b/atusb/fw/mac.c index 835002c..b52ba1a 100644 --- a/atusb/fw/mac.c +++ b/atusb/fw/mac.c @@ -32,7 +32,7 @@ static uint8_t tx_buf[MAX_PSDU]; static uint8_t tx_size = 0; static bool txing = 0; static bool queued_tx_ack = 0; -static uint8_t next_seq, this_seq, queued_seq; +static uint8_t next_seq, this_seq, queued_seq, queued_tx_trac; /* ----- Receive buffer management ----------------------------------------- */ @@ -57,6 +57,7 @@ static void tx_ack_done(void *user); static void usb_next(void) { const uint8_t *buf; + uint8_t data[2]; if (rx_in != rx_out) { buf = rx_buf[rx_out]; @@ -65,7 +66,9 @@ static void usb_next(void) } if (queued_tx_ack) { - usb_send(&eps[1], &queued_seq, 1, tx_ack_done, NULL); + data[0] = queued_seq; + data[1] = queued_tx_trac; + usb_send(&eps[1], data, sizeof(data), tx_ack_done, NULL); queued_tx_ack = 0; } } @@ -116,7 +119,7 @@ static void receive_frame(void) static bool handle_irq(void) { - uint8_t irq; + uint8_t irq, data[2]; irq = reg_read(REG_IRQ_STATUS); if (!(irq & IRQ_TRX_END)) @@ -124,10 +127,13 @@ static bool handle_irq(void) if (txing) { if (eps[1].state == EP_IDLE) { - usb_send(&eps[1], &this_seq, 1, tx_ack_done, NULL); + data[0] = this_seq; + data[1] = reg_read(REG_TRX_STATE); + usb_send(&eps[1], data, sizeof(data), tx_ack_done, NULL); } else { queued_tx_ack = 1; queued_seq = this_seq; + queued_tx_trac = reg_read(REG_TRX_STATE); } txing = 0; return 1;
Hi Alexander, aahringo@redhat.com wrote on Sat, 3 Sep 2022 15:40:35 -0400: > Hi, > > On Sat, Sep 3, 2022 at 3:10 PM Alexander Aring <aahringo@redhat.com> wrote: > > > > On Sat, Sep 3, 2022 at 3:07 PM Alexander Aring <aahringo@redhat.com> wrote: > > > > > > Hi, > > > > > > On Sat, Sep 3, 2022 at 12:06 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > ... > > > > > > > > On the Tx side, when sending eg. an association request or an > > > > association response, I must expect and wait for an ack. This is > > > > what I am struggling to do. How can I know that a frame which I just > > > > transmitted has been acked? Bonus points, how can I do that in such a > > > > way that it will work with other devices? (hints below) > > > > > > > > > AACK will send a back if a frame with ack request bit was received. > > > > > > > > > > > say in a commit) I have seen no further updates about it so I guess > > > > > > it's still not available. I don't see any other way to know if a > > > > > > frame's ack has been received or not reliably. > > > > > > > > > > You implemented it for the at86rf230 driver (the spi one which is what > > > > > also atusb uses). You implemented the > > > > > > > > > > ctx->trac = IEEE802154_NO_ACK; > > > > > > > > > > which signals the upper layer that if the ack request bit is set, that > > > > > there was no ack. > > > > > > > > > > But yea, there is a missing feature for atusb yet which requires > > > > > firmware changes as well. > > > > > > > > :'( > > > > > > There is a sequence handling in tx done on atusb firmware and I think > > > it should be pretty easy to add a byte for trac status. > > > > > > diff --git a/atusb/fw/mac.c b/atusb/fw/mac.c > > > index 835002c..156bd95 100644 > > > --- a/atusb/fw/mac.c > > > +++ b/atusb/fw/mac.c > > > @@ -116,7 +116,7 @@ static void receive_frame(void) > > > > > > static bool handle_irq(void) > > > { > > > - uint8_t irq; > > > + uint8_t irq, data[2]; > > > > > > irq = reg_read(REG_IRQ_STATUS); > > > if (!(irq & IRQ_TRX_END)) > > > @@ -124,7 +124,15 @@ static bool handle_irq(void) > > > > > > if (txing) { > > > if (eps[1].state == EP_IDLE) { > > > - usb_send(&eps[1], &this_seq, 1, tx_ack_done, NULL); > > > + data[0] = tx_ack_done; > > > + > > > + spi_begin(); > > > + spi_io(REG_TRX_STATE); > > > + > > > + data[1] = spi_recv(); > > > + spi_end(); > > > > data[1] = reg_read(REG_TRX_STATE) as seen above for REG_IRQ_STATUS > > would be better here... > > > > after digging the code more, there is another queue case which we > should handle, also correct using buffer parameter instead of the > callback parameter which was stupid... However I think the direction > is clear. Sorry for the spam. Don't be, your feedback is just super useful. > diff --git a/atusb/fw/mac.c b/atusb/fw/mac.c > index 835002c..b52ba1a 100644 > --- a/atusb/fw/mac.c > +++ b/atusb/fw/mac.c > @@ -32,7 +32,7 @@ static uint8_t tx_buf[MAX_PSDU]; > static uint8_t tx_size = 0; > static bool txing = 0; > static bool queued_tx_ack = 0; > -static uint8_t next_seq, this_seq, queued_seq; > +static uint8_t next_seq, this_seq, queued_seq, queued_tx_trac; > > > /* ----- Receive buffer management ----------------------------------------- */ > @@ -57,6 +57,7 @@ static void tx_ack_done(void *user); > static void usb_next(void) > { > const uint8_t *buf; > + uint8_t data[2]; > > if (rx_in != rx_out) { > buf = rx_buf[rx_out]; > @@ -65,7 +66,9 @@ static void usb_next(void) > } > > if (queued_tx_ack) { > - usb_send(&eps[1], &queued_seq, 1, tx_ack_done, NULL); > + data[0] = queued_seq; > + data[1] = queued_tx_trac; > + usb_send(&eps[1], data, sizeof(data), tx_ack_done, NULL); > queued_tx_ack = 0; > } > } > @@ -116,7 +119,7 @@ static void receive_frame(void) > > static bool handle_irq(void) > { > - uint8_t irq; > + uint8_t irq, data[2]; I don't know why, but defining data on the stack just does not work. Defining it above with the other static variables is okay. I won't fight more for "today" but if someone has an explanation I am all hears. > irq = reg_read(REG_IRQ_STATUS); > if (!(irq & IRQ_TRX_END)) > @@ -124,10 +127,13 @@ static bool handle_irq(void) > > if (txing) { > if (eps[1].state == EP_IDLE) { > - usb_send(&eps[1], &this_seq, 1, tx_ack_done, NULL); > + data[0] = this_seq; > + data[1] = reg_read(REG_TRX_STATE); > + usb_send(&eps[1], data, sizeof(data), > tx_ack_done, NULL); > } else { > queued_tx_ack = 1; > queued_seq = this_seq; > + queued_tx_trac = reg_read(REG_TRX_STATE); > } > txing = 0; > return 1; > Thanks, Miquèl
Hi, On Sun, Sep 4, 2022 at 11:16 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > aahringo@redhat.com wrote on Sat, 3 Sep 2022 15:40:35 -0400: > > > Hi, > > > > On Sat, Sep 3, 2022 at 3:10 PM Alexander Aring <aahringo@redhat.com> wrote: > > > > > > On Sat, Sep 3, 2022 at 3:07 PM Alexander Aring <aahringo@redhat.com> wrote: > > > > > > > > Hi, > > > > > > > > On Sat, Sep 3, 2022 at 12:06 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > ... > > > > > > > > > > On the Tx side, when sending eg. an association request or an > > > > > association response, I must expect and wait for an ack. This is > > > > > what I am struggling to do. How can I know that a frame which I just > > > > > transmitted has been acked? Bonus points, how can I do that in such a > > > > > way that it will work with other devices? (hints below) > > > > > > > > > > > AACK will send a back if a frame with ack request bit was received. > > > > > > > > > > > > > say in a commit) I have seen no further updates about it so I guess > > > > > > > it's still not available. I don't see any other way to know if a > > > > > > > frame's ack has been received or not reliably. > > > > > > > > > > > > You implemented it for the at86rf230 driver (the spi one which is what > > > > > > also atusb uses). You implemented the > > > > > > > > > > > > ctx->trac = IEEE802154_NO_ACK; > > > > > > > > > > > > which signals the upper layer that if the ack request bit is set, that > > > > > > there was no ack. > > > > > > > > > > > > But yea, there is a missing feature for atusb yet which requires > > > > > > firmware changes as well. > > > > > > > > > > :'( > > > > > > > > There is a sequence handling in tx done on atusb firmware and I think > > > > it should be pretty easy to add a byte for trac status. > > > > > > > > diff --git a/atusb/fw/mac.c b/atusb/fw/mac.c > > > > index 835002c..156bd95 100644 > > > > --- a/atusb/fw/mac.c > > > > +++ b/atusb/fw/mac.c > > > > @@ -116,7 +116,7 @@ static void receive_frame(void) > > > > > > > > static bool handle_irq(void) > > > > { > > > > - uint8_t irq; > > > > + uint8_t irq, data[2]; > > > > > > > > irq = reg_read(REG_IRQ_STATUS); > > > > if (!(irq & IRQ_TRX_END)) > > > > @@ -124,7 +124,15 @@ static bool handle_irq(void) > > > > > > > > if (txing) { > > > > if (eps[1].state == EP_IDLE) { > > > > - usb_send(&eps[1], &this_seq, 1, tx_ack_done, NULL); > > > > + data[0] = tx_ack_done; > > > > + > > > > + spi_begin(); > > > > + spi_io(REG_TRX_STATE); > > > > + > > > > + data[1] = spi_recv(); > > > > + spi_end(); > > > > > > data[1] = reg_read(REG_TRX_STATE) as seen above for REG_IRQ_STATUS > > > would be better here... > > > > > > > after digging the code more, there is another queue case which we > > should handle, also correct using buffer parameter instead of the > > callback parameter which was stupid... However I think the direction > > is clear. Sorry for the spam. > > Don't be, your feedback is just super useful. > > > diff --git a/atusb/fw/mac.c b/atusb/fw/mac.c > > index 835002c..b52ba1a 100644 > > --- a/atusb/fw/mac.c > > +++ b/atusb/fw/mac.c > > @@ -32,7 +32,7 @@ static uint8_t tx_buf[MAX_PSDU]; > > static uint8_t tx_size = 0; > > static bool txing = 0; > > static bool queued_tx_ack = 0; > > -static uint8_t next_seq, this_seq, queued_seq; > > +static uint8_t next_seq, this_seq, queued_seq, queued_tx_trac; > > > > > > /* ----- Receive buffer management ----------------------------------------- */ > > @@ -57,6 +57,7 @@ static void tx_ack_done(void *user); > > static void usb_next(void) > > { > > const uint8_t *buf; > > + uint8_t data[2]; > > > > if (rx_in != rx_out) { > > buf = rx_buf[rx_out]; > > @@ -65,7 +66,9 @@ static void usb_next(void) > > } > > > > if (queued_tx_ack) { > > - usb_send(&eps[1], &queued_seq, 1, tx_ack_done, NULL); > > + data[0] = queued_seq; > > + data[1] = queued_tx_trac; > > + usb_send(&eps[1], data, sizeof(data), tx_ack_done, NULL); This is also broken, see below. > > queued_tx_ack = 0; > > } > > } > > @@ -116,7 +119,7 @@ static void receive_frame(void) > > > > static bool handle_irq(void) > > { > > - uint8_t irq; > > + uint8_t irq, data[2]; > > I don't know why, but defining data on the stack just does not work. > Defining it above with the other static variables is okay. I won't > fight more for "today" but if someone has an explanation I am all hears. I can explain it... following the usb_send() it will end in usb_io() and this is an asynchronous function to use somehow the USB IP core API of the mcu... it's wrong to use a stack variable here because it can be overwritten. I am sorry, I did not keep that in mind... - Alex
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; if (!ieee802154_sdata_running(sdata))
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(-)