diff mbox series

[v3,07/41] net: ieee802154: mcr20a: Fix lifs/sifs periods

Message ID 20220117115440.60296-8-miquel.raynal@bootlin.com (mailing list archive)
State Not Applicable
Delegated to: Johannes Berg
Headers show
Series IEEE 802.15.4 scan support | expand

Commit Message

Miquel Raynal Jan. 17, 2022, 11:54 a.m. UTC
These periods are expressed in time units (microseconds) while 40 and 12
are the number of symbol durations these periods will last. We need to
multiply them both with phy->symbol_duration in order to get these
values in microseconds.

Fixes: 8c6ad9cc5157 ("ieee802154: Add NXP MCR20A IEEE 802.15.4 transceiver driver")
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/net/ieee802154/mcr20a.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Alexander Aring Jan. 17, 2022, 10:52 p.m. UTC | #1
Hi,

On Mon, 17 Jan 2022 at 06:54, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> These periods are expressed in time units (microseconds) while 40 and 12
> are the number of symbol durations these periods will last. We need to
> multiply them both with phy->symbol_duration in order to get these
> values in microseconds.
>
> Fixes: 8c6ad9cc5157 ("ieee802154: Add NXP MCR20A IEEE 802.15.4 transceiver driver")
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/net/ieee802154/mcr20a.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ieee802154/mcr20a.c b/drivers/net/ieee802154/mcr20a.c
> index f0eb2d3b1c4e..e2c249aef430 100644
> --- a/drivers/net/ieee802154/mcr20a.c
> +++ b/drivers/net/ieee802154/mcr20a.c
> @@ -976,8 +976,8 @@ static void mcr20a_hw_setup(struct mcr20a_local *lp)
>         dev_dbg(printdev(lp), "%s\n", __func__);
>
>         phy->symbol_duration = 16;
> -       phy->lifs_period = 40;
> -       phy->sifs_period = 12;
> +       phy->lifs_period = 40 * phy->symbol_duration;
> +       phy->sifs_period = 12 * phy->symbol_duration;

I thought we do that now in register_hw(). Why does this patch exist?

- Alex
Miquel Raynal Jan. 18, 2022, 6:20 p.m. UTC | #2
Hi Alexander,

alex.aring@gmail.com wrote on Mon, 17 Jan 2022 17:52:10 -0500:

> Hi,
> 
> On Mon, 17 Jan 2022 at 06:54, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > These periods are expressed in time units (microseconds) while 40 and 12
> > are the number of symbol durations these periods will last. We need to
> > multiply them both with phy->symbol_duration in order to get these
> > values in microseconds.
> >
> > Fixes: 8c6ad9cc5157 ("ieee802154: Add NXP MCR20A IEEE 802.15.4 transceiver driver")
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/net/ieee802154/mcr20a.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ieee802154/mcr20a.c b/drivers/net/ieee802154/mcr20a.c
> > index f0eb2d3b1c4e..e2c249aef430 100644
> > --- a/drivers/net/ieee802154/mcr20a.c
> > +++ b/drivers/net/ieee802154/mcr20a.c
> > @@ -976,8 +976,8 @@ static void mcr20a_hw_setup(struct mcr20a_local *lp)
> >         dev_dbg(printdev(lp), "%s\n", __func__);
> >
> >         phy->symbol_duration = 16;
> > -       phy->lifs_period = 40;
> > -       phy->sifs_period = 12;
> > +       phy->lifs_period = 40 * phy->symbol_duration;
> > +       phy->sifs_period = 12 * phy->symbol_duration;  
> 
> I thought we do that now in register_hw(). Why does this patch exist?

The lifs and sifs period are wrong.

Fixing this silently by generalizing the calculation is simply wrong. I
feel we need to do this in order:
1- Fix the period because it is wrong.
2- Now that the period is set to a valid value and the core is able to
   do the same operation and set the variables to an identical content,
   we can drop these lines from the driver. 

#2 being a mechanical change, doing it without #1 means that something
that appears harmless actually changes the behavior of the driver. We
generally try to avoid that, no?

Thanks,
Miquèl
Alexander Aring Jan. 18, 2022, 10:53 p.m. UTC | #3
Hi,

On Tue, 18 Jan 2022 at 13:20, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> alex.aring@gmail.com wrote on Mon, 17 Jan 2022 17:52:10 -0500:
>
> > Hi,
> >
> > On Mon, 17 Jan 2022 at 06:54, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > These periods are expressed in time units (microseconds) while 40 and 12
> > > are the number of symbol durations these periods will last. We need to
> > > multiply them both with phy->symbol_duration in order to get these
> > > values in microseconds.
> > >
> > > Fixes: 8c6ad9cc5157 ("ieee802154: Add NXP MCR20A IEEE 802.15.4 transceiver driver")
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  drivers/net/ieee802154/mcr20a.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ieee802154/mcr20a.c b/drivers/net/ieee802154/mcr20a.c
> > > index f0eb2d3b1c4e..e2c249aef430 100644
> > > --- a/drivers/net/ieee802154/mcr20a.c
> > > +++ b/drivers/net/ieee802154/mcr20a.c
> > > @@ -976,8 +976,8 @@ static void mcr20a_hw_setup(struct mcr20a_local *lp)
> > >         dev_dbg(printdev(lp), "%s\n", __func__);
> > >
> > >         phy->symbol_duration = 16;
> > > -       phy->lifs_period = 40;
> > > -       phy->sifs_period = 12;
> > > +       phy->lifs_period = 40 * phy->symbol_duration;
> > > +       phy->sifs_period = 12 * phy->symbol_duration;
> >
> > I thought we do that now in register_hw(). Why does this patch exist?
>
> The lifs and sifs period are wrong.
>
> Fixing this silently by generalizing the calculation is simply wrong. I
> feel we need to do this in order:
> 1- Fix the period because it is wrong.
> 2- Now that the period is set to a valid value and the core is able to
>    do the same operation and set the variables to an identical content,
>    we can drop these lines from the driver.
>
> #2 being a mechanical change, doing it without #1 means that something
> that appears harmless actually changes the behavior of the driver. We
> generally try to avoid that, no?

yes, maybe Stefan can get this patch then somehow to wpan and queue it
for stable.

Thanks for clarification.

- Alex
diff mbox series

Patch

diff --git a/drivers/net/ieee802154/mcr20a.c b/drivers/net/ieee802154/mcr20a.c
index f0eb2d3b1c4e..e2c249aef430 100644
--- a/drivers/net/ieee802154/mcr20a.c
+++ b/drivers/net/ieee802154/mcr20a.c
@@ -976,8 +976,8 @@  static void mcr20a_hw_setup(struct mcr20a_local *lp)
 	dev_dbg(printdev(lp), "%s\n", __func__);
 
 	phy->symbol_duration = 16;
-	phy->lifs_period = 40;
-	phy->sifs_period = 12;
+	phy->lifs_period = 40 * phy->symbol_duration;
+	phy->sifs_period = 12 * phy->symbol_duration;
 
 	hw->flags = IEEE802154_HW_TX_OMIT_CKSUM |
 			IEEE802154_HW_AFILT |