Hi,
On Wed, Oct 19, 2022 at 5:52 AM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
>
> Hi Alexander,
>
> aahringo@xxxxxxxxxx wrote on Tue, 18 Oct 2022 19:57:19 -0400:
>
> > 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?
>
> You are totally right this is inconsistent, I'll drop the else block
> entirely. The fact that HW_PROMISCUOUS is supported when starting a
> scan is handled by the -EOPNOTSUPP return code from
> drv_set_promiscuous_mode() called by drv_start() in
> mac802154_trigger_scan().
>
> However I need your input on the following topic: in my branch I
> have somewhere a patch adding IFTYPE_COORD to the list of
> phy->supported.iftypes in each individual driver. But right now, if we
> drop the promiscuous constraint as you suggest, I don't see anymore the
> need for setting this as a per-driver value.
>
> Should we make the possibility to create IFTYPE_COORD interfaces the
> default instead, something like this?
>
> --- a/net/mac802154/main.c
> +++ b/net/mac802154/main.c
> @@ -118,7 +118,7 @@ ieee802154_alloc_hw(size_t priv_data_len, const struct ieee802154_ops *ops)
> phy->supported.lbt = NL802154_SUPPORTED_BOOL_FALSE;
>
> /* always supported */
> - phy->supported.iftypes = BIT(NL802154_IFTYPE_NODE);
> + phy->supported.iftypes = BIT(NL802154_IFTYPE_NODE) | BIT(NL802154_IFTYPE_COORD);
>
okay.
> > > rc = wpan_phy_register(local->phy);
> > > if (rc < 0)
> > > diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> > > index 2ae23a2f4a09..aca348d7834b 100644
> > > --- a/net/mac802154/rx.c
> > > +++ b/net/mac802154/rx.c
> > > @@ -208,6 +208,7 @@ __ieee802154_rx_handle_packet(struct
> > > ieee802154_local *local, int ret;
> > > struct ieee802154_sub_if_data *sdata;
> > > struct ieee802154_hdr hdr;
> > > + struct sk_buff *skb2;
> > >
> > > ret = ieee802154_parse_frame_start(skb, &hdr);
> > > if (ret) {
> > > @@ -217,7 +218,7 @@ __ieee802154_rx_handle_packet(struct
> > > ieee802154_local *local, }
> > >
> > > list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> > > - if (sdata->wpan_dev.iftype != NL802154_IFTYPE_NODE)
> > > + if (sdata->wpan_dev.iftype ==
> > > NL802154_IFTYPE_MONITOR) continue;
> >
> > I guess this will work but I would like to see no logic about if it's
> > not MONITOR it's NODE or COORD, because introducing others requires
> > updating those again... however I think it's fine.
>
> Actually I don't get why we would not want this logic, it seem very
> relevant to me. Do you want a helper instead and hide the condition
> inside? Or something else? Or is it just fine for now?
>
> > I would like to see
> > a different receive path for coord_rx() and node_rx(), but yea
> > currently I guess they are mostly the same... in future I think they
> > are required as PACKTE_HOST, etc. will be changed regarding pan
> > coordinator or just coordinator (so far I understood).
>
yes, I think so too.
> I agree it is tempting, but yeah, there is really very little changes
> between the two, for me splitting the rx path would just darken the
> code without bringing much...
>
ok.
> About the way we handle the PAN coordinator role I have a couple of
> questions:
> - shall we consider only the PAN coordinator to be able to accept
> associations or is any coordinator in the PAN able to do it? (this is
> not clear to me)
Me either, it sounds for me that coordinators are "leaves" and pan
coordinators are not. It is like in IPv6 level it is a host or router.
> - If a coordinator receives a packet with no destination it should
> expect it to be targeted at the PAN controller. Should we consider
> relaying the packet?
I guess it depends what the standard says here?
> - Is relaying a hardware feature or should we do it in software?
>
I think for SoftMAC it is only the address filter which needs to be
changed. The rest is in software. So far what I can see here.
Question is what we are using here in the Linux kernel to provide such
functionality...
e.g. see:
include/net/dst.h
> > > if (!ieee802154_sdata_running(sdata))
> > > @@ -230,9 +231,11 @@ __ieee802154_rx_handle_packet(struct
> > > ieee802154_local *local, sdata->required_filtering ==
> > > IEEE802154_FILTERING_4_FRAME_FIELDS) continue;
> > >
> > > - ieee802154_subif_frame(sdata, skb, &hdr);
> > > - skb = NULL;
> > > - break;
> > > + skb2 = skb_clone(skb, GFP_ATOMIC);
> > > + if (skb2) {
> > > + skb2->dev = sdata->dev;
> > > + ieee802154_subif_frame(sdata, skb2, &hdr);
> > > + }
> > > }
> > >
> > > kfree_skb(skb);
> >
> > If we do the clone above this kfree_skb() should be move to
> > ieee802154_rx() right after __ieee802154_rx_handle_packet().
>
> Ok!
>
> > This patch also changes that we deliver one skb to multiple interfaces if
> > there are more than one and I was not aware that we currently do that.
> > :/
>
> Just as a side note: we do that already if we have several MONITOR
> interfaces opened on the same PHY, it is possible to have them all open.
>
yes, I used that feature with hwsim once.
> Regarding the situation where we would have NODE + MONITOR or COORD +
> MONITOR, while the interface creation would work, both could not be
> open at the same time because the following happens:
> mac802154_wpan_open() {
> ieee802154_check_concurrent_iface() {
> ieee802154_check_mac_settings() {
> /* prevent the two interface types from being
> * open at the same time because the filtering
> * needs are not compatible. */
> }
> }
> }
>
> Then, because you asked me to anticipate if we ever want to support more
> than one NODE or COORD interface at the same time, or at least not to
> do anything that would lead to a step back on this regard, I decided I
> would provide all the infrastructure to gracefully handle this
> situation in the Rx path, even though right now it still cannot happen
> because when opening an interface, ieee802154_check_concurrent_iface()
> will also prevent two NODE / COORD interfaces to be opened at the same
> time.
yes, but you are assuming the actual hardware here. A hardware with
multiple address filters can indeed support other interfaces at the
same time. I can also name one, hwsim and a real one - atusb.
>
> TL;DR
> * MONITOR + MONITOR
> = already supported and working
> * (NODE + MONITOR) || (COORD + MONITOR)
> = iface creation allowed, but cannot be opened at the same time
> because of the filtering level incompatibility on a single PHY
> * (NODE + NODE) || (COORD + COORD) || (NODE + COORD)
> = iface creation allowed, but cannot be opened at the same time
> because only one PHY available on the device
>
> So for me we are safe and future proof.
>
Yes, this is currently kind of difficult to handle, but I see that we
check such default filtering type thing per phy which depends on what
kind of interface is currently running there? Something like that...
- Alex
