2019-11-04 15:05:39

by Markus Theil

[permalink] [raw]
Subject: [PATCH] mt76: mt76x02: fix num slots in beacon config init

mt76x02 mmio and usb devices use a different number of beacon slots (8
vs. 5). Consider this in mt76x02_init_beacon_config.

Signed-off-by: Markus Theil <[email protected]>
---
drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c b/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c
index 4209209ac940..b7412953ff26 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c
@@ -249,7 +249,7 @@ void mt76x02_init_beacon_config(struct mt76x02_dev *dev)
mt76_set(dev, MT_BEACON_TIME_CFG, MT_BEACON_TIME_CFG_SYNC_MODE);
mt76_wr(dev, MT_BCN_BYPASS_MASK, 0xffff);

- for (i = 0; i < 8; i++)
+ for (i = 0; i < dev->beacon_ops->nslots; i++)
mt76x02_mac_set_beacon(dev, i, NULL);

mt76x02_set_beacon_offsets(dev);
--
2.17.1


2019-11-04 15:49:17

by Lorenzo Bianconi

[permalink] [raw]
Subject: Re: [PATCH] mt76: mt76x02: fix num slots in beacon config init

> mt76x02 mmio and usb devices use a different number of beacon slots (8
> vs. 5). Consider this in mt76x02_init_beacon_config.
>
> Signed-off-by: Markus Theil <[email protected]>
> ---
> drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c b/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c
> index 4209209ac940..b7412953ff26 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c
> @@ -249,7 +249,7 @@ void mt76x02_init_beacon_config(struct mt76x02_dev *dev)
> mt76_set(dev, MT_BEACON_TIME_CFG, MT_BEACON_TIME_CFG_SYNC_MODE);
> mt76_wr(dev, MT_BCN_BYPASS_MASK, 0xffff);
>
> - for (i = 0; i < 8; i++)
> + for (i = 0; i < dev->beacon_ops->nslots; i++)
> mt76x02_mac_set_beacon(dev, i, NULL);
>
> mt76x02_set_beacon_offsets(dev);

Hi Markus,

mt76x02_init_beacon_config is run just at bootstrap and it is used to clean all
beacon RAM memory. It can't see any issue with the current code.

Regards,
Lorenzo

> --
> 2.17.1
>


Attachments:
(No filename) (1.15 kB)
signature.asc (235.00 B)
Download all attachments

2019-11-04 16:10:36

by Markus Theil

[permalink] [raw]
Subject: Re: [PATCH] mt76: mt76x02: fix num slots in beacon config init

On 04.11.19 16:45, Lorenzo Bianconi wrote:
>> mt76x02 mmio and usb devices use a different number of beacon slots (8
>> vs. 5). Consider this in mt76x02_init_beacon_config.
>>
>> Signed-off-by: Markus Theil <[email protected]>
>> ---
>> drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c b/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c
>> index 4209209ac940..b7412953ff26 100644
>> --- a/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c
>> +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c
>> @@ -249,7 +249,7 @@ void mt76x02_init_beacon_config(struct mt76x02_dev *dev)
>> mt76_set(dev, MT_BEACON_TIME_CFG, MT_BEACON_TIME_CFG_SYNC_MODE);
>> mt76_wr(dev, MT_BCN_BYPASS_MASK, 0xffff);
>>
>> - for (i = 0; i < 8; i++)
>> + for (i = 0; i < dev->beacon_ops->nslots; i++)
>> mt76x02_mac_set_beacon(dev, i, NULL);
>>
>> mt76x02_set_beacon_offsets(dev);
> Hi Markus,
>
> mt76x02_init_beacon_config is run just at bootstrap and it is used to clean all
> beacon RAM memory. It can't see any issue with the current code.
>
> Regards,
> Lorenzo
>
>> --
>> 2.17.1
>>
Hi Lorenzo,

I just thought this function should overwrite all 8192 byte beacon RAM
memory. If the loop count is set to 8 it would overwrite 8 x 1024 = 8192
byte in the mmio case and 8 x 1638 = 13104 byte in the USB case. 1638 is
8192 / N_BCN_SLOTS. N_BCN_SLOTS is currently 5 for USB. mt76x02_beacon.c
has no further checks for beacon_ops->nslots in the case of setting a
beacon.

Markus


2019-11-04 16:18:54

by Lorenzo Bianconi

[permalink] [raw]
Subject: Re: [PATCH] mt76: mt76x02: fix num slots in beacon config init

On Nov 04, Markus Theil wrote:
> On 04.11.19 16:45, Lorenzo Bianconi wrote:
> >> mt76x02 mmio and usb devices use a different number of beacon slots (8
> >> vs. 5). Consider this in mt76x02_init_beacon_config.
> >>
> >> Signed-off-by: Markus Theil <[email protected]>
> >> ---
> >> drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c b/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c
> >> index 4209209ac940..b7412953ff26 100644
> >> --- a/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c
> >> +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c
> >> @@ -249,7 +249,7 @@ void mt76x02_init_beacon_config(struct mt76x02_dev *dev)
> >> mt76_set(dev, MT_BEACON_TIME_CFG, MT_BEACON_TIME_CFG_SYNC_MODE);
> >> mt76_wr(dev, MT_BCN_BYPASS_MASK, 0xffff);
> >>
> >> - for (i = 0; i < 8; i++)
> >> + for (i = 0; i < dev->beacon_ops->nslots; i++)
> >> mt76x02_mac_set_beacon(dev, i, NULL);
> >>
> >> mt76x02_set_beacon_offsets(dev);
> > Hi Markus,
> >
> > mt76x02_init_beacon_config is run just at bootstrap and it is used to clean all
> > beacon RAM memory. It can't see any issue with the current code.
> >
> > Regards,
> > Lorenzo
> >
> >> --
> >> 2.17.1
> >>
> Hi Lorenzo,
>
> I just thought this function should overwrite all 8192 byte beacon RAM
> memory. If the loop count is set to 8 it would overwrite 8 x 1024 = 8192
> byte in the mmio case and 8 x 1638 = 13104 byte in the USB case. 1638 is
> 8192 / N_BCN_SLOTS. N_BCN_SLOTS is currently 5 for USB. mt76x02_beacon.c
> has no further checks for beacon_ops->nslots in the case of setting a
> beacon.

ops..I did not notice beacon_len in __mt76x02_mac_set_beacon

Lorenzo

>
> Markus
>
>


Attachments:
(No filename) (1.82 kB)
signature.asc (235.00 B)
Download all attachments

2019-11-05 08:53:53

by Stanislaw Gruszka

[permalink] [raw]
Subject: Re: [PATCH] mt76: mt76x02: fix num slots in beacon config init

On Mon, Nov 04, 2019 at 05:07:16PM +0100, Markus Theil wrote:
> On 04.11.19 16:45, Lorenzo Bianconi wrote:
> >> mt76x02 mmio and usb devices use a different number of beacon slots (8
> >> vs. 5). Consider this in mt76x02_init_beacon_config.
> >>
> >> Signed-off-by: Markus Theil <[email protected]>
> >> ---
> >> drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c b/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c
> >> index 4209209ac940..b7412953ff26 100644
> >> --- a/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c
> >> +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_beacon.c
> >> @@ -249,7 +249,7 @@ void mt76x02_init_beacon_config(struct mt76x02_dev *dev)
> >> mt76_set(dev, MT_BEACON_TIME_CFG, MT_BEACON_TIME_CFG_SYNC_MODE);
> >> mt76_wr(dev, MT_BCN_BYPASS_MASK, 0xffff);
> >>
> >> - for (i = 0; i < 8; i++)
> >> + for (i = 0; i < dev->beacon_ops->nslots; i++)
> >> mt76x02_mac_set_beacon(dev, i, NULL);
> >>
> >> mt76x02_set_beacon_offsets(dev);
> > Hi Markus,
> >
> > mt76x02_init_beacon_config is run just at bootstrap and it is used to clean all
> > beacon RAM memory. It can't see any issue with the current code.
> >
> > Regards,
> > Lorenzo
> >
> >> --
> >> 2.17.1
> >>
> Hi Lorenzo,
>
> I just thought this function should overwrite all 8192 byte beacon RAM
> memory. If the loop count is set to 8 it would overwrite 8 x 1024 = 8192
> byte in the mmio case and 8 x 1638 = 13104 byte in the USB case. 1638 is
> 8192 / N_BCN_SLOTS. N_BCN_SLOTS is currently 5 for USB. mt76x02_beacon.c
> has no further checks for beacon_ops->nslots in the case of setting a
> beacon.

We do not override beacon SRAM memory in mt76x02_init_beacon_config()
as bcn_idx increase only if !!dev->beacons[i], so we 8 times nullify
beacon 0 .

Patch is fine though, but things can be optimized more ...

We possibly can skip beacon SRAM nullification at all (even if there is
garbage in the memory, beacon will not be sent if blocked by
MT_BCN_BYPASS_MASK = 0xffff). Or nullify SRAM at once. And just set
proper MT_MAC_BSSID_DW1_MBEACON_N value.

Stanislaw