| Message ID | 20220201180629.93410-4-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ieee802154: Improve durations handling | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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 diff --git a/net/mac802154/main.c b/net/mac802154/main.c index 5546ef86e231..bbbdac6ee028 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 (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 (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 (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;
Hello. On 28.04.22 17:58, Miquel Raynal wrote: > 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. No forced push to a public branch in my public repo. Please provide a fixup patch that I can apply on top. regards Stefan Schmidt
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;
Hi Stefan, > > 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. > > No forced push to a public branch in my public repo. Please provide a fixup patch that I can apply on top. Duly noted. Thanks, Miquèl
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) + /* 868 MHz BPSK 802.15.4-2003: 20 ksym/s */ + duration = 50 * NSEC_PER_USEC; + else if (phy->current_page & 0x7FE) + /* 915 MHz BPSK 802.15.4-2003: 40 ksym/s */ + duration = 25 * NSEC_PER_USEC; + else if (phy->current_page & 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) + /* 868 MHz O-QPSK 802.15.4-2006: 25 ksym/s */ + duration = 40 * NSEC_PER_USEC; + else if (phy->current_page & 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) + /* 2.4 GHz CSS 802.15.4a-2007: 1/6 Msym/s */ + duration = 6 * NSEC_PER_USEC; + break; + default: + break; + } + + if (!duration) { + pr_debug("Unknown PHY symbol duration\n"); + return; + } + + phy->symbol_duration = duration; + phy->lifs_period = (IEEE802154_LIFS_PERIOD * phy->symbol_duration) / NSEC_PER_SEC; + phy->sifs_period = (IEEE802154_SIFS_PERIOD * phy->symbol_duration) / NSEC_PER_SEC; +} +EXPORT_SYMBOL(ieee802154_configure_durations); + void ieee802154_free_hw(struct ieee802154_hw *hw) { struct ieee802154_local *local = hw_to_local(hw); @@ -157,6 +201,8 @@ int ieee802154_register_hw(struct ieee802154_hw *hw) ieee802154_setup_wpan_phy_pib(local->phy); + ieee802154_configure_durations(local->phy); + if (!(hw->flags & IEEE802154_HW_CSMA_PARAMS)) { local->phy->supported.min_csma_backoffs = 4; local->phy->supported.max_csma_backoffs = 4;
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(+)