Hi,
On Tue, Oct 18, 2022 at 2:36 PM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
>
> As a first strep in introducing proper PAN management and association,
> we need to be able to create coordinator interfaces which might act as
> coordinator or PAN coordinator.
>
> Hence, let's add the minimum support to allow the creation of these
> interfaces. This support will be improved later, in particular regarding
> the filtering.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> ---
> net/mac802154/iface.c | 14 ++++++++------
> net/mac802154/main.c | 2 ++
> net/mac802154/rx.c | 11 +++++++----
> 3 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
> index d9b50884d34e..682249f3369b 100644
> --- a/net/mac802154/iface.c
> +++ b/net/mac802154/iface.c
> @@ -262,13 +262,13 @@ ieee802154_check_concurrent_iface(struct ieee802154_sub_if_data *sdata,
> if (nsdata != sdata && ieee802154_sdata_running(nsdata)) {
> int ret;
>
> - /* TODO currently we don't support multiple node types
> - * we need to run skb_clone at rx path. Check if there
> - * exist really an use case if we need to support
> - * multiple node types at the same time.
> + /* TODO currently we don't support multiple node/coord
> + * types we need to run skb_clone at rx path. Check if
> + * there exist really an use case if we need to support
> + * multiple node/coord types at the same time.
> */
> - if (wpan_dev->iftype == NL802154_IFTYPE_NODE &&
> - nsdata->wpan_dev.iftype == NL802154_IFTYPE_NODE)
> + if (wpan_dev->iftype != NL802154_IFTYPE_MONITOR &&
> + nsdata->wpan_dev.iftype != NL802154_IFTYPE_MONITOR)
> return -EBUSY;
>
> /* check all phy mac sublayer settings are the same.
> @@ -565,6 +565,7 @@ ieee802154_setup_sdata(struct ieee802154_sub_if_data *sdata,
> wpan_dev->short_addr = cpu_to_le16(IEEE802154_ADDR_BROADCAST);
>
> switch (type) {
> + case NL802154_IFTYPE_COORD:
> case NL802154_IFTYPE_NODE:
> ieee802154_be64_to_le64(&wpan_dev->extended_addr,
> sdata->dev->dev_addr);
> @@ -624,6 +625,7 @@ ieee802154_if_add(struct ieee802154_local *local, const char *name,
> ieee802154_le64_to_be64(ndev->perm_addr,
> &local->hw.phy->perm_extended_addr);
> switch (type) {
> + case NL802154_IFTYPE_COORD:
> case NL802154_IFTYPE_NODE:
> ndev->type = ARPHRD_IEEE802154;
> if (ieee802154_is_valid_extended_unicast_addr(extended_addr)) {
> diff --git a/net/mac802154/main.c b/net/mac802154/main.c
> index 40fab08df24b..d03ecb747afc 100644
> --- a/net/mac802154/main.c
> +++ b/net/mac802154/main.c
> @@ -219,6 +219,8 @@ int ieee802154_register_hw(struct ieee802154_hw *hw)
>
> if (hw->flags & IEEE802154_HW_PROMISCUOUS)
> local->phy->supported.iftypes |= BIT(NL802154_IFTYPE_MONITOR);
> + else
> + local->phy->supported.iftypes &= ~BIT(NL802154_IFTYPE_COORD);
>
So this means if somebody in the driver sets iftype COORD is supported
but then IEEE802154_HW_PROMISCUOUS is not supported it will not
support COORD?
Why is IEEE802154_HW_PROMISCUOUS required for COORD iftype? I thought
IEEE802154_HW_PROMISCUOUS is required to do a scan?
> rc = wpan_phy_register(local->phy);
> if (rc < 0)
> diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> index 2ae23a2f4a09..aca348d7834b 100644
> --- a/net/mac802154/rx.c
> +++ b/net/mac802154/rx.c
> @@ -208,6 +208,7 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local,
> int ret;
> struct ieee802154_sub_if_data *sdata;
> struct ieee802154_hdr hdr;
> + struct sk_buff *skb2;
>
> ret = ieee802154_parse_frame_start(skb, &hdr);
> if (ret) {
> @@ -217,7 +218,7 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local,
> }
>
> list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> - if (sdata->wpan_dev.iftype != NL802154_IFTYPE_NODE)
> + if (sdata->wpan_dev.iftype == NL802154_IFTYPE_MONITOR)
> continue;
I guess this will work but I would like to see no logic about if it's
not MONITOR it's NODE or COORD, because introducing others requires
updating those again... however I think it's fine. I would like to see
a different receive path for coord_rx() and node_rx(), but yea
currently I guess they are mostly the same... in future I think they
are required as PACKTE_HOST, etc. will be changed regarding pan
coordinator or just coordinator (so far I understood).
>
> if (!ieee802154_sdata_running(sdata))
> @@ -230,9 +231,11 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local,
> sdata->required_filtering == IEEE802154_FILTERING_4_FRAME_FIELDS)
> continue;
>
> - ieee802154_subif_frame(sdata, skb, &hdr);
> - skb = NULL;
> - break;
> + skb2 = skb_clone(skb, GFP_ATOMIC);
> + if (skb2) {
> + skb2->dev = sdata->dev;
> + ieee802154_subif_frame(sdata, skb2, &hdr);
> + }
> }
>
> kfree_skb(skb);
If we do the clone above this kfree_skb() should be move to
ieee802154_rx() right after __ieee802154_rx_handle_packet(). This
patch also changes that we deliver one skb to multiple interfaces if
there are more than one and I was not aware that we currently do that.
:/
- Alex
