From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 51F5EECAAA2 for ; Fri, 26 Aug 2022 01:35:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229470AbiHZBfX (ORCPT ); Thu, 25 Aug 2022 21:35:23 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36066 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231848AbiHZBfV (ORCPT ); Thu, 25 Aug 2022 21:35:21 -0400 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8D22F28E13 for ; Thu, 25 Aug 2022 18:35:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1661477718; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=D+ja7am3QeRHI6I33cst2TOCn2FBIIdrvDrYrb42nMI=; b=XKGWG7ChgSewrM0fkY3B1RZFF0u6nikJbLs7grWo4w+aPR9g9pa8Undq011R46AEaue59d DZFtntJEVZh2n7Y8va4C7kZm7/+NdazAIc53Uo73dTkShs+wViEfFziZJ+zWqnr20b7XzE QBkJVOejCI2rr8c8MNM9Gdp1qCg41Rk= Received: from mail-qt1-f199.google.com (mail-qt1-f199.google.com [209.85.160.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-112-0N7MWpsaNjOdnUnW_vUEUA-1; Thu, 25 Aug 2022 21:35:17 -0400 X-MC-Unique: 0N7MWpsaNjOdnUnW_vUEUA-1 Received: by mail-qt1-f199.google.com with SMTP id bq11-20020a05622a1c0b00b003434f125b77so284552qtb.20 for ; Thu, 25 Aug 2022 18:35:17 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc; bh=D+ja7am3QeRHI6I33cst2TOCn2FBIIdrvDrYrb42nMI=; b=UwPMSie1JCAYrTXwaaDA31C1uUW7uMzoDMXNcVCvUq3tlO/oBLNANc8n8gicIWAGIM eViPdVQbn3Gu+3Qn07+dd0HOl1lnT+gWoWZX4+haQVBoluKAl8LTI95yq0se0m01upz4 mgeyG2gkNLXScvMo3UJ+yVkXZktlUJgqr6s61WnYXRGxFXBGoyn2puKln5M252E/xfCR Yno9QOGVoixmCkHOfq/fkPy82gANjujddOfdGZVGybpAHuW98wH+qhiyUL1PuvuS7pAj F6sRqBlOb9qF5KSDNwHhgAta5vBmQx/Ui1PVfbKWWYLEPhWD4P2KOpBwLO7dVCmnPBEq q0bw== X-Gm-Message-State: ACgBeo19w0Xrz9Zstrir9iGsXssKYGp/MMJD5KNZAgnHiuKmQt8cIjak P8ZSlFSGTAhipQpR/FUy7CGrS4kNUKPfbi5NEgeJtJGiq9jzv5gajHFrQvQGCthiByvhHRBfIrV Mz9VIXCfawyVtD/jwpmL8u84LZgmofQQ3HfIvwA== X-Received: by 2002:ae9:e70f:0:b0:6bb:eb30:4916 with SMTP id m15-20020ae9e70f000000b006bbeb304916mr5128626qka.691.1661477716988; Thu, 25 Aug 2022 18:35:16 -0700 (PDT) X-Google-Smtp-Source: AA6agR4dvlJkz1vC2Hu8SO52FlMk78H4r60AwPa83vIOplBiO8ghA8N+ih9P7REhqNlQtBzwJM/IIUL92PuTKfEY8Gk= X-Received: by 2002:ae9:e70f:0:b0:6bb:eb30:4916 with SMTP id m15-20020ae9e70f000000b006bbeb304916mr5128603qka.691.1661477716656; Thu, 25 Aug 2022 18:35:16 -0700 (PDT) MIME-Version: 1.0 References: <20220701143052.1267509-1-miquel.raynal@bootlin.com> <20220701143052.1267509-2-miquel.raynal@bootlin.com> <20220819191109.0e639918@xps-13> <20220823182950.1c722e13@xps-13> <20220824093547.16f05d15@xps-13> <20220825104035.11806a67@xps-13> In-Reply-To: From: Alexander Aring Date: Thu, 25 Aug 2022 21:35:05 -0400 Message-ID: Subject: Re: [PATCH wpan-next 01/20] net: mac802154: Allow the creation of coordinator interfaces To: Miquel Raynal Cc: Alexander Aring , Stefan Schmidt , linux-wpan - ML , "David S. Miller" , Jakub Kicinski , Paolo Abeni , Eric Dumazet , Network Development , David Girault , Romuald Despres , Frederic Blain , Nicolas Schodet , Thomas Petazzoni Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-wpan@vger.kernel.org Hi, On Thu, Aug 25, 2022 at 8:51 PM Alexander Aring wrote= : > > Hi, > > On Thu, Aug 25, 2022 at 4:41 AM Miquel Raynal = 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 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 > > > > > 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 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 wrote: > > > > > > > > > > > > > > > > > > > > As a first strep in introducing proper PAN management a= nd association, > > > > > > > > > > we need to be able to create coordinator interfaces whi= ch might act as > > > > > > > > > > coordinator or PAN coordinator. > > > > > > > > > > > > > > > > > > > > Hence, let's add the minimum support to allow the creat= ion of these > > > > > > > > > > interfaces. This might be restrained and improved later= . > > > > > > > > > > > > > > > > > > > > Signed-off-by: Miquel Raynal > > > > > > > > > > --- > > > > > > > > > > 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/ifac= e.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 !=3D sdata && ieee802154_sda= ta_running(nsdata)) { > > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > > > - /* TODO currently we don't supp= ort 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 s= ame time. > > > > > > > > > > + /* TODO currently we don't supp= ort multiple node/coord > > > > > > > > > > + * types we need to run skb_clo= ne at rx path. Check if > > > > > > > > > > + * there exist really an use ca= se if we need to support > > > > > > > > > > + * multiple node/coord types at= the same time. > > > > > > > > > > */ > > > > > > > > > > - if (wpan_dev->iftype =3D=3D NL8= 02154_IFTYPE_NODE && > > > > > > > > > > - nsdata->wpan_dev.iftype =3D= =3D NL802154_IFTYPE_NODE) > > > > > > > > > > + if (wpan_dev->iftype !=3D NL802= 154_IFTYPE_MONITOR && > > > > > > > > > > + nsdata->wpan_dev.iftype != =3D NL802154_IFTYPE_MONITOR) > > > > > > > > > > return -EBUSY; > > > > > > > > > > > > > > > > > > > > /* check all phy mac sublayer s= ettings are the same. > > > > > > > > > > @@ -577,6 +577,7 @@ ieee802154_setup_sdata(struct ieee8= 02154_sub_if_data *sdata, > > > > > > > > > > wpan_dev->short_addr =3D cpu_to_le16(IEEE802154= _ADDR_BROADCAST); > > > > > > > > > > > > > > > > > > > > switch (type) { > > > > > > > > > > + case NL802154_IFTYPE_COORD: > > > > > > > > > > case NL802154_IFTYPE_NODE: > > > > > > > > > > ieee802154_be64_to_le64(&wpan_dev->exte= nded_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_ex= tended_addr); > > > > > > > > > > switch (type) { > > > > > > > > > > + case NL802154_IFTYPE_COORD: > > > > > > > > > > case NL802154_IFTYPE_NODE: > > > > > > > > > > ndev->type =3D ARPHRD_IEEE802154; > > > > > > > > > > if (ieee802154_is_valid_extended_unicas= t_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(struc= t ieee802154_local *local, > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > list_for_each_entry_rcu(sdata, &local->interfac= es, list) { > > > > > > > > > > - if (sdata->wpan_dev.iftype !=3D NL80215= 4_IFTYPE_NODE) > > > > > > > > > > + if (sdata->wpan_dev.iftype =3D=3D NL802= 154_IFTYPE_MONITOR) > > > > > > > > > > continue; > > > > > > > > > > > > > > > > > > I probably get why you are doing that, but first the over= all 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 handli= ng 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 coordin= ator receive > > > > > > > > > path but there are things missing. > > > > > > > > > > > > > > > > > > 1. Changing the address filters that it signals the trans= ceiver it's > > > > > > > > > acting as coordinator > > > > > > > > > 2. We _should_ also have additional handling for whatever= the > > > > > > > > > additional handling what address filters are doing in mac= 802154 > > > > > > > > > _because_ there is hardware which doesn't have address fi= ltering 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 assum= e that > > > > > > > > > everything is working correctly here but what I want to s= ee is a > > > > > > > > > separate receive path for coordinators that people can se= nd patches to > > > > > > > > > fix it. > > > > > > > > > > > > > > > > Yes, we do very little differently between the two modes, t= hat's why I > > > > > > > > took the easy way: just changing the condition. I really do= n'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 th= at? > > > > > > > > > > > > > > mostly yes, but there exists a difference and we should at le= ast check > > > > > > > if the node receive path violates the coordinator receive pat= h and > > > > > > > vice versa. > > > > > > > Put it in a receive_path() function and then coord_receive_pa= th(), > > > > > > > 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 fil= ter rule > > > > > > > if it's a coordinator". > > > > > > > btw: this is because the address filter on the transceiver ne= eds 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 filter= ing or > > > > > > > not. > > > > > > > > > > > > I must be missing some information because I can't find any pla= ces > > > > > > 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 ty= pe. > > > > > > 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 wh= at > > > > > > should be included in either node_receive_path() which should b= e > > > > > > 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, whic= h 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 R= equest > > > > > > command is optional for an FFD and an RFD", so this series wa= s only > > > > > > targeting basic beaconing for now. > > > > > > - In relaying mode, the destination address must not be validat= ed > > > > > > because the message needs to be re-emitted. Indeed, a receive= r 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 th= e 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. Yo= u can > > > > > also read that up in datasheets of transceivers as atf86rf233, se= arch > > > > > 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 k= ernel > > > > > 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 P= AN > > > > coordinator is the coordinator with no parent (association-wise). W= ith > > > > the addition of the association series, I can handle that, so I wil= l > > > > create the two path as you advise, add a comment about this additio= nal > > > > filter rule that we don't yet support, and finally after the > > > > association series add another commit to make this filtering rule r= eal. > > > > > > > > > > > > > > > Thanks, > > > > > > Miqu=C3=A8l > > > > > > > > > > > > --- > > > > > > > > > > > > --- a/net/mac802154/rx.c > > > > > > +++ b/net/mac802154/rx.c > > > > > > @@ -194,6 +194,7 @@ __ieee802154_rx_handle_packet(struct ieee80= 2154_local *local, > > > > > > int ret; > > > > > > struct ieee802154_sub_if_data *sdata; > > > > > > struct ieee802154_hdr hdr; > > > > > > + bool iface_found =3D false; > > > > > > > > > > > > ret =3D ieee802154_parse_frame_start(skb, &hdr); > > > > > > if (ret) { > > > > > > @@ -203,18 +204,31 @@ __ieee802154_rx_handle_packet(struct ieee= 802154_local *local, > > > > > > } > > > > > > > > > > > > list_for_each_entry_rcu(sdata, &local->interfaces, list= ) { > > > > > > - if (sdata->wpan_dev.iftype !=3D NL802154_IFTYPE= _NODE) > > > > > > + if (sdata->wpan_dev.iftype =3D=3D NL802154_IFTY= PE_MONITOR) > > > > > > continue; > > > > > > > > > > > > if (!ieee802154_sdata_running(sdata)) > > > > > > continue; > > > > > > > > > > > > + iface_found =3D true; > > > > > > + break; > > > > > > + } > > > > > > + > > > > > > + if (!iface_found) { > > > > > > + kfree_skb(skb); > > > > > > + return; > > > > > > + } > > > > > > + > > > > > > + /* TBD: Additional filtering is possible on NODEs and/o= r COORDINATORs */ > > > > > > + switch (sdata->wpan_dev.iftype) { > > > > > > + case NL802154_IFTYPE_COORD: > > > > > > + case NL802154_IFTYPE_NODE: > > > > > > ieee802154_subif_frame(sdata, skb, &hdr); > > > > > > - skb =3D NULL; > > > > > > + break; > > > > > > + default: > > > > > > + kfree_skb(skb); > > > > > > break; > > > > > > } > > > > > > > > > > Why do you remove the whole interface looping above and make it o= nly > > > > > 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