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
