Re: [PATCH wpan-next v3 3/4] net: mac802154: Set durations automatically

[Miquel Raynal <miquel.raynal@bootlin.com>]

miquel.raynal@bootlin.com wrote on Thu, 28 Apr 2022 17:58:38 +0200:

> Hi Stefan,
> 
> miquel.raynal@bootlin.com wrote on Tue,  1 Feb 2022 19:06:28 +0100:
> 
> > As depicted in the IEEE 802.15.4 specification, modulation/bands are
> > tight to a number of page/channels so we can for most of them derive the
> > durations automatically.
> > 
> > The two locations that must call this new helper to set the variou
> > symbol durations are:
> > - when manually requesting a channel change though the netlink interface
> > - at PHY creation, once the device driver has set the default
> >   page/channel
> > 
> > If an information is missing, the symbol duration is not touched, a
> > debug message is eventually printed. This keeps the compatibility with
> > the unconverted drivers for which it was too complicated for me to find
> > their precise information. If they initially provided a symbol duration,
> > it would be kept. If they don't, the symbol duration value is left
> > untouched.
> > 
> > Once the symbol duration derived, the lifs and sifs durations are
> > updated as well.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  include/net/cfg802154.h |  2 ++
> >  net/mac802154/cfg.c     |  1 +
> >  net/mac802154/main.c    | 46 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 49 insertions(+)
> > 
> > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> > index 8a4b6a50452f..49b4bcc24032 100644
> > --- a/include/net/cfg802154.h
> > +++ b/include/net/cfg802154.h
> > @@ -405,4 +405,6 @@ static inline const char *wpan_phy_name(struct wpan_phy *phy)
> >  	return dev_name(&phy->dev);
> >  }
> >  
> > +void ieee802154_configure_durations(struct wpan_phy *phy);
> > +
> >  #endif /* __NET_CFG802154_H */
> > diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c
> > index fbeebe3bc31d..1e4a9f74ed43 100644
> > --- a/net/mac802154/cfg.c
> > +++ b/net/mac802154/cfg.c
> > @@ -118,6 +118,7 @@ ieee802154_set_channel(struct wpan_phy *wpan_phy, u8 page, u8 channel)
> >  	if (!ret) {
> >  		wpan_phy->current_page = page;
> >  		wpan_phy->current_channel = channel;
> > +		ieee802154_configure_durations(wpan_phy);
> >  	}
> >  
> >  	return ret;
> > diff --git a/net/mac802154/main.c b/net/mac802154/main.c
> > index 53153367f9d0..5546ef86e231 100644
> > --- a/net/mac802154/main.c
> > +++ b/net/mac802154/main.c
> > @@ -113,6 +113,50 @@ ieee802154_alloc_hw(size_t priv_data_len, const struct ieee802154_ops *ops)
> >  }
> >  EXPORT_SYMBOL(ieee802154_alloc_hw);
> >  
> > +void ieee802154_configure_durations(struct wpan_phy *phy)
> > +{
> > +	u32 duration = 0;
> > +
> > +	switch (phy->current_page) {
> > +	case 0:
> > +		if (BIT(phy->current_page) & 0x1)  
> 
> I am very sorry to spot this only now but this is wrong. 
> 
> all the conditions from here and below should be:
> 
> 		if (BIT(phy->current_channel & <mask>))
> 
> The masks look good, the durations as well, but the conditions are
> wrong.
> 
> > +			/* 868 MHz BPSK 802.15.4-2003: 20 ksym/s */
> > +			duration = 50 * NSEC_PER_USEC;
> > +		else if (phy->current_page & 0x7FE)  
> 
> Ditto
> 
> > +			/* 915 MHz BPSK	802.15.4-2003: 40 ksym/s */
> > +			duration = 25 * NSEC_PER_USEC;
> > +		else if (phy->current_page & 0x7FFF800)  
> 
> Ditto
> 
> > +			/* 2400 MHz O-QPSK 802.15.4-2006: 62.5 ksym/s */
> > +			duration = 16 * NSEC_PER_USEC;
> > +		break;
> > +	case 2:
> > +		if (BIT(phy->current_page) & 0x1)  
> 
> Ditto
> 
> > +			/* 868 MHz O-QPSK 802.15.4-2006: 25 ksym/s */
> > +			duration = 40 * NSEC_PER_USEC;
> > +		else if (phy->current_page & 0x7FE)  
> 
> Ditto
> 
> > +			/* 915 MHz O-QPSK 802.15.4-2006: 62.5 ksym/s */
> > +			duration = 16 * NSEC_PER_USEC;
> > +		break;
> > +	case 3:
> > +		if (BIT(phy->current_page) & 0x3FFF)  
> 
> Ditto
> 
> > +			/* 2.4 GHz CSS 802.15.4a-2007: 1/6 Msym/s */
> > +			duration = 6 * NSEC_PER_USEC;
> > +		break;  
> 
> I see it's "only" in wpan-next (781830c800dd "net: mac802154: Set
> durations automatically") and was not yet pulled in the net-next
> tree so please let me know what you prefer: I can either provide a
> proper patch to fit it (without upstream Fixes reference), or you can
> just apply this diff below and push -f the branch. Let me know what you
> prefer.
> 
> Again, sorry to only see this now.
> 
> Thanks,
> Miquèl
> 
> commit 4122765e5f982ed8f0ccea5fd813ef4d53d20a90 (HEAD)
> Author: Miquel Raynal <miquel.raynal@bootlin.com>
> Date:   Thu Apr 28 17:57:59 2022 +0200
> 
>     fixup! net: mac802154: Set durations automatically

And I keep having it wrong. Here is the right fixup!...

commit 676aa77f1c279a90f521c1c05757c702e1538472 (HEAD)
Author: Miquel Raynal <miquel.raynal@bootlin.com>
Date:   Thu Apr 28 17:57:59 2022 +0200

    fixup! net: mac802154: Set durations automatically

diff --git a/net/mac802154/main.c b/net/mac802154/main.c
index 5546ef86e231..bd7bdb1219dd 100644
--- a/net/mac802154/main.c
+++ b/net/mac802154/main.c
@@ -119,26 +119,26 @@ void ieee802154_configure_durations(struct wpan_phy *phy)
 
        switch (phy->current_page) {
        case 0:
-               if (BIT(phy->current_page) & 0x1)
+               if (BIT(phy->current_channel) & 0x1)
                        /* 868 MHz BPSK 802.15.4-2003: 20 ksym/s */
                        duration = 50 * NSEC_PER_USEC;
-               else if (phy->current_page & 0x7FE)
+               else if (BIT(phy->current_channel) & 0x7FE)
                        /* 915 MHz BPSK 802.15.4-2003: 40 ksym/s */
                        duration = 25 * NSEC_PER_USEC;
-               else if (phy->current_page & 0x7FFF800)
+               else if (BIT(phy->current_channel) & 0x7FFF800)
                        /* 2400 MHz O-QPSK 802.15.4-2006: 62.5 ksym/s */
                        duration = 16 * NSEC_PER_USEC;
                break;
        case 2:
-               if (BIT(phy->current_page) & 0x1)
+               if (BIT(phy->current_channel) & 0x1)
                        /* 868 MHz O-QPSK 802.15.4-2006: 25 ksym/s */
                        duration = 40 * NSEC_PER_USEC;
-               else if (phy->current_page & 0x7FE)
+               else if (BIT(phy->current_channel) & 0x7FE)
                        /* 915 MHz O-QPSK 802.15.4-2006: 62.5 ksym/s */
                        duration = 16 * NSEC_PER_USEC;
                break;
        case 3:
-               if (BIT(phy->current_page) & 0x3FFF)
+               if (BIT(phy->current_channel) & 0x3FFF)
                        /* 2.4 GHz CSS 802.15.4a-2007: 1/6 Msym/s */
                        duration = 6 * NSEC_PER_USEC;
                break;