| Message ID | 1548344649-10404-2-git-send-email-sgruszka@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | mt76x02: Beacon support for USB | expand |
> Use vif_mask to count interfaces to allow to set mac address > if there is only one interface and support more STA vifs in > the future. > > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> > --- > drivers/net/wireless/mediatek/mt76/mt76x02.h | 2 ++ > drivers/net/wireless/mediatek/mt76/mt76x02_util.c | 18 +++++++++++++----- > 2 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02.h b/drivers/net/wireless/mediatek/mt76/mt76x02.h > index 6d96766a6ed3..be077443bdb0 100644 > --- a/drivers/net/wireless/mediatek/mt76/mt76x02.h > +++ b/drivers/net/wireless/mediatek/mt76/mt76x02.h > @@ -73,6 +73,8 @@ struct mt76x02_dev { > > struct mutex phy_mutex; > > + u16 vif_mask; > + > u8 txdone_seq; > DECLARE_KFIFO_PTR(txstatus_fifo, struct mt76x02_tx_status); > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c > index 062614ad0d51..1a949453dc25 100644 > --- a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c > +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c > @@ -288,10 +288,7 @@ void mt76x02_vif_init(struct mt76x02_dev *dev, struct ieee80211_vif *vif, > mt76x02_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif) > { > struct mt76x02_dev *dev = hw->priv; > - unsigned int idx = 0; > - > - if (vif->addr[0] & BIT(1)) > - idx = 1 + (((dev->mt76.macaddr[0] ^ vif->addr[0]) >> 2) & 7); > + unsigned int idx, offset = 0; > > /* > * Client mode typically only has one configurable BSSID register, > @@ -307,7 +304,16 @@ void mt76x02_vif_init(struct mt76x02_dev *dev, struct ieee80211_vif *vif, > * The resulting bssidx mismatch for unicast frames is ignored by hw. > */ > if (vif->type == NL80211_IFTYPE_STATION) > - idx += 8; > + offset = 8; > + > + idx = ffs(~(dev->vif_mask >> offset)) - 1; > + idx += offset; > + > + /* Allow to change address is only one interface. */ > + if (!dev->vif_mask && (!ether_addr_equal(dev->mt76.macaddr, vif->addr))) > + mt76x02_mac_setaddr(dev, vif->addr); I guess this does not work if you add 2 vifs and then you remove the first one (you will end up with a wrong configuration in MT_MAC_ADDR_DW{0,1}). I guess the hw will not work well if MT_MAC_ADDR_DW{0,1} is not properly configured Regards, Lorenzo > + > + dev->vif_mask |= BIT(idx); > > mt76x02_vif_init(dev, vif, idx); > return 0; > @@ -318,8 +324,10 @@ void mt76x02_remove_interface(struct ieee80211_hw *hw, > struct ieee80211_vif *vif) > { > struct mt76x02_dev *dev = hw->priv; > + struct mt76x02_vif *mvif = (struct mt76x02_vif *)vif->drv_priv; > > mt76_txq_remove(&dev->mt76, vif->txq); > + dev->vif_mask &= ~BIT(mvif->idx); > } > EXPORT_SYMBOL_GPL(mt76x02_remove_interface); > > -- > 1.9.3 >
On Thu, Jan 24, 2019 at 05:12:37PM +0100, Lorenzo Bianconi wrote: > > Use vif_mask to count interfaces to allow to set mac address > > if there is only one interface and support more STA vifs in > > the future. > > > > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> > > --- > > drivers/net/wireless/mediatek/mt76/mt76x02.h | 2 ++ > > drivers/net/wireless/mediatek/mt76/mt76x02_util.c | 18 +++++++++++++----- > > 2 files changed, 15 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02.h b/drivers/net/wireless/mediatek/mt76/mt76x02.h > > index 6d96766a6ed3..be077443bdb0 100644 > > --- a/drivers/net/wireless/mediatek/mt76/mt76x02.h > > +++ b/drivers/net/wireless/mediatek/mt76/mt76x02.h > > @@ -73,6 +73,8 @@ struct mt76x02_dev { > > > > struct mutex phy_mutex; > > > > + u16 vif_mask; > > + > > u8 txdone_seq; > > DECLARE_KFIFO_PTR(txstatus_fifo, struct mt76x02_tx_status); > > > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c > > index 062614ad0d51..1a949453dc25 100644 > > --- a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c > > +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c > > @@ -288,10 +288,7 @@ void mt76x02_vif_init(struct mt76x02_dev *dev, struct ieee80211_vif *vif, > > mt76x02_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif) > > { > > struct mt76x02_dev *dev = hw->priv; > > - unsigned int idx = 0; > > - > > - if (vif->addr[0] & BIT(1)) > > - idx = 1 + (((dev->mt76.macaddr[0] ^ vif->addr[0]) >> 2) & 7); > > + unsigned int idx, offset = 0; > > > > /* > > * Client mode typically only has one configurable BSSID register, > > @@ -307,7 +304,16 @@ void mt76x02_vif_init(struct mt76x02_dev *dev, struct ieee80211_vif *vif, > > * The resulting bssidx mismatch for unicast frames is ignored by hw. > > */ > > if (vif->type == NL80211_IFTYPE_STATION) > > - idx += 8; > > + offset = 8; > > + > > + idx = ffs(~(dev->vif_mask >> offset)) - 1; > > + idx += offset; > > + > > + /* Allow to change address is only one interface. */ > > + if (!dev->vif_mask && (!ether_addr_equal(dev->mt76.macaddr, vif->addr))) > > + mt76x02_mac_setaddr(dev, vif->addr); > > I guess this does not work if you add 2 vifs and then you remove the first one > (you will end up with a wrong configuration in MT_MAC_ADDR_DW{0,1}). I guess > the hw will not work well if MT_MAC_ADDR_DW{0,1} is not properly configured This is only done when when vif_mask is 0 i.e. no interfaces. When some interface is already created, changing MAC address will now work anyway. Thanks Stanislaw
> On Thu, Jan 24, 2019 at 05:12:37PM +0100, Lorenzo Bianconi wrote: > > > Use vif_mask to count interfaces to allow to set mac address > > > if there is only one interface and support more STA vifs in > > > the future. > > > > > > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> > > > --- > > > drivers/net/wireless/mediatek/mt76/mt76x02.h | 2 ++ > > > drivers/net/wireless/mediatek/mt76/mt76x02_util.c | 18 +++++++++++++----- > > > 2 files changed, 15 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02.h b/drivers/net/wireless/mediatek/mt76/mt76x02.h > > > index 6d96766a6ed3..be077443bdb0 100644 > > > --- a/drivers/net/wireless/mediatek/mt76/mt76x02.h > > > +++ b/drivers/net/wireless/mediatek/mt76/mt76x02.h > > > @@ -73,6 +73,8 @@ struct mt76x02_dev { > > > > > > struct mutex phy_mutex; > > > > > > + u16 vif_mask; > > > + > > > u8 txdone_seq; > > > DECLARE_KFIFO_PTR(txstatus_fifo, struct mt76x02_tx_status); > > > > > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c > > > index 062614ad0d51..1a949453dc25 100644 > > > --- a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c > > > +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c > > > @@ -288,10 +288,7 @@ void mt76x02_vif_init(struct mt76x02_dev *dev, struct ieee80211_vif *vif, > > > mt76x02_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif) > > > { > > > struct mt76x02_dev *dev = hw->priv; > > > - unsigned int idx = 0; > > > - > > > - if (vif->addr[0] & BIT(1)) > > > - idx = 1 + (((dev->mt76.macaddr[0] ^ vif->addr[0]) >> 2) & 7); > > > + unsigned int idx, offset = 0; > > > > > > /* > > > * Client mode typically only has one configurable BSSID register, > > > @@ -307,7 +304,16 @@ void mt76x02_vif_init(struct mt76x02_dev *dev, struct ieee80211_vif *vif, > > > * The resulting bssidx mismatch for unicast frames is ignored by hw. > > > */ > > > if (vif->type == NL80211_IFTYPE_STATION) > > > - idx += 8; > > > + offset = 8; > > > + > > > + idx = ffs(~(dev->vif_mask >> offset)) - 1; > > > + idx += offset; > > > + > > > + /* Allow to change address is only one interface. */ > > > + if (!dev->vif_mask && (!ether_addr_equal(dev->mt76.macaddr, vif->addr))) > > > + mt76x02_mac_setaddr(dev, vif->addr); > > > > I guess this does not work if you add 2 vifs and then you remove the first one > > (you will end up with a wrong configuration in MT_MAC_ADDR_DW{0,1}). I guess > > the hw will not work well if MT_MAC_ADDR_DW{0,1} is not properly configured Maybe I am missing something, but let's assume you add the interface vif0 with address 00:11:22:33:44:55 (MT_MAC_ADDR_DW{0,1} will be set to 00:11:22:33:44:55) and then you add vif1 with address 00:aa:bb:cc:dd:ee. If at some point you remove vif0 MT_MAC_ADDR_DW{0,1} will not be properly reconfigured. The problem will be more complex if you have more interfaces > > This is only done when when vif_mask is 0 i.e. no interfaces. When > some interface is already created, changing MAC address will now work > anyway. > > Thanks > Stanislaw
> > > > > > I guess this does not work if you add 2 vifs and then you remove the first one > > > (you will end up with a wrong configuration in MT_MAC_ADDR_DW{0,1}). I guess > > > the hw will not work well if MT_MAC_ADDR_DW{0,1} is not properly configured > > Maybe I am missing something, but let's assume you add the interface vif0 with address > 00:11:22:33:44:55 (MT_MAC_ADDR_DW{0,1} will be set to 00:11:22:33:44:55) and > then you add vif1 with address 00:aa:bb:cc:dd:ee. If at some point you remove > vif0 MT_MAC_ADDR_DW{0,1} will not be properly reconfigured. The problem will > be more complex if you have more interfaces Moreover if you add 2 vif, vif0 with address 00:11:22:33:44:55 and vif1 with address 00:aa:bb:cc:dd:ee, have you double-checked you are able to get the same tpt on both interfaces? In the past IIRC there were issues if we use multibss with completely different mac addresses Regards, Lorenzo > > > > > This is only done when when vif_mask is 0 i.e. no interfaces. When > > some interface is already created, changing MAC address will now work > > anyway. > > > > Thanks > > Stanislaw
On Thu, Jan 24, 2019 at 11:20:42PM +0100, Lorenzo Bianconi wrote: > > > > > > > > I guess this does not work if you add 2 vifs and then you remove the first one > > > > (you will end up with a wrong configuration in MT_MAC_ADDR_DW{0,1}). I guess > > > > the hw will not work well if MT_MAC_ADDR_DW{0,1} is not properly configured > > > > Maybe I am missing something, but let's assume you add the interface vif0 with address > > 00:11:22:33:44:55 (MT_MAC_ADDR_DW{0,1} will be set to 00:11:22:33:44:55) and > > then you add vif1 with address 00:aa:bb:cc:dd:ee. If at some point you remove > > vif0 MT_MAC_ADDR_DW{0,1} will not be properly reconfigured. The problem will > > be more complex if you have more interfaces Ok, so in remove_interface extra code can be added to implement that. > Moreover if you add 2 vif, vif0 with address 00:11:22:33:44:55 and vif1 with > address 00:aa:bb:cc:dd:ee, have you double-checked you are able to get the same > tpt on both interfaces? In the past IIRC there were issues if we use multibss > with completely different mac addresses I haven't check that. But what I can tell multi vif STA does not work, if we do not enable PROMISC on rx filter. But this is not diffrent from MMIO version, which mark multi vif support. Regards Stanislaw
> On Thu, Jan 24, 2019 at 11:20:42PM +0100, Lorenzo Bianconi wrote: > > > > > > > > > > I guess this does not work if you add 2 vifs and then you remove the first one > > > > > (you will end up with a wrong configuration in MT_MAC_ADDR_DW{0,1}). I guess > > > > > the hw will not work well if MT_MAC_ADDR_DW{0,1} is not properly configured > > > > > > Maybe I am missing something, but let's assume you add the interface vif0 with address > > > 00:11:22:33:44:55 (MT_MAC_ADDR_DW{0,1} will be set to 00:11:22:33:44:55) and > > > then you add vif1 with address 00:aa:bb:cc:dd:ee. If at some point you remove > > > vif0 MT_MAC_ADDR_DW{0,1} will not be properly reconfigured. The problem will > > > be more complex if you have more interfaces > > Ok, so in remove_interface extra code can be added to implement that. > > > Moreover if you add 2 vif, vif0 with address 00:11:22:33:44:55 and vif1 with > > address 00:aa:bb:cc:dd:ee, have you double-checked you are able to get the same > > tpt on both interfaces? In the past IIRC there were issues if we use multibss > > with completely different mac addresses > > I haven't check that. But what I can tell multi vif STA does not work, if > we do not enable PROMISC on rx filter. But this is not diffrent from MMIO > version, which mark multi vif support. Since this is not strictly related to AP/IBSS support I guess we can just use mt76x02_add_interface for mt76x2u or modify mt76x2u_add_interface in order to use ap_cli registers for sta mode and drop this patch. I think we can address it later after more testing Regards, Lorenzo > > Regards > Stanislaw
On Fri, Jan 25, 2019 at 10:02:38AM +0100, Lorenzo Bianconi wrote: > > On Thu, Jan 24, 2019 at 11:20:42PM +0100, Lorenzo Bianconi wrote: > > > > > > > > > > > > I guess this does not work if you add 2 vifs and then you remove the first one > > > > > > (you will end up with a wrong configuration in MT_MAC_ADDR_DW{0,1}). I guess > > > > > > the hw will not work well if MT_MAC_ADDR_DW{0,1} is not properly configured > > > > > > > > Maybe I am missing something, but let's assume you add the interface vif0 with address > > > > 00:11:22:33:44:55 (MT_MAC_ADDR_DW{0,1} will be set to 00:11:22:33:44:55) and > > > > then you add vif1 with address 00:aa:bb:cc:dd:ee. If at some point you remove > > > > vif0 MT_MAC_ADDR_DW{0,1} will not be properly reconfigured. The problem will > > > > be more complex if you have more interfaces > > > > Ok, so in remove_interface extra code can be added to implement that. > > > > > Moreover if you add 2 vif, vif0 with address 00:11:22:33:44:55 and vif1 with > > > address 00:aa:bb:cc:dd:ee, have you double-checked you are able to get the same > > > tpt on both interfaces? In the past IIRC there were issues if we use multibss > > > with completely different mac addresses > > > > I haven't check that. But what I can tell multi vif STA does not work, if > > we do not enable PROMISC on rx filter. But this is not diffrent from MMIO > > version, which mark multi vif support. > > Since this is not strictly related to AP/IBSS support I guess we can just use > mt76x02_add_interface for mt76x2u or modify mt76x2u_add_interface in order to > use ap_cli registers for sta mode and drop this patch. I think we can address it > later after more testing I do not see the reason of dropping the patch. Thanks Stanislaw
On Fri, Jan 25, 2019 at 10:02:38AM +0100, Lorenzo Bianconi wrote: > > On Thu, Jan 24, 2019 at 11:20:42PM +0100, Lorenzo Bianconi wrote: > > > > > > > > > > > > I guess this does not work if you add 2 vifs and then you remove the first one > > > > > > (you will end up with a wrong configuration in MT_MAC_ADDR_DW{0,1}). I guess > > > > > > the hw will not work well if MT_MAC_ADDR_DW{0,1} is not properly configured > > > > > > > > Maybe I am missing something, but let's assume you add the interface vif0 with address > > > > 00:11:22:33:44:55 (MT_MAC_ADDR_DW{0,1} will be set to 00:11:22:33:44:55) and > > > > then you add vif1 with address 00:aa:bb:cc:dd:ee. If at some point you remove > > > > vif0 MT_MAC_ADDR_DW{0,1} will not be properly reconfigured. The problem will > > > > be more complex if you have more interfaces > > > > Ok, so in remove_interface extra code can be added to implement that. Something like this should address the issue you raised: diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c index 3880caa0c64a..44b4af928a4e 100644 --- a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c @@ -320,6 +320,15 @@ void mt76x02_sta_remove(struct mt76_dev *mdev, struct ieee80211_vif *vif, } EXPORT_SYMBOL_GPL(mt76x02_add_interface); +static void mt76x02_setaddr_iterator(void *data, u8 *mac, + struct ieee80211_vif *vif) +{ + struct mt76x02_dev *dev = data; + + if (!ether_addr_equal(dev->mt76.macaddr, vif->addr)) + mt76x02_mac_setaddr(dev, vif->addr); +} + void mt76x02_remove_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif) { @@ -328,6 +337,10 @@ void mt76x02_remove_interface(struct ieee80211_hw *hw, mt76_txq_remove(&dev->mt76, vif->txq); dev->vif_mask &= ~BIT(mvif->idx); + + if (hweight16(dev->vif_mask) == 1) + ieee80211_iterate_interfaces(hw, 0, mt76x02_setaddr_iterator, + dev); } EXPORT_SYMBOL_GPL(mt76x02_remove_interface);
> On Fri, Jan 25, 2019 at 10:02:38AM +0100, Lorenzo Bianconi wrote: > > > On Thu, Jan 24, 2019 at 11:20:42PM +0100, Lorenzo Bianconi wrote: > > > > > > > > > > > > > > I guess this does not work if you add 2 vifs and then you remove the first one > > > > > > > (you will end up with a wrong configuration in MT_MAC_ADDR_DW{0,1}). I guess > > > > > > > the hw will not work well if MT_MAC_ADDR_DW{0,1} is not properly configured > > > > > > > > > > Maybe I am missing something, but let's assume you add the interface vif0 with address > > > > > 00:11:22:33:44:55 (MT_MAC_ADDR_DW{0,1} will be set to 00:11:22:33:44:55) and > > > > > then you add vif1 with address 00:aa:bb:cc:dd:ee. If at some point you remove > > > > > vif0 MT_MAC_ADDR_DW{0,1} will not be properly reconfigured. The problem will > > > > > be more complex if you have more interfaces > > > > > > Ok, so in remove_interface extra code can be added to implement that. > > Something like this should address the issue you raised: > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c > index 3880caa0c64a..44b4af928a4e 100644 > --- a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c > +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c > @@ -320,6 +320,15 @@ void mt76x02_sta_remove(struct mt76_dev *mdev, struct ieee80211_vif *vif, > } > EXPORT_SYMBOL_GPL(mt76x02_add_interface); > > +static void mt76x02_setaddr_iterator(void *data, u8 *mac, > + struct ieee80211_vif *vif) > +{ > + struct mt76x02_dev *dev = data; > + > + if (!ether_addr_equal(dev->mt76.macaddr, vif->addr)) > + mt76x02_mac_setaddr(dev, vif->addr); This will end-up with multiple configurations if we have more than two interfaces, right? > +} > + > void mt76x02_remove_interface(struct ieee80211_hw *hw, > struct ieee80211_vif *vif) > { > @@ -328,6 +337,10 @@ void mt76x02_remove_interface(struct ieee80211_hw *hw, > > mt76_txq_remove(&dev->mt76, vif->txq); > dev->vif_mask &= ~BIT(mvif->idx); > + > + if (hweight16(dev->vif_mask) == 1) > + ieee80211_iterate_interfaces(hw, 0, mt76x02_setaddr_iterator, > + dev); I guess we have the same issue if we have more than two interfaces and we remove the first one that has been configured. Moreover I am a little worried about tpt regressions with this patch. Are you sure that if you use complete different mac addresses on a multivif scenario you can get the same tpt on all the interfaces? Could you please provide some tpt results? Vendor driver relies on a strict scheme for mac addresses in multi-bss. If tpt is nice I am ok with this patch Regards, Lorenzo > } > EXPORT_SYMBOL_GPL(mt76x02_remove_interface);
On Fri, Jan 25, 2019 at 11:25:46AM +0100, Lorenzo Bianconi wrote: > > On Fri, Jan 25, 2019 at 10:02:38AM +0100, Lorenzo Bianconi wrote: > > > > On Thu, Jan 24, 2019 at 11:20:42PM +0100, Lorenzo Bianconi wrote: > > > > > > > > > > > > > > > > I guess this does not work if you add 2 vifs and then you remove the first one > > > > > > > > (you will end up with a wrong configuration in MT_MAC_ADDR_DW{0,1}). I guess > > > > > > > > the hw will not work well if MT_MAC_ADDR_DW{0,1} is not properly configured > > > > > > > > > > > > Maybe I am missing something, but let's assume you add the interface vif0 with address > > > > > > 00:11:22:33:44:55 (MT_MAC_ADDR_DW{0,1} will be set to 00:11:22:33:44:55) and > > > > > > then you add vif1 with address 00:aa:bb:cc:dd:ee. If at some point you remove > > > > > > vif0 MT_MAC_ADDR_DW{0,1} will not be properly reconfigured. The problem will > > > > > > be more complex if you have more interfaces > > > > > > > > Ok, so in remove_interface extra code can be added to implement that. > > > > Something like this should address the issue you raised: > > > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c > > index 3880caa0c64a..44b4af928a4e 100644 > > --- a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c > > +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c > > @@ -320,6 +320,15 @@ void mt76x02_sta_remove(struct mt76_dev *mdev, struct ieee80211_vif *vif, > > } > > EXPORT_SYMBOL_GPL(mt76x02_add_interface); > > > > +static void mt76x02_setaddr_iterator(void *data, u8 *mac, > > + struct ieee80211_vif *vif) > > +{ > > + struct mt76x02_dev *dev = data; > > + > > + if (!ether_addr_equal(dev->mt76.macaddr, vif->addr)) > > + mt76x02_mac_setaddr(dev, vif->addr); > > This will end-up with multiple configurations if we have more than two > interfaces, right? No. It is called only when number of vifs change from 2 to 1 . But there should be additional check 'dev->vif_mask & BIT(mvif->idx)' since we can have removing interface still present in mac80211. > > void mt76x02_remove_interface(struct ieee80211_hw *hw, > > struct ieee80211_vif *vif) > > { > > @@ -328,6 +337,10 @@ void mt76x02_remove_interface(struct ieee80211_hw *hw, > > > > mt76_txq_remove(&dev->mt76, vif->txq); > > dev->vif_mask &= ~BIT(mvif->idx); > > + > > + if (hweight16(dev->vif_mask) == 1) > > + ieee80211_iterate_interfaces(hw, 0, mt76x02_setaddr_iterator, > > + dev); > > I guess we have the same issue if we have more than two interfaces and we > remove the first one that has been configured. We can not configure more than one MAC address without using MT_MAC_ADDR_EXT registers. If there are more than one vif, there is no point to set MT_MAC_ADDR. > Moreover I am a little worried about tpt regressions with this patch. > Are you sure that if you use complete different mac addresses on a multivif scenario > you can get the same tpt on all the interfaces? Could you please provide some > tpt results? How exactly posted patch can cause tpt regression ? Posted patch just add possibility to configure HW MAC address by this: iw dev wlan0 del iw phy phy0 interface add wlan0 type managed addr 00:11:22:33:44:55 what is feature of mt76x2u. Patch just extend that possibly to other mt76x02 devices and allow to remove custom mt76x2u add_interfacea callback. Thanks Stanislaw
On 2019-01-25 13:41, Stanislaw Gruszka wrote: > On Fri, Jan 25, 2019 at 11:25:46AM +0100, Lorenzo Bianconi wrote: >> > On Fri, Jan 25, 2019 at 10:02:38AM +0100, Lorenzo Bianconi wrote: >> > > > On Thu, Jan 24, 2019 at 11:20:42PM +0100, Lorenzo Bianconi wrote: >> > > > > > > > >> > > > > > > > I guess this does not work if you add 2 vifs and then you remove the first one >> > > > > > > > (you will end up with a wrong configuration in MT_MAC_ADDR_DW{0,1}). I guess >> > > > > > > > the hw will not work well if MT_MAC_ADDR_DW{0,1} is not properly configured >> > > > > > >> > > > > > Maybe I am missing something, but let's assume you add the interface vif0 with address >> > > > > > 00:11:22:33:44:55 (MT_MAC_ADDR_DW{0,1} will be set to 00:11:22:33:44:55) and >> > > > > > then you add vif1 with address 00:aa:bb:cc:dd:ee. If at some point you remove >> > > > > > vif0 MT_MAC_ADDR_DW{0,1} will not be properly reconfigured. The problem will >> > > > > > be more complex if you have more interfaces >> > > > >> > > > Ok, so in remove_interface extra code can be added to implement that. >> > >> > Something like this should address the issue you raised: >> > >> > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c >> > index 3880caa0c64a..44b4af928a4e 100644 >> > --- a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c >> > +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c >> > @@ -320,6 +320,15 @@ void mt76x02_sta_remove(struct mt76_dev *mdev, struct ieee80211_vif *vif, >> > } >> > EXPORT_SYMBOL_GPL(mt76x02_add_interface); >> > >> > +static void mt76x02_setaddr_iterator(void *data, u8 *mac, >> > + struct ieee80211_vif *vif) >> > +{ >> > + struct mt76x02_dev *dev = data; >> > + >> > + if (!ether_addr_equal(dev->mt76.macaddr, vif->addr)) >> > + mt76x02_mac_setaddr(dev, vif->addr); >> >> This will end-up with multiple configurations if we have more than two >> interfaces, right? > > No. It is called only when number of vifs change from 2 to 1 . But there > should be additional check 'dev->vif_mask & BIT(mvif->idx)' since we can > have removing interface still present in mac80211. > >> > void mt76x02_remove_interface(struct ieee80211_hw *hw, >> > struct ieee80211_vif *vif) >> > { >> > @@ -328,6 +337,10 @@ void mt76x02_remove_interface(struct ieee80211_hw *hw, >> > >> > mt76_txq_remove(&dev->mt76, vif->txq); >> > dev->vif_mask &= ~BIT(mvif->idx); >> > + >> > + if (hweight16(dev->vif_mask) == 1) >> > + ieee80211_iterate_interfaces(hw, 0, mt76x02_setaddr_iterator, >> > + dev); >> >> I guess we have the same issue if we have more than two interfaces and we >> remove the first one that has been configured. > > We can not configure more than one MAC address without using MT_MAC_ADDR_EXT > registers. If there are more than one vif, there is no point to set MT_MAC_ADDR. > >> Moreover I am a little worried about tpt regressions with this patch. >> Are you sure that if you use complete different mac addresses on a multivif scenario >> you can get the same tpt on all the interfaces? Could you please provide some >> tpt results? > > How exactly posted patch can cause tpt regression ? > > Posted patch just add possibility to configure HW MAC address > by this: > > iw dev wlan0 del > iw phy phy0 interface add wlan0 type managed addr 00:11:22:33:44:55 > > what is feature of mt76x2u. Patch just extend that possibly to other > mt76x02 devices and allow to remove custom mt76x2u add_interfacea > callback. The main part that could cause issues is that you're changing the way that the vif index is calculated. Without the patch, it's calculated from the MAC address in a way consistent with what the hardware expects. With the patch, it's just allocated from a mask. The vif index ends up being passed down to the hardware as a BSS index WCID attribute in mt76x02_mac_wcid_setup. We would have to run some tests with multiple AP interfaces, bringing up secondary interfaces in a different order to see if there are any regressions there if the BSS index no longer matches the MAC address based index. - Felix
On Mon, Jan 28, 2019 at 09:41:45AM +0100, Felix Fietkau wrote: > >> Moreover I am a little worried about tpt regressions with this patch. > >> Are you sure that if you use complete different mac addresses on a multivif scenario > >> you can get the same tpt on all the interfaces? Could you please provide some > >> tpt results? > > > > How exactly posted patch can cause tpt regression ? > > > > Posted patch just add possibility to configure HW MAC address > > by this: > > > > iw dev wlan0 del > > iw phy phy0 interface add wlan0 type managed addr 00:11:22:33:44:55 > > > > what is feature of mt76x2u. Patch just extend that possibly to other > > mt76x02 devices and allow to remove custom mt76x2u add_interfacea > > callback. > The main part that could cause issues is that you're changing the way > that the vif index is calculated. Without the patch, it's calculated > from the MAC address in a way consistent with what the hardware expects. > With the patch, it's just allocated from a mask. > The vif index ends up being passed down to the hardware as a BSS index > WCID attribute in mt76x02_mac_wcid_setup. > We would have to run some tests with multiple AP interfaces, bringing up > secondary interfaces in a different order to see if there are any > regressions there if the BSS index no longer matches the MAC address > based index. Ok, that objection make sense. I'll check that. Regards Stanislaw
On Mon, Jan 28, 2019 at 10:02:01AM +0100, Stanislaw Gruszka wrote: > On Mon, Jan 28, 2019 at 09:41:45AM +0100, Felix Fietkau wrote: > > >> Moreover I am a little worried about tpt regressions with this patch. > > >> Are you sure that if you use complete different mac addresses on a multivif scenario > > >> you can get the same tpt on all the interfaces? Could you please provide some > > >> tpt results? > > > > > > How exactly posted patch can cause tpt regression ? > > > > > > Posted patch just add possibility to configure HW MAC address > > > by this: > > > > > > iw dev wlan0 del > > > iw phy phy0 interface add wlan0 type managed addr 00:11:22:33:44:55 > > > > > > what is feature of mt76x2u. Patch just extend that possibly to other > > > mt76x02 devices and allow to remove custom mt76x2u add_interfacea > > > callback. > > The main part that could cause issues is that you're changing the way > > that the vif index is calculated. Without the patch, it's calculated > > from the MAC address in a way consistent with what the hardware expects. > > With the patch, it's just allocated from a mask. > > The vif index ends up being passed down to the hardware as a BSS index > > WCID attribute in mt76x02_mac_wcid_setup. > > We would have to run some tests with multiple AP interfaces, bringing up > > secondary interfaces in a different order to see if there are any > > regressions there if the BSS index no longer matches the MAC address > > based index. > > Ok, that objection make sense. I'll check that. I'm not sure if I'm doing something wrong or current code is wrong. But when I configure multi bssid's is hostapd.conf like this: 02:aa:bb:cc:dd:e0 06:aa:bb:cc:dd:e0 0a:aa:bb:cc:dd:e0 hostapd fail with "Too many bits in the BSSID mask" error . To make hostapd work, I have configure bssid's like this: 02:aa:bb:cc:dd:e0 02:aa:bb:cc:dd:e4 02:aa:bb:cc:dd:e8 But this ends up with the same index in mt76 for diffrent vif's. Seems like calculating of the index should be fixed by: if (vif->addr[0] & BIT(1)) idx = 1 + (((dev->mt76.macaddr[5] ^ vif->addr[5]) >> 2) & 7); Regards Stanislaw
On 2019-01-28 12:16, Stanislaw Gruszka wrote: > On Mon, Jan 28, 2019 at 10:02:01AM +0100, Stanislaw Gruszka wrote: >> On Mon, Jan 28, 2019 at 09:41:45AM +0100, Felix Fietkau wrote: >> > >> Moreover I am a little worried about tpt regressions with this patch. >> > >> Are you sure that if you use complete different mac addresses on a multivif scenario >> > >> you can get the same tpt on all the interfaces? Could you please provide some >> > >> tpt results? >> > > >> > > How exactly posted patch can cause tpt regression ? >> > > >> > > Posted patch just add possibility to configure HW MAC address >> > > by this: >> > > >> > > iw dev wlan0 del >> > > iw phy phy0 interface add wlan0 type managed addr 00:11:22:33:44:55 >> > > >> > > what is feature of mt76x2u. Patch just extend that possibly to other >> > > mt76x02 devices and allow to remove custom mt76x2u add_interfacea >> > > callback. >> > The main part that could cause issues is that you're changing the way >> > that the vif index is calculated. Without the patch, it's calculated >> > from the MAC address in a way consistent with what the hardware expects. >> > With the patch, it's just allocated from a mask. >> > The vif index ends up being passed down to the hardware as a BSS index >> > WCID attribute in mt76x02_mac_wcid_setup. >> > We would have to run some tests with multiple AP interfaces, bringing up >> > secondary interfaces in a different order to see if there are any >> > regressions there if the BSS index no longer matches the MAC address >> > based index. >> >> Ok, that objection make sense. I'll check that. > > I'm not sure if I'm doing something wrong or current code is > wrong. But when I configure multi bssid's is hostapd.conf like this: > > 02:aa:bb:cc:dd:e0 > 06:aa:bb:cc:dd:e0 > 0a:aa:bb:cc:dd:e0 > > hostapd fail with "Too many bits in the BSSID mask" error . To make hostapd > work, I have configure bssid's like this: Do you have a bssid mask set? I don't think the current code does that. Also, in OpenWrt, I didn't see any issue like that. > 02:aa:bb:cc:dd:e0 > 02:aa:bb:cc:dd:e4 > 02:aa:bb:cc:dd:e8 > > But this ends up with the same index in mt76 for diffrent vif's. Seems like > calculating of the index should be fixed by: > > if (vif->addr[0] & BIT(1)) > idx = 1 + (((dev->mt76.macaddr[5] ^ vif->addr[5]) >> 2) & 7); This MAC adddress allocation scheme will cause problems if you have multiple devices next to each other that have consecutive MAC addresses. While it may sound unlikely, I do know people that encountered this issue (though with QCA based hardware), and it took us quite a while to figure out what was messing up their network ;) - Felix
On Mon, Jan 28, 2019 at 01:29:27PM +0100, Felix Fietkau wrote: > On 2019-01-28 12:16, Stanislaw Gruszka wrote: > > On Mon, Jan 28, 2019 at 10:02:01AM +0100, Stanislaw Gruszka wrote: > >> On Mon, Jan 28, 2019 at 09:41:45AM +0100, Felix Fietkau wrote: > >> > >> Moreover I am a little worried about tpt regressions with this patch. > >> > >> Are you sure that if you use complete different mac addresses on a multivif scenario > >> > >> you can get the same tpt on all the interfaces? Could you please provide some > >> > >> tpt results? > >> > > > >> > > How exactly posted patch can cause tpt regression ? > >> > > > >> > > Posted patch just add possibility to configure HW MAC address > >> > > by this: > >> > > > >> > > iw dev wlan0 del > >> > > iw phy phy0 interface add wlan0 type managed addr 00:11:22:33:44:55 > >> > > > >> > > what is feature of mt76x2u. Patch just extend that possibly to other > >> > > mt76x02 devices and allow to remove custom mt76x2u add_interfacea > >> > > callback. > >> > The main part that could cause issues is that you're changing the way > >> > that the vif index is calculated. Without the patch, it's calculated > >> > from the MAC address in a way consistent with what the hardware expects. > >> > With the patch, it's just allocated from a mask. > >> > The vif index ends up being passed down to the hardware as a BSS index > >> > WCID attribute in mt76x02_mac_wcid_setup. > >> > We would have to run some tests with multiple AP interfaces, bringing up > >> > secondary interfaces in a different order to see if there are any > >> > regressions there if the BSS index no longer matches the MAC address > >> > based index. > >> > >> Ok, that objection make sense. I'll check that. > > > > I'm not sure if I'm doing something wrong or current code is > > wrong. But when I configure multi bssid's is hostapd.conf like this: > > > > 02:aa:bb:cc:dd:e0 > > 06:aa:bb:cc:dd:e0 > > 0a:aa:bb:cc:dd:e0 > > > > hostapd fail with "Too many bits in the BSSID mask" error . To make hostapd > > work, I have configure bssid's like this: > Do you have a bssid mask set? I don't think the current code does that. > Also, in OpenWrt, I didn't see any issue like that. Not sure if there is bssid mask option , seems hostapd calculate that from provided bssid= fields. However I realized there is use_driver_iface_addr=1 and that make things work as expected. Thanks Stanislaw
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02.h b/drivers/net/wireless/mediatek/mt76/mt76x02.h index 6d96766a6ed3..be077443bdb0 100644 --- a/drivers/net/wireless/mediatek/mt76/mt76x02.h +++ b/drivers/net/wireless/mediatek/mt76/mt76x02.h @@ -73,6 +73,8 @@ struct mt76x02_dev { struct mutex phy_mutex; + u16 vif_mask; + u8 txdone_seq; DECLARE_KFIFO_PTR(txstatus_fifo, struct mt76x02_tx_status); diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c index 062614ad0d51..1a949453dc25 100644 --- a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c @@ -288,10 +288,7 @@ void mt76x02_vif_init(struct mt76x02_dev *dev, struct ieee80211_vif *vif, mt76x02_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif) { struct mt76x02_dev *dev = hw->priv; - unsigned int idx = 0; - - if (vif->addr[0] & BIT(1)) - idx = 1 + (((dev->mt76.macaddr[0] ^ vif->addr[0]) >> 2) & 7); + unsigned int idx, offset = 0; /* * Client mode typically only has one configurable BSSID register, @@ -307,7 +304,16 @@ void mt76x02_vif_init(struct mt76x02_dev *dev, struct ieee80211_vif *vif, * The resulting bssidx mismatch for unicast frames is ignored by hw. */ if (vif->type == NL80211_IFTYPE_STATION) - idx += 8; + offset = 8; + + idx = ffs(~(dev->vif_mask >> offset)) - 1; + idx += offset; + + /* Allow to change address is only one interface. */ + if (!dev->vif_mask && (!ether_addr_equal(dev->mt76.macaddr, vif->addr))) + mt76x02_mac_setaddr(dev, vif->addr); + + dev->vif_mask |= BIT(idx); mt76x02_vif_init(dev, vif, idx); return 0; @@ -318,8 +324,10 @@ void mt76x02_remove_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif) { struct mt76x02_dev *dev = hw->priv; + struct mt76x02_vif *mvif = (struct mt76x02_vif *)vif->drv_priv; mt76_txq_remove(&dev->mt76, vif->txq); + dev->vif_mask &= ~BIT(mvif->idx); } EXPORT_SYMBOL_GPL(mt76x02_remove_interface);
Use vif_mask to count interfaces to allow to set mac address if there is only one interface and support more STA vifs in the future. Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> --- drivers/net/wireless/mediatek/mt76/mt76x02.h | 2 ++ drivers/net/wireless/mediatek/mt76/mt76x02_util.c | 18 +++++++++++++----- 2 files changed, 15 insertions(+), 5 deletions(-)