| Message ID | 20220112173312.764660-2-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | IEEE 802.15.4 scan support | expand |
Hi, On Wed, 12 Jan 2022 at 12:33, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > As it is currently designed, the set_channel() cfg802154 hook > implemented in the softMAC is doing a couple of checks before actually > performing the channel change. However, as we enhance the support for > automatically setting the symbol duration during channel changes, it > will also be needed to ensure that the corresponding channel as properly > be selected at probe time. In order to verify this, we will need to no, we don't set channels at probe time. We set the current_page/channel whatever the default is according to the hardware datasheet. I think this channel should be dropped and all drivers set the defaults before registering hw as what we do at e.g. at86rf230, see [0]. - Alex [0] https://elixir.bootlin.com/linux/v5.16/source/drivers/net/ieee802154/at86rf230.c#L1553
Hi, On Wed, 12 Jan 2022 at 17:30, Alexander Aring <alex.aring@gmail.com> wrote: > > Hi, > > On Wed, 12 Jan 2022 at 12:33, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > As it is currently designed, the set_channel() cfg802154 hook > > implemented in the softMAC is doing a couple of checks before actually > > performing the channel change. However, as we enhance the support for > > automatically setting the symbol duration during channel changes, it > > will also be needed to ensure that the corresponding channel as properly > > be selected at probe time. In order to verify this, we will need to > > no, we don't set channels at probe time. We set the > current_page/channel whatever the default is according to the hardware > datasheet. I think this channel should be dropped and all drivers set s/channel/patch/ - Alex
Hi Alexander, alex.aring@gmail.com wrote on Wed, 12 Jan 2022 17:30:35 -0500: > Hi, > > On Wed, 12 Jan 2022 at 12:33, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > As it is currently designed, the set_channel() cfg802154 hook > > implemented in the softMAC is doing a couple of checks before actually > > performing the channel change. However, as we enhance the support for > > automatically setting the symbol duration during channel changes, it > > will also be needed to ensure that the corresponding channel as properly > > be selected at probe time. In order to verify this, we will need to > > no, we don't set channels at probe time. We set the > current_page/channel whatever the default is according to the hardware > datasheet. I think this channel should be dropped and all drivers set > the defaults before registering hw as what we do at e.g. at86rf230, > see [0]. Is there a reason for refusing to call ->set_channel() at probe time? Anyway, I'll put the symbol duration setting in the registration helper and I will fix hwsim aside. > > - Alex > > [0] https://elixir.bootlin.com/linux/v5.16/source/drivers/net/ieee802154/at86rf230.c#L1553 Thanks, Miquèl
Hi Alexander, alex.aring@gmail.com wrote on Wed, 12 Jan 2022 17:53:57 -0500: > Hi, > > On Wed, 12 Jan 2022 at 17:30, Alexander Aring <alex.aring@gmail.com> wrote: > > > > Hi, > > > > On Wed, 12 Jan 2022 at 12:33, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > As it is currently designed, the set_channel() cfg802154 hook > > > implemented in the softMAC is doing a couple of checks before actually > > > performing the channel change. However, as we enhance the support for > > > automatically setting the symbol duration during channel changes, it > > > will also be needed to ensure that the corresponding channel as properly > > > be selected at probe time. In order to verify this, we will need to > > > > no, we don't set channels at probe time. We set the > > current_page/channel whatever the default is according to the hardware > > datasheet. I think this channel should be dropped and all drivers set > > s/channel/patch/ I've dropped the patch and put an additional call to _set_symbol_duration() in the hw registration routine as discussed initially. Thanks, Miquèl
Hi, On Thu, 13 Jan 2022 at 04:32, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > alex.aring@gmail.com wrote on Wed, 12 Jan 2022 17:30:35 -0500: > > > Hi, > > > > On Wed, 12 Jan 2022 at 12:33, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > As it is currently designed, the set_channel() cfg802154 hook > > > implemented in the softMAC is doing a couple of checks before actually > > > performing the channel change. However, as we enhance the support for > > > automatically setting the symbol duration during channel changes, it > > > will also be needed to ensure that the corresponding channel as properly > > > be selected at probe time. In order to verify this, we will need to > > > > no, we don't set channels at probe time. We set the > > current_page/channel whatever the default is according to the hardware > > datasheet. I think this channel should be dropped and all drivers set > > the defaults before registering hw as what we do at e.g. at86rf230, > > see [0]. > > Is there a reason for refusing to call ->set_channel() at probe time? > because the current drivers work the way to not set the channel/page during probe time. Also the 802.15.4 spec states that this default value is hardware specific and this goes back whatever the vendor decides. Also you drop the check that if the same channel is already set don't set it which works like a shadow register for registers. Is there a reason why to set a channel during probe time? Are you setting the value which is the default one again? If the driver has a random default value it should choose some and stick to one, the others do whatever the datasheet has after resetting the hardware. I really don't see the sense here why every driver should introduce on driver level a set channel call. At probing time the transceiver registers are already in a state which we should reflect. > Anyway, I'll put the symbol duration setting in the registration helper > and I will fix hwsim aside. > ok, thanks. - Alex
diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c index fbeebe3bc31d..870e442bbff0 100644 --- a/net/mac802154/cfg.c +++ b/net/mac802154/cfg.c @@ -103,17 +103,13 @@ ieee802154_del_iface(struct wpan_phy *wpan_phy, struct wpan_dev *wpan_dev) } static int -ieee802154_set_channel(struct wpan_phy *wpan_phy, u8 page, u8 channel) +ieee802154_change_channel(struct wpan_phy *wpan_phy, u8 page, u8 channel) { struct ieee802154_local *local = wpan_phy_priv(wpan_phy); int ret; ASSERT_RTNL(); - if (wpan_phy->current_page == page && - wpan_phy->current_channel == channel) - return 0; - ret = drv_set_channel(local, page, channel); if (!ret) { wpan_phy->current_page = page; @@ -123,6 +119,18 @@ ieee802154_set_channel(struct wpan_phy *wpan_phy, u8 page, u8 channel) return ret; } +static int +ieee802154_set_channel(struct wpan_phy *wpan_phy, u8 page, u8 channel) +{ + ASSERT_RTNL(); + + if (wpan_phy->current_page == page && + wpan_phy->current_channel == channel) + return 0; + + return ieee802154_change_channel(wpan_phy, page, channel); +} + static int ieee802154_set_cca_mode(struct wpan_phy *wpan_phy, const struct wpan_phy_cca *cca)
As it is currently designed, the set_channel() cfg802154 hook implemented in the softMAC is doing a couple of checks before actually performing the channel change. However, as we enhance the support for automatically setting the symbol duration during channel changes, it will also be needed to ensure that the corresponding channel as properly be selected at probe time. In order to verify this, we will need to separate the channel change from the previous checks. These checks are actually here to speed up the process when the user request necessitates no change, so we can safely bypass them. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- net/mac802154/cfg.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)