Re: [PATCH wpan-next 07/11] mac802154: Handle association requests from peers

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Alexander Aring <aahringo@redhat.com>
Cc: Alexander Aring <alex.aring@gmail.com>,
	Stefan Schmidt <stefan@datenfreihafen.org>,
	linux-wpan@vger.kernel.org,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Eric Dumazet <edumazet@google.com>,
	netdev@vger.kernel.org, David Girault <david.girault@qorvo.com>,
	Romuald Despres <romuald.despres@qorvo.com>,
	Frederic Blain <frederic.blain@qorvo.com>,
	Nicolas Schodet <nico@ni.fr.eu.org>,
	Guilhem Imberton <guilhem.imberton@qorvo.com>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH wpan-next 07/11] mac802154: Handle association requests from peers
Date: Fri, 1 Sep 2023 17:45:37 +0200	[thread overview]
Message-ID: <20230901174537.50f88d60@xps-13> (raw)
In-Reply-To: <20230821105259.4659dd74@xps-13>

Hi Alexander,

> > > --- a/net/ieee802154/pan.c
> > > +++ b/net/ieee802154/pan.c
> > > @@ -66,3 +66,30 @@ cfg802154_device_is_child(struct wpan_dev *wpan_dev,
> > >         return NULL;
> > >  }
> > >  EXPORT_SYMBOL_GPL(cfg802154_device_is_child);
> > > +
> > > +__le16 cfg802154_get_free_short_addr(struct wpan_dev *wpan_dev)
> > > +{
> > > +       struct ieee802154_pan_device *child;
> > > +       __le16 addr;
> > > +
> > > +       lockdep_assert_held(&wpan_dev->association_lock);
> > > +
> > > +       do {
> > > +               get_random_bytes(&addr, 2);    
> > 
> > This is combined with the max associations setting? I am not sure if
> > this is the best way to get free values from a u16 value where we have
> > some data structure of "given" addresses to a node. I recently was
> > looking into idr/xarray data structure... maybe we can use something
> > from there.  
> 
> I actually thought about using an increasing index, but the pseudo
> random generator seemed appropriate because of its "unpredictability",
> but there is not real use for that (besides maybe testing purposes). I
> can definitely switch to another solution.

I looked into this deeper. I didn't felt like idr would be so much
useful, but I started converting the code to use ida instead (so the
simplest approach, with no associated pointer). There are actually two
use cases which clearly match better the random address mechanism.

a/ One can freely decide the short address of the coordinator (it is
freely selectable by the user) but ida has no mechanism to handle this
with an API which would prevent such "number to be used".

In practice, this could be workarounded "easily", even though the
implementation would be dirty IMHO: getting an IDA, if it matches ours,
just try again without freeing it. TBH I don't like much this idea.

b/ In case we ever want to support master handover, the ida solution
does not work well...

For now I've kept the current approach (actually adding a missing
check), but if you know how to solve that I can either update the
implementation or make a followup patch, especially since the current
approach is not bounded (in the theoretical case where we have 65k
devices in the same PAN).

I believe the allocation strategies are not set in stone anyway and can
easily evolve.

Thanks,
Miquèl