| Message ID | 20220128110825.1120678-6-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| 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, > --- a/drivers/net/ieee802154/ca8210.c > +++ b/drivers/net/ieee802154/ca8210.c > @@ -2978,7 +2978,6 @@ static void ca8210_hw_setup(struct ieee802154_hw *ca8210_hw) > ca8210_hw->phy->cca.mode = NL802154_CCA_ENERGY_CARRIER; > ca8210_hw->phy->cca.opt = NL802154_CCA_OPT_ENERGY_CARRIER_AND; > ca8210_hw->phy->cca_ed_level = -9800; > - ca8210_hw->phy->symbol_duration = 16 * NSEC_PER_USEC; > ca8210_hw->phy->lifs_period = 40; > ca8210_hw->phy->sifs_period = 12; I've missed that error ^^ This driver should be fixed first (that's probably a copy/paste of the error from the other driver which did the same). As the rest of the series will depend on this fix (or conflict) we could merge it through wpan-next anyway, if you don't mind, as it was there since 2017 and these numbers had no real impact so far (I believe). I just figure this out now while searching for leftovers after a rebase operation, sorry. Thanks, Miquèl
Hello. On 01.02.22 18:40, Miquel Raynal wrote: > Hi, > >> --- a/drivers/net/ieee802154/ca8210.c >> +++ b/drivers/net/ieee802154/ca8210.c >> @@ -2978,7 +2978,6 @@ static void ca8210_hw_setup(struct ieee802154_hw *ca8210_hw) >> ca8210_hw->phy->cca.mode = NL802154_CCA_ENERGY_CARRIER; >> ca8210_hw->phy->cca.opt = NL802154_CCA_OPT_ENERGY_CARRIER_AND; >> ca8210_hw->phy->cca_ed_level = -9800; >> - ca8210_hw->phy->symbol_duration = 16 * NSEC_PER_USEC; >> ca8210_hw->phy->lifs_period = 40; >> ca8210_hw->phy->sifs_period = 12; > > I've missed that error ^^ > > This driver should be fixed first (that's probably a copy/paste of the > error from the other driver which did the same). > > As the rest of the series will depend on this fix (or conflict) we could > merge it through wpan-next anyway, if you don't mind, as it was there > since 2017 and these numbers had no real impact so far (I believe). Not sure I follow this logic. The fix you do is being removed in 4/4 of your v3 set again. So it would only be in place for these two in between commits. As you laid out above this has been in place since 2017 and the number have no real impact. Getting the fix in wpan-next to remove it again two patches later would not be needed here. If you would like to have this fixed for 5.16 and older stable kernels I could go ahead and apply it to wpan and let it trickle down into stable trees. We would have to deal with either a merge of net into net-next or with a merge conflicts when sending the pull request. Both can be done. But given the circumstances above I have no problem to drop this fix completely and have it fixed implicitly with the rest of the patchset. regards Stefan Schmidt
Hi Stefan, stefan@datenfreihafen.org wrote on Tue, 1 Feb 2022 21:51:04 +0100: > Hello. > > On 01.02.22 18:40, Miquel Raynal wrote: > > Hi, > > > >> --- a/drivers/net/ieee802154/ca8210.c > >> +++ b/drivers/net/ieee802154/ca8210.c > >> @@ -2978,7 +2978,6 @@ static void ca8210_hw_setup(struct ieee802154_hw *ca8210_hw) > >> ca8210_hw->phy->cca.mode = NL802154_CCA_ENERGY_CARRIER; > >> ca8210_hw->phy->cca.opt = NL802154_CCA_OPT_ENERGY_CARRIER_AND; > >> ca8210_hw->phy->cca_ed_level = -9800; > >> - ca8210_hw->phy->symbol_duration = 16 * NSEC_PER_USEC; > >> ca8210_hw->phy->lifs_period = 40; > >> ca8210_hw->phy->sifs_period = 12; > > > > I've missed that error ^^ > > > > This driver should be fixed first (that's probably a copy/paste of the > > error from the other driver which did the same). > > > > As the rest of the series will depend on this fix (or conflict) we could > > merge it through wpan-next anyway, if you don't mind, as it was there > > since 2017 and these numbers had no real impact so far (I believe). > > Not sure I follow this logic. The fix you do is being removed in 4/4 of your v3 set again. So it would only be in place for these two in between commits. Exactly. > As you laid out above this has been in place since 2017 and the number have no real impact. Getting the fix in wpan-next to remove it again two patches later would not be needed here. > > If you would like to have this fixed for 5.16 and older stable kernels I could go ahead and apply it to wpan and let it trickle down into stable trees. I'm fine "ignoring" the issue in stable kernels, it was just a warning for you that this would happen otherwise, given the fact that this is the second driver doing so (first fix has already been merged) and that I just realized it now. > We would have to deal with either a merge of net into net-next or with > a merge conflicts when sending the pull request. Both can be done. > > But given the circumstances above I have no problem to drop this fix completely and have it fixed implicitly with the rest of the patchset. Fine by me! Thanks, Miquèl
Hello. On 02.02.22 08:40, Miquel Raynal wrote: > Hi Stefan, > > stefan@datenfreihafen.org wrote on Tue, 1 Feb 2022 21:51:04 +0100: > >> Hello. >> >> On 01.02.22 18:40, Miquel Raynal wrote: >>> Hi, >>> >>>> --- a/drivers/net/ieee802154/ca8210.c >>>> +++ b/drivers/net/ieee802154/ca8210.c >>>> @@ -2978,7 +2978,6 @@ static void ca8210_hw_setup(struct ieee802154_hw *ca8210_hw) >>>> ca8210_hw->phy->cca.mode = NL802154_CCA_ENERGY_CARRIER; >>>> ca8210_hw->phy->cca.opt = NL802154_CCA_OPT_ENERGY_CARRIER_AND; >>>> ca8210_hw->phy->cca_ed_level = -9800; >>>> - ca8210_hw->phy->symbol_duration = 16 * NSEC_PER_USEC; >>>> ca8210_hw->phy->lifs_period = 40; >>>> ca8210_hw->phy->sifs_period = 12; >>> >>> I've missed that error ^^ >>> >>> This driver should be fixed first (that's probably a copy/paste of the >>> error from the other driver which did the same). >>> >>> As the rest of the series will depend on this fix (or conflict) we could >>> merge it through wpan-next anyway, if you don't mind, as it was there >>> since 2017 and these numbers had no real impact so far (I believe). >> >> Not sure I follow this logic. The fix you do is being removed in 4/4 of your v3 set again. So it would only be in place for these two in between commits. > > Exactly. > >> As you laid out above this has been in place since 2017 and the number have no real impact. Getting the fix in wpan-next to remove it again two patches later would not be needed here. >> >> If you would like to have this fixed for 5.16 and older stable kernels I could go ahead and apply it to wpan and let it trickle down into stable trees. > > I'm fine "ignoring" the issue in stable kernels, it was just a warning > for you that this would happen otherwise, given the fact that this is > the second driver doing so (first fix has already been merged) and that > I just realized it now. > >> We would have to deal with either a merge of net into net-next or with >> a merge conflicts when sending the pull request. Both can be done. >> >> But given the circumstances above I have no problem to drop this fix completely and have it fixed implicitly with the rest of the patchset. > > Fine by me! Let's do it like this. You drop it from this series against wpan-next. I will pull it out of the series and apply to wpan directly. That way we get it into the stable kernels as well. You already did the work so we should not waste it. I will deal with the merge conflict get get between wpan/net and wpan-next/net-next on my side. Nothing to worry for you. regards Stefan Schmidt
Hi Stefan, stefan@datenfreihafen.org wrote on Wed, 2 Feb 2022 13:17:39 +0100: > Hello. > > On 02.02.22 08:40, Miquel Raynal wrote: > > Hi Stefan, > > > > stefan@datenfreihafen.org wrote on Tue, 1 Feb 2022 21:51:04 +0100: > > > >> Hello. > >> > >> On 01.02.22 18:40, Miquel Raynal wrote: > >>> Hi, > >>> >>>> --- a/drivers/net/ieee802154/ca8210.c > >>>> +++ b/drivers/net/ieee802154/ca8210.c > >>>> @@ -2978,7 +2978,6 @@ static void ca8210_hw_setup(struct ieee802154_hw *ca8210_hw) > >>>> ca8210_hw->phy->cca.mode = NL802154_CCA_ENERGY_CARRIER; > >>>> ca8210_hw->phy->cca.opt = NL802154_CCA_OPT_ENERGY_CARRIER_AND; > >>>> ca8210_hw->phy->cca_ed_level = -9800; > >>>> - ca8210_hw->phy->symbol_duration = 16 * NSEC_PER_USEC; > >>>> ca8210_hw->phy->lifs_period = 40; > >>>> ca8210_hw->phy->sifs_period = 12; > >>> > >>> I've missed that error ^^ > >>> > >>> This driver should be fixed first (that's probably a copy/paste of the > >>> error from the other driver which did the same). > >>> > >>> As the rest of the series will depend on this fix (or conflict) we could > >>> merge it through wpan-next anyway, if you don't mind, as it was there > >>> since 2017 and these numbers had no real impact so far (I believe). > >> > >> Not sure I follow this logic. The fix you do is being removed in 4/4 of your v3 set again. So it would only be in place for these two in between commits. > > > > Exactly. > > > >> As you laid out above this has been in place since 2017 and the number have no real impact. Getting the fix in wpan-next to remove it again two patches later would not be needed here. > >> > >> If you would like to have this fixed for 5.16 and older stable kernels I could go ahead and apply it to wpan and let it trickle down into stable trees. > > > > I'm fine "ignoring" the issue in stable kernels, it was just a warning > > for you that this would happen otherwise, given the fact that this is > > the second driver doing so (first fix has already been merged) and that > > I just realized it now. > > > >> We would have to deal with either a merge of net into net-next or with > >> a merge conflicts when sending the pull request. Both can be done. > >> > >> But given the circumstances above I have no problem to drop this fix completely and have it fixed implicitly with the rest of the patchset. > > > > Fine by me! > > Let's do it like this. > You drop it from this series against wpan-next. > I will pull it out of the series and apply to wpan directly. That way we get it into the stable kernels as well. You already did the work so we should not waste it. > I will deal with the merge conflict get get between wpan/net and wpan-next/net-next on my side. Nothing to worry for you. That's very kind, but don't feel forced to do that, I won't turn mad if you finally decide that this requires too much handling for such a short-in-time improvement ;) Thanks, Miquèl
Hello. On 02.02.22 14:50, Miquel Raynal wrote: > Hi Stefan, > > stefan@datenfreihafen.org wrote on Wed, 2 Feb 2022 13:17:39 +0100: > >> Hello. >> >> On 02.02.22 08:40, Miquel Raynal wrote: >>> Hi Stefan, >>> >>> stefan@datenfreihafen.org wrote on Tue, 1 Feb 2022 21:51:04 +0100: >>> >>>> Hello. >>>> >>>> On 01.02.22 18:40, Miquel Raynal wrote: >>>>> Hi, >>>>> >>>> --- a/drivers/net/ieee802154/ca8210.c >>>>>> +++ b/drivers/net/ieee802154/ca8210.c >>>>>> @@ -2978,7 +2978,6 @@ static void ca8210_hw_setup(struct ieee802154_hw *ca8210_hw) >>>>>> ca8210_hw->phy->cca.mode = NL802154_CCA_ENERGY_CARRIER; >>>>>> ca8210_hw->phy->cca.opt = NL802154_CCA_OPT_ENERGY_CARRIER_AND; >>>>>> ca8210_hw->phy->cca_ed_level = -9800; >>>>>> - ca8210_hw->phy->symbol_duration = 16 * NSEC_PER_USEC; >>>>>> ca8210_hw->phy->lifs_period = 40; >>>>>> ca8210_hw->phy->sifs_period = 12; >>>>> >>>>> I've missed that error ^^ >>>>> >>>>> This driver should be fixed first (that's probably a copy/paste of the >>>>> error from the other driver which did the same). >>>>> >>>>> As the rest of the series will depend on this fix (or conflict) we could >>>>> merge it through wpan-next anyway, if you don't mind, as it was there >>>>> since 2017 and these numbers had no real impact so far (I believe). >>>> >>>> Not sure I follow this logic. The fix you do is being removed in 4/4 of your v3 set again. So it would only be in place for these two in between commits. >>> >>> Exactly. >>> >>>> As you laid out above this has been in place since 2017 and the number have no real impact. Getting the fix in wpan-next to remove it again two patches later would not be needed here. >>>> >>>> If you would like to have this fixed for 5.16 and older stable kernels I could go ahead and apply it to wpan and let it trickle down into stable trees. >>> >>> I'm fine "ignoring" the issue in stable kernels, it was just a warning >>> for you that this would happen otherwise, given the fact that this is >>> the second driver doing so (first fix has already been merged) and that >>> I just realized it now. >>> >>>> We would have to deal with either a merge of net into net-next or with >>>> a merge conflicts when sending the pull request. Both can be done. >>>> >>>> But given the circumstances above I have no problem to drop this fix completely and have it fixed implicitly with the rest of the patchset. >>> >>> Fine by me! >> >> Let's do it like this. >> You drop it from this series against wpan-next. >> I will pull it out of the series and apply to wpan directly. That way we get it into the stable kernels as well. You already did the work so we should not waste it. >> I will deal with the merge conflict get get between wpan/net and wpan-next/net-next on my side. Nothing to worry for you. > > That's very kind, but don't feel forced to do that, I won't turn mad if > you finally decide that this requires too much handling for such a > short-in-time improvement ;) Nah, don't worry. I just applied it to wpan. It's the right thing to do as we simply do not know who is using the driving in what situation. Holding off a fix that you already did would be silly. I deal with the merge conflict when it comes to it. regards Stefan Schmidt
diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c index b08f08475426..9f3b4a8d151a 100644 --- a/drivers/net/ieee802154/at86rf230.c +++ b/drivers/net/ieee802154/at86rf230.c @@ -1064,36 +1064,6 @@ at86rf212_set_channel(struct at86rf230_local *lp, u8 page, u8 channel) if (rc < 0) return rc; - /* This sets the symbol_duration according frequency on the 212. - * TODO move this handling while set channel and page in cfg802154. - * We can do that, this timings are according 802.15.4 standard. - * If we do that in cfg802154, this is a more generic calculation. - * - * This should also protected from ifs_timer. Means cancel timer and - * init with a new value. For now, this is okay. - */ - if (channel == 0) { - if (page == 0) { - /* SUB:0 and BPSK:0 -> BPSK-20 */ - lp->hw->phy->symbol_duration = 50 * NSEC_PER_USEC; - } else { - /* SUB:1 and BPSK:0 -> BPSK-40 */ - lp->hw->phy->symbol_duration = 25 * NSEC_PER_USEC; - } - } else { - if (page == 0) - /* SUB:0 and BPSK:1 -> OQPSK-100/200/400 */ - lp->hw->phy->symbol_duration = 40 * NSEC_PER_USEC; - else - /* SUB:1 and BPSK:1 -> OQPSK-250/500/1000 */ - lp->hw->phy->symbol_duration = 16 * NSEC_PER_USEC; - } - - lp->hw->phy->lifs_period = - (IEEE802154_LIFS_PERIOD * lp->hw->phy->symbol_duration) / 1000; - lp->hw->phy->sifs_period = - (IEEE802154_SIFS_PERIOD * lp->hw->phy->symbol_duration) / 1000; - return at86rf230_write_subreg(lp, SR_CHANNEL, channel); } @@ -1572,7 +1542,6 @@ at86rf230_detect_device(struct at86rf230_local *lp) lp->hw->phy->supported.page[0].chunk[0].protocol = IEEE802154_OQPSK_PHY; lp->hw->phy->supported.page[0].chunk[0].band = IEEE802154_2400_MHZ_BAND; lp->hw->phy->current_channel = 11; - lp->hw->phy->symbol_duration = 16 * NSEC_PER_USEC; lp->hw->phy->supported.tx_powers = at86rf231_powers; lp->hw->phy->supported.tx_powers_size = ARRAY_SIZE(at86rf231_powers); lp->hw->phy->supported.cca_ed_levels = at86rf231_ed_levels; @@ -1604,7 +1573,6 @@ at86rf230_detect_device(struct at86rf230_local *lp) lp->hw->phy->supported.page[2].chunk[1].band = IEEE802154_915_MHZ_BAND; lp->hw->phy->current_channel = 5; - lp->hw->phy->symbol_duration = 25 * NSEC_PER_USEC; lp->hw->phy->supported.lbt = NL802154_SUPPORTED_BOOL_BOTH; lp->hw->phy->supported.tx_powers = at86rf212_powers; lp->hw->phy->supported.tx_powers_size = ARRAY_SIZE(at86rf212_powers); @@ -1619,7 +1587,6 @@ at86rf230_detect_device(struct at86rf230_local *lp) lp->hw->phy->supported.page[0].chunk[0].protocol = IEEE802154_OQPSK_PHY; lp->hw->phy->supported.page[0].chunk[0].band = IEEE802154_2400_MHZ_BAND; lp->hw->phy->current_channel = 13; - lp->hw->phy->symbol_duration = 16 * NSEC_PER_USEC; lp->hw->phy->supported.tx_powers = at86rf233_powers; lp->hw->phy->supported.tx_powers_size = ARRAY_SIZE(at86rf233_powers); lp->hw->phy->supported.cca_ed_levels = at86rf233_ed_levels; diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c index 80382919520e..1a56073c1c52 100644 --- a/drivers/net/ieee802154/atusb.c +++ b/drivers/net/ieee802154/atusb.c @@ -667,36 +667,6 @@ static int hulusb_set_channel(struct ieee802154_hw *hw, u8 page, u8 channel) if (rc < 0) return rc; - /* This sets the symbol_duration according frequency on the 212. - * TODO move this handling while set channel and page in cfg802154. - * We can do that, this timings are according 802.15.4 standard. - * If we do that in cfg802154, this is a more generic calculation. - * - * This should also protected from ifs_timer. Means cancel timer and - * init with a new value. For now, this is okay. - */ - if (channel == 0) { - if (page == 0) { - /* SUB:0 and BPSK:0 -> BPSK-20 */ - lp->hw->phy->symbol_duration = 50 * NSEC_PER_USEC; - } else { - /* SUB:1 and BPSK:0 -> BPSK-40 */ - lp->hw->phy->symbol_duration = 25 * NSEC_PER_USEC; - } - } else { - if (page == 0) - /* SUB:0 and BPSK:1 -> OQPSK-100/200/400 */ - lp->hw->phy->symbol_duration = 40 * NSEC_PER_USEC; - else - /* SUB:1 and BPSK:1 -> OQPSK-250/500/1000 */ - lp->hw->phy->symbol_duration = 16 * NSEC_PER_USEC; - } - - lp->hw->phy->lifs_period = - (IEEE802154_LIFS_PERIOD * lp->hw->phy->symbol_duration) / 1000; - lp->hw->phy->sifs_period = - (IEEE802154_SIFS_PERIOD * lp->hw->phy->symbol_duration) / 1000; - return atusb_write_subreg(lp, SR_CHANNEL, channel); } @@ -919,7 +889,6 @@ static int atusb_get_and_conf_chip(struct atusb *atusb) atusb->hw->phy->supported.page[0].chunk[0].protocol = IEEE802154_OQPSK_PHY; atusb->hw->phy->supported.page[0].chunk[0].band = IEEE802154_2400_MHZ_BAND; atusb->hw->phy->current_channel = 11; /* reset default */ - atusb->hw->phy->symbol_duration = 16 * NSEC_PER_USEC; atusb->hw->phy->supported.tx_powers = atusb_powers; atusb->hw->phy->supported.tx_powers_size = ARRAY_SIZE(atusb_powers); hw->phy->supported.cca_ed_levels = atusb_ed_levels; @@ -932,7 +901,6 @@ static int atusb_get_and_conf_chip(struct atusb *atusb) atusb->hw->phy->supported.page[0].chunk[0].protocol = IEEE802154_OQPSK_PHY; atusb->hw->phy->supported.page[0].chunk[0].band = IEEE802154_2400_MHZ_BAND; atusb->hw->phy->current_channel = 11; /* reset default */ - atusb->hw->phy->symbol_duration = 16 * NSEC_PER_USEC; atusb->hw->phy->supported.tx_powers = atusb_powers; atusb->hw->phy->supported.tx_powers_size = ARRAY_SIZE(atusb_powers); hw->phy->supported.cca_ed_levels = atusb_ed_levels; @@ -963,7 +931,6 @@ static int atusb_get_and_conf_chip(struct atusb *atusb) atusb->hw->phy->supported.page[2].chunk[1].band = IEEE802154_915_MHZ_BAND; atusb->hw->phy->current_channel = 5; - atusb->hw->phy->symbol_duration = 25 * NSEC_PER_USEC; atusb->hw->phy->supported.lbt = NL802154_SUPPORTED_BOOL_BOTH; atusb->hw->phy->supported.tx_powers = at86rf212_powers; atusb->hw->phy->supported.tx_powers_size = ARRAY_SIZE(at86rf212_powers); diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c index 8eb48a8014db..baa4392d20c2 100644 --- a/drivers/net/ieee802154/ca8210.c +++ b/drivers/net/ieee802154/ca8210.c @@ -2978,7 +2978,6 @@ static void ca8210_hw_setup(struct ieee802154_hw *ca8210_hw) ca8210_hw->phy->cca.mode = NL802154_CCA_ENERGY_CARRIER; ca8210_hw->phy->cca.opt = NL802154_CCA_OPT_ENERGY_CARRIER_AND; ca8210_hw->phy->cca_ed_level = -9800; - ca8210_hw->phy->symbol_duration = 16 * NSEC_PER_USEC; ca8210_hw->phy->lifs_period = 40; ca8210_hw->phy->sifs_period = 12; ca8210_hw->flags = diff --git a/drivers/net/ieee802154/mcr20a.c b/drivers/net/ieee802154/mcr20a.c index 34063b7e663e..8d12c2968302 100644 --- a/drivers/net/ieee802154/mcr20a.c +++ b/drivers/net/ieee802154/mcr20a.c @@ -975,10 +975,6 @@ static void mcr20a_hw_setup(struct mcr20a_local *lp) dev_dbg(printdev(lp), "%s\n", __func__); - phy->symbol_duration = 16 * NSEC_PER_USEC; - phy->lifs_period = (40 * phy->symbol_duration) / NSEC_PER_USEC; - phy->sifs_period = (12 * phy->symbol_duration) / NSEC_PER_USEC; - hw->flags = IEEE802154_HW_TX_OMIT_CKSUM | IEEE802154_HW_AFILT | IEEE802154_HW_PROMISCUOUS; @@ -1010,7 +1006,6 @@ static void mcr20a_hw_setup(struct mcr20a_local *lp) phy->current_page = 0; /* MCR20A default reset value */ phy->current_channel = 20; - phy->symbol_duration = 16 * NSEC_PER_USEC; phy->supported.tx_powers = mcr20a_powers; phy->supported.tx_powers_size = ARRAY_SIZE(mcr20a_powers); phy->cca_ed_level = phy->supported.cca_ed_levels[75];
The core now knows how to set the symbol duration in a few cases, when drivers correctly advertise the protocols used on each channel. For these drivers, there is no more need to bother with symbol duration, lifs and sifs periods so just drop the duplicated code. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/net/ieee802154/at86rf230.c | 33 ------------------------------ drivers/net/ieee802154/atusb.c | 33 ------------------------------ drivers/net/ieee802154/ca8210.c | 1 - drivers/net/ieee802154/mcr20a.c | 5 ----- 4 files changed, 72 deletions(-)