| Message ID | 20211222155743.256280-2-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | IEEE 802.15.4 passive scan support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
Hi, On Wed, 22 Dec 2021 at 10:57, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > A default channel is selected by default (13), let's clarify that this > is page 0 channel 13. Call the right helper to ensure the necessary > configuration for this channel has been applied. > > So far there is very little configuration done in this helper but we > will soon add more information (like the symbol duration which is > missing) and having this helper called at probe time will prevent us to > this type of initialization at two different locations. > I see why this patch is necessary because in later patches the symbol duration is set at ".set_channel()" callback like the at86rf230 driver is doing it. However there is an old TODO [0]. I think we should combine it and implement it in ieee802154_set_channel() of "net/mac802154/cfg.c". Also do the symbol duration setting according to the channel/page when we call ieee802154_register_hw(), so we have it for the default settings. > So far there is very little configuration done in this helper but thanks > to this improvement, future enhancements in this area (like setting a > symbol duration, which is missing) will be reflected automatically in > the default probe state. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/net/ieee802154/mac802154_hwsim.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c > index 62ced7a30d92..b1a4ee7dceda 100644 > --- a/drivers/net/ieee802154/mac802154_hwsim.c > +++ b/drivers/net/ieee802154/mac802154_hwsim.c > @@ -778,8 +778,6 @@ static int hwsim_add_one(struct genl_info *info, struct device *dev, > > ieee802154_random_extended_addr(&hw->phy->perm_extended_addr); > > - /* hwsim phy channel 13 as default */ > - hw->phy->current_channel = 13; > pib = kzalloc(sizeof(*pib), GFP_KERNEL); > if (!pib) { > err = -ENOMEM; > @@ -793,6 +791,11 @@ static int hwsim_add_one(struct genl_info *info, struct device *dev, > hw->flags = IEEE802154_HW_PROMISCUOUS | IEEE802154_HW_RX_DROP_BAD_CKSUM; sadly this patch doesn't apply on current net-next/master because IEEE802154_HW_RX_DROP_BAD_CKSUM is not set. I agree that it should be set, so we need a patch for it. - Alex [0] https://elixir.bootlin.com/linux/v5.16-rc7/source/drivers/net/ieee802154/at86rf230.c#L1059
Hi Alexander, alex.aring@gmail.com wrote on Tue, 28 Dec 2021 16:05:43 -0500: > Hi, > > On Wed, 22 Dec 2021 at 10:57, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > A default channel is selected by default (13), let's clarify that this > > is page 0 channel 13. Call the right helper to ensure the necessary > > configuration for this channel has been applied. > > > > So far there is very little configuration done in this helper but we > > will soon add more information (like the symbol duration which is > > missing) and having this helper called at probe time will prevent us to > > this type of initialization at two different locations. > > > > I see why this patch is necessary because in later patches the symbol > duration is set at ".set_channel()" callback like the at86rf230 driver > is doing it. > However there is an old TODO [0]. I think we should combine it and > implement it in ieee802154_set_channel() of "net/mac802154/cfg.c". > Also do the symbol duration setting according to the channel/page when > we call ieee802154_register_hw(), so we have it for the default > settings. While I totally agree on the background idea, I don't really see how this is possible. Every driver internally knows what it supports but AFAIU the core itself has no easy and standard access to it? Another question that I have: is the protocol and center frequency enough to always derive the symbol rate? I am not sure this is correct, but I thought not all symbol rates could be derived, like for example certain UWB PHY protocols which can use different PRF on a single channel which has an effect on the symbol duration? > > So far there is very little configuration done in this helper but thanks > > to this improvement, future enhancements in this area (like setting a > > symbol duration, which is missing) will be reflected automatically in > > the default probe state. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/net/ieee802154/mac802154_hwsim.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c > > index 62ced7a30d92..b1a4ee7dceda 100644 > > --- a/drivers/net/ieee802154/mac802154_hwsim.c > > +++ b/drivers/net/ieee802154/mac802154_hwsim.c > > @@ -778,8 +778,6 @@ static int hwsim_add_one(struct genl_info *info, struct device *dev, > > > > ieee802154_random_extended_addr(&hw->phy->perm_extended_addr); > > > > - /* hwsim phy channel 13 as default */ > > - hw->phy->current_channel = 13; > > pib = kzalloc(sizeof(*pib), GFP_KERNEL); > > if (!pib) { > > err = -ENOMEM; > > @@ -793,6 +791,11 @@ static int hwsim_add_one(struct genl_info *info, struct device *dev, > > hw->flags = IEEE802154_HW_PROMISCUOUS | IEEE802154_HW_RX_DROP_BAD_CKSUM; > > sadly this patch doesn't apply on current net-next/master because > IEEE802154_HW_RX_DROP_BAD_CKSUM is not set. > I agree that it should be set, so we need a patch for it. Right, I just have a patch aside setting this to enforce beacons checksum were good. I can certainly set this flag officially. > > - Alex > > [0] https://elixir.bootlin.com/linux/v5.16-rc7/source/drivers/net/ieee802154/at86rf230.c#L1059 Thanks, Miquèl
Hi, On Tue, 4 Jan 2022 at 10:44, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > alex.aring@gmail.com wrote on Tue, 28 Dec 2021 16:05:43 -0500: > > > Hi, > > > > On Wed, 22 Dec 2021 at 10:57, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > A default channel is selected by default (13), let's clarify that this > > > is page 0 channel 13. Call the right helper to ensure the necessary > > > configuration for this channel has been applied. > > > > > > So far there is very little configuration done in this helper but we > > > will soon add more information (like the symbol duration which is > > > missing) and having this helper called at probe time will prevent us to > > > this type of initialization at two different locations. > > > > > > > I see why this patch is necessary because in later patches the symbol > > duration is set at ".set_channel()" callback like the at86rf230 driver > > is doing it. > > However there is an old TODO [0]. I think we should combine it and > > implement it in ieee802154_set_channel() of "net/mac802154/cfg.c". > > Also do the symbol duration setting according to the channel/page when > > we call ieee802154_register_hw(), so we have it for the default > > settings. > > While I totally agree on the background idea, I don't really see how > this is possible. Every driver internally knows what it supports but > AFAIU the core itself has no easy and standard access to it? > I am a little bit confused here, because a lot of timing related things in the phy information rate points to "x times symbols". If this value depends on the transceiver, how are they compatible then? > Another question that I have: is the protocol and center frequency > enough to always derive the symbol rate? I am not sure this is correct, > but I thought not all symbol rates could be derived, like for example > certain UWB PHY protocols which can use different PRF on a single > channel which has an effect on the symbol duration? Regarding UWB PHY I see that for values like LIFS/SIFS they reference a "preambleSymbols" value which is defined. I need to do more research regarding this. - Alex
Hi, On Tue, 4 Jan 2022 at 18:08, Alexander Aring <alex.aring@gmail.com> wrote: > > Hi, > > On Tue, 4 Jan 2022 at 10:44, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Alexander, > > > > alex.aring@gmail.com wrote on Tue, 28 Dec 2021 16:05:43 -0500: > > > > > Hi, > > > > > > On Wed, 22 Dec 2021 at 10:57, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > A default channel is selected by default (13), let's clarify that this > > > > is page 0 channel 13. Call the right helper to ensure the necessary > > > > configuration for this channel has been applied. > > > > > > > > So far there is very little configuration done in this helper but we > > > > will soon add more information (like the symbol duration which is > > > > missing) and having this helper called at probe time will prevent us to > > > > this type of initialization at two different locations. > > > > > > > > > > I see why this patch is necessary because in later patches the symbol > > > duration is set at ".set_channel()" callback like the at86rf230 driver > > > is doing it. > > > However there is an old TODO [0]. I think we should combine it and > > > implement it in ieee802154_set_channel() of "net/mac802154/cfg.c". > > > Also do the symbol duration setting according to the channel/page when > > > we call ieee802154_register_hw(), so we have it for the default > > > settings. > > > > While I totally agree on the background idea, I don't really see how > > this is possible. Every driver internally knows what it supports but > > AFAIU the core itself has no easy and standard access to it? > > > > I am a little bit confused here, because a lot of timing related > things in the phy information rate points to "x times symbols". If s/rate/base/ - Alex
Hi Alexander, alex.aring@gmail.com wrote on Tue, 4 Jan 2022 18:10:44 -0500: > Hi, > > On Tue, 4 Jan 2022 at 18:08, Alexander Aring <alex.aring@gmail.com> wrote: > > > > Hi, > > > > On Tue, 4 Jan 2022 at 10:44, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > Hi Alexander, > > > > > > alex.aring@gmail.com wrote on Tue, 28 Dec 2021 16:05:43 -0500: > > > > > > > Hi, > > > > > > > > On Wed, 22 Dec 2021 at 10:57, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > > > A default channel is selected by default (13), let's clarify that this > > > > > is page 0 channel 13. Call the right helper to ensure the necessary > > > > > configuration for this channel has been applied. > > > > > > > > > > So far there is very little configuration done in this helper but we > > > > > will soon add more information (like the symbol duration which is > > > > > missing) and having this helper called at probe time will prevent us to > > > > > this type of initialization at two different locations. > > > > > > > > > > > > > I see why this patch is necessary because in later patches the symbol > > > > duration is set at ".set_channel()" callback like the at86rf230 driver > > > > is doing it. > > > > However there is an old TODO [0]. I think we should combine it and > > > > implement it in ieee802154_set_channel() of "net/mac802154/cfg.c". > > > > Also do the symbol duration setting according to the channel/page when > > > > we call ieee802154_register_hw(), so we have it for the default > > > > settings. > > > > > > While I totally agree on the background idea, I don't really see how > > > this is possible. Every driver internally knows what it supports but > > > AFAIU the core itself has no easy and standard access to it? > > > > > > > I am a little bit confused here, because a lot of timing related > > things in the phy information rate points to "x times symbols". If > > s/rate/base/ Yes indeed, but I bet it works because the phy drivers set the symbol duration by themselves. You can see that none of them does something clever like: switch (phy.protocol) { case XXXXX: symbol_duration = y; break; ... Instead, they all go through the page/channel list in a quite hardcoded way because the phy driver knows internally that protocol X is used on {page, channel}, but the protocol id, while not being totally absent from drivers, is always provided as a comment. So getting rid of the core TODO you mentioned earlier means: - Listing properly the PHY protocols in the core (if not already done) - For each PHY protocol knowing the possible base frequencies - For each of these base frequencies knowing the symbol duration - Having the possibility to add more information such as the PRF in order to let the core pick the right symbol duration when there is more than one possibility for a {protocol, base frequency} couple - Convert the phy drivers (at least hwsim) to fill these new fields correctly and expect the core to set the symbol duration properly. Two side notes as well: - I was not able to find all the the corresponding protocol from the hwsim driver in the spec (these channels are marked "unknown") - The symbol duration in a few specific UWB cases is below 1us while the core expects a value in us. Should we change the symbol duration to ns? I believe all this is doable in a reasonable time frame provided that I only focus on the few protocols supported by hwsim which I already "addressed" and perhaps a couple of simple drivers. On the core side, the logic might be: "is the driver providing information about the phy protocols used? if yes, then set the symbol duration if we have enough data, otherwise let the driver handle it by itself". Such logic would allow a progressive shift towards the situation where drivers do not have to bother with symbol duration by themselves. As this looks like a project on its own and my first goal was to be able to use hwsim to demonstrate the different scan features, maybe we can continue to discuss this and consider tackling it as a separate series whish would apply on top of the current one, what do you think? Thanks, Miquèl
Hi, On Wed, 5 Jan 2022 at 03:14, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > alex.aring@gmail.com wrote on Tue, 4 Jan 2022 18:10:44 -0500: > > > Hi, > > > > On Tue, 4 Jan 2022 at 18:08, Alexander Aring <alex.aring@gmail.com> wrote: > > > > > > Hi, > > > > > > On Tue, 4 Jan 2022 at 10:44, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > Hi Alexander, > > > > > > > > alex.aring@gmail.com wrote on Tue, 28 Dec 2021 16:05:43 -0500: > > > > > > > > > Hi, > > > > > > > > > > On Wed, 22 Dec 2021 at 10:57, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > > > > > A default channel is selected by default (13), let's clarify that this > > > > > > is page 0 channel 13. Call the right helper to ensure the necessary > > > > > > configuration for this channel has been applied. > > > > > > > > > > > > So far there is very little configuration done in this helper but we > > > > > > will soon add more information (like the symbol duration which is > > > > > > missing) and having this helper called at probe time will prevent us to > > > > > > this type of initialization at two different locations. > > > > > > > > > > > > > > > > I see why this patch is necessary because in later patches the symbol > > > > > duration is set at ".set_channel()" callback like the at86rf230 driver > > > > > is doing it. > > > > > However there is an old TODO [0]. I think we should combine it and > > > > > implement it in ieee802154_set_channel() of "net/mac802154/cfg.c". > > > > > Also do the symbol duration setting according to the channel/page when > > > > > we call ieee802154_register_hw(), so we have it for the default > > > > > settings. > > > > > > > > While I totally agree on the background idea, I don't really see how > > > > this is possible. Every driver internally knows what it supports but > > > > AFAIU the core itself has no easy and standard access to it? > > > > > > > > > > I am a little bit confused here, because a lot of timing related > > > things in the phy information rate points to "x times symbols". If > > > > s/rate/base/ > > Yes indeed, but I bet it works because the phy drivers set the symbol > duration by themselves. You can see that none of them does something > clever like: > > switch (phy.protocol) { > case XXXXX: > symbol_duration = y; > break; > ... > > Instead, they all go through the page/channel list in a quite hardcoded > way because the phy driver knows internally that protocol X is used on > {page, channel}, but the protocol id, while not being totally absent > from drivers, is always provided as a comment. > > So getting rid of the core TODO you mentioned earlier means: > - Listing properly the PHY protocols in the core (if not already done) > - For each PHY protocol knowing the possible base frequencies > - For each of these base frequencies knowing the symbol duration > - Having the possibility to add more information such as the PRF in > order to let the core pick the right symbol duration when there is > more than one possibility for a {protocol, base frequency} couple > - Convert the phy drivers (at least hwsim) to fill these new fields > correctly and expect the core to set the symbol duration properly. > I think this becomes quite large to provide all that information and somehow this reminds me about the other TODO to extend the wireless regdb with 802.15.4 specs and somehow let the kernel get the information from there. > Two side notes as well: > - I was not able to find all the the corresponding protocol from the > hwsim driver in the spec (these channels are marked "unknown") I think those frequencies were taken from the fakelb driver, I think currently that the static channel array has a limitation on its own to provide all channel/page combinations which could be set. > - The symbol duration in a few specific UWB cases is below 1us while > the core expects a value in us. Should we change the symbol duration > to ns? > At some point yes, I think we need to switch to ns. > I believe all this is doable in a reasonable time frame provided that > I only focus on the few protocols supported by hwsim which I already > "addressed" and perhaps a couple of simple drivers. On the core side, > the logic might be: "is the driver providing information about the phy > protocols used? if yes, then set the symbol duration if we have enough > data, otherwise let the driver handle it by itself". Such logic would > allow a progressive shift towards the situation where drivers do not > have to bother with symbol duration by themselves. > > As this looks like a project on its own and my first goal was to be > able to use hwsim to demonstrate the different scan features, maybe we > can continue to discuss this and consider tackling it as a separate > series whish would apply on top of the current one, what do you think? I think the current way that the driver sets is fine, we can still find other possible ways... for doing it better (e.g. regdb) is a new project, I agree with that. - Alex
diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c index 62ced7a30d92..b1a4ee7dceda 100644 --- a/drivers/net/ieee802154/mac802154_hwsim.c +++ b/drivers/net/ieee802154/mac802154_hwsim.c @@ -778,8 +778,6 @@ static int hwsim_add_one(struct genl_info *info, struct device *dev, ieee802154_random_extended_addr(&hw->phy->perm_extended_addr); - /* hwsim phy channel 13 as default */ - hw->phy->current_channel = 13; pib = kzalloc(sizeof(*pib), GFP_KERNEL); if (!pib) { err = -ENOMEM; @@ -793,6 +791,11 @@ static int hwsim_add_one(struct genl_info *info, struct device *dev, hw->flags = IEEE802154_HW_PROMISCUOUS | IEEE802154_HW_RX_DROP_BAD_CKSUM; hw->parent = dev; + /* Set page 0 / channel 13 as default */ + hw->phy->current_page = 0; + hw->phy->current_channel = 13; + hwsim_hw_channel(hw, hw->phy->current_page, hw->phy->current_channel); + err = ieee802154_register_hw(hw); if (err) goto err_reg;
A default channel is selected by default (13), let's clarify that this is page 0 channel 13. Call the right helper to ensure the necessary configuration for this channel has been applied. So far there is very little configuration done in this helper but we will soon add more information (like the symbol duration which is missing) and having this helper called at probe time will prevent us to this type of initialization at two different locations. So far there is very little configuration done in this helper but thanks to this improvement, future enhancements in this area (like setting a symbol duration, which is missing) will be reflected automatically in the default probe state. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/net/ieee802154/mac802154_hwsim.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)